Merge #1423

1423: rp: fix gpio InputFuture and inefficiencies r=pennae a=pennae

InputFuture could not wait for edges without breaking due to a broken From impl, but even if the impl had been correct it would not have worked correctly because raw edge interrupts are sticky and must be cleared from software. also replace critical sections with atomic accesses, and do nvic setup only once.

Co-authored-by: pennae <github@quasiparticle.net>

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();
@@ -136,16 +136,11 @@ pub enum InterruptTrigger {
     AnyEdge,
 }
 
-impl InterruptTrigger {
+pub(crate) unsafe fn init() {
-    fn from_u32(value: u32) -> Option<InterruptTrigger> {
+    let irq = interrupt::IO_IRQ_BANK0::steal();
-        match value {
+    irq.disable();
-            1 => Some(InterruptTrigger::LevelLow),
+    irq.set_priority(interrupt::Priority::P3);
-            2 => Some(InterruptTrigger::LevelHigh),
+    irq.enable();
-            3 => Some(InterruptTrigger::EdgeLow),
-            4 => Some(InterruptTrigger::EdgeHigh),
-            _ => None,
-        }
-    }
 }
 
 #[interrupt]
@@ -166,27 +161,15 @@ 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
-            critical_section::with(|_| {
+        // we can just clear all interrupt enables for that pin without having
-                proc_intx.inte(pin / 8).modify(|w| match trigger {
+        // to check which event was signalled.
-                    InterruptTrigger::AnyEdge => {
+        if event != 0 {
-                        w.set_edge_high(pin_group, false);
+            proc_intx.inte(pin / 8).write_clear(|w| {
-                        w.set_edge_low(pin_group, false);
+                w.set_edge_high(pin_group, true);
-                    }
+                w.set_edge_low(pin_group, true);
-                    InterruptTrigger::LevelHigh => {
+                w.set_level_high(pin_group, true);
-                        trace!("IO_IRQ_BANK0 pin {} LevelHigh triggered", pin);
+                w.set_level_low(pin_group, true);
-                        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);
-                    }
-                });
             });
             INTERRUPT_WAKERS[pin as usize].wake();
         }
@@ -203,16 +186,26 @@ impl<'d, T: Pin> InputFuture<'d, T> {
     pub fn new(pin: impl Peripheral<P = T> + 'd, level: InterruptTrigger) -> Self {
         into_ref!(pin);
         unsafe {
-            let irq = interrupt::IO_IRQ_BANK0::steal();
+            let pin_group = (pin.pin() % 8) as usize;
-            irq.disable();
+            // first, clear the INTR register bits. without this INTR will still
-            irq.set_priority(interrupt::Priority::P3);
+            // 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;
+            pin.int_proc()
-            critical_section::with(|_| {
+                .inte((pin.pin() / 8) as usize)
-                pin.int_proc().inte((pin.pin() / 8) as usize).modify(|w| match level {
+                .write_set(|w| match level {
                     InterruptTrigger::LevelHigh => {
                         trace!("InputFuture::new enable LevelHigh for pin {}", pin.pin());
                         w.set_level_high(pin_group, true);
@@ -227,12 +220,10 @@ 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);
                     }
                 });
-            });
-
-            irq.enable();
         }
 
         Self { pin, level }
@@ -257,48 +248,22 @@ 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
+        // since the interrupt handler clears all INTE flags we'll check that
-        // the pin and if it has been disabled that means it was done by the
+        // all have been cleared and unconditionally return Ready(()) if so.
-        // interrupt service routine, so we then know that the event/trigger
+        // we don't need further handshaking since only a single event wait
-        // happened and Poll::Ready will be returned.
+        // is possible for any given pin at any given time.
-        trace!("{:?} for pin {}", self.level, self.pin.pin());
+        if !inte.edge_high(pin_group)
-        match self.level {
+            && !inte.edge_low(pin_group)
-            InterruptTrigger::AnyEdge => {
+            && !inte.level_high(pin_group)
-                if !inte.edge_high(pin_group) && !inte.edge_low(pin_group) {
+            && !inte.level_low(pin_group)
-                    #[rustfmt::skip]
+        {
-                    trace!("{:?} for pin {} was cleared, return Poll::Ready", self.level, self.pin.pin());
+            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(());
-                }
-            }
-        }
         trace!("InputFuture::poll return Poll::Pending");
         Poll::Pending
     }
@@ -644,23 +609,17 @@ impl<'d, T: Pin> Flex<'d, T> {
 
     #[inline]
     pub async fn wait_for_rising_edge(&mut self) {
-        self.wait_for_low().await;
+        InputFuture::new(&mut self.pin, InterruptTrigger::EdgeHigh).await;
-        self.wait_for_high().await;
     }
 
     #[inline]
     pub async fn wait_for_falling_edge(&mut self) {
-        self.wait_for_high().await;
+        InputFuture::new(&mut self.pin, InterruptTrigger::EdgeLow).await;
-        self.wait_for_low().await;
     }
 
     #[inline]
     pub async fn wait_for_any_edge(&mut self) {
-        if self.is_high() {
+        InputFuture::new(&mut self.pin, InterruptTrigger::AnyEdge).await;
-            self.wait_for_low().await;
-        } else {
-            self.wait_for_high().await;
-        }
     }
 }
 

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

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

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<Executor> = StaticCell::new();
+static EXECUTOR1: StaticCell<Executor> = StaticCell::new();
+static CHANNEL0: Channel<CriticalSectionRawMutex, (), 1> = Channel::new();
+static CHANNEL1: Channel<CriticalSectionRawMutex, (), 1> = 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;
+}