Support port expander interrupts

[Rahix]

I also wanted to add a PortDriverInterruptChange trait that would allow the user to wait on one or multiple pins changing state.

My approach:

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum InterruptChangeError<EI, EP> {
    I2cError(EI),
    PinError(EP),
}

pub trait PortDriverInterruptChange: PortDriver {
    /// Wait until at least one pin in `mask` switches
    /// to the state indicated by the corresponding bit in `target_state`.
    ///
    /// If a pin already has the target state, it will not trigger an interrupt,
    /// unless it first switches to the opposite state.
    ///
    /// Returns the bits that switched to the state indicated in `target_state`
    ///
    /// Arguments:
    /// * `int_pin` - The mcu pin to wait for an interrupt by the port expander
    /// * `mask` - The port expander pins to wait for a level change on
    /// * `target_state` - For each port expander pin, the state to wait for.
    /// A 1 in the mask indicates a high level, a 0 indicates a low level.
    async fn wait_for_change_to<P>(
        &mut self,
        int_pin: P,
        mask: u32,
        target_state: u32,
    ) -> Result<u32, InterruptChangeError<Self::Error, P::Error>>
    where
        P: embedded_hal_async::digital::Wait;
}

(API is based on what the PI4IOE5V6408 offers. We could add other traits for edge detection later)

My idea was to make this accessible via:

• A wait_for_change_to method for each pin
• A wait_change_multiple method (similar to read_multiple)

Problem: We can not keep the driver locked, while awaiting an async function.
This would require an extension of the shared-bus crate to either allow passing an async closure to the lock method, or have a lock method that returns a guard.

Before I do this, I wanted to ask you, if I'm on the right track here, or whether you have a different solution in mind regarding interrupts.

Originally posted by @t-moe in #17 (comment)

[Rahix]

Hi @t-moe,

I also thought about how to support pin change interrupts in port-expander. The situation is a bit tricky. We want to be able to reliably find out which event triggered the interrupt. But this isn't straight forward: Some port expanders expose a register that you can read once to get the interrupt state of all pins. After reading the register, this state is cleared again. So if we were to have per-pin methods, they would possibly erase events from other pins by accessing this flag register.

So an API for pin change interrupts always needs to query all pins of the expander at once. Or possibly mirror the flags locally such that we can track what pins fetched their flag already. But this quickly runs the risk of desynchronizing between the real flag and the locally cached one.

Not really sure what kind of solution works best here...

---

Beyond these specifics, I would not design the API using a async wait_ function but rather leave the "wait for interrupt" part to the user. They can wait on their interrupt line in whatever way they like and then just call some port-expander APIs to fetch the interrupt information from the chip. Keeping the waiting part outside makes things more flexible and doesn't lock the user into using embedded-hal-async.

[t-moe]

Hi @Rahix,
Thanks for your response and input regarding good API design.

Currently I mainly need the "wait for pin changed" interrupt, on all pins of the PI4OE5V6408 at once. But someone else might have a different requirement.

Which methods should I add to port_expander::dev::pi4ioe5v6408::Driver ?

listen_for_change(&mut self, mask: u8) -> Result<(),E> and get_interrupt_status(&mut self) -> Result<u8,E> to facilitate my specific usecase

or

(unsafe ?) fn write_reg(&mut self, reg: Regs, val: u8) -> Result<(),E> (or update_reg) and fn read_reg(&mut self, reg: Regs) -> Result<u8,E> to allow more flexible customization?

[Rahix]

I think it's easier to talk about code so I created #28 as an idea. Please take a look and let me know your thoughts.

The idea is that you get a port_expander::fetch_pin_change() function to fetch the change-state of a number of pins. This function will always reset the change state of all pins on the expander afterwards.

Similarly, a port_expander::fetch_interrupt_state() function can be used to fetch the pin-states that caused the interrupt. Similarly, it also resets the interrupt info for all pins on the expander.

What I'm unsure about is whether this is really a usable API for downstream applications. I very much dislike the fact that interrupt status needs to be queried in a central place with this API. You can't really handle the indivudual pins in isolation with this interface...

[Rahix]

Alright, because it's so much fun, here is a second alternative in #29. This one is better at decoupling the individual pins. The downside is that an explicit fetch_all_interrupt_state() is needed somewhere and missing that will lead to never receiving any interrupts (and no warning => hard to debug).

[t-moe]

I really depends a bit on the usecase.

In my case, I use the port-expander to wake the chip from deepsleep. Upon power-up I decode the interrupt status in a central place and map it to an enum WakeupSource. So #28 would work fine for me.

Additionally:

• [WIP] Sketch out an API for managing interrupts #28 is missing pin.listen() and I can not configure the interrupt mask.
• Having to manually call pin.fetch_all_interrupt_state() in [WIP] Sketch out alternative interrupts API #29 is really a bit unfortunate...

What if we combine #28 + #29 and offer both APIs to the user ?
port_expander::fetch_pin_change(pins) and pin.query_pin_change()

We could use the same traits under the hood.

The user can either call port_expander::fetch_pin_change(pins) which is straight-forward.
Or if he needs per-pin calls, he can call pin.fetch_all_interrupt_state() followed by pin.query_pin_change()