Bugfix and improvements for async GPIO

Author: robamu

examples/embassy/Cargo.toml

@@ -27,8 +27,8 @@ embassy-executor = { version = "0.7", features = [
   "executor-interrupt"
 ]}
 
-va108xx-hal = "0.9"
-va108xx-embassy = "0.1"
+va108xx-hal = { version = "0.9", path = "../../va108xx-hal" }
+va108xx-embassy = { version = "0.1", path = "../../va108xx-embassy" }
 
 [features]
 default = ["ticks-hz-1_000", "va108xx-embassy/irq-oc30-oc31"]

examples/embassy/src/bin/async-gpio.rs

@@ -14,7 +14,10 @@ use embedded_hal_async::digital::Wait;
 use panic_rtt_target as _;
 use rtt_target::{rprintln, rtt_init_print};
 use va108xx_embassy::embassy;
-use va108xx_hal::gpio::{on_interrupt_for_asynch_gpio, InputDynPinAsync, InputPinAsync, PinsB};
+use va108xx_hal::gpio::{
+    on_interrupt_for_async_gpio_port_a, on_interrupt_for_async_gpio_port_b, InputDynPinAsync,
+    InputPinAsync, PinsB,
+};
 use va108xx_hal::{
     gpio::{DynPin, PinsA},
     pac::{self, interrupt},
@@ -244,15 +247,16 @@ async fn output_task(
 }
 
 // PB22 to PB23 can be handled by both OC10 and OC11 depending on configuration.
-
 #[interrupt]
 #[allow(non_snake_case)]
 fn OC10() {
-    on_interrupt_for_asynch_gpio();
+    on_interrupt_for_async_gpio_port_a();
+    on_interrupt_for_async_gpio_port_b();
 }
 
+// This interrupt only handles PORT B interrupts.
 #[interrupt]
 #[allow(non_snake_case)]
 fn OC11() {
-    on_interrupt_for_asynch_gpio();
+    on_interrupt_for_async_gpio_port_b();
 }

va108xx-embassy/Cargo.toml

@@ -20,7 +20,7 @@ embassy-time-queue-utils = "0.1"
 
 once_cell = { version = "1", default-features = false, features = ["critical-section"] }
 
-va108xx-hal = "0.9"
+va108xx-hal = { version = "0.9", path = "../va108xx-hal" }
 
 [target.'cfg(all(target_arch = "arm", target_os = "none"))'.dependencies]
 portable-atomic = { version = "1", features = ["unsafe-assume-single-core"] }

va108xx-hal/CHANGELOG.md

@@ -17,6 +17,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ## Changed
 
 - Missing GPIO API replacements from `x` to `configure_x`
+- Split up generic GPIO interrupt handler into `on_interrupt_for_asynch_gpio`
+  into port specific variants.
+
+## Fixed
+
+- Bug in async GPIO interrupt handler where all enabled interrupts, even the ones which might
+  be unrelated to the pin, were disabled.
 
 ## [v0.9.0]
 

va108xx-hal/src/gpio/asynch.rs

@@ -3,9 +3,14 @@
 //! This module provides the [InputPinAsync] and [InputDynPinAsync] which both implement
 //! the [embedded_hal_async::digital::Wait] trait. These types allow for asynchronous waiting
 //! on GPIO pins. Please note that this module does not specify/declare the interrupt handlers
-//! which must be provided for async support to work. However, it provides one generic
-//! [handler][on_interrupt_for_asynch_gpio] which should be called in ALL user interrupt handlers
-//! which handle GPIO interrupts.
+//! which must be provided for async support to work. However, it provides two generic interrupt
+//! handlers:
+//!
+//!  - [on_interrupt_for_async_gpio_port_a]
+//!  - [on_interrupt_for_async_gpio_port_b]
+//!
+//! Those should be called in the interrupt handlers which handle GPIO interrupts for port A
+//! and/or port B.
 //!
 //! # Example
 //!
@@ -22,59 +27,80 @@ use crate::InterruptConfig;
 
 use super::{
     pin, DynGroup, DynPin, DynPinId, InputConfig, InterruptEdge, InvalidPinTypeError, Pin, PinId,
-    NUM_GPIO_PINS, NUM_PINS_PORT_A,
+    NUM_PINS_PORT_A, NUM_PINS_PORT_B,
 };
 
-static WAKERS: [AtomicWaker; NUM_GPIO_PINS] = [const { AtomicWaker::new() }; NUM_GPIO_PINS];
-static EDGE_DETECTION: [AtomicBool; NUM_GPIO_PINS] =
-    [const { AtomicBool::new(false) }; NUM_GPIO_PINS];
-
-#[inline]
-fn pin_id_to_offset(dyn_pin_id: DynPinId) -> usize {
-    match dyn_pin_id.group {
-        DynGroup::A => dyn_pin_id.num as usize,
-        DynGroup::B => NUM_PINS_PORT_A + dyn_pin_id.num as usize,
-    }
-}
+static WAKERS_FOR_PORT_A: [AtomicWaker; NUM_PINS_PORT_A] =
+    [const { AtomicWaker::new() }; NUM_PINS_PORT_A];
+static WAKERS_FOR_PORT_B: [AtomicWaker; NUM_PINS_PORT_B] =
+    [const { AtomicWaker::new() }; NUM_PINS_PORT_B];
+static EDGE_DETECTION_PORT_A: [AtomicBool; NUM_PINS_PORT_A] =
+    [const { AtomicBool::new(false) }; NUM_PINS_PORT_A];
+static EDGE_DETECTION_PORT_B: [AtomicBool; NUM_PINS_PORT_B] =
+    [const { AtomicBool::new(false) }; NUM_PINS_PORT_B];
 
-/// Generic interrupt handler for GPIO interrupts to support the async functionalities.
+/// Generic interrupt handler for GPIO interrupts on PORT A to support the async functionalities.
+///
+/// This function should be called in all interrupt hanflers which handle any PORT A GPIO
+/// interrupts.
 ///
 /// This handler will wake the correspoding wakers for the pins which triggered an interrupt
 /// as well as updating the static edge detection structures. This allows the pin future to
 /// complete async operations. The user should call this function in ALL interrupt handlers
 /// which handle any GPIO interrupts.
 #[inline]
-pub fn on_interrupt_for_asynch_gpio() {
+pub fn on_interrupt_for_async_gpio_port_a() {
     let periphs = unsafe { pac::Peripherals::steal() };
 
     handle_interrupt_for_gpio_and_port(
         periphs.porta.irq_enb().read().bits(),
         periphs.porta.edge_status().read().bits(),
-        0,
+        &WAKERS_FOR_PORT_A,
+        &EDGE_DETECTION_PORT_A,
     );
+}
+
+/// Generic interrupt handler for GPIO interrupts on PORT B to support the async functionalities.
+///
+/// This function should be called in all interrupt hanflers which handle any PORT B GPIO
+/// interrupts.
+///
+/// This handler will wake the correspoding wakers for the pins which triggered an interrupt
+/// as well as updating the static edge detection structures. This allows the pin future to
+/// complete async operations. The user should call this function in ALL interrupt handlers
+/// which handle any GPIO interrupts.
+#[inline]
+pub fn on_interrupt_for_async_gpio_port_b() {
+    let periphs = unsafe { pac::Peripherals::steal() };
+
     handle_interrupt_for_gpio_and_port(
         periphs.portb.irq_enb().read().bits(),
         periphs.portb.edge_status().read().bits(),
-        NUM_PINS_PORT_A,
+        &WAKERS_FOR_PORT_B,
+        &EDGE_DETECTION_PORT_B,
     );
 }
 
 // Uses the enabled interrupt register and the persistent edge status to capture all GPIO events.
 #[inline]
-fn handle_interrupt_for_gpio_and_port(mut irq_enb: u32, edge_status: u32, pin_base_offset: usize) {
+fn handle_interrupt_for_gpio_and_port(
+    mut irq_enb: u32,
+    edge_status: u32,
+    wakers: &'static [AtomicWaker],
+    edge_detection: &'static [AtomicBool],
+) {
     while irq_enb != 0 {
         let bit_pos = irq_enb.trailing_zeros() as usize;
         let bit_mask = 1 << bit_pos;
 
-        WAKERS[pin_base_offset + bit_pos].wake();
+        wakers[bit_pos].wake();
 
         if edge_status & bit_mask != 0 {
-            EDGE_DETECTION[pin_base_offset + bit_pos]
-                .store(true, core::sync::atomic::Ordering::Relaxed);
-        }
+            edge_detection[bit_pos].store(true, core::sync::atomic::Ordering::Relaxed);
 
-        // Clear the processed bit
-        irq_enb &= !bit_mask;
+            // Clear the processed bit
+            irq_enb &= !bit_mask;
+        }
     }
 }
 
@@ -85,6 +111,8 @@ fn handle_interrupt_for_gpio_and_port(mut irq_enb: u32, edge_status: u32, pin_ba
 /// struture is granted  to allow writing custom async structures.
 pub struct InputPinFuture {
     pin_id: DynPinId,
+    waker_group: &'static [AtomicWaker],
+    edge_detection_group: &'static [AtomicBool],
 }
 
 impl InputPinFuture {
@@ -102,6 +130,15 @@ impl InputPinFuture {
         Self::new_with_dyn_pin(pin, irq, edge, &mut periphs.sysconfig, &mut periphs.irqsel)
     }
 
+    pub fn pin_group_to_waker_and_edge_detection_group(
+        group: DynGroup,
+    ) -> (&'static [AtomicWaker], &'static [AtomicBool]) {
+        match group {
+            DynGroup::A => (WAKERS_FOR_PORT_A.as_ref(), EDGE_DETECTION_PORT_A.as_ref()),
+            DynGroup::B => (WAKERS_FOR_PORT_B.as_ref(), EDGE_DETECTION_PORT_B.as_ref()),
+        }
+    }
+
     pub fn new_with_dyn_pin(
         pin: &mut DynPin,
         irq: pac::Interrupt,
@@ -113,7 +150,9 @@ impl InputPinFuture {
             return Err(InvalidPinTypeError(pin.mode()));
         }
 
-        EDGE_DETECTION[pin_id_to_offset(pin.id())]
+        let (waker_group, edge_detection_group) =
+            Self::pin_group_to_waker_and_edge_detection_group(pin.id().group);
+        edge_detection_group[pin.id().num as usize]
             .store(false, core::sync::atomic::Ordering::Relaxed);
         pin.configure_edge_interrupt(
             edge,
@@ -122,7 +161,11 @@ impl InputPinFuture {
             Some(irq_sel),
         )
         .unwrap();
-        Ok(Self { pin_id: pin.id() })
+        Ok(Self {
+            pin_id: pin.id(),
+            waker_group,
+            edge_detection_group,
+        })
     }
 
     /// # Safety
@@ -146,15 +189,21 @@ impl InputPinFuture {
         sys_cfg: &mut Sysconfig,
         irq_sel: &mut Irqsel,
     ) -> Self {
-        EDGE_DETECTION[pin_id_to_offset(pin.id())]
+        let (waker_group, edge_detection_group) =
+            Self::pin_group_to_waker_and_edge_detection_group(pin.id().group);
+        edge_detection_group[pin.id().num as usize]
             .store(false, core::sync::atomic::Ordering::Relaxed);
         pin.configure_edge_interrupt(
             edge,
             InterruptConfig::new(irq, true, true),
             Some(sys_cfg),
             Some(irq_sel),
         );
-        Self { pin_id: pin.id() }
+        Self {
+            pin_id: pin.id(),
+            edge_detection_group,
+            waker_group,
+        }
     }
 }
 
@@ -181,9 +230,9 @@ impl Future for InputPinFuture {
         self: core::pin::Pin<&mut Self>,
         cx: &mut core::task::Context<'_>,
     ) -> core::task::Poll<Self::Output> {
-        let idx = pin_id_to_offset(self.pin_id);
-        WAKERS[idx].register(cx.waker());
-        if EDGE_DETECTION[idx].swap(false, core::sync::atomic::Ordering::Relaxed) {
+        let idx = self.pin_id.num as usize;
+        self.waker_group[idx].register(cx.waker());
+        if self.edge_detection_group[idx].swap(false, core::sync::atomic::Ordering::Relaxed) {
             return core::task::Poll::Ready(());
         }
         core::task::Poll::Pending
@@ -200,8 +249,8 @@ impl InputDynPinAsync {
     /// passed as well and is used to route and enable the interrupt.
     ///
     /// Please note that the interrupt handler itself must be provided by the user and the
-    /// generic [on_interrupt_for_asynch_gpio] function must be called inside that function for
-    /// the asynchronous functionality to work.
+    /// generic [on_interrupt_for_async_gpio_port_a] or [on_interrupt_for_async_gpio_port_b]
+    /// function must be called inside that function for the asynchronous functionality to work.
     pub fn new(pin: DynPin, irq: pac::Interrupt) -> Result<Self, InvalidPinTypeError> {
         if !pin.is_input_pin() {
             return Err(InvalidPinTypeError(pin.mode()));
@@ -335,8 +384,8 @@ impl<I: PinId, C: InputConfig> InputPinAsync<I, C> {
     /// passed as well and is used to route and enable the interrupt.
     ///
     /// Please note that the interrupt handler itself must be provided by the user and the
-    /// generic [on_interrupt_for_asynch_gpio] function must be called inside that function for
-    /// the asynchronous functionality to work.
+    /// generic [on_interrupt_for_async_gpio_port_a] or [on_interrupt_for_async_gpio_port_b]
+    /// function must be called inside that function for the asynchronous functionality to work.
     pub fn new(pin: Pin<I, pin::Input<C>>, irq: pac::Interrupt) -> Self {
         Self { pin, irq }
     }