ryankurte / embedded-hal-compat

Rust embedded-hal inter-version compatibility layer
MIT License
15 stars 11 forks source link

Implementation of OutputPin requires InputPin #24

Closed natrow closed 1 year ago

natrow commented 1 year ago

Hey, me again. So the SPI bus seems to be working, but now I'm realizing that I can't use .forward() to convert my embedded hal 2.x output pins into embedded hal 1.0.0-alpha.y versions. It appears that the implementation requires the pin to be both an input and an output for some reason. Possibly a bug?

https://github.com/ryankurte/embedded-hal-compat/blob/2311e03e3304e3f76c09dc52bbd1cb1ad98875c8/src/forward.rs#L92-L94

error[E0277]: the trait bound `stm32l4xx_hal::gpio::Pin<Output<PushPull>, stm32l4xx_hal::gpio::L8, 'A', 4>: _embedded_hal_digital_InputPin` is not satisfied
  --> src/lib.rs:81:25
   |
81 |             trait_check(&reader_cs.forward());
   |             ----------- ^^^^^^^^^^^^^^^^^^^^ the trait `_embedded_hal_digital_InputPin` is not implemented for `stm32l4xx_hal::gpio::Pin<Output<PushPull>, stm32l4xx_hal::gpio::L8, 'A', 4>`
   |             |
   |             required by a bound introduced by this call
   |
   = help: the trait `_embedded_hal_digital_InputPin` is implemented for `OldInputPin<T>`
   = note: required for `stm32l4xx_hal::gpio::Pin<Output<PushPull>, stm32l4xx_hal::gpio::L8, 'A', 4>` to implement `stm32l4xx_hal::prelude::InputPin`
   = note: required for `Forward<stm32l4xx_hal::gpio::Pin<Output<PushPull>, stm32l4xx_hal::gpio::L8, 'A', 4>>` to implement `embedded_hal_compat::embedded_hal::digital::OutputPin`
ryankurte commented 1 year ago

ooops, yeah seems like a bug to me!

eldruin commented 1 year ago

This seems like a real problem. It comes from the unified eh1_0::digital::ErrorType trait and the fact that Forward<T> is generic over whatever T is. This makes it impossible to have two separate implementations of eh1_0::digital::ErrorType for Forward<T> where T is an InputPin and where T is an OutputPin, but not necessarily both.

i.e. we would need something like this but this creates an implementation conflict:

impl<T, E> eh1_0::digital::ErrorType for Forward<T>
where
    T: eh0_2::digital::v2::InputPin<Error = E>, // <----- one for input
    E: core::fmt::Debug,
{
    type Error = super::ForwardError<E>;
}

impl<T, E> eh1_0::digital::ErrorType for Forward<T>
where
    T: eh0_2::digital::v2::OutputPin<Error = E>, // <---- one for output
    E: core::fmt::Debug,
{
    type Error = super::ForwardError<E>;
}

Solutions to this would be to add some kind of marker type to Forward<T>'s type or add some kind of specific type like ForwardInputGPIO<T>. I am not sure how complicated the implementation would become and how the API would be impacted.

A small mitigation would be to swap the InputPin and OutputPin in the bounds, so that the OutputPin works (which is much more popular) and InputPin forwarding is broken instead.

To be clear, this was already the case before my recent changes and has been there at least since -alpha.7

natrow commented 1 year ago

This temporary fix works well enough for my current needs. Until a better solution can be found, perhaps the user can be given the option to toggle between using OutputPin or InputPin using a feature gate.

ryankurte commented 1 year ago

@natrow deployed a fix in v0.11.1, let us know how this works for you?

natrow commented 1 year ago

Everything seems to compile, I'll let you know once I test it with hardware.