From 8b55d0923f796d7d6de8470303966a8d0cc53ad5 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 | 11 +- va108xx-embassy/Cargo.toml | 2 +- va108xx-hal/CHANGELOG.md | 8 ++ va108xx-hal/src/gpio/asynch.rs | 136 +++++++++++++++---------- va108xx-hal/src/gpio/dynpin.rs | 18 ++-- va108xx-hal/src/gpio/mod.rs | 39 ++++++- va108xx-hal/src/gpio/pin.rs | 32 +----- va108xx-hal/src/gpio/reg.rs | 14 +-- 9 files changed, 153 insertions(+), 111 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..2a44cc4 100644 --- a/examples/embassy/src/bin/async-gpio.rs +++ b/examples/embassy/src/bin/async-gpio.rs @@ -14,7 +14,9 @@ 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_for_port, InputDynPinAsync, InputPinAsync, PinsB, Port, +}; use va108xx_hal::{ gpio::{DynPin, PinsA}, pac::{self, interrupt}, @@ -244,15 +246,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_for_port(Port::A); + on_interrupt_for_async_gpio_for_port(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_for_port(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..e102d7c 100644 --- a/va108xx-hal/CHANGELOG.md +++ b/va108xx-hal/CHANGELOG.md @@ -17,6 +17,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## Changed - Missing GPIO API replacements from `x` to `configure_x` +- Renamed GPIO `DynGroup` to `Port` +- Rename generic GPIO interrupt handler into `on_interrupt_for_asynch_gpio` + into `on_interrupt_for_async_gpio_for_port` which expects a Port argument + +## 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..7e23648 100644 --- a/va108xx-hal/src/gpio/asynch.rs +++ b/va108xx-hal/src/gpio/asynch.rs @@ -3,9 +3,9 @@ //! 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 the +//! [on_interrupt_for_async_gpio_for_port] generic interrupt handler. This should be called in all +//! IRQ functions which handle any GPIO interrupts with the corresponding [Port] argument. //! //! # Example //! @@ -21,60 +21,66 @@ use va108xx::{self as pac, Irqsel, Sysconfig}; use crate::InterruptConfig; use super::{ - pin, DynGroup, DynPin, DynPinId, InputConfig, InterruptEdge, InvalidPinTypeError, Pin, PinId, - NUM_GPIO_PINS, NUM_PINS_PORT_A, + pin, DynPin, DynPinId, InputConfig, InterruptEdge, InvalidPinTypeError, Pin, PinId, Port, + 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 a specific port to support async functionalities /// -/// 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() { +/// This function should be called in all interrupt handlers which handle any GPIO interrupts +/// matching the [Port] argument. +/// The handler will wake the corresponding wakers for the pins that triggered an interrupts +/// as well as update the static edge detection structures. This allows the pin future tocomplete +/// complete async operations. +pub fn on_interrupt_for_async_gpio_for_port(port: Port) { 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, - ); - handle_interrupt_for_gpio_and_port( - periphs.portb.irq_enb().read().bits(), - periphs.portb.edge_status().read().bits(), - NUM_PINS_PORT_A, - ); + let (irq_enb, edge_status, wakers, edge_detection) = match port { + Port::A => ( + periphs.porta.irq_enb().read().bits(), + periphs.porta.edge_status().read().bits(), + WAKERS_FOR_PORT_A.as_ref(), + EDGE_DETECTION_PORT_A.as_ref(), + ), + Port::B => ( + periphs.portb.irq_enb().read().bits(), + periphs.portb.edge_status().read().bits(), + WAKERS_FOR_PORT_B.as_ref(), + EDGE_DETECTION_PORT_B.as_ref(), + ), + }; + + on_interrupt_for_port(irq_enb, edge_status, wakers, edge_detection); } -// 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 on_interrupt_for_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 +91,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 +110,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: Port, + ) -> (&'static [AtomicWaker], &'static [AtomicBool]) { + match group { + Port::A => (WAKERS_FOR_PORT_A.as_ref(), EDGE_DETECTION_PORT_A.as_ref()), + Port::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 +131,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 +142,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 +170,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,14 +180,18 @@ impl InputPinFuture { Some(sys_cfg), Some(irq_sel), ); - Self { pin_id: pin.id() } + Self { + pin_id: pin.id(), + edge_detection_group, + waker_group, + } } } impl Drop for InputPinFuture { fn drop(&mut self) { let periphs = unsafe { pac::Peripherals::steal() }; - if self.pin_id.group == DynGroup::A { + if self.pin_id.group == Port::A { periphs .porta .irq_enb() @@ -181,9 +211,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 +230,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_for_port] 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 +365,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_for_port] 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 } } diff --git a/va108xx-hal/src/gpio/dynpin.rs b/va108xx-hal/src/gpio/dynpin.rs index 4dc56ee..29725d6 100644 --- a/va108xx-hal/src/gpio/dynpin.rs +++ b/va108xx-hal/src/gpio/dynpin.rs @@ -57,9 +57,9 @@ //! [InvalidPinTypeError]. use super::{ - pin::{FilterType, InterruptEdge, InterruptLevel, Pin, PinId, PinMode, PinState}, + pin::{FilterType, Pin, PinId, PinMode}, reg::RegisterInterface, - InputDynPinAsync, + InputDynPinAsync, InterruptEdge, InterruptLevel, PinState, }; use crate::{clock::FilterClkSel, enable_nvic_interrupt, pac, FunSel, InterruptConfig}; @@ -156,19 +156,13 @@ pub const DYN_ALT_FUNC_3: DynPinMode = DynPinMode::Alternate(DynAlternate::Sel3) // DynGroup & DynPinId //================================================================================================== -/// Value-level `enum` for pin groups -#[derive(Debug, PartialEq, Eq, Clone, Copy)] -#[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum DynGroup { - A, - B, -} +pub type DynGroup = super::Port; /// Value-level `struct` representing pin IDs #[derive(Debug, PartialEq, Eq, Clone, Copy)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct DynPinId { - pub group: DynGroup, + pub group: super::Port, pub num: u8, } @@ -369,12 +363,12 @@ impl DynPin { if irq_cfg.route { match self.regs.id().group { // Set the correct interrupt number in the IRQSEL register - DynGroup::A => { + super::Port::A => { irqsel .porta0(self.regs.id().num as usize) .write(|w| unsafe { w.bits(irq_cfg.id as u32) }); } - DynGroup::B => { + super::Port::B => { irqsel .portb0(self.regs.id().num as usize) .write(|w| unsafe { w.bits(irq_cfg.id as u32) }); diff --git a/va108xx-hal/src/gpio/mod.rs b/va108xx-hal/src/gpio/mod.rs index af1084e..8a521fa 100644 --- a/va108xx-hal/src/gpio/mod.rs +++ b/va108xx-hal/src/gpio/mod.rs @@ -22,14 +22,47 @@ //! //! - [Blinky example](https://egit.irs.uni-stuttgart.de/rust/va108xx-rs/src/branch/main/examples/simple/examples/blinky.rs) +//================================================================================================== +// Errors, Definitions and Constants +//================================================================================================== + +pub const NUM_PINS_PORT_A: usize = 32; +pub const NUM_PINS_PORT_B: usize = 24; +pub const NUM_GPIO_PINS: usize = NUM_PINS_PORT_A + NUM_PINS_PORT_B; + #[derive(Debug, PartialEq, Eq, thiserror::Error)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] #[error("The pin is masked")] pub struct IsMaskedError; -pub const NUM_PINS_PORT_A: usize = 32; -pub const NUM_PINS_PORT_B: usize = 24; -pub const NUM_GPIO_PINS: usize = NUM_PINS_PORT_A + NUM_PINS_PORT_B; +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum Port { + A, + B, +} + +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum InterruptEdge { + HighToLow, + LowToHigh, + BothEdges, +} + +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum InterruptLevel { + Low = 0, + High = 1, +} + +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum PinState { + Low = 0, + High = 1, +} pub mod dynpin; pub use dynpin::*; diff --git a/va108xx-hal/src/gpio/pin.rs b/va108xx-hal/src/gpio/pin.rs index 14ab13b..1e11fde 100644 --- a/va108xx-hal/src/gpio/pin.rs +++ b/va108xx-hal/src/gpio/pin.rs @@ -70,9 +70,9 @@ //! This module implements all of the embedded HAL GPIO traits for each [`Pin`] //! in the corresponding [`PinMode`]s, namely: [`InputPin`], [`OutputPin`], //! and [`StatefulOutputPin`]. -use super::dynpin::{DynAlternate, DynGroup, DynInput, DynOutput, DynPinId, DynPinMode}; +use super::dynpin::{DynAlternate, DynInput, DynOutput, DynPinId, DynPinMode}; use super::reg::RegisterInterface; -use super::{DynPin, InputPinAsync}; +use super::{DynPin, InputPinAsync, InterruptEdge, InterruptLevel, PinState, Port}; use crate::{ pac::{Irqsel, Porta, Portb, Sysconfig}, typelevel::Sealed, @@ -84,32 +84,6 @@ use core::mem::transmute; use embedded_hal::digital::{InputPin, OutputPin, StatefulOutputPin}; use paste::paste; -//================================================================================================== -// Errors and Definitions -//================================================================================================== - -#[derive(Debug, PartialEq, Eq)] -#[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum InterruptEdge { - HighToLow, - LowToHigh, - BothEdges, -} - -#[derive(Debug, PartialEq, Eq)] -#[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum InterruptLevel { - Low = 0, - High = 1, -} - -#[derive(Debug, PartialEq, Eq)] -#[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum PinState { - Low = 0, - High = 1, -} - //================================================================================================== // Input configuration //================================================================================================== @@ -321,7 +295,7 @@ macro_rules! pin_id { impl Sealed for $Id {} impl PinId for $Id { const DYN: DynPinId = DynPinId { - group: DynGroup::$Group, + group: Port::$Group, num: $NUM, }; } diff --git a/va108xx-hal/src/gpio/reg.rs b/va108xx-hal/src/gpio/reg.rs index d8d4993..55f4b11 100644 --- a/va108xx-hal/src/gpio/reg.rs +++ b/va108xx-hal/src/gpio/reg.rs @@ -1,6 +1,6 @@ -use super::dynpin::{self, DynGroup, DynPinId, DynPinMode}; -use super::pin::{FilterType, InterruptEdge, InterruptLevel, PinState}; -use super::IsMaskedError; +use super::dynpin::{self, DynPinId, DynPinMode}; +use super::pin::FilterType; +use super::{InterruptEdge, InterruptLevel, IsMaskedError, PinState, Port}; use crate::clock::FilterClkSel; use va108xx::{ioconfig, porta}; @@ -146,15 +146,15 @@ pub(super) unsafe trait RegisterInterface { #[inline] fn port_reg(&self) -> &PortRegisterBlock { match self.id().group { - DynGroup::A => unsafe { &(*Self::PORTA) }, - DynGroup::B => unsafe { &(*Self::PORTB) }, + Port::A => unsafe { &(*Self::PORTA) }, + Port::B => unsafe { &(*Self::PORTB) }, } } fn iocfg_port(&self) -> &PortReg { let ioconfig = unsafe { va108xx::Ioconfig::ptr().as_ref().unwrap() }; match self.id().group { - DynGroup::A => ioconfig.porta(self.id().num as usize), - DynGroup::B => ioconfig.portb0(self.id().num as usize), + Port::A => ioconfig.porta(self.id().num as usize), + Port::B => ioconfig.portb0(self.id().num as usize), } }