digital operations do not allow error propagation

[eldruin]

Currently operations defined in the digital module traits do not allow error propagation.

For example, in an I2C I/O expander driver I am writing I want to provide access to the individual pins as a bunch of Px structs implementing OutputPin and InputPin. Setting a pin or reading it involves I2C communication, which could fail.
At the moment I just panic if there was a problem for either reading or writing.
The same would happen with the other traits: StatefulOutputPin and ToggleableOutputPin

Another example from @caemor:

The only problem I found with this api since using it is that errors can't get propagated any further and need to be converted to a default value.

E.g. the linux sys_fs can receive an file error while trying to read the value (https://github.com/rust-embedded/rust-sysfs-gpio/blob/master/src/lib.rs#L337) and this can't be handled by a simple bool.

Discussion started here

[ryankurte]

Seems like a good candidate for some Try versions of the digital traits?

The downside is this would result in two possible digital IO interfaces.
The upside is error handling can be explicit where it needs to, and we can smooth that over with an automatic implementation from infallible to try, and a helper for the other direction.

[eldruin]

@ryankurte Sounds good to me :)
If you want, I can prepare a PR and then we can discuss the concrete proposal.

[hannobraun]

@ryankurte @eldruin

Seems like a good candidate for some Try versions of the digital traits?

I may be missing something, but can't we just add an associated error type to the existing traits, and set those to Void for infallible implementations? This would be an inconvenience in the short term, but once the never type (!) is stabilized, the compiler should be smart enough to figure out that these Resultss don't need to be handled. I think I'd prefer that to another set of traits.

(Update: Part of my comment is factually wrong.)

[eldruin]

@hannobraun

I may be missing something, but can't we just add an associated error type to the existing traits, and set those to Void for infallible implementations? This would be an inconvenience in the short term, but once the never type (!) is stabilized, the compiler should be smart enough to figure out that these Resultss don't need to be handled. I think I'd prefer that to another set of traits.

Hmm, I think maybe that could work for OutputPin methods like fn set_high(&mut self); but could this be implemented for InputPin's fn is_high(&self) -> bool;, for example, where there is a return value already?

[hannobraun]

@eldruin

pub trait InputPin {
    type Error;
    fn is_high(&self) -> Result<bool, Self::Error>;
}

[hannobraun]

I just noticed that my previous comment about the stabilization of ! making a difference here is mostly wrong. It applies to code that knows which embedded-hal implementation it is using, but it won't make a difference for drivers, embedded-hal's primary consumer.

[ryankurte]

@hannobraun that'd also be a good approach, I do like the idea of only one interface. My concerns about that are that we'll break everything existing, which is ok but also not insignificant friction to users, and what the overhead is / whether it'll be optimised out for the infallible case.

That said, maybe that's a question of optimiser behaviour, and the more I think about it the more I like the idea of pin errors always bubbling through in a sensible way.

[hannobraun]

@ryankurte Yeah, the breakage is a legitimate concern. I don't think it can be avoided though. I've come to the conclusion that keeping the infallible traits would just be a bad idea.

An implementation that has to deal with errors has three options regarding what to do about the infallible traits:

1. Implement them and panic on errors. Bad, because drivers will not expect the panics.
2. Implement them and ignore errors. Worse, because now we have problems that are just as unpredictable but less obvious.
3. Not implement them. The only sane choice, but it will mean that drivers that are built on the infallible traits won't run on that platform.

Given those options, we can't recommend that drivers use the infallible traits, as it will either cause them problems or restrict them in their portability. But since they're simpler than the fallible ones, many drivers probably will use them.

I think that leaves us with two viable options:

1. Add the error type and make people deal with the breakage and additional complexity.
2. Decide that embedded-hal just doesn't support cases where pin operations could fail.

I prefer option 1.

Aside: An embedded-hal user that doesn't want to deal with errors can restrict itself to infallible implementations at compile-time using something like where P: InputPin<Error=!>.

[ryankurte]

True, I'm onboard with moving everything to be fallible. Maybe we could open another issue wrt. communicating breaking changes and releases for embedded-hal downstream projects?

@eldruin if you're still interested a PR updating the digital traits to use fallible types would be excellent! You can look at the blocking SPI traits for examples, afaik anything which returns data should return Result<Whatever, Self::Error> and without a return should return Result<(), Self::Error>.

[eldruin]

Ok, I was mostly concerned about the breakage but I see your points.
I will prepare a PR soon.

[hannobraun]

@eldruin

I will prepare a PR soon.

Thank you!

Maybe we should alleviate the breakage by doing the following:

1. Only replace unproven traits outright.
2. For traits that are not unproven, don't replace them but add new traits (with names like InputPin2).
3. Once the new traits have proven themselves, mark the old traits as #[deprecated].
4. Wait for the next breaking release. After that release, replace the old, deprecated traits with the new ones.

That way, there's one full release cycle where the old traits are deprecated and people can migrate. With the next breaking release after that, all they'll have to do is update the name of the traits.

Thoughts?

[ryankurte]

Hmm, I like the idea of alleviating the issue, it but that seems to kindof double the breakage in code changes required? Find and replace is easy enough, but, slightly annoying to have to go through the code to fix things twice.

I wonder if for the proven traits we could do it with feature flags to switch between them.
So the next release we mark the existing ones deprecated to let users no and have an opt in feature use-fallible-digital-traits that exposes the new ones so they can immediately upgrade if they choose.
The following release we would swap that to default and have a feature use-infallible-digital-traits to opt to regress should they need to.
That way the names could be the same, and it's just a question of consumers removing the first feature when it's replaced, and our removing the second feature when it's unneeded.

That said, we are 0.x, people can choose when to update HAL versions, and by doing either we're causing fragmentation. If we could poll for broader consensus from the wg / some heavy users maybe it'd be simpler for everyone just to replace it all at once?

[hannobraun]

You're right. I like your solution better. There's an open pull request now: #97

I think we should direct further discussion there.

[eldruin]

This has been solved in #108

[ryankurte]

Hey, thanks for all the effort! Might just keep this open until we've got the compatibility shim in place and a release done.

[ryankurte]

woooo!