Make digital pin traits fallible #97

eldruin · 2018-09-28T20:08:06Z

Following the discussion in #95, I have made these changes:

Make InputPin trait methods fallible.
Add FallibleOutputPin trait with fallible versions of the OutputPin methods.
Make ToggeableOutputPin trait method fallible.

Now, should StatefulOutputPin methods also be fallible? I think so, but I was not able to get it to work:
In the implementation of ToggeableOutputPin for the software-driven toggle (FallibleOutputPin + StatefulOutputPin) there are now two Error types and I do not know how to solve this correctly.

You can see precisely what I mean by applying the patch below on top of this branch. If you do so,
the compiler will rightfully complain that the Error associated type in bounds of P is ambiguous.

What do you think?

diff --git a/src/digital.rs b/src/digital.rs
index 45a72bc..7d6f4f5 100644
--- a/src/digital.rs
+++ b/src/digital.rs
@@ -39,15 +39,18 @@ pub trait FallibleOutputPin {
 /// *This trait is available if embedded-hal is built with the `"unproven"` feature.*
 #[cfg(feature = "unproven")]
 pub trait StatefulOutputPin {
+    /// Error type
+    type Error;
+
     /// Is the pin in drive high mode?
     ///
     /// *NOTE* this does *not* read the electrical state of the pin
-    fn is_set_high(&self) -> bool;
+    fn is_set_high(&self) -> Result<bool, Self::Error>;
 
     /// Is the pin in drive low mode?
     ///
     /// *NOTE* this does *not* read the electrical state of the pin
-    fn is_set_low(&self) -> bool;
+    fn is_set_low(&self) -> Result<bool, Self::Error>;
 }
 
 /// Output pin that can be toggled
@@ -93,11 +96,13 @@ pub trait ToggleableOutputPin {
 /// }
 ///
 /// impl StatefulOutputPin for MyPin {
-///    fn is_set_low(&self) -> bool {
-///        !self.state
+///    type Error = void::Void;
+///
+///    fn is_set_low(&self) -> Result<bool, Self::Error> {
+///        Ok(!self.state)
 ///    }
-///    fn is_set_high(&self) -> bool {
-///        self.state
+///    fn is_set_high(&self) -> Result<bool, Self::Error> {
+///        Ok(self.state)
 ///    }
 /// }
 ///
@@ -106,9 +111,9 @@ pub trait ToggleableOutputPin {
 ///
 /// let mut pin = MyPin { state: false };
 /// pin.toggle().unwrap();
-/// assert!(pin.is_set_high());
+/// assert!(pin.is_set_high().unwrap());
 /// pin.toggle().unwrap();
-/// assert!(pin.is_set_low());
+/// assert!(pin.is_set_low().unwrap());
 /// ```
 #[cfg(feature = "unproven")]
 pub mod toggleable {

hannobraun · 2018-09-28T21:48:07Z

@eldruin

You can see precisely what I mean by applying the patch below on top of this branch. If you do so,
the compiler will rightfully complain that the Error associated type in bounds of P is ambiguous.

What do you think?

I have two ideas. First, one that I'm pretty sure will work, but might be too heavy-handed. Define the following enum:

pub enum ToggleError<OP, SOP> {
    OutputPin(OP),
    StatefulOutputPin(SOP),
}

Use that as the error type for the default implementation, like this:

type Error = ToggleError<<P as FallibleOutputPin>::Error, <P as StatefulOutputPin>::Error>;

Then update the implementation of toggle like this:

fn toggle(&mut self) -> Result<(), Self::Error>{
    if self.is_set_low().map_err(|error| ToggleError::StatefulOutputPin(error))? {
        self.set_high().map_err(|error| ToggleError::FallibleOutputPin(error))?;
    } else {
        // ...

As I said, pretty sure that will work, but it might be too heavy-handed.

My second idea is more elegant, but might not work. Simply make StatefulOutputPin inherit from FallibleOutputPin and don't give it its own error:

pub trait StatefulOutputPin : FallibleOutputPin {
    fn is_set_high(&self) -> Result<bool, Self::Error>;

    fn is_set_low(&self) -> Result<bool, Self::Error>;
}

Problem solved, because they both have the same error now. The big question is, are there valid use cases where both traits need different errors?

Bonus: A third idea that's not implementable right now. Give StatefulOutputPin its own error, but only provide the default implementation, if they both have the same error type. This can one day be achieved by adding the following where clause:

where <P as FallibleOutputPin>::Error == <P as StatefulOutputPin>::Error

This is called an equality constraint, and it's not implemented yet.

hannobraun

This looks good to me, thank you @eldruin!

There are two reasons I'm not approving this right now:

The outstanding problem with the ToggleableOutputPin default implementation.
@ryankurte suggested a different transition plan.

This fallible version is only available if both "unproven" and "use-fallible-digital-traits" features are enabled.

eldruin · 2018-09-29T06:32:58Z

I also think the use-fallible-digital-traits idea is great, so I implemented it already.
For the duplicated Error type, inheriting StatefulOutputPin from OutputPin actually works :)
I think this version is better than the ToggleError enum type because:

As previously stated, ToggleError would be pretty heavy-handed.
From a conceptual point of view, a StatefulOutputPin does not make much sense without being able to set the pin high or low, so inheriting from OutputPin is ok.
If cases for separate Error types arise, I think we can avoid breakage by breaking the inheritance and adding a second Error type if the equality constraints have landed by then.

Thank you both @ryankurte and @hannobraun!

hannobraun · 2018-09-29T08:16:21Z

This is looking good! From my perspective, here's what's left to to get this merged:

Fix the Travis failure (missing import, should be simple).
Wait for feedback from @ryankurte.
Squash this into fewer commits (I'd prefer one commit changing the unproven trait, another to add the fallible versions; but squashing everything into one commit is fine, too).

hannobraun · 2018-09-29T08:19:00Z

src/digital.rs

 /// assert!(pin.is_set_low());
+/// #[cfg(feature = "use-fallible-digital-traits")]
+/// assert!(pin.is_set_low().unwrap());


Hmm, I'm wondering, if we should simplify the example. I mean, we want people to use the new fallible versions, so why even show the infallible versions here? Thoughts?

I agree that the docs should prioritise the fallible version, though maybe we could keep the infallible documentation until the opt-out release?

I moved the fallible versions up. However, the example still looks quite cluttered with #cfg... to me. Any ideas?

It is a bit cluttered, maybe grouping each example would help?
I'm ok with merging this either way.

I found no way to group the statements so that I can have fewer #cfgs. Do you have an idea?

ryankurte

Thanks for the PR! Looks excellent. I have commented with a few changes, please let me know if you disagree with any of them.

The only other thing that would be neat is to test / demonstrate that it all works by updating a driver crate, but we can either do that here or once it's landed before we release.

ryankurte · 2018-09-29T23:29:57Z

CHANGELOG.md

@@ -7,6 +7,22 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]

+### Added
+
+- A `FallibleOutputPin` trait has been added. It is a version of the `OutputPin` trait


I think this needs to be updated to match the feature based approach, and we probably need to document that this release is opt-in and the next will be opt-out.

src/digital.rs

ryankurte · 2018-09-29T23:35:10Z

src/digital.rs

 /// assert!(pin.is_set_low());
+/// #[cfg(feature = "use-fallible-digital-traits")]
+/// assert!(pin.is_set_low().unwrap());


I agree that the docs should prioritise the fallible version, though maybe we could keep the infallible documentation until the opt-out release?

eldruin · 2018-09-30T09:39:44Z

Ok here is a new version. Please review the points raised and let me know if you want further changes.
To summarize, I would say the definitely open points are:

Squash into fewer commits (will do at the end)
ToggleableOutputPin example code
Demonstrate on other crate (I can do this with my pcf857x I/O expander driver)

Thoughts?

eldruin · 2018-09-30T12:26:03Z

Ok, I implemented the new traits on my pcf857x I/O expander driver on this branch.
It builds fine but the embedded-hal-mock trait seems not compatible with the embedded-hal master so the tests cannot be run :/ I opened an issue for this

ryankurte · 2018-09-30T21:28:41Z

CHANGELOG.md

+- Fallible versions of the `OutputPin` and `StatefulOutputPin` traits have been added.
+  These are versions where the methods return a `Result` type as setting an output pin
+  could potentially fail.
+  The current trait versions are now marked as deprecated. The fallible version


Should we be marking the new traits with unproven too? (I'm not sure if there is a precedent, seems ok either way)

StatefulOutputPin is already marked as unproven and its interface will not change if not activating use-fallible-digital-traits. The only fallible trait one not behind the unproven feature is the fallible OutputPin.

ryankurte · 2018-09-30T21:40:07Z

Thanks for the excellent work @eldruin! I appreciate your effort in working through these changes with us ^_^

The only niggling worry I have is whether we should be feature gating the unproven traits as well. While there is no guarantee that they don't change, the reality is everyone using it probably builds with the unproven feature on (I know I do) and is going to get hit by the API breakage, somewhat undermining our plan for a smooth transition.

I think my preference would be to take the same feature-flag migration approach with the unproven traits, so all the digital traits are feature-flagged one way or the other and marked unproven again. Any thoughts on this @hannobraun?

eldruin · 2018-10-01T08:12:49Z

Additional info: I took care of changing the code so that it is possible to activate the unproven and use-fallible-digital-traits features independently of each other.
If activating unproven but not use-fallible-digital-traits, only InputPin and ToggleableOutputPin have changed to be fallible. I could also re-add their infallible version for this case.

hannobraun · 2018-10-01T12:34:25Z

@ryankurte

The only niggling worry I have is whether we should be feature gating the unproven traits as well. While there is no guarantee that they don't change, the reality is everyone using it probably builds with the unproven feature on (I know I do) and is going to get hit by the API breakage, somewhat undermining our plan for a smooth transition.

I think my preference would be to take the same feature-flag migration approach with the unproven traits, so all the digital traits are feature-flagged one way or the other and marked unproven again. Any thoughts on this @hannobraun?

Well, if you think it should be done, and if @eldruin is willing to do it, then I'm fine with that. But honestly, I think it's unnecessary. I we can't change unproven traits, then what is the point of having the flag?

New thought: Should we (temporarily) test the new feature flags in the Travis build, or is it enough to test them locally before merging?

ryankurte · 2018-10-01T21:52:59Z

Whoops, pushed the wrong button sorry.

I think it's unnecessary. I we can't change unproven traits, then what is the point of having the flag?

I agree in theory but am not really sure the unproven flag works the way it was intended, maybe when the bulk of the library has stabilised so it's not so compulsory it'll fit better, maybe it'd be better with more granularity (a flag per trait), but that is probably a discussion for another place.

If we don't do it, we are causing breakage for the people who do use those traits, but maybe they'll just opt in to the new traits and fix everything then anyway. I can see pros and cons of both and am happy merging either, so, @eldruin up to you.

(Regardless of the direction, given the widespread ramifications of changes to this project it'd be neat to have feedback on the release process from users? Maybe we could have a forum post or an issue for release feedback / experiences.)

Should we (temporarily) test the new feature flags in the Travis build

Ahh, yeah that should probably go into the build matrix, to be changed when we change and eventually remove the flags.

hannobraun · 2018-10-02T12:58:39Z

@ryankurte

I agree in theory but am not really sure the unproven flag works the way it was intended, maybe when the bulk of the library has stabilised so it's not so compulsory it'll fit better, maybe it'd be better with more granularity (a flag per trait), but that is probably a discussion for another place.

I agree. I've asked myself a few times what the point of the flag is, given the early state that this library still is in. More granular flags sound like a good idea, and would fit well with what we're doing in this pull request.

(Regardless of the direction, given the widespread ramifications of changes to this project it'd be neat to have feedback on the release process from users? Maybe we could have a forum post or an issue for release feedback / experiences.)

I agree. We have an embedded category in u.r-l.o now, which seems like a good place for this kind of thing.

Ahh, yeah that should probably go into the build matrix, to be changed when we change and eventually remove the flags.

Okay, so I guess this updates the todo list for this pull request as follows:

Do or don't use feature flags for the unproven traits too, according to @eldruin's decision.
Add new features to Travis build matrix.
Open issues to discuss the future use of the unproven feature and release procedures (forum post for feedback).

therealprof · 2018-10-02T13:13:08Z

@hannobraun The main problem I see with unproven is that despite the availability of proven implementations we're not changing the flags. I think this flag would work a treat if we would actually remove the flags in a timely manner.

hannobraun · 2018-10-03T12:36:49Z

@therealprof

The main problem I see with unproven is that despite the availability of proven implementations we're not changing the flags. I think this flag would work a treat if we would actually remove the flags in a timely manner.

Yes, that certainly doesn't help.

eldruin · 2018-10-04T08:49:29Z

Ok, just to let you know, I am on vacation and will have a look at this next week :)

…recated - Add general documentation about transition process to digital module - Update CHANGELOG

eldruin · 2018-10-12T05:27:48Z

Ok I pushed my last local changes but interesting arguments were brought up in #100 so let's settle that first.

hannobraun · 2018-10-12T11:23:26Z

@eldruin Agreed. Best to wait for the result of that discussion.

eldruin · 2018-10-15T06:35:22Z

Ok, there seems to be some consensus in #100 that this should be implemented as a clean break. I will prepare a new version soon.

eldruin · 2018-10-16T18:55:45Z

Ok I did not want to squash the commits in this branch in case github makes a mess of the discussion in this PR so I opened #105 with everything in one commit.
I will close this in favor of that one but feel free to reopen if appropriate.

eldruin added 7 commits September 28, 2018 19:27

Make InputPin trait methods fallible

227468b

Add documentation for InputPin Error type

611ec43

Add FallibleOutputPin trait

2e90b03

Make ToggeableOutputPin fallible

7c2167b

Fix unwrapping toggle result in example

156c861

Fix spacing

34cd2b8

Add ToggleableOutputPin change to changelog

db447ce

eldruin requested review from ilya-epifanov, thejpster and a team as code owners September 28, 2018 20:08

hannobraun mentioned this pull request Sep 28, 2018

digital operations do not allow error propagation #95

Closed

hannobraun reviewed Sep 28, 2018

View reviewed changes

eldruin added 2 commits September 29, 2018 07:46

Put fallible OutputPin behind use-fallible-digital-traits feature

e959f02

Make StatefulOutputPin inherit from OutputPin to inherit its error type

f873a73

This fallible version is only available if both "unproven" and "use-fallible-digital-traits" features are enabled.

hannobraun reviewed Sep 29, 2018

View reviewed changes

ryankurte requested changes Sep 29, 2018

View reviewed changes

eldruin added 4 commits September 30, 2018 11:08

Mark infallible digital traits as deprecated

2a1d72e

Update changelog

b681162

Add missing import

2bad26e

Prioritize fallible version

ea14b73

ryankurte reviewed Sep 30, 2018

View reviewed changes

ryankurte mentioned this pull request Oct 2, 2018

[Discussion] the future of Unproven traits and practices for new and breaking changes #99

Closed

ryankurte mentioned this pull request Oct 4, 2018

[RFC] Managing breaking changes to existing traits #100

Closed

hannobraun added the S-waiting-on-author Author needs to make changes to address reviewer comments label Oct 10, 2018

This was referenced Oct 10, 2018

Added input pin impl (requires unproven hal trait) rust-embedded/linux-embedded-hal#11

Merged

Remove InputPin unproven flag per #41 #102

Closed

eldruin added 2 commits October 12, 2018 07:23

Split implementations to leave infallible traits untouched though dep…

623aaf8

…recated - Add general documentation about transition process to digital module - Update CHANGELOG

Add use-fallible-digital-traits to travis matrix

2ab76fe

hannobraun added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Author needs to make changes to address reviewer comments labels Oct 12, 2018

hannobraun added S-waiting-on-author Author needs to make changes to address reviewer comments and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 16, 2018

eldruin added 2 commits October 16, 2018 19:27

Implement fallible digital traits as a clean break

6224d88

Remove builds with use-fallible-digital-traits feature

3422af2

eldruin mentioned this pull request Oct 16, 2018

Make digital traits fallible #105

Closed

eldruin closed this Oct 16, 2018

ryankurte removed the S-waiting-on-author Author needs to make changes to address reviewer comments label Oct 16, 2018

idubrov mentioned this pull request Nov 6, 2018

Thoughts on Flash API? #28

Closed

eldruin mentioned this pull request Mar 14, 2020

[towards 1.0]: Fallible proven traits #192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make digital pin traits fallible #97

Make digital pin traits fallible #97

eldruin commented Sep 28, 2018

hannobraun commented Sep 28, 2018

hannobraun left a comment

eldruin commented Sep 29, 2018

hannobraun commented Sep 29, 2018

hannobraun Sep 29, 2018

ryankurte Sep 29, 2018

eldruin Sep 30, 2018

ryankurte Sep 30, 2018

eldruin Oct 1, 2018

ryankurte left a comment

ryankurte Sep 29, 2018

eldruin Sep 30, 2018

ryankurte Sep 29, 2018

eldruin commented Sep 30, 2018 •

edited

Loading

eldruin commented Sep 30, 2018

ryankurte Sep 30, 2018

eldruin Oct 1, 2018

ryankurte commented Sep 30, 2018

eldruin commented Oct 1, 2018

hannobraun commented Oct 1, 2018

ryankurte commented Oct 1, 2018

hannobraun commented Oct 2, 2018

therealprof commented Oct 2, 2018

hannobraun commented Oct 3, 2018

eldruin commented Oct 4, 2018

eldruin commented Oct 12, 2018

hannobraun commented Oct 12, 2018

eldruin commented Oct 15, 2018

eldruin commented Oct 16, 2018

Make digital pin traits fallible #97

Make digital pin traits fallible #97

Conversation

eldruin commented Sep 28, 2018

hannobraun commented Sep 28, 2018

hannobraun left a comment

Choose a reason for hiding this comment

eldruin commented Sep 29, 2018

hannobraun commented Sep 29, 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

Choose a reason for hiding this comment

ryankurte left a comment

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

eldruin commented Sep 30, 2018 • edited Loading

eldruin commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankurte commented Sep 30, 2018

eldruin commented Oct 1, 2018

hannobraun commented Oct 1, 2018

ryankurte commented Oct 1, 2018

hannobraun commented Oct 2, 2018

therealprof commented Oct 2, 2018

hannobraun commented Oct 3, 2018

eldruin commented Oct 4, 2018

eldruin commented Oct 12, 2018

hannobraun commented Oct 12, 2018

eldruin commented Oct 15, 2018

eldruin commented Oct 16, 2018

eldruin commented Sep 30, 2018 •

edited

Loading