From 7fddac5a80b7082cf14645cee522799360d5949b Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 14 Feb 2025 17:09:37 +0100 Subject: [PATCH] Bugfix and improvements for async GPIO --- examples/embassy/Cargo.toml | 4 +- examples/embassy/src/bin/async-gpio.rs | 12 ++- va108xx-embassy/Cargo.toml | 2 +- va108xx-hal/CHANGELOG.md | 7 ++ va108xx-hal/src/gpio/asynch.rs | 124 +++++++++++++++++-------- 5 files changed, 105 insertions(+), 44 deletions(-) diff --git a/examples/embassy/Cargo.toml b/examples/embassy/Cargo.toml index 3589c24..405132e 100644 --- a/examples/embassy/Cargo.toml +++ b/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"] diff --git a/examples/embassy/src/bin/async-gpio.rs b/examples/embassy/src/bin/async-gpio.rs index 32e4e79..cc95a5d 100644 --- a/examples/embassy/src/bin/async-gpio.rs +++ b/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(); } diff --git a/va108xx-embassy/Cargo.toml b/va108xx-embassy/Cargo.toml index 1b519b5..a2b01f6 100644 --- a/va108xx-embassy/Cargo.toml +++ 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"] } diff --git a/va108xx-hal/CHANGELOG.md b/va108xx-hal/CHANGELOG.md index cd01c09..d0bd579 100644 --- a/va108xx-hal/CHANGELOG.md +++ b/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] diff --git a/va108xx-hal/src/gpio/asynch.rs b/va108xx-hal/src/gpio/asynch.rs index 40ccd14..2c60f49 100644 --- a/va108xx-hal/src/gpio/asynch.rs +++ b/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]; +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]; -#[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, - } -} - -/// 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,16 @@ impl InputPinFuture { Self::new_with_dyn_pin(pin, irq, edge, &mut periphs.sysconfig, &mut periphs.irqsel) } + #[inline] + 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 +151,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 +162,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,7 +190,9 @@ 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, @@ -154,7 +200,11 @@ impl InputPinFuture { Some(sys_cfg), Some(irq_sel), ); - Self { pin_id: pin.id() } + Self { + pin_id: pin.id(), + edge_detection_group, + waker_group, + } } } @@ -181,9 +231,9 @@ impl Future for InputPinFuture { self: core::pin::Pin<&mut Self>, cx: &mut core::task::Context<'_>, ) -> core::task::Poll { - 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 +250,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 { if !pin.is_input_pin() { return Err(InvalidPinTypeError(pin.mode())); @@ -335,8 +385,8 @@ impl InputPinAsync { /// 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>, irq: pac::Interrupt) -> Self { Self { pin, irq } }