From c6424fdc112776b5ceeef4a01c56b1479c2901c5 Mon Sep 17 00:00:00 2001 From: pennae Date: Tue, 2 May 2023 08:29:32 +0200 Subject: [PATCH 1/3] gp/gpio: fix InputFuture edge waits InputFuture did not use and check edge interrupts correctly. InterruptTrigger should've checked for not 1,2,3,4 but 1,2,4,8 since the inte fields are bitmasks, and not clearing INTR would have repeatedly triggered edge interrupts early. --- embassy-rp/src/gpio.rs | 126 ++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 83 deletions(-) diff --git a/embassy-rp/src/gpio.rs b/embassy-rp/src/gpio.rs index 98e18286..7a86418a 100644 --- a/embassy-rp/src/gpio.rs +++ b/embassy-rp/src/gpio.rs @@ -136,18 +136,6 @@ pub enum InterruptTrigger { AnyEdge, } -impl InterruptTrigger { - fn from_u32(value: u32) -> Option { - match value { - 1 => Some(InterruptTrigger::LevelLow), - 2 => Some(InterruptTrigger::LevelHigh), - 3 => Some(InterruptTrigger::EdgeLow), - 4 => Some(InterruptTrigger::EdgeHigh), - _ => None, - } - } -} - #[interrupt] unsafe fn IO_IRQ_BANK0() { let cpu = SIO.cpuid().read() as usize; @@ -166,26 +154,16 @@ unsafe fn IO_IRQ_BANK0() { let pin_group = (pin % 8) as usize; let event = (intsx.read().0 >> pin_group * 4) & 0xf as u32; - if let Some(trigger) = InterruptTrigger::from_u32(event) { + // no more than one event can be awaited per pin at any given time, so + // we can just clear all interrupt enables for that pin without having + // to check which event was signalled. + if event != 0 { critical_section::with(|_| { - proc_intx.inte(pin / 8).modify(|w| match trigger { - InterruptTrigger::AnyEdge => { - w.set_edge_high(pin_group, false); - w.set_edge_low(pin_group, false); - } - InterruptTrigger::LevelHigh => { - trace!("IO_IRQ_BANK0 pin {} LevelHigh triggered", pin); - w.set_level_high(pin_group, false); - } - InterruptTrigger::LevelLow => { - w.set_level_low(pin_group, false); - } - InterruptTrigger::EdgeHigh => { - w.set_edge_high(pin_group, false); - } - InterruptTrigger::EdgeLow => { - w.set_edge_low(pin_group, false); - } + proc_intx.inte(pin / 8).modify(|w| { + w.set_edge_high(pin_group, false); + w.set_edge_low(pin_group, false); + w.set_level_high(pin_group, false); + w.set_level_low(pin_group, false); }); }); INTERRUPT_WAKERS[pin as usize].wake(); @@ -207,10 +185,23 @@ impl<'d, T: Pin> InputFuture<'d, T> { irq.disable(); irq.set_priority(interrupt::Priority::P3); + let pin_group = (pin.pin() % 8) as usize; + // first, clear the INTR register bits. without this INTR will still + // contain reports of previous edges, causing the IRQ to fire early + // on stale state. clearing these means that we can only detect edges + // that occur *after* the clear happened, but since both this and the + // alternative are fundamentally racy it's probably fine. + // (the alternative being checking the current level and waiting for + // its inverse, but that requires reading the current level and thus + // missing anything that happened before the level was read.) + pac::IO_BANK0.intr(pin.pin() as usize / 8).write(|w| { + w.set_edge_high(pin_group, true); + w.set_edge_low(pin_group, true); + }); + // Each INTR register is divided into 8 groups, one group for each // pin, and each group consists of LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, // and EGDE_HIGH. - let pin_group = (pin.pin() % 8) as usize; critical_section::with(|_| { pin.int_proc().inte((pin.pin() / 8) as usize).modify(|w| match level { InterruptTrigger::LevelHigh => { @@ -227,7 +218,8 @@ impl<'d, T: Pin> InputFuture<'d, T> { w.set_edge_low(pin_group, true); } InterruptTrigger::AnyEdge => { - // noop + w.set_edge_high(pin_group, true); + w.set_edge_low(pin_group, true); } }); }); @@ -257,47 +249,21 @@ impl<'d, T: Pin> Future for InputFuture<'d, T> { // LEVEL_HIGH, EDGE_LOW, and EDGE_HIGH for each pin. let pin_group = (self.pin.pin() % 8) as usize; - // This should check the the level of the interrupt trigger level of - // the pin and if it has been disabled that means it was done by the - // interrupt service routine, so we then know that the event/trigger - // happened and Poll::Ready will be returned. - trace!("{:?} for pin {}", self.level, self.pin.pin()); - match self.level { - InterruptTrigger::AnyEdge => { - if !inte.edge_high(pin_group) && !inte.edge_low(pin_group) { - #[rustfmt::skip] - trace!("{:?} for pin {} was cleared, return Poll::Ready", self.level, self.pin.pin()); - return Poll::Ready(()); - } - } - InterruptTrigger::LevelHigh => { - if !inte.level_high(pin_group) { - #[rustfmt::skip] - trace!("{:?} for pin {} was cleared, return Poll::Ready", self.level, self.pin.pin()); - return Poll::Ready(()); - } - } - InterruptTrigger::LevelLow => { - if !inte.level_low(pin_group) { - #[rustfmt::skip] - trace!("{:?} for pin {} was cleared, return Poll::Ready", self.level, self.pin.pin()); - return Poll::Ready(()); - } - } - InterruptTrigger::EdgeHigh => { - if !inte.edge_high(pin_group) { - #[rustfmt::skip] - trace!("{:?} for pin {} was cleared, return Poll::Ready", self.level, self.pin.pin()); - return Poll::Ready(()); - } - } - InterruptTrigger::EdgeLow => { - if !inte.edge_low(pin_group) { - #[rustfmt::skip] - trace!("{:?} for pin {} was cleared, return Poll::Ready", self.level, self.pin.pin()); - return Poll::Ready(()); - } - } + // since the interrupt handler clears all INTE flags we'll check that + // all have been cleared and unconditionally return Ready(()) if so. + // we don't need further handshaking since only a single event wait + // is possible for any given pin at any given time. + if !inte.edge_high(pin_group) + && !inte.edge_low(pin_group) + && !inte.level_high(pin_group) + && !inte.level_low(pin_group) + { + trace!( + "{:?} for pin {} was cleared, return Poll::Ready", + self.level, + self.pin.pin() + ); + return Poll::Ready(()); } trace!("InputFuture::poll return Poll::Pending"); Poll::Pending @@ -644,23 +610,17 @@ impl<'d, T: Pin> Flex<'d, T> { #[inline] pub async fn wait_for_rising_edge(&mut self) { - self.wait_for_low().await; - self.wait_for_high().await; + InputFuture::new(&mut self.pin, InterruptTrigger::EdgeHigh).await; } #[inline] pub async fn wait_for_falling_edge(&mut self) { - self.wait_for_high().await; - self.wait_for_low().await; + InputFuture::new(&mut self.pin, InterruptTrigger::EdgeLow).await; } #[inline] pub async fn wait_for_any_edge(&mut self) { - if self.is_high() { - self.wait_for_low().await; - } else { - self.wait_for_high().await; - } + InputFuture::new(&mut self.pin, InterruptTrigger::AnyEdge).await; } } From 8fc92fdf6285190a1ba5ddf356958d49ed4225a3 Mon Sep 17 00:00:00 2001 From: pennae Date: Tue, 2 May 2023 08:34:36 +0200 Subject: [PATCH 2/3] rp/gpio: drop critical_section use we don't need critical sections if we just use atomic access aliases. --- embassy-rp/src/gpio.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/embassy-rp/src/gpio.rs b/embassy-rp/src/gpio.rs index 7a86418a..66b9af04 100644 --- a/embassy-rp/src/gpio.rs +++ b/embassy-rp/src/gpio.rs @@ -9,7 +9,7 @@ use embassy_sync::waitqueue::AtomicWaker; use crate::pac::common::{Reg, RW}; use crate::pac::SIO; -use crate::{interrupt, pac, peripherals, Peripheral}; +use crate::{interrupt, pac, peripherals, Peripheral, RegExt}; const PIN_COUNT: usize = 30; const NEW_AW: AtomicWaker = AtomicWaker::new(); @@ -158,13 +158,11 @@ unsafe fn IO_IRQ_BANK0() { // we can just clear all interrupt enables for that pin without having // to check which event was signalled. if event != 0 { - critical_section::with(|_| { - proc_intx.inte(pin / 8).modify(|w| { - w.set_edge_high(pin_group, false); - w.set_edge_low(pin_group, false); - w.set_level_high(pin_group, false); - w.set_level_low(pin_group, false); - }); + proc_intx.inte(pin / 8).write_clear(|w| { + w.set_edge_high(pin_group, true); + w.set_edge_low(pin_group, true); + w.set_level_high(pin_group, true); + w.set_level_low(pin_group, true); }); INTERRUPT_WAKERS[pin as usize].wake(); } @@ -202,8 +200,9 @@ impl<'d, T: Pin> InputFuture<'d, T> { // Each INTR register is divided into 8 groups, one group for each // pin, and each group consists of LEVEL_LOW, LEVEL_HIGH, EDGE_LOW, // and EGDE_HIGH. - critical_section::with(|_| { - pin.int_proc().inte((pin.pin() / 8) as usize).modify(|w| match level { + pin.int_proc() + .inte((pin.pin() / 8) as usize) + .write_set(|w| match level { InterruptTrigger::LevelHigh => { trace!("InputFuture::new enable LevelHigh for pin {}", pin.pin()); w.set_level_high(pin_group, true); @@ -222,7 +221,6 @@ impl<'d, T: Pin> InputFuture<'d, T> { w.set_edge_low(pin_group, true); } }); - }); irq.enable(); } From 849011b8261194629830b4b85d062394e8eb3c58 Mon Sep 17 00:00:00 2001 From: pennae Date: Tue, 2 May 2023 08:39:05 +0200 Subject: [PATCH 3/3] rp/gpio: set up gpio interrupts only once doing this setup work repeatedly, on every wait, is unnecessary. with nothing ever disabling the interrupt it is sufficient to enable it once during device init and never touch it again. --- embassy-rp/src/gpio.rs | 13 +++--- embassy-rp/src/lib.rs | 1 + embassy-rp/src/multicore.rs | 5 ++- tests/rp/src/bin/gpio_multicore.rs | 63 ++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 7 deletions(-) create mode 100644 tests/rp/src/bin/gpio_multicore.rs diff --git a/embassy-rp/src/gpio.rs b/embassy-rp/src/gpio.rs index 66b9af04..da8efba9 100644 --- a/embassy-rp/src/gpio.rs +++ b/embassy-rp/src/gpio.rs @@ -136,6 +136,13 @@ pub enum InterruptTrigger { AnyEdge, } +pub(crate) unsafe fn init() { + let irq = interrupt::IO_IRQ_BANK0::steal(); + irq.disable(); + irq.set_priority(interrupt::Priority::P3); + irq.enable(); +} + #[interrupt] unsafe fn IO_IRQ_BANK0() { let cpu = SIO.cpuid().read() as usize; @@ -179,10 +186,6 @@ impl<'d, T: Pin> InputFuture<'d, T> { pub fn new(pin: impl Peripheral

+ 'd, level: InterruptTrigger) -> Self { into_ref!(pin); unsafe { - let irq = interrupt::IO_IRQ_BANK0::steal(); - irq.disable(); - irq.set_priority(interrupt::Priority::P3); - let pin_group = (pin.pin() % 8) as usize; // first, clear the INTR register bits. without this INTR will still // contain reports of previous edges, causing the IRQ to fire early @@ -221,8 +224,6 @@ impl<'d, T: Pin> InputFuture<'d, T> { w.set_edge_low(pin_group, true); } }); - - irq.enable(); } Self { pin, level } diff --git a/embassy-rp/src/lib.rs b/embassy-rp/src/lib.rs index d69d12a3..cba7559d 100644 --- a/embassy-rp/src/lib.rs +++ b/embassy-rp/src/lib.rs @@ -157,6 +157,7 @@ pub fn init(_config: config::Config) -> Peripherals { timer::init(); dma::init(); pio::init(); + gpio::init(); } peripherals diff --git a/embassy-rp/src/multicore.rs b/embassy-rp/src/multicore.rs index e51fc218..c84fea5c 100644 --- a/embassy-rp/src/multicore.rs +++ b/embassy-rp/src/multicore.rs @@ -33,7 +33,7 @@ use core::sync::atomic::{compiler_fence, AtomicBool, Ordering}; use crate::interrupt::{Interrupt, InterruptExt}; use crate::peripherals::CORE1; -use crate::{interrupt, pac}; +use crate::{gpio, interrupt, pac}; const PAUSE_TOKEN: u32 = 0xDEADBEEF; const RESUME_TOKEN: u32 = !0xDEADBEEF; @@ -68,6 +68,9 @@ fn install_stack_guard(stack_bottom: *mut usize) { #[inline(always)] fn core1_setup(stack_bottom: *mut usize) { install_stack_guard(stack_bottom); + unsafe { + gpio::init(); + } } /// Data type for a properly aligned stack of N bytes diff --git a/tests/rp/src/bin/gpio_multicore.rs b/tests/rp/src/bin/gpio_multicore.rs new file mode 100644 index 00000000..6c13ccaa --- /dev/null +++ b/tests/rp/src/bin/gpio_multicore.rs @@ -0,0 +1,63 @@ +#![no_std] +#![no_main] +#![feature(type_alias_impl_trait)] + +use defmt::{info, unwrap}; +use embassy_executor::Executor; +use embassy_executor::_export::StaticCell; +use embassy_rp::gpio::{Input, Level, Output, Pull}; +use embassy_rp::multicore::{spawn_core1, Stack}; +use embassy_rp::peripherals::{PIN_0, PIN_1}; +use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex; +use embassy_sync::channel::Channel; +use {defmt_rtt as _, panic_probe as _}; + +static mut CORE1_STACK: Stack<1024> = Stack::new(); +static EXECUTOR0: StaticCell = StaticCell::new(); +static EXECUTOR1: StaticCell = StaticCell::new(); +static CHANNEL0: Channel = Channel::new(); +static CHANNEL1: Channel = Channel::new(); + +#[cortex_m_rt::entry] +fn main() -> ! { + let p = embassy_rp::init(Default::default()); + spawn_core1(p.CORE1, unsafe { &mut CORE1_STACK }, move || { + let executor1 = EXECUTOR1.init(Executor::new()); + executor1.run(|spawner| unwrap!(spawner.spawn(core1_task(p.PIN_1)))); + }); + let executor0 = EXECUTOR0.init(Executor::new()); + executor0.run(|spawner| unwrap!(spawner.spawn(core0_task(p.PIN_0)))); +} + +#[embassy_executor::task] +async fn core0_task(p: PIN_0) { + info!("CORE0 is running"); + + let mut pin = Output::new(p, Level::Low); + + CHANNEL0.send(()).await; + CHANNEL1.recv().await; + + pin.set_high(); + + CHANNEL1.recv().await; + + info!("Test OK"); + cortex_m::asm::bkpt(); +} + +#[embassy_executor::task] +async fn core1_task(p: PIN_1) { + info!("CORE1 is running"); + + CHANNEL0.recv().await; + + let mut pin = Input::new(p, Pull::Down); + let wait = pin.wait_for_rising_edge(); + + CHANNEL1.send(()).await; + + wait.await; + + CHANNEL1.send(()).await; +}