[RFC] digital::v3 interface

[Disasm]

Rendered

See also: rust-embedded/embedded-hal#135 (comment)

[nickray]

TL;DR: How about this: https://gist.github.com/rust-play/c7a1d6681a557a8172f7d05d7321f879?

As context for what follows, I think having versions (at all) for digital pin traits was a mistake in the first place, for several reasons:

• as a newcomer, with the usual goal of blinking an LED, my first interaction with the HAL should not be wondering what the API version du jour is. (I understand the intent was to be backwards compatible)
• the (very commendable!) stated goal of the HAL is that: "People that want higher level abstraction should prefer to use this HAL rather than re-implement register manipulation code." For me at least, this means it should "make the easy things easy, and the hard things possible".
• Enforcing fallible pins on the majority of people who will never meet such a thing is not just an implementation chore, but also gives the impression that Rust makes the easy things difficult, for - from a probably majority use case viewpoint - no good reason (an opinion we should probably try our best to avoid in general).
• expanding on this, as few return values as possible should be Results, as "Result fatigue" will lead people in practice to lazily use .ok() and .unwrap(). (Sometimes wonder if there could be a hierarchy of result importance, as they're clearly not all equally relevant...)
• as a newcomer, the fact that traits need to be "used" is quite confusing generally, and having use xxx::* to provide the various vX <--> vY compatibility layers as done in v2 and suggested for v3 does not help. (I consider such silent operations from afar an anti-pattern anyway)

Therefore, I'd prefer a way forward that reverts to no versioning of the pin traits. (Alternatively, if this turns out to be impossible, separate traits for the non-majority use case). Unless we want to by default version all traits (~Rust editions) and look like... Kubernetes, there should be a way (or at least plan how) to get rid of API cruft before Rust even becomes the box office success we all hope for :)

I have sketched an alternative approach to this RFC in the interactive https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c7a1d6681a557a8172f7d05d7321f879.

Briefly, the idea is that a "maybe fallible" trait would expose both the fallible and infallible methods in its interface (with the try_ prefix as suggested, in line with From/TryFrom etc.), where

• the infallible version is marked as unimplemented! (alternative: not marked; each fallible implementor would have to mark. alternative 2: an impossible! macro, or other bikeshed name)
• the fallible version has a default implementation in terms of the infallible version
• there are one or more associated Error types

So:

• infallible abstraction layers simply set Error = Infallible and implement the infallible methods. Existing such implementations would need to be updated only by setting this Error (associated type defaults are not stable).
• fallible abstraction layers (HALs or BSPs with internal pins and e.g. GPIO expanders) define Error appropriately, and implement the fallible versions.
• drivers that can or want to only handle infallible implementations only use the infallible methods, and specify Error = Infallible assumed. The latter is optional (so not all drivers need updating).
• drivers that handle fallible implementations do so \o/ To be fair, AFAIU all they can really do is pass errors through to the application using them.

In the future, Infallible would be replaced by !, and possibly set as the default associated error type.

I don't quite like that this suggestion mixes two interfaces in one. Unfortunately it's not possible to my knowledge to "blanket" implement one trait in terms of another (which would be much nicer: hide the infallible interface from those that don't need it).

I have not thought about how to transition to this approach, I think it's more important to first think about where we want to go with these traits, rather than how to get there.

[Disasm]

I like your approach, but I see some cons:

• One can forget to implement one of the trait methods (a pair of methods), this will be compiled without problems and will panic upon call.
• It's not clear how to implement infallible methods for fallible implementations. The default "panic" implementation is not ok: maybe you want to call the fallible method first and panic on error. This way infallible methods should be implemented for every fallible type and there should be a way to implement them "correctly" and not just in a way one feels "right".
• It's not clear how to move to this model without a lot of changes everywhere. I believe that we can't forget v1 and v2 traits in future versions just because a lot of code based on them.

[nickray]

* One can forget to implement one of the trait methods (a pair of methods), this will be compiled without problems and will panic upon call.

True. I think an approach like panic-never is appropriate regarding panics. Also, actual forgetting is a bug that needs fixing :)

* It's not clear how to implement infallible methods for fallible implementations. The default "panic" implementation is not ok: maybe you want to call the fallible method first and panic on error. This way infallible methods should be implemented for every fallible type and there should be a way to implement them "correctly" and not just in a way one feels "right".

I don't quite follow. I think for a driver, (in this alternative approach) it's only OK to call either the fallible xor the infallible methods. There's not really a way for the infallible method to be correctly implemented without either panics or hiding errors for fallible pins, is there? Calling the fallible method and then panicking is not OK - panics are for logic errors, not environment errors. As sketched, for a driver to exclude this case, it would demand Error to be Infallible (possibly excluding infallible implementations that have upgraded "silently" and not updated - but this can be PR'd in independently).

* It's not clear how to move to this model without a lot of changes everywhere. I believe that we can't forget v1 and v2 traits in future versions just because a lot of code based on them.

As stated, I feel quite strongly (negatively) about a 3-version interface remaining in these traits "for evermore", for incidental complexity, cruft and consistency (why are some traits versioned and others not) reasons. So I'd like to see some discussion about the actual other options whatever the final decision. In particular, since there will be other traits that, in reality, come in versions that may return values or results (with minority case the fallible version), so solving this kind of problem (without result-ing everything) seems useful. "Not clear" does not mean impossible :) I'd be fine-ish with crufty compatibility layers (v* -> "clean"), if there's such a core that's "clean".

[ryankurte]

hey thanks for your efforts! don't take this as anything but my personal opinion, but, i am not a huge fan of either approach.

the v1/v2 thing is annoying but exists as a shim because people weren't happy with making a breaking change to the hal, and because we didn't want a future with N different incompatible fundamental traits. i like the ideal of thinking about where we want to be (though in this case disagree with the desired outcome), but, the how we get there is the tricky part.

we've definitely learned some things about traits with duplicate names, but again, this was a compromise to avoid a future where we had something like v2_get_pin, and i still am not sure of another way of achieving this.

some thoughts:

• afaik the only place this causes major friction is with hal maintainers (who have to decide which one to use, though tbqh they could stay on v1 indefinitely), in most other cases (driver and app developers) people can use the v2 traits and have the casting magically work
  • the case where this is not true is when you're using a v1 driver on a v2 implementation, which requires explicit .unwrap()'s because you now have to handle errors somehow
  • anecdotally, i've written 6 or so drivers and as many applications using a variety of v1/v2 hals over the last 8 or so months with no issue. that's not saying issues / difficulties don't exist, but, at least that it can work.
• i think it would be a mistake to write off fallible cases, rust is all about explicit error handling and it's super annoying when you can't pass these through in a useful way
  • remembering that embedded also covers linux devices in which case everything is fallible
  • the corollary to this is that it is slightly more difficult for driver authors create and bubble errors (Correct usage of v2 fallible pin traits embedded-hal#135), but, i would suggest we mitigate this with documentation and examples of how to structure drivers and handle errors.

my preferred alternative to this would be to work towards removing the v1 traits and only having fallible versions (with Error=!where appropriate), so we end up with one set of traits and well handled errors.

as far as i remember the plan from there was to eventually drop v1 and semver-trick us to backwards compatibility, i think the process for this would be something like:

• make a breaking release switching from v1 to v2 as the default
• survey published drivers (because we can't usefully look at unpublished) crates and make a concerted effort to pull them all up to v2
• eventually drop v1 completely (with the semver-trick to previous versions if possible)

[thejpster]

Link to original issue: rust-embedded/embedded-hal#95

Link to original PR: rust-embedded/embedded-hal#108

[Disasm]

@ryankurte Thank you for the feedback!
I believe that we need a future with 2 different compatible traits. Compatible in terms of infallible<->fallible conversions.
Maybe we need to add v3 interface with both fallible and infallible traits and deprecate both v1 and v2.

As for the error handling, fallible traits increase the code complexity for infallible cases.
With only fallible traits present, how one should choose where to handle errors properly and where not? You can force Pin<Error=Infallible> in the driver interface, but the same thing in firmware code will increase code bloat even more. Infallible traits solve this problem.

So how about digital::v3 with both fallible and infallible traits and this "move to the only API" direction?

[TeXitoi]

The state of the art is From and TryFrom, i.e. a infallible trait, and a faillible trait, and default impl of the fallible for the type that impl the infallible.

Thus, this proposition seems correct in philosophy to me.

[TeXitoi]

I'd be more to have a TryT for each trait, that are fn try_fn() -> Result<T, Error> for each trait, and just have that outside of vX.

With the instruction to device drivers to implement on the fallible trait without unwrap, or, if really not possible, on the infallible trait.

For hal implementers, only implement infallible if its really infallible.

[Disasm]

@TeXitoi The idea is to have both of them in v3 and then, after moving the ecosystem, make v3 the only interface without "v3" name in path. I'm not sure about TryT naming, though. FallibleT sounds better for me.

[TeXitoi]

pub trait TryOutputPin {
    type Error;
    fn try_set_low(&mut self) -> Result<(), Self::Error>;
    fn try_set_high(&mut self) -> Result<(), Self::Error>;
    fn into_panicing_infallible(self) -> PanicInfallibleOutputPin<Self> where Self: Sized {
        PanicInfallibleOutputPin(self)
    }
}

pub trait OutputPin {
    fn set_low(&mut self);
    fn set_high(&mut self);
}
impl<T: OutputPin> TryOutputPin for T {
    type Error = core::convert::Infallible;
     fn try_set_low(&mut self) -> Result<(), Self::Error> {
        Ok(self.set_low())
    }
    fn try_set_high(&mut self) -> Result<(), Self::Error> {
        Ok(self.set_high())
    }
}

pub struct PanicInfallibleOutputPin<T>(T);
impl<T: TryOutputPin<Error = E>, E: core::fmt::Debug> OutputPin for PanicInfallibleOutputPin<T> {
    fn set_low(&mut self) {
        self.0.try_set_low().unwrap()
    }
    fn set_high(&mut self) {
        self.0.try_set_high().unwrap()
    }
}

[TeXitoi]

That's back compat, remove deprecated on v1, add deprecated on v2, and no need of a breaking change

[TeXitoi]

The TryT name is to mimic TryFrom, but no hard opinion on the name.

[Disasm]

I think that it's possible to use the same name, like OutputPin and fallible::OutputPin, maybe even reexported under FallibleOutputPin or TryOutputPin.

[TeXitoi]

@Disasm will not be convenient in generic code when importing the 2. Also, having a trait in a submodule might make it apear as a minor trait, even if we want it to be the one used.

Now, that's prettier I admit ;-)

[Disasm]

@TeXitoi Well, we can move both traits into submodules for equality, but I don't think it's needed.

[almindor]

It should be possible to expand on the infallible traits such that you can do:

let pin = get_infallible_pin_somehow().into_fallible();

Considering that the fallible pin scenario is the more rare of the two it should probably be ok to make that one as a special option rather than a default visible interface. The fallible Traits should probably be behind digital::fallible or such.

fn into_fallible(self) -> FallibleOutputPin

should be a new function of each pin mode required to be implemented and return the new Fallible<Mode>Pin trait that basically equals the current v2 Traits. In implementations where the pins are always infallible we can just use () for Error and switch to ! in the future.

This way the public interface will give you Infallible options by default and if you need to get fallible you switch to it as an optional step that should always work and be implemented.

Old v1 code will "just work" and new v2 code that actually needs it will also "just work" if they simply add the into_fallible call on init. As an added bonus anyone who switched to v2 to avoid a warning can now just remove the unwraps for 99% of their use cases.

I would remove the versioning as well, speaking from experience it is confusing. We could then deprecate the vX submodules and simply have the main interface in digital.

[Disasm]

@almindor Infallible types are converted into fallible ones in a transparent manner via blanket impls. There is no need for a method.

[almindor]

@almindor Infallible types are converted into fallible ones in a transparent manner via blanket impls. There is no need for a method.

Indeed, but what I wanted to achieve with into_fallible was to enforce v1 trait implementors to implement v2 as well.

We could then simply rename the existing v1 and v2 modules appropriately and have two traits connected so that both are always implemented and yet compatible in code and naming to previous v1/v2.

Example of v1 code: (no changes required to "upgrade" apart from module name change if we decide to go that route)

let pin = pins.pin1.into_output();
pin.set_low();
pin.set_high();

Example of v2 code: (one line change in pin init)

let pin = pins.pin1.into_output().into_fallible(); // or possibly just .fallible() ?
pin.set_low()?;
pin.toggle()?;

The main point is that this way the traits are still separate, yet we gain the enforced implementation of fallible version, the code is compatible to v1 and v2 at the same time and only a one liner per pin should be needed in case of the fallible cases.

I'm however also fully in support for going directly v3 with try_XXX, which IMO is cleaner, but more painful to update to for anyone using v2 already.

[ryankurte]

I believe that we need a future with 2 different compatible traits. Compatible in terms of infallible<->fallible conversions.

I understand the current state is not ideal, but, I just don't see the use case for this / what we would be gaining by creating ambiguity here?

For compatibility, driver authors must always implement the fallible version (as is suggested above, and consistent with almost all the other hal traits), in which case we can either:

• Have fallible and infallible traits, introducing a possible incompatibility (fallible hal + infallible driver) and ambiguity
• Have a single set of fallible traits and allow hals to specify type Error=! when errors are not possible, all drivers and implementations are compatible, no incompatibility, correct error handling.

As for the error handling, fallible traits increase the code complexity for infallible cases.

It does implement the complexity, but, is not significantly different from the error handling requirements we have for all other peripheral interfaces? The only reason this is considered a different case is that pin operations can sometimes be infallible, but, this stands for all other drivers too (i suspect had we got this right initially this would not be a necessary discussion).

You end up with an error type that looks something like:

#[derive(Debug)]
pub enum Error<ConnErr, PinErr> {
    Conn(ConnErr),
    Pin(PinErr),
    ...
}

And with implementations along the line of:

...
self.reset_pin.set_low().map_err(|e| Error::Pin(e) )?;
...

With only fallible traits present, how one should choose where to handle errors properly and where not?

One should always handle errors properly? When writing a driver (unless you do not intend to share it) it would incorrect to assume that the underlying implementation is infallible.

You can force Pin<Error=Infallible> in the driver interface, but the same thing in firmware code will increase code bloat even more. Infallible traits solve this problem.

Being able to define type Error=! (see rust-lang/rust#35121 for more detail) or type Error=Never (until this has stabilized) when implementing hals informs the compiler that the error path is unreachable and should be optimised, binding Pin<Error=!> in a driver implementation would limit a driver to a subset of hals, which imo would be an anti-pattern.

As an aside, I am not sure (apart from renaming things) what the significant difference between the current proposal and the existing (again, intended to be temporary) situation is? We currently have:

	Fallible Driver	Infallible Driver
Fallible Hal	✔️	- (explicit coercion)
Infallible Hal	✔️ (silent coercion)	✔️

cc. @therealprof because it'd be good to hear what you think if you have the time ^_^

[Disasm]

@ryankurte Frankly speaking, I don't see any ambiguity here. There will be two different traits with different meanings and method names. There are two use cases for such an approach:

• The ability to express the fallible/infallible nature of the pin. Even type restrictions (like "this function wants OutputPin<Error = Infallible>") do not work here, because some users use Void for the error type, sometimes even union type.
• The ability to call infallible methods without error "processing" due to the usage of v2 fallible traits.

Driver authors should use fallible traits, yes. Using infallible traits in drivers should be considered an anti-pattern. For abandoned drivers that use infallible traits, you will still have a way to explicitly convert a fallible object into the infallible wrapper for such drivers.

In my opinion, fallible traits should be used only in drivers and in other places where they are absolutely necessary. All the naturally-infallible types should implement infallible traits.

Again, the current implementation is perfect for drivers, but far from perfect for application code and hal libraries. The proposed implementation will still be perfect for drivers, but also good for applications and hals.

Major differences between the proposal and the current state:

• Different method names for the fallible traits.
• Infallible traits are no longer deprecated and co-exist with the fallible ones.

[therealprof]

because it'd be good to hear what you think if you have the time ^_^

I don't feel I have much new to add here.

I think digital::v2 was a mistake for a number of reasons:

• Importing the right traits can be a trap for beginners
• It adds overhead to applications using it
• The deprecation of digital::v1 caused lots of errors (due #[deny(warnings]) which was only later decided to be and declared as anti-pattern) and still causes tons of annoying warnings
• It should have never reused the method names but in hindsight have followed the usual try_ naming

At this point it's almost a bit late in the game to start fixing it since most of the obnoxious fallout has already been dealt with. I'm still very open to addressing this since a lot of people are still confused and dismayed and I feel we should put in a bit of effort to fix some of the shortcomings in the ecosystem to attract more people.

[thejpster]

So all this v1, v2 etc business feels complicated. Most people just want to get an IO pin and toggle it high or low. I also suggest we have a perfectly good mechanism for semantically versioning APIs - that's the crate version number.

My suggestion is therefore that we break up embedded-hal into:

• embedded-hal-serial
• embedded-hal-i2c
• embedded-hal-spi
• embedded-hal-digital
• embedded-hal-analog
• etc.

This then ties in with rust-embedded/embedded-hal#163, because the way to official 'annoint' some HAL, is to re-export it from the top-level embedded-hal crate (which needs to remain for compatibility, and because it's easier to import one crate than five).

For the record, I'm happy with an approach where chip HALs implement non-failable pins and get a failable implementation for free, and where drivers usually consume failable pins but can consume infallible pins if they just don't want to deal with the return codes.

[NickeZ]

So all this v1, v2 etc business feels complicated. Most people just want to get an IO pin and toggle it high or low. I also suggest we have a perfectly good mechanism for semantically versioning APIs - that's the crate version number.

My suggestion is therefore that we break up embedded-hal into:

* embedded-hal-serial

* embedded-hal-i2c

* embedded-hal-spi

* embedded-hal-digital

* embedded-hal-analog

* etc.

This then ties in with rust-embedded/embedded-hal#163, because the way to official 'annoint' some HAL, is to re-export it from the top-level embedded-hal crate (which needs to remain for compatibility, and because it's easier to import one crate than five).

For the record, I'm happy with an approach where chip HALs implement non-failable pins and get a failable implementation for free, and where drivers usually consume failable pins but can consume infallible pins if they just don't want to deal with the return codes.

Tokio is consdiering going the other way: tokio-rs/tokio#1264

I'm only an outsider. But it seems like a huge maintenance burden to split up crates like this.

The v1/v2 thing for pins have also hit me because I tried to combine crates that used different versions of pins. I'm in favor of getting rid of it and simply have one trait. It is a bit overkill to separately version the pin traits.

[almindor]

I'm leaning towards just doing the new trait and removing the old v1/v2 submodules completely in a breaking major version upgrade. It's painful but clean and will provide the best interface going forward. The later something like this happens the more painful it'll be to get there.

[ryankurte]

The ability to express the fallible/infallible nature of the pin. Even type restrictions (like "this function wants OutputPin<Error = Infallible>") do not work here, because some users use Void for the error type, sometimes even union type.

Using infallible traits in drivers should be considered an anti-pattern. For abandoned drivers that use infallible traits, you will still have a way to explicitly convert a fallible object into the infallible wrapper for such drivers.

The type restrictions problem is a fair point, but relates to the second. If creating drivers that don't handle errors is an antipattern, why support it at all, in which case we only need the fallible version?

• You can already .unwrap() or let _ = ... in the driver if you really want to throw caution to the wind
• We can document what should be returned from infallible implementations (and re-export it from the HAL) to mitigate incompatibility of infallible bounds
• This is consistent with all other hal trait errors and error handling

I'm in favor of getting rid of it and simply have one trait. It is a bit overkill to separately version the pin traits.

This was the original plan and I believe is still the best option. A single set of traits, that are all fallible and consistent with the rest of the hal.

The version only exists because we were worried about making significant breaking changes. Having multiple traits just introduces the opportunity for incompatibility, and it's trivial to take errors and handle/ignore/panic on them but super difficult (impossible?) to go the other way.

I'm leaning towards just doing the new trait and removing the old v1/v2 submodules completely in a breaking major version upgrade

Totally agreed that it's time we continued the cleanup / removed the ambiguity, and probably for a breaking minor version (we're still < 0) bump, and I'd also like to see the submodules gone.

Regardless of what we do, we'll need to do the semver trick thing and maintain compatibility with previous versions (and demonstrate this for either approach).

I would however prefer that we just promoted the v2/fallible trait to be the only one (and backported the conversions to the previous crate as part of the semver trickery).

[Disasm]

The type restrictions problem is a fair point, but relates to the second. If creating drivers that don't handle errors is an antipattern, why support it at all, in which case we only need the fallible version?

@ryankurte Drivers are not the only crates in the ecosystem. That's why I suggest having the infallible traits too. While they are not useful in the driver world, they are still useful in end-user code (proper error handling) and device hal implementatations (expressing the infallible nature of GPIOs).

Having just one version with both fallible and infallible traits and semver trick also works for me: it's like jumping to the end of the plan, but with a compatibility layer.

[rubberduck203]

I'm leaning towards just doing the new trait and removing the old v1/v2 submodules completely in a breaking major version upgrade. It's painful but clean and will provide the best interface going forward. The later something like this happens the more painful it'll be to get there.

I second this sentiment. Whether there is a single trait, or separate fallible and infallible traits, I would like to see the v2 module go away and certainly don't want to end up with a digital::v3. If possible, it would be best to add the new interface, deprecate the rest, and then remove the old traits in a later breaking change.

[ryankurte]

Agreed that further complexity is ideally avoided.

My biggest complaints are that this is inconsistent with every single other hal trait which return errors and document that infallible implementations should return core::convert::Infallible, and that whatever we do here introduces further complexity and breakage than the approach that we'd already come up with.

I've opened a PR with the alternative / original approach for comparison.

[Disasm]

@ryankurte What complexity are you talking about? I think that my current idea is in line with your plans for replacing digital::v1 with digital::v2.

[rubberduck203]

unwrap() can have a lot of overhead.
https://jamesmunns.com/blog/fmt-unreasonably-expensive/

We should also provide some guidance for driver implementers on how to properly call the failable interface.

[ryankurte]

@ryankurte What complexity are you talking about? I think that my current idea is in line with your plans for replacing digital::v1 with digital::v2.

I think this is the same plan as we had for the v1 -> v2 translation, but we'd be basically restarting the process so the result for now would be that we'd have v1, v2, and v3 traits, which would arguably be even worse than the current situation? Also having travelled this path once, the default impls to make them all work together are unlikely to be simple.

[Disasm]

A per #435 we decided to go the other way: release new embedded-hal with only fallible traits and add infallible traits later if necessary.