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...