[DISCUSSION] GPIO Invert API

[felipebalbi]

trafficstars

Introduction

Some GPIO controllers have the ability to invert the logic level in HW. This is useful in cases where some generic code samples the state of a pin and take actions depending on the state. Rather than having the generic know about ActiveHigh vs ActiveLow, the HW invert the logic level and the generic code can rely solely on is_low()/is_high() as if the level was not inverted.

Proposal

There are two options here.

1. Add a new set_inverted(&mut self) API to the existing InputPin struct
2. Add a new InvertedInputPin which assumes GPIO is inverted.

Challenges

• It's unlikely that every GPIO controller is capable of inverting the logic level in HW. How can we guarantee that the HAL's SW implementation is sound?
• Adding a single new function to InputPin would allow for drivers to implement that API and maintain all their types. Existing code using InputPin would continue to work, but it would be a breaking change if the new method is required.
• Adding a new InvertedInputPin would avoid a breaking change, but any piece of code polymorphic over InputPin would have to be modified in order to use InvertedInputPin.

[Wassasin]

To check, as either a driver or in generic application code you want to:

• check if the pin is inverted (i.e. decouple digital high/low from logic active/inactive)
• switch between inverted and non-inverted

If we disregard hardware support for this for a second, we can implement both outside of the HAL, by wrapping any InputPin:

struct InvertedInputPin<T: InputPin>(T);

impl<T: InputPin> InputPin for InvertedInputPin<T> {
    fn is_high(&mut self) -> Result<bool, Self::Error> {
        Ok(!self.0.is_high()?)
    }
}

Software-based inversion at runtime can be implemented using a bool and a XOR. Hence if we only want software support for this, this is already do-able outside of embedded-hal.

If we want to support the hardware inversion I would first suggest that we change embedded_hal::digital to distinguish between digital-level and logic-level, similar to how Zephyr does it. Currently we have almost zero documentation for this module, and it is implied that it uses digital-level only. Then when converting between digital-level to logic-level, you can arrange for this inversion to be configured.

As an aside, I am curious to see what kind of generic code does not know about the HAL implementation, but does want to switch between inversion modes.

Feel free to correct anything I misunderstood!

[Dirbaio]

I'm having trouble imagining a driver that could benefit from this. If a driver wants inverted gpio, it can always just exchange is_high/is_low, set_high/set_low.

[felipebalbi]

I'm having trouble imagining a driver that could benefit from this. If a driver wants inverted gpio, it can always just exchange is_high/is_low, set_high/set_low.

Simple example: something like rmk. That is, you have a GPIO matrix and you want to scan it. The matrix could, potentially, be wired up such that pressing a key pulls the input GPIO either high or low; rmk could be given inverted GPIOs where applicable, then the scanner remains the same regardless of how the GPIOs were wired up. That being said, this is a configuration detail that could be handled before giving ownership of the pins to rmk.

[Dirbaio]

The thing is the GPIO traits are explicitly for GPIO, ie they get/set the physical voltage on a digital pin. They're not "logical active/inactive signal input".

Adding support for inversion (in e-h or in a wrapper written by the user) would be "lying" to the driver. The driver would read "voltage is low" while the reality is it's high.

I think if we want to support logical signals it'd make more sense to have LogicalInput, LogicalOutput traits. Then we could have wrappers that convert from "gpio input" to "logical input" that can be configured to handle the "active low" and "active high" cases (ie invert or not invert). This way the drivers would declare if they expect physical or logical signals, and no lying would be required.

However, I think doing this would be a bit too over-abstracted. It will be confusing for the users, as the logical/physical distinction is somewhat subtle. I'm not sure if it's worth it.

IMO it's simpler to keep the traits for phyiscal voltage only, and let drivers that want inversion handle it themselves.

[sirhcel]

Simple example: something like rmk. That is, you have a GPIO matrix and you want to scan it. The matrix could, potentially, be wired up such that pressing a key pulls the input GPIO either high or low; rmk could be given inverted GPIOs where applicable, then the scanner remains the same regardless of how the GPIOs were wired up. That being said, this is a configuration detail that could be handled before giving ownership of the pins to rmk.

IMO it's simpler to keep the traits for phyiscal voltage only, and let drivers that want inversion handle it themselves.

I'm with @Dirbaio and @Wassasin in the sense that handling inversion directly in the GPIO traits likely will cause more confusion than having the scope limited to the physical GPIO pin.

But what about an adapter @felipebalbi? There is @Wassasin's suggestion InvertedInputPin adapting from InputPin to InputPin which expresses the inversion also in the pin's type. There is also the proof-of-concept switch-hal around which seems to have reached some adoption and adapts from InputPin to InputSwitch clearly separating the concerns of GPIO pin state of the state of some other coupled net. And in https://github.com/rubberduck203/switch-hal/pull/12 there were at least two hands raised to get this closer to to embedded-hal.

[Dirbaio]

InvertedInputPin adapting from InputPin to InputPin

this is the "lying" that I think is bad.

For example, InputSwitch should take InputPin, enum{ ActiveHigh, ActiveLow} and handle the inversion itself. It should not take just InputPin and expect the user to wrap the input pin into an InvertedInputPin

Similarly for any other driver that wants to take input pins that might be active-high or active-low.

[sirhcel]

InvertedInputPin adapting from InputPin to InputPin

this is the "lying" that I think is bad.

Now I see what you were referencing to. Yes, this would water the scope of a physical GPIO pin down. But if one really really want to do this for something expecting InputPin or OutputPin, then the adapter approach puts at least some "warning tape" in place. I wanted to state that from my point of view this is somewhat less confusing than making inversion run-time configurable and generally available.

For example, InputSwitch should take InputPin, enum{ ActiveHigh, ActiveLow} and handle the inversion itself. It should not take just InputPin and expect the user to wrap the input pin into an InvertedInputPin

Similarly for any other driver that wants to take input pins that might be active-high or active-low.

That's what switch-hal's Switch is doing. Not by means of an enum but with a marker type.

[i509VCB]

The thing is the GPIO traits are explicitly for GPIO, ie they get/set the physical voltage on a digital pin. They're not "logical active/inactive signal input".

Adding support for inversion (in e-h or in a wrapper written by the user) would be "lying" to the driver. The driver would read "voltage is low" while the reality is it's high.

This does make the implementation in embassy-mspm0 where can invert a gpio pin feel sketchy then: https://docs.embassy.dev/embassy-mspm0/git/mspm0c1103dgs20/gpio/struct.Flex.html#method.set_inversion

However the e-h docs for digital are written using "high" or "low". An active-low pin like an open drain interrupt line is technically at 3.3V but has a "low" state. And vice versa for 0V being a "high" state.

https://docs.rs/embedded-hal/1.0.0/embedded_hal/digital/trait.OutputPin.html#tymethod.set_low

    /// Drives the pin low.
    ///
    /// *NOTE* the actual electrical state of the pin may not actually be low, e.g. due to external
    /// electrical sources.
    fn set_low(&mut self) -> Result<(), Self::Error>;

We can ignore the second part of the docs, as blasting a pin with 3.3V when it's an outputting a low (0V) is user error. But I do think the semantics that embedded_hal::digital is trying to guarantee are unclear. Is the pin state of low/high meaning from perspective of the GPIO controller or the signal?