[RFC] digital::IoPin, pins that can switch between input and output modes at runtime

[japaric]

Some people have expressed interest in a trait like this for writing generic LCD drivers but there are probably other use cases.

We were discussing this on IRC yesterday and I proposed three different APIs:

Proposals

Result based API

/// A pin that can switch between input and output modes at runtime
pub trait IoPin {
    /// Signals that a method was used in the wrong mode
    type Error;

    /// Configures the pin to operate in input mode
    fn as_input(&mut self);
    /// Configures the pin to operate in output mode
    fn as_output(&mut self);

    /// Sets the pin low
    fn set_low(&mut self) -> Result<(), Self::Error>;
    /// Sets the pin high
    fn set_high(&mut self) -> Result<(), Self::Error>;

    /// Checks if the pin is being driven low
    fn is_low(&self) -> Result<bool, Self::Error>;
    /// Checks if the pin is being driven high
    fn is_high(&self) -> Result<bool, Self::Error>;
}

// this function won't panic; LLVM should be able to opt way the panicking branches
fn example(mut io: impl IoPin) {
    io.as_input();
    if io.is_low().unwrap() {
        /* .. */
    }
    if io.is_high().unwrap() {
        /* .. */
    }

    io.as_output();
    io.set_low().unwrap();
    io.set_high().unwrap();
}

Closure based API

/// A pin that can switch between input and output modes at runtime
pub trait IoPin {
    /// Pin configured in input mode
    type Input: InputPin;

    /// Pin configured in output mode
    type Output: OutputPin;

    /// Puts the pin in input mode and performs the operations in the closure `f`
    fn as_input<R, F>(&mut self, f: F) -> R
    where
        F: FnOnce(&Self::Input) -> R;

    /// Puts the pin in output mode and performs the operations in the closure `f`
    fn as_output<R, F>(&mut self, f: F) -> R
    where
        F: FnOnce(&mut Self::Output) -> R;
}

fn example(mut io: impl IoPin) {
    io.as_input(|i| {
        if i.is_low() { /* .. */ }
        if i.is_high() { /* .. */ }
    });

    io.as_output(|o| {
        o.set_low();
        o.set_high();
    });
}

Implicit / Automatic API

/// A pin that can switch between input and output modes at runtime
pub trait IoPin {
    /// Signals that a method was used in the wrong mode
    type Error;

    /// Sets the pin low
    ///
    /// **NOTE** Automatically switches the pin to output mode
    fn set_low(&mut self);
    /// Sets the pin high
    ///
    /// **NOTE** Automatically switches the pin to output mode
    fn set_high(&mut self);

    /// Checks if the pin is being driven low
    ///
    /// **NOTE** Automatically switches the pin to input mode
    /// **NOTE** Takes `&mut self` because needs to modify the configuration register
    fn is_low(&mut self) -> bool;
    /// Checks if the pin is being driven high
    ///
    /// **NOTE** Automatically switches the pin to input mode
    /// **NOTE** Takes `&mut self` because needs to modify the configuration register
    fn is_high(&mut self) -> bool;
}

fn example(mut io: impl IoPin) {
    if io.is_low() {
        /* .. */
    }
    if io.is_high() {
        /* .. */
    }

    io.set_low();
    io.set_high();
}

I have implemented the proposals as branches io-1, io-2, io-3. You can try them out by adding something like this to your Cargo.toml file:

[dependencies]
embedded-hal = "0.1.0"

[replace]
"embedded-hal:0.1.0" = { git = "https://github.com/japaric/embedded-hal", branch = "io-1" }

Implementation concerns

There were some concerns about whether these traits can actually be implemented for all possible scenarios given that switching the mode of any pin on say, GPIOA, usually requires a RMW operation on some control register; thus the pins need exclusive access to the control register when switching modes.

Following the CRL idea of the Brave new IO blog post it seems that implementations would run into this problem:

struct IoPin {
    // pin number
    n: u8,
    crl: CRL,
    // ..
}

let pa0 = IoPin { n: 0, crl, .. };
let pa1 = IoPin { n: 1, crl, .. };
//^ error: use of moved value: `x`

But you can use a RefCell to modify the CRL register in turns:

struct IoPin<'a> {
    // pin number
    n: u8,
    crl: &'a RefCell<CRL>,
    // ..
}

let crl = RefCell::new(crl);
let pa0 = IoPin { n: 0, crl: &crl, .. };
let pa1 = IoPin { n: 1, crl: &crl, .. };

This limits you to a single context of execution though because RefCell is not Sync which means &'_ RefCell is not Send which means IoPin is not Send either.

But you can recover the Send-ness and avoid runtime checks by splitting CRL in independent parts that can be modified using bit banding, if your chip supports that:

let crls = crl.spilt();
let crl0: CRL0 = crls.crl0;
let crl1: CRL1 = crls.crl1;

let pa0 = IoPin0 { crl0, .. };
let pa1 = IoPin0 { crl0, .. };

---

Thoughts on these two proposals? Do they fit your use case? Do you have a different proposal?

cc @kunerd @therealprof

[therealprof]

@japaric I don't think it is quite common to switch a Pin between reading and writing.

However often used is tje ability to "tri-state" a pin i.e. set high, low and floating. Reading is not necessarily the same as setting it floating because often a pull-up or pull-down can be configured and in that case the pin would not be floating but sink or source current, driving the pin useless. Also for the "Implicit / Automatic API" case above I can imagine that the read might not be optimised away, although the result is actually irrelevant, because it is done via volatile read.

For my tri-state uses the "Implicit / Automatic API" is actually very intriguing but instead of providing the ability to read the value I'd add set_floating and is_floating.

[japaric]

@therealprof OK, that sounds like a different use case from what was mentioned on IRC. A different trait for a tri-state pin makes sense.

[kunerd]

One other example that requires switching between in- and output of a pin is when implementing drivers for 1-wire protocols, like the one that the DS1820 temperature sensor uses I think.

[therealprof]

@japaric I agree. However it's a very important one because it's required for multiplexing and especially charlieplexing which is very common. Not sure how often direction change is used in real life -- I'm just very sure I've never encountered a use for that before.

So we're going to have a TriPin then? 😁

[therealprof]

@japaric I just noticed that although #11 talks about having an InputPin, it doesn't exist yet, nor does an issue for its creation. Is the IoPin going to replace both the OutputPin and the InputPin? Or are the planned new traits going to be generic abstractions over more primitive traits?

[idubrov]

I think, closure-base API places significant limitation on when you can change pin direction.

It would be fine for localized direction changes (like in case of LCD, you would change the direction, wait for busy flag and switch it back to output), but I wonder, would it work for longer pin direction changes (walkie-talkie style)?

Implicit API requires exclusive ownership, which is also somewhat limiting (although, I can't think of the realistic scenario)

With result-based API, I wonder, what would be the realistic scenario of checking that error? Wouldn't it be better to simply panic instead of reporting an error?

[japaric]

@therealprof

Is the IoPin going to replace both the OutputPin and the InputPin?

No. All those have different use cases and can be used independently.

@idubrov

Wouldn't it be better to simply panic instead of reporting an error?

Yeah, we could chalk it up as a programmer error and simply panic. I think (hope) the compiler will be able to optimize away the panic branches.

---

Yet another option is to encode the type state change in the trait. I think it would look like this:

pub trait IoPin {
    type Input: InputPin + IoPin<Input = Self::Input, Output = Self::Output>;
    type Output: OutputPin + IoPin<Input = Self::Input, Output = Self::Output>;

    fn into_input(self) -> Self::Input;
    fn into_output(self) -> Self::Output;
}

fn example<IO>(io: IO) -> IO::Output
where
    IO: IoPin,
{
    let input /* : IO::Input */ = io.into_input();

    if input.is_low() {
        // ..
    }

    let mut output = input.into_output();

    output.set_high();

    output
}

It seems to compile at least.

[therealprof]

No. All those have different use cases and can be used independently.

👍

Not quite sure why the OutputPin as it is has is_low and is_high functions in it. There're actually platforms where pins configured as output cannot be read back.

[astro]

I like the ability to encode direction in the type. Automatic switching at run-time requires checking overhead.

The presence of is_low and is_high on OutputPin allows you to accidentally call them when you actually wanted to read from an InputPin. If you keep these function in OutputPin please consider naming them differently in InputPin.

[astro]

Here is a DHT22 driver using proposal io-2: https://github.com/astro/dht22-rs

[austinglaser]

I strongly dislike the automatic mode switching variant. I've fought with it before in Particle land, where I wanted an analog input to be configured with a pulldown to present the proper output impedance to an external analog circuit. However, because AnalogRead automatically sets the analog input mode, this was nearly impossible to do.

Of the Result and closure based approaches, I favor the Result-based one. However, I would propose having the signatures of is_high() and is_low() return a bare boolean:

/// A pin that can switch between input and output modes at runtime
pub trait IoPin {
    // ...

    /// Checks if the pin is being driven low
    fn is_low(&self) -> bool;
    /// Checks if the pin is being driven high
    fn is_high(&self) -> bool;
}

Consider, for instance, the case where a pin configured as an open-drain input implements IoPin. When the pin is "high", its actual electrical state will depend on external circuitry, and the ability to read it becomes critical (for instance, if implementing a bit-banged I2C bus).

Alternatively, an Err() result could be returned when it's impossible to read the electrical state of the pin (for instance, nRF5x MCUs support disconnecting the input buffer for a pin). This would make sure the programmer's expectation of getting the physical state of a pin is met, without enforcing that the reading be done with the pin configured as an input.

[japaric]

@therealprof

Not quite sure why the OutputPin as it is has is_low and is_high functions in it. There're actually platforms where pins configured as output cannot be read back.

In that case the state can be stored in the Pin struct as a boolean, but it probably makes sense to split the is_low / is_high functionality into its own trait to not burden implementers that only want to provide output functionality.

@astro

The presence of is_low and is_high on OutputPin allows you to accidentally call them when you actually wanted to read from an InputPin.

The compiler doesn't allow ambiguous calls like that. You'll get an error about two methods, that come from different traits, named the same being in scope. You'll have to disambiguate using UFCS: <pin as OutputPin>::is_low().

[tib888]

Hello, I haven't read the full thread yet but let me list a few related things:

• On the OutputPin trait it is not clear for me that the is_low()/is_high() returns the intended output state of a pin, or it is based on the input voltage level of that pin.
  The Toggle implementation is based on the first assumption, but if you are using an open drain output, then you may want to know is someone else keeps the line low, which would require the second assumption.
  So better naming, documentation is needed and maybe both functions should be accessible.
  (Also I would vote for the second approach, and remove toggle form the API.)

• A mixed mode use case:
  I used too have a 1-wire implementation where I used push-pull output mode and input mode mixed that way I was able to help the external pullup resistor to pull up the line more quickly.

[tib888]

As @therealprof pointed out there might be platforms which does not support this is_high/is_low at all. In that case I would propose to introduce a

trait StatefulOutputPin : OutputPin {
    fn is_set_high(&self) -> bool;
    fn is_set_low(&self) -> bool;
    fn toggle();
}

Or at least to rename the is_low / is_high in the current OutputPin to:

fn is_set_high(&self) -> bool;
fn is_set_low(&self) -> bool;

Because in OpenDrain mode both the InputPin and OutputPin traits need to be implemented and they work in the same time. The naming conflict between InputPin and OutputPin is not nice.
Also the new names would better reflect what the are: any kind of output pin can be set high, while the voltage level on the pin still can be low due to a short circuit condition.

[austinglaser]

Second @tib888's proposal to resolve the is_low()/is_high() name collision, for ergonomics and API clarity. I'm in favor of the first of the two proposals), but I agree that the functions should at minimum be renamed

I'm assuming you meant, in the first case:

trait StatefulOutputPin: OutputPin {
    fn is_set_high(&self) -> bool;
    fn is_set_low(&self) -> bool;
    fn toggle();
}

[tib888]

@austinglaser, thank you, I have fixed the typo.

[therealprof]

Well I can create a PR but I think thing that such behavior should be discussed, probably because it is API that should be eventually implemented in many other libraries.

If we have an actionable PR that'd be much easier to discuss then a potpourri of ideas floating in free space.

Unfortunately I see that there are almost no discussion about this subject.

Due to lack of interest. Feel free to reignite the discussion...

[rumatoest]

Well here is my PR.
There also can be another approach like having 2 traits for Input and Output state that can consume itself and produce opposite trait. Input consume itself and returns Output.
But I do not really think that such approach like mapping one entity into another is good for embedded API because it implies that there have to be some work with memory and more lines of codes that can affect performance.

[ryankurte]

Well here is my PR.

thanks for the PR! i've added some further comments over there.

But I do not really think that such approach like mapping one entity into another is good for embedded API because it implies that there have to be some work with memory and more lines of codes that can affect performance.

i've recently basically stopped using type-state for this reason, it's a nice abstraction for compile-time configuration, but, ends up requiring inordinate amounts of boilerplate when you have to wrap the states in enums and re-expose methods etc.

[luojia65]

I love this API for following reasons. There are some MCUs in market that could lock the input/output type of pins. Some of them may switch between input and output even after locked, where the input/output type cannot change. For example, I can set pin type to Output<PushPull> and Input<PullUp>, we can switch between them, but cannot switch to for example Output<OpenDrain> on a locked pin. The IoPin could be a good solution on such locked pins.

BTW, is there any other good words other than IoPin for its name? IoPin is good and in the future we may have IoPort, but we could think out of other trait names to decide.

[andresovela]

Hi, I would like to reignite this discussion. I also came to a roadblock when trying to implement a generic LCD interface that requires changing pins from output to input and vice versa.

I personally favor this proposal by @japaric , and it's something that I see fitting well with I've seen so far of Rust:

pub trait IoPin {
    type Input: InputPin + IoPin<Input = Self::Input, Output = Self::Output>;
    type Output: OutputPin + IoPin<Input = Self::Input, Output = Self::Output>;

    fn into_input(self) -> Self::Input;
    fn into_output(self) -> Self::Output;
}

fn example<IO>(io: IO) -> IO::Output
where
    IO: IoPin,
{
    let input /* : IO::Input */ = io.into_input();

    if input.is_low() {
        // ..
    }

    let mut output = input.into_output();

    output.set_high();

    output
}

I even saw that @astro posted some sample code here that implements that proposal.

[Windfisch]

+1 for @japaric's proposal. It uses Rust's type system to make errors impossible, i.e. to catch them at compile time. This is one of the main reasons why I find Rust great.

I greatly dislike closure-based solutions because I find them quite unreadable.
Also, automatic mode switching both collides with "you don't pay what you don't use" and also can exhibit surprising behaviour. Hidden state machines are usually a bad thing.
The manual mode switching and panic approach won't fit well with the generic InputPin and OutputPin traits; it's not cleanly possible to implement those, since they cannot return errors (or can they?).

[TheZoq2]

Before discovering this issue, I made a PR for a similar API in the f1xx_hal. I made 2 implementations, one similar to the closure based API and one to the result based API as I think both have their own merits.

The closure based one is good since it still preserves all the safety we're used to, but is not as flexible and slightly messy.

stm32-rs/stm32f1xx-hal#211

[therealprof]

The manual mode switching and panic approach won't fit well with the generic InputPin and OutputPin traits; it's not cleanly possible to implement those, since they cannot return errors (or can they?).

They do return Results in digital::v2 and in the future everything will be fallible by default.

[therealprof]

Reviewing the proposals again there's also another problem with the closure approach: In which mode will be GPIO be after the end of the closure? In some usecases you might want to stay in the previous state mode until you explicitly change it, in other cases you may want to switch modes.

The Result based API could theoretically work with the caveat that switching modes will often safety measures like a critical section, bit banding, atomic CAS which will mess with runtime behaviour of the chip.

[TheZoq2]

Reviewing the proposals again there's also another problem with the closure approach: In which mode will be GPIO be after the end of the closure? In some usecases you might want to stay in the previous state mode until you explicitly change it, in other cases you may want to switch modes.

Hmm, interesting point. I would assume that the pin is only in the new mode while the closure is running. Otherwise, would it change mode the first time you try to use it as the original mode?

[chrismoos]

When working on an LCD driver I ran into the same issue with needing an I/O pin (that's switchable). I ended up going with one of @japaric's options.

The IoPin trait I used is pretty much identical -- except ensuring Debug exists on the Error type for the pins, so that Result.unwrap() can be called (needs Debug). At some point this may change as error handling in embedded-hal is revamped (see #229).

pub trait IoPin {
    type InputPinError: Debug;
    type OutputPinError: Debug;
    type Input: InputPin<Error = Self::InputPinError>
        + IoPin<
            Input = Self::Input,
            Output = Self::Output,
            InputPinError = Self::InputPinError,
            OutputPinError = Self::OutputPinError,
        >;
    type Output: OutputPin<Error = Self::OutputPinError>
        + IoPin<
            Input = Self::Input,
            Output = Self::Output,
            InputPinError = Self::InputPinError,
            OutputPinError = Self::OutputPinError,
        >;

    /// Switch to input mode
    fn into_input(&mut self) -> &mut Self::Input;

    /// Switch to output mode
    fn into_output(&mut self) -> &mut Self::Output;
}

An implementation of the above IoPin can be seen here: https://github.com/chrismoos/ili9486-driver/blob/master/src/io/stm32f1xx.rs. This is an implementation for STM32F1xx device types, using the into_dynamic support for the pins.

Getting an IoPin:

let pb9 = GPIOB::PB9::<PullDownInput, PushPullOutput>(
        gpiob.pb9.into_dynamic(&mut gpiob_crh_ref.borrow_mut()),
        &gpiob_crh_ref,
);

IoPin, in the above example, would switch been PullDownInput and PushPullOutput. The user of the IoPin is abstracted from the pin mode, and just needs to be able to write/read.

This is definitely an important issue though, there are many instances where a Rust device library would want switchable/RW pin functionality, and to have it cross-platform supported via embedded-hal is obviously ideal.

[chrismoos]

This still hasn't been resolved...can we consider adding this API finally, or at least adding it and marking unstable or gating it, to get some form of an IoPin out there. If so, I can send a PR with:

pub trait IoPin {
    type Input: InputPin + IoPin<Input = Self::Input, Output = Self::Output>;
    type Output: OutputPin + IoPin<Input = Self::Input, Output = Self::Output>;

    fn into_input(self) -> Self::Input;
    fn into_output(self) -> Self::Output;
}

Thoughts?

[Windfisch]

@chrismoos don't ask, do it. This a bit of a do-o-cracy. Ideally, also add PRs to some actual hal implementations and show an example project where this is needed.
I think you'll have good chances to get accepted.

[MorganR]

I've implemented a trait based loosely on the suggestion above by @chrismoos: #269

This is also implemented in linux-embedded-hal (PR), and its use is demonstrated here (no-std DHT11 driver).