Pin methods returning results is extremely unfortunate

[coffeenotfound]

Most pin functions (is_high(), set_low(), etc.) return results, which are virtually always infallible, but we are still forced to unwrap them which is a very unfortunate and bad design smell.

I'm not sure if the boat has already sailed (I hope not) but here are two ways I think are pretty good solutions:

Approach 1:

Use GATs (or not use GATs and assume pin output is always bool?) to have
return values either be a Result or a value.

pub trait InputPin {
    type Output<T> = T;

    fn is_high(&self) -> Self::Output<bool>;
    fn is_low(&self) -> Self::Output<bool>;
}

Approach 2:

Extract into seperate trait and prefix fallible methods with try_ and make non-prefixed ones infallible

pub trait InputPin: FallibleInputPin<Error = Infallible> {
    type Error;
    fn try_is_high(&self) -> Result<bool, Self::Error>;
    fn try_is_low(&self) -> Result<bool, Self::Error>;
}
// Could also require `FallibleInputPin<Error = Infallible>`
// but I'm not sure if the bang/buck is worth it
pub trait InputPin: FallibleInputPin {
    fn is_high(&self) -> bool;
    fn is_low(&self) -> bool;
}

Overall I prefer approach 1 as it works well and is simple. Approach 2 could be better for highly generic code since it's easier to extract the error type for further composing

[ryankurte]

hi @coffeenotfound! we appreciate the feedback, however, we've been through -a lot- of revisions of this over the years, and the point we've ended up is that if we want embedded-hal to provide a generic abstraction, we need to have explicit error handling in all cases.

this is to support devices which can have failure in setting pins (for example, when you have an OS or another communication bus involved), and so that a driver that needs for example, GPIO pins, can be instantiated over either fallible or infallible implementations of this. while it's annoying when using pins directly, drivers need to handle this condition, and adding GATs or splitting these types creates opportunities for driver incompatibility.

there is however hope on the horizon with Infallible = !;, when the never type lands the compiler should no longer require infallible results to be handled. also as a stylistic note, in some situations instead of .unwrap() you can assign the result to an ignored value to when using infallible methods (let _ = pin.set_high()) to signal these can be ignored.

[eldruin]

@coffeenotfound Please note that HAL implementations are encouraged to and often offer infallible pin methods in addition to implementing the embedded-hal traits.
See here for an example (note that there is no .unwrap() calls).
This allows for simple (i.e. infallible) user code as well as interoperability for HAL-implementation-independent drivers, which use the embedded-hal traits rather than the additional inherent methods.

[coffeenotfound]

@ryankurte @eldruin I do understand the need for fallibility and I'm glad this is addressed in some hal implementations but I think it would be good to standardize this -- even if just a specialized impl over any T that has Error = Infallible or similar

[ryankurte]

interesting idea! the trouble with a specialised impl is that they can only be used in certain situations, so by standardising we would still be introducing a canonical source of incompatible driver bounds. ime the less ambiguity we have in interfaces the better the ecosystem pieces seem to fit together.

understanding the appeal of paving over this, but given it would introduce a minor foot-gun and this will eventually be solved by a language feature, i don't think a specialised impl should go in e-h... we could however improve the current situation with documentation for HAL implementers, and perhaps your specialisation idea could be executed in a third party crate in the interim?

[coffeenotfound]

If in future infallible Results are going to no longer be must_use than I think the current situation is fine -- if and when that happens... (still waiting for try blocks to be stabilized :( )

My personal opinion is still that this is a moderately important design defect (introducing potentially illegal state -- making software less robust) that should be addressed in the embedded-hal itself instead of hoisting that work to unstandardized (and not available in case of rp2040-hal) library implementations

Would something like this be an acceptable solution?

use std::convert::Infallible;

pub trait InputPin {
    type Error;
    
    fn is_high(&self) -> Result<bool, Self::Error>;
    fn is_low(&self) -> Result<bool, Self::Error>;
    
    /// Placeholder name (best I could come up with without changing `is_high` to `try_is_high`)
    fn is_high_ok(&self) -> bool where
//      Self::Error = Infallible
        Self::Error: IsInfallible
    {
        self.is_high()
            .ok()
            .unwrap()
    }
}

// Work-around for equality constraints not being stable yet
pub unsafe trait IsInfallible {}
unsafe impl IsInfallible for Infallible {}

[burrbull]

The goal of embedded-hal is interacting with drivers. Nothing more. Using of it in user code is bad idea.

[coffeenotfound]

Well, embedded-hal is the standard which target specific crates implement. If the functionality is not in the standard it's not standardized across the implementations