Embedded-hal: Correct usage of v2 fallible pin traits

[jamwaffles]

I'm migrating one of my crates to use the new v2 traits in this PR. Before, I had a few trait bounds on OutputPin. Is it enough to just change these to OutputPin<Error = ()>?

Before, I had:

use hal::digital::v2::OutputPin;

impl<SPI, DC, CS> SpiInterface<SPI, DC, CS>
where
    SPI: hal::blocking::spi::Write<u8>,
    DC: OutputPin,
    CS: OutputPin,
{
    // ...
}

Which now becomes:

use hal::digital::v2::OutputPin;

impl<SPI, DC, CS> SpiInterface<SPI, DC, CS>
where
    SPI: hal::blocking::spi::Write<u8>,
    DC: OutputPin<Error = ()>,
    CS: OutputPin<Error = ()>,
{
    // ...
}

I don't really care what the error type is for these pins so () is enough for this crate's purposes I think, but that's another assumption that might be incorrect. Am I on the right track here?

[ryankurte]

oh neat! so, if you’re _implementing_ the traits it’s totally reasonable to specify type Error = (); (or hopefully in future, type Error = !) for infallible implementations.

as a _consumer_ of the trait, you probably want to keep the where bound as general as possible by making your types generic over the error type as well, something like this. the example you’ve provided will only work with OutputPin impls that define type Error = () which is i suspect not what you intended?

[jamwaffles]

will only work with OutputPin impls that define type Error = () which is i suspect not what you intended?

Woops, no absolutely not. For this first iteration, I want to not care about the error type, but tell the user that _something_ went wrong by returning Result<(), ()> from various methods. That could be expanded in the future with an enum for better error handling, but right now I want to keep the new API as close to the old as possible.

I attempted using PinError as in the code you linked, but now I get the following error in my code:

error[E0207]: the type parameter `PinError` is not constrained by the impl trait, self type, or predicates 

which is totally understandable because I haven't put any bounds on PinError. That said, I don't know what bounds to use so I could use some help there.

Are there any other known occurrences of the v2 traits in use in the wild?

[therealprof]

@ryankurte I also noticed that the v2 traits are not included in the prelude, is this on purpose?

[therealprof]

@ryankurte And how would I use the v2 -> v1 conversion routines? 😅

[ryankurte]

which is totally understandable because I haven't put any bounds on PinError. That said, I don't know what bounds to use so I could use some help there.

the problem is the <PinError = ()> definitions on your structs, rather than propagating the traits through.
i started refactoring it, but, it's gonna require a bit of a redesign. you probably need to rethink the approach to error handling (and how to get all of the possible I2C, SPI, and Pin errors back to callers).

a patch for my partial refactoring is here if that's useful.

Are there any other known occurrences of the v2 traits in use in the wild?

the example i linked is the only _public_ one i have, but afaik it demonstrates everything you need (with the addition of an I2C bound on the error type perhaps).

@ryankurte I also noticed that the v2 traits are not included in the prelude, is this on purpose?

it seems like it'd create more naming problems to export them and then change it later..? i figured we'd put em in the prelude in place of the v1 ones when we do a minor release and make the breaking change to v2 as default?

@ryankurte And how would I use the v2 -> v1 conversion routines?

something like this? which might be a good addition to the docs...

extern crate embedded_hal;
use embedded_hal::digital::{v1, v2, v1_compat::OldOutputPin};

struct NewOutputPinImpl {}

impl v2::OutputPin for NewOutputPinImpl {
    type Error = ();

    fn set_low(&mut self) -> Result<(), Self::Error> {
        Ok(())
    }
    fn set_high(&mut self) -> Result<(), Self::Error>{
        Ok(())
    }
}

struct OldOutputPinConsumer<T: v1::OutputPin> {
    _pin: T,
}

impl <T>OldOutputPinConsumer<T> 
where T: v1::OutputPin 
{
    pub fn new(pin: T) -> OldOutputPinConsumer<T> {
        OldOutputPinConsumer{ _pin: pin }
    }
}

fn main() {
    let pin = NewOutputPinImpl{};

    let _consumer: OldOutputPinConsumer<OldOutputPin<_>> = OldOutputPinConsumer::new(pin.into());
}

[therealprof]

it seems like it'd create more naming problems to export them and then change it later..? i figured we'd put em in the prelude in place of the v1 ones when we do a minor release and make the breaking change to v2 as default?

I'm on the fence about this. This is causing anything from annoying warnings, to annoying errors (if there's a version mismatch of embedded-hal between a driver and an application, a #[deny(warning)] in the driver turns into a hard error. No idea how to counter this problem but the least we want is that people are majorly annoyed by this change.

something like this? which might be a good addition to the docs...

Hm... I think we need good stories for all of these cases.

• HAL impls
• drivers using the HAL impls
• applications

I thought the idea was to implement v2 in the HAL impls and have auto-conversions for the other two. So I switched a HAL impl to implement v2 but now the applications don't compile anymore because the drivers are incompatible...

 --> examples/gpio_hal_7seg.rs:93:28
   |
93 |             let sevenseg = SevenSeg::new(seg_a, seg_b, seg_c, seg_d, seg_e, seg_f, seg_g);
   |                            ^^^^^^^^^^^^^ the trait `embedded_hal::digital::v1::OutputPin` is not implemented for `stm32f0xx_hal::gpio::gpiob::PB4<stm32f0xx_hal::gpio::Output<stm32f0xx_hal::gpio::PushPull>>`
   |
   = note: required by `<sevensegment::SevenSeg<A, B, C, D, E, F, G>>::new`

[ryankurte]

I'm on the fence about this. This is causing anything from annoying warnings, to annoying errors (if there's a version mismatch of embedded-hal between a driver and an application, a #[deny(warning)] in the driver turns into a hard error.

i'm not sure i follow, i can see how the updated deprecation marker is causing annoying warnings (or errors in case of #[deny(warning)], but, that's outside of our control and considered an antipattern iirc), but not how that's related to the prelude?

No idea how to counter this problem but the least we want is that people are majorly annoyed by this change.

yeah :-( but this is the best approach all our bikeshedding came up with.

I thought the idea was to implement v2 in the HAL impls and have auto-conversions for the other two. So I switched a HAL impl to implement v2 but now the applications don't compile anymore because the drivers are incompatible...

so you can't auto-implement both ways because you put the compiler into loops. v1 impls are silently compatible with v2 consumers, v2 traits (which hide an .unwrap()) must be explicitly converted.

this asymmetry is noted in the pr and the docs. the v1 -> v2 approach is much nicer:

extern crate embedded_hal;
use embedded_hal::digital::{v1, v2};

struct OldOutputPinImpl { }

impl v1::OutputPin for OldOutputPinImpl {
    fn set_low(&mut self) { }
    fn set_high(&mut self) { }
}

struct NewOutputPinConsumer<T: v2::OutputPin> {
    _pin: T,
}

impl <T>NewOutputPinConsumer<T> 
where T: v2::OutputPin {
    pub fn new(pin: T) -> NewOutputPinConsumer<T> {
        NewOutputPinConsumer{ _pin: pin }
    }
}

fn main() {
    let pin = OldOutputPinImpl{};
    let _consumer = NewOutputPinConsumer::new(pin);
}

ideally you should just be able to .into() the new traits into the old ones, _however_, the type system doesn't seem to like it... i wonder if there's a better way of implementing this that would appease the inference engine?

Hm... I think we need good stories for all of these cases.

yeah, i think that'd be an excellent addition. i also opened #136 with the examples posted here added as rust-docs to their respective modules.

[little-arhat]

Too bad embedded-hal::prelude exports v1 traits, so if someone use v2 as well, they'll get multiple applicable items in scope. Not very smooth migration process.

Is it possible to hide v1 somehow in user code?

[ryankurte]

Too bad embedded-hal::prelude exports v1 traits, so if someone use v2 as well, they'll get multiple applicable items in scope. Not very smooth migration process.

oof, that's particularly annoying, @therealprof any ideas?
points in favor of the breaking-change approach i guess :-/ (and possible releasing a breaking 0.2.3 that swaps the preludes?)

[therealprof]

@ryankurte I'm sort of out of ideas. I haven't quite figured out how to deal with this to be honest. As I've mentioned I've tried to convert some crates but stumbled into lots of problems and haven't quite figured out how to work around them for a smooth transition or better yet, friendly co-existence. From what I've experienced I'm very convinced that we should remove the deprecation marker ASAP and re-release a patch version so that old crates continue to just work.

[ryankurte]

yeeah, at least we're learning how hard it is now rather than later i guess...

i'm not convinced the deprecation maker is our responsibility, afaik we're using it exactly as it's meant to be used and use of #[deny(warning)] is the broken part.

That said, given it's causing friction, i would be _okay_ with a patch with it removed.

my thinking at the moment is that the smoothest approach is an 0.3.0 release that swaps v2 to default and updates the prelude, then to semver trick it all back into the 0.2.x series, but i don't feel comfortable enough with semver tricks to try it :-/

[hannobraun]

@ryankurte

i'm not convinced the deprecation maker is our responsibility, afaik we're using it exactly as it's meant to be used and use of #[deny(warning)] is the broken part.

I agree with this. I can think of two reasons to add #![deny(warnings)] to one's code:

1. To prevent contributors from adding warnings to the code, by catching those warnings early on CI. If that were the only reason, then the usage of #![deny(warnings)] would be an anti-pattern, due to the side effect of every new warning breaking the build. I don't think it's our responsibility to work around broken parts of downstream crates.
2. To make sure you're notified of changes early, so you can adapt to them now and not later. In that case, you would _want_ your build to break due to a deprecation and we would be doing those users a disservice by not adding deprecation markers.

Am I missing something?

[eldruin]

For the record, those use cases can be achieved by using RUSTFLAGS="-D warnings" in the CI configuration. The whole thing is explained here.
I agree embedded-hal cannot be responsible for user's broken code. Maybe this serves as a heads up for crate maintainers not to use #![deny(warnings)]. I also thought that was a good thing until I was told about the anti-pattern.

[therealprof]

@eldruin @hannobraun @ryankurte I totally agree with the anti-pattern. However it is embedded-hal that makes other code not compile anymore and since it is a patch release there's no way around it. It's very nasty to force someone to change to the better with a weapon in hand...

[therealprof]

@ryankurte Maybe it's time to start over again with those traits and go for a v3 which offers the ability to be composable with v1. I don't think we can expect v1 to be gone any time soon so the design goals would be:

• HAL impls can implement both v1 and v3 at the same time without clashes, preferably automatically via shim
• Drivers can choose to either use v1 or v3
• Applications can mix and match v1 and v3 as required by drivers

[ryankurte]

comments

@little-arhat

Is it possible to hide v1 somehow in user code?

i realised this morning that the reason i haven't observed this is that i've been exporting digital::v2 and using them as v2::OutputPin and v2::InputPin.

@therealprof

Maybe it's time to start over again with those traits and go for a v3 which offers the ability to be composable with v1

i'm not seeing anything here that's is a failing of the impl, or any approach that would be measurably different to the approach we've taken tbqh. i'm also reasonably confident that most of these issues can be resolved by a minor release.

i've also been using these v2 traits developing drivers against the unchanged linux-embedded-hal for the last few months with no issues, so i'm pretty comfortable that the driver and application cases _just work_.

HAL impls can implement both v1 and v3 at the same time without clashes, preferably automatically via shim

why would they need to implement more than one version on a given pin? afaik this is incompatible with the silent coercion we have from v1 -> v2, and is a duplication of effort when we can provide coercion in the hal for drivers and applications (as we have).

Drivers can choose to either use v1 or v3

this is the case with v1 and v2, using the v2 traits provides silent compatibility with v1 impls.

Applications can mix and match v1 and v3 as required by drivers

this is already the case with v1 and v2. coercion from v1 -> v2 is automatic, reversion from v2 -> v1 requires an explicit v1_compat::OldInputPin::from(p) or v1_compat::OldOutputPin::from(p).

Hm... I think we need good stories for all of these cases.

• HAL impls
• drivers using the HAL impls
• applications

On reflection i think the only important guideline here is that HALs and drivers _must_ not implement both, and coercion needs more documentation.

issues and resolutions

• #[deny(warnings)]
  
  
  
  • correct approach, though annoying to consumers
  
  
  • theoretically possible to remove depreciation marker from v0.2.x series and release v0.3.x with marked depreciation, practically this appears incompatible with the semver trick
  
  
  • document #[allow(deprecated)]and note #[deny(warnings)] antipattern in readme / docs
  
  
• what error trait bounds to use
  
  
  
  • same as any other generic trait type (ie. all other fallible drivers)
  
  
  • provide an example driver / better examples
  
  
• how to use the v2 -> v1 conversion routines
  
  
  
  • improve documentation (see: #136)
  
  
• export v2 types and fix prelude issue
  
  
  
  • v0.3.0 release which actually swaps the defaults
  
  
  • v0.2.4 release that executes the semver trick
  
  
• HAL impl and example incompatibility
  
  
  
  • unfortunately this one doesn't have a simple fix, coersion can only be silent _one way_
  
  
  • option a: update dependencies to support the v2 traits and get v1 compatibility for free
  
  
  • option b: update examples to use v1_compat::OldInputPin::from(p) and v1_compat::OldOutputPin::from(p)
  
  

proposed release

i've put a minor release together (with the an attempt at the semver trick) if you'd like to try patching to either.
heads up patches don't seem to work with crate aliasing, so you may have to clone locally and patch with path dependencies.

embedded-hal = { git="https://github.com/ryankurte/embedded-hal", branch="v0.2.x" }
embedded-hal = { git="https://github.com/ryankurte/embedded-hal", branch="v0.3.x" }

[therealprof]

i'm not seeing anything here that's is a failing of the impl, or any approach that would be measurably different to the approach we've taken tbqh. i'm also reasonably confident that most of these issues can be resolved by a minor release.

The problem is that you cannot have both v1 and v2 in scope, if you do then you need to specify the type everywhere you use it:

error[E0034]: multiple applicable items in scope
  --> examples/flash_systick.rs:78:21
   |
78 |                 led.set_high().ok();
   |                     ^^^^^^^^ multiple `set_high` found
   |
   = note: candidate #1 is defined in an impl of the trait `embedded_hal::digital::v1::OutputPin` for the type `stm32f0xx_hal::gpio::gpioa::PA1<stm32f0xx_hal::gpio::Output<_>>`
   = note: candidate #2 is defined in an impl of the trait `embedded_hal::digital::v2::OutputPin` for the type `_`

If you don't have v2 in scope you cannot use the v2 traits at all:

error[E0599]: no method named `ok` found for type `()` in the current scope
  --> examples/flash_systick.rs:72:31
   |
72 |                 led.set_low().ok();
   |                               ^^

And this is just a HAL impl. It gets far worse if you actually try to use it in an application with drivers requiring v1.

why would they need to implement more than one version on a given pin? afaik this is incompatible with the silent coercion we have from v1 -> v2, and is a duplication of effort when we can provide coercion in the hal for drivers and applications (as we have).

Silent coercion?

If the trait had different method names it would be possible to have them both in available and simply use them as needed.

this is already the case with v1 and v2. coercion from v1 -> v2 is automatic, reversion from v2 -> v1 requires an explicit v1_compat::OldInputPin::from(p) or v1_compat::OldOutputPin::from(p).

I haven't been able to make it work despite trying multiple times... maybe I'm doing some nonsense but at least I can make a claim that it is absolutely not straight forward.

On reflection i think the only important guideline here is that HALs and drivers must not implement both, and coercion needs more documentation.

That's not going to happen in the foreseeable future. Especially drivers are not going adapt quickly.

[ryankurte]

The problem is that you cannot have both v1 and v2 in scope, if you do then you need to specify the type everywhere you use it:

yep, you have to have them in scope. fortunately they can be aliased and/or called using the universal method call syntax.

And this is just a HAL impl. It gets far worse if you actually try to use it in an application with drivers requiring v1.

could you share an example of this so i can see if i can help?

Silent coercion?

you can _theoretically_ drop a v1 impl into anything with a v2 bound without doing anything. i haven't found any situations where this isn't true, but, i write drivers more than hals...

I haven't been able to make it work despite trying multiple times... maybe I'm doing some nonsense but at least I can make a claim that it is absolutely not straight forward.

i've added examples to the docs in: #136 that might help

That's not going to happen in the foreseeable future. Especially drivers are not going adapt quickly.

The embedded-hal crate provides mechanisms for conversion between the two implementations, which precludes anyone implementing _both_ the v1 and v2 traits either in a driver or in a hal, but smooths the process by providing helpers for the conversion between the two kinds.

We did it this way to relax pressure on hals / drivers / applications to upgrade (there's no issue with HALs staying at v1 almost indefinitely / until we actually remove the old impl), and to minimize the amount of work required by hals/drivers by allowing them to take a single upgrade step / not worry about implementing or maintaining both impls / bounds.

| | v1 bound | v2 bound |
|---------|----------|----------|
| v1 impl | ✔️ | ✔️ (Uses implicit compatibility) |
| v2 impl | ✔️ (Requires explicit conversion) | ✔️ |

That's not to say you can't use hals or drivers (or applications) that require a mix of both, or even piecewise upgrade a HAL, just, that you must not use _both_ v1::OutputPin and v2::OutputPin in either a pin implementation or a trait bound.

i think we're talking past each other a bit again :-/ i'd be happy to have a skype chat or something to chat about this / see if we can clarify / resolve your issues ^_^

[eldruin]

Here is an usage example of the v2::OutputPin traits in a device driver.
Yesterday I started a driver for flash memory devices using the v2::OutputPin for SPI's chip-select.

You can see the Error type now contains two parameters. This made the code somewhat bloated as I had to write the Error<SpiE, PinE> error type in each method, among the usual SPI, CS type parameters.

To reduce the number of parameters necessary in the implementation of the W25 struct and its methods I created a trait with an associated Error type which can be defined in its implementation for an SpiInterface struct. This way I can use it in the W25 method implementation, resulting in quite acceptable code:

impl<DI, SpiE, PinE> W25<DI>
where
    DI: ReadWrite<Error = Error<SpiE, PinE>>,
{
    /// Get the JEDEC ID
    pub fn get_jedec_id(&mut self) -> Result<[u8; 3], DI::Error> {
        let mut id = [0; 3];
        self.iface.write_read(&[Commands::JEDEC_ID], &mut id)?;
        Ok(id)
    }
}

I wrote a program using it that reads the JEDEC ID from the device and prints it to an OLED display. All running on the STM32F3Discovery board using the usual board/mcu support crates. See the source code here. I only needed to replace the f3::hal::prelude::* import.

@jamwaffles I can also send you a PR following a similar strategy.

[ninjasource]

Thanks @eldruin, this is actually the clearest error handling example of how to use the new v2 traits that I have found. @therealprof and @ryankurte, would it maybe be a good idea to scrap using the prelude altogether while such significant design changes are happening and are foreseen to happen in the future? For newcomers to crates like embedded_hal, explicit name spaces are a godsend because they remove some of the "magic" which makes troubleshooting the other unrelated problems easier.

David

[ryankurte]

i put together a small driver example / template ryankurte/rust-embedded-driver, do any of y'all have thoughts about this? otherwise i might look at adding it to our org / docs somewhere to make this whole process easier for people.

[ryankurte]

Closing as resolved with documentation work in: https://github.com/rust-embedded/book/issues/246