Move reading functions from OutputPin trait to StatefulOutputPin #72

astro · 2018-04-07T22:22:18Z

As it has come up in #29, here is a proposal for removing the burden of caching the state on implementations that cannot read it from an output pin.

therealprof

I'm very much in favour of this change but I'd like a second opinion about how we're going to do this.

Also are you planning to follow up with the additional traits?

astro · 2018-04-07T23:03:47Z

Also are you planning to follow up with the additional traits?

See #73. StatefulOutputPin looks good to me. Is there any shorter name for it?

astro · 2018-04-07T23:33:22Z

Please refresh your browser, I edited my comment several times. :) Sorry!

I think this and #73 are separate problems. You can implement StatefulOutputPin (which I pushed later) without switching modes.

therealprof · 2018-04-07T23:37:27Z

Err, yes, my bad, the comment was really meant for #73. Let me move it there.

hannobraun

Looks good to me! Thank you, @astro.

therealprof

I'd prefer having the composable traits instead of a composed trait but it is better than what we have now so I'm good with it.

The only problem I see is that this might potentially break existing implementations?

hannobraun · 2018-04-11T10:58:10Z

@therealprof

The only problem I see is that this might potentially break existing implementations?

As I stated before elsewhere, I don't think we should be losing sleep over backwards-compatibility at this point. Yes, breaking implementations sucks, but I'm more concerned about whether embedded-hal is going to look good a few years from now.

therealprof · 2018-04-11T11:01:05Z

@hannobraun That's fine with me, too. Just wanted to raise the concern others might have to feel whether there's consensus on that topic.

austinbes · 2018-04-11T16:09:15Z

FWIW, I'm totally of @hannobraun's opinion. Break things early, if they're to break at all

ryankurte · 2018-04-12T04:15:05Z

src/digital.rs

+    /// Is the pin set to low?
+    fn is_set_low(&self) -> bool;
+
+    /// Toggle pin output


Sorry if I am late to the party. Can you override a method in a trait? On some cores toggle is exposed as a separate register / no read state is required.

I'd propose having the toggle as a separate trait that you can implement on StatefulOutputPin if toggle function is not available.

As discussed in #31 (et al) I also think that different capabilities should be modelled in different composable (and mode dependent) traits instead of being munged together into a few traits that do a lot of things at once. Something like is_set_low() on a StatefulOutputPin is not clear what it actually means, i.e. is that supposed to be the electrical or the logical state of the pin? For toggle() it has to be the latter, not the former, in other cases you might want to have the electrical state, though and cannot have this. Also (as you said) there are implementations allowing an efficient toggle without reading the logical state from a pin. There might even be implementations which do not allow you to read back the logical state, although I don't recall any from the top of my head.

I only agreed to moving it out of OutputPin because it belongs there even less and this change turns into into an unproven functionality so if it doesn't provide the hoped for benefit (as I suspect it will not) then we can replace it by something better.

Sounds reasonable. I'll remove it in favour of adding ToggleableOutputPin later.

Can you override a method in a trait?

@ryankurte Yes, you can.

astro · 2018-04-12T15:36:34Z

I have moved toggle() into a separate ToggleableOutputPin trait with a default impl for StatefulOutputPin.

Also, I removed the OutputPin constraint from StatefulOutputPin as it is now optional and possible for theoretical output pins from which you can read but not write the state (eg. set by hardware).

… impl for StatefulOutputPin

austinglaser · 2018-04-12T16:43:55Z

src/digital.rs

+/// toggleable by software. You may override the `toggle()` method
+/// with a hardware implementation.
+#[cfg(feature = "unproven")]
+impl<PIN: OutputPin + StatefulOutputPin> ToggleableOutputPin for PIN {


Would it be good to add an empty DefaultToggle trait here, and thus make this implementation opt-in rather than opt-out?

It would match the way the blocking SPI traits are handled. On the other hand, this is a simpler implementation.

I'm not particularly attached to one way or the other, but I do think consistency throughout the embedded-hal will be valuable.

Thanks for the suggestion. I added that now by copying the style of the opt-in default traits in spi and serial. I hope that the doc example is helpful.

astro · 2018-04-29T19:14:34Z

Is there anything I can do to help this move forward?

therealprof · 2018-04-30T08:02:01Z

src/digital.rs

    /// Sets the pin low
    fn set_low(&mut self);

    /// Sets the pin high
    fn set_high(&mut self);
 }

+/// Output pin that can read its output state


As I've mentioned before there's a distinction between a logical state and an electrical state. It should be made clear that this is meant to represent the logical (or register) state of a pin and not the electrical state.

therealprof · 2018-04-30T08:08:07Z

src/digital.rs

+/// Output pin that can read its output state
+#[cfg(feature = "unproven")]
+pub trait StatefulOutputPin {
+    /// Is the pin set to high?


A pin could also be logically floating so either it needs to be made clear that a tri-stated pin may not implement this trait or that the implicit assumption p.is_set_high() == !p.is_set_low() does not necessarily hold and should not be assumed.

japaric · 2018-05-11T21:34:14Z

This seems OK to me but it should be explicitly noted that these traits should be implemented only for push pull output pins and that low / high are referring to the electrical output of the pin. That was missing in the original trait as well.

@hannobraun just for the record: releasing a new minor version doesn't "just breaks implementations"; it fragments the ecosystem. Once a HAL implementation moves from e.g. v0.1.x to v0.2.x it can no longer be used with driver crates that depend on embedded hal v0.1.x. This happens even if the trait that the driver crate uses as a bound has not changed at all between the two minor releases. This situation increases the maintenance work of driver authors which now have to publish a new minor version of their crate that supports the new embedded-hal and probably will also have to maintain the v0.1.x version until all / most-of the existing / active / maintained HAL implementations move to v0.2.x. So bumping the minor version shouldn't be done often / lightly; as the embedded-hal ecosystem grows we'll have to allow more "settling time" between each new minor version release.

That being said we have to make a new minor version release beacuse of #80 so let's include this PR which also contains breaking changes in v0.2.0. @astro could you update the docs?

therealprof · 2018-05-12T14:45:30Z

that low / high are referring to the electrical output of the pin.

I disagree. We can not make any assumption of the electrical state of a pin, only the logical state. Even if you drive it low on a push/pull pin there might be a stronger source keeping it high and vice versa.

japaric · 2018-05-12T18:27:33Z

@therealprof point taken

I'll merge this PR as it is and adjust the docs locally before releasing v0.2.0. Thanks for the PR, @astro!

bors r+

bors · 2018-05-12T18:27:35Z

👎 Rejected by code reviews

japaric · 2018-05-12T18:28:11Z

bors r+

bors · 2018-05-12T18:28:12Z

👎 Rejected by code reviews

docs will be updated in a follow up commit

japaric · 2018-05-12T18:29:05Z

bors r+

72: Move reading functions from OutputPin trait to StatefulOutputPin r=japaric a=astro As it has come up in #29, here is a proposal for removing the burden of caching the state on implementations that cannot read it from an output pin. Co-authored-by: Astro <[email protected]>

bors · 2018-05-12T18:31:15Z

Build succeeded

continuous-integration/travis-ci/push

hannobraun · 2018-05-15T09:38:40Z

@japaric

just for the record: releasing a new minor version doesn't "just breaks implementations"; it fragments the ecosystem.

Good point, thanks for the explanation. While that doesn't change my stance about breaking changes in general, it's good to keep in mind that we need to release them strategically.

Remove reading functions from OutputPin trait

97fac76

therealprof reviewed Apr 7, 2018

View reviewed changes

therealprof requested review from hannobraun and japaric April 7, 2018 22:26

Add StatefulOutputPin to read and toggle the state of OutputPin

3f61ef2

astro changed the title ~~Remove reading functions from OutputPin trait~~ Move reading functions from OutputPin trait to StatefulOutputPin Apr 7, 2018

astro mentioned this pull request Apr 8, 2018

4th digital::IoPin proposal from #29 #73

Closed

hannobraun approved these changes Apr 11, 2018

View reviewed changes

therealprof approved these changes Apr 11, 2018

View reviewed changes

tib888 approved these changes Apr 11, 2018

View reviewed changes

ryankurte reviewed Apr 12, 2018

View reviewed changes

astro force-pushed the rm-read-from-outputpin branch from 99b9937 to 81f3408 Compare April 12, 2018 15:38

separate OutputPin.toggle() into ToggleableOutputPin, provide default…

ac965a3

… impl for StatefulOutputPin

astro force-pushed the rm-read-from-outputpin branch from 81f3408 to ac965a3 Compare April 12, 2018 15:39

austinglaser reviewed Apr 12, 2018

View reviewed changes

astro force-pushed the rm-read-from-outputpin branch 3 times, most recently from b47006d to 2fd1a3a Compare April 12, 2018 17:37

move ToggleableOutputPin default impl to opt-in trait + doc

6549d73

astro force-pushed the rm-read-from-outputpin branch from 2fd1a3a to 6549d73 Compare April 12, 2018 17:40

therealprof previously requested changes Apr 30, 2018

View reviewed changes

therealprof mentioned this pull request May 11, 2018

replace uses of ! with Void #80

Merged

japaric approved these changes May 12, 2018

View reviewed changes

bors bot merged commit 6549d73 into rust-embedded:master May 12, 2018

japaric mentioned this pull request May 14, 2018

Major faux I2C interface incompatibility between 0.1.2 and 0.2.0 #81

Closed

tib8 mentioned this pull request Jul 9, 2018

Re-Introduce into_open_drain_output japaric/stm32f103xx-hal#51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move reading functions from OutputPin trait to StatefulOutputPin #72

Move reading functions from OutputPin trait to StatefulOutputPin #72

astro commented Apr 7, 2018 •

edited

Loading

therealprof left a comment

astro commented Apr 7, 2018 •

edited

Loading

astro commented Apr 7, 2018

therealprof commented Apr 7, 2018

hannobraun left a comment

therealprof left a comment •

edited

Loading

hannobraun commented Apr 11, 2018

therealprof commented Apr 11, 2018

austinbes commented Apr 11, 2018

ryankurte Apr 12, 2018

therealprof Apr 12, 2018

astro Apr 12, 2018

astro Apr 12, 2018

astro commented Apr 12, 2018

austinglaser Apr 12, 2018

astro Apr 12, 2018

astro commented Apr 29, 2018

therealprof Apr 30, 2018

therealprof Apr 30, 2018

japaric commented May 11, 2018

therealprof commented May 12, 2018

japaric commented May 12, 2018

bors bot commented May 12, 2018

japaric commented May 12, 2018

bors bot commented May 12, 2018

japaric commented May 12, 2018

bors bot commented May 12, 2018

hannobraun commented May 15, 2018

Move reading functions from OutputPin trait to StatefulOutputPin #72

Move reading functions from OutputPin trait to StatefulOutputPin #72

Conversation

astro commented Apr 7, 2018 • edited Loading

therealprof left a comment

Choose a reason for hiding this comment

astro commented Apr 7, 2018 • edited Loading

astro commented Apr 7, 2018

therealprof commented Apr 7, 2018

hannobraun left a comment

Choose a reason for hiding this comment

therealprof left a comment • edited Loading

Choose a reason for hiding this comment

hannobraun commented Apr 11, 2018

therealprof commented Apr 11, 2018

austinbes commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astro commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astro commented Apr 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japaric commented May 11, 2018

therealprof commented May 12, 2018

japaric commented May 12, 2018

bors bot commented May 12, 2018

japaric commented May 12, 2018

bors bot commented May 12, 2018

japaric commented May 12, 2018

bors bot commented May 12, 2018

Build succeeded

hannobraun commented May 15, 2018

astro commented Apr 7, 2018 •

edited

Loading

astro commented Apr 7, 2018 •

edited

Loading

therealprof left a comment •

edited

Loading