Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap errors for embedded-hal trait implementations #30

Merged
merged 3 commits into from
Apr 6, 2024

Conversation

markus-k
Copy link
Contributor

@markus-k markus-k commented Apr 4, 2024

The current implementations of the OutputPin, InputPin and StatefulOutputPin traits require that PortDriver::Error implements embedded_hal::digital::Error, which is never going to be the case. Thus, these traits are never usable.

Here is a small example that should result in lots of compiler errors because of unsatisfied trait bounds:

use embedded_hal::digital::{InputPin, OutputPin};

fn main() {
    let mut expander =
        port_expander::Tca6408a::new(embedded_hal_mock::eh1::i2c::Mock::new(&[]), false);
    let pins = expander.split();

    let mut pin0 = pins.io0.into_output().unwrap();
    let pin1 = pins.io1.into_input().unwrap();

    println!("Is pin1 low? {}", pin1.is_low().unwrap());
    print_it(pin1).unwrap();

    pin0.set_low().unwrap();
    set_it_high(pin0).unwrap();
}

fn set_it_high<P>(mut pin: P) -> Result<(), P::Error>
where
    P: OutputPin,
{
    pin.set_high()?;

    Ok(())
}

fn print_it<P>(mut pin: P) -> Result<(), P::Error>
where
    P: InputPin,
{
    println!("Pin: {}", pin.is_high()?);

    Ok(())
}

This change wraps the error type used in ErrorKind::Error in a wrapper struct PinError<BE> which implements embedded_hal::digital::Error. Like this all upstream errors that are core::fmt::Debug should be compatible with embedded-hal.

The return types for Pin::set_high() and OutputPin::set_high() are different now, since the returned error from the Pin implementation do not wrap errors in PinError. This could cause problems, so maybe it's a good idea to wrap these errors as well?

The current implementations of the `OutputPin`, `InputPin` and `StatefulOutputPin`
traits require that `PortDriver::Error` implements `embedded_hal::digital::Error`,
which is never going to be the case. Thus, these traits are never usable.

This change wraps the error type used in `ErrorKind::Error` in a wrapper struct
which implements `embedded_hal::digital::Error`. Like this all upstream errors that
are `core::fmt::Debug` should be compatible with `embedded-hal`.
@Rahix
Copy link
Owner

Rahix commented Apr 5, 2024

Ouch, you are totally right... Thanks for the fix.

The return types for Pin::set_high() and OutputPin::set_high() are different now, since the returned error from the Pin implementation do not wrap errors in PinError. This could cause problems, so maybe it's a good idea to wrap these errors as well?

I'd rather have both methods return the same type. So the inherent methods should also return PinError.

@markus-k
Copy link
Contributor Author

markus-k commented Apr 5, 2024

Alright, I agree thats better. I have updated the PR.

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Rahix Rahix merged commit 8326525 into Rahix:main Apr 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants