GPIO interrupts, once more (#3408)

* Add test case

* Split into files

* Do not clear interrupt status bits in handler

* Document changes

* Refactor gpio tests to test the default interrupt handler by default

* Fix typo

* Extract mk_static

* Write a bit about interrupt handling

* Various fixes

* Test that the future doesn't resolve for a preceding interrupt

* Add multi-core test

* ESP32: handle GPIO interrupt on the listening core

* Fix multi-core edge case

esp-hal/CHANGELOG.md

@@ -55,6 +55,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Make `esp_hal::interrupt::current_runlevel` public under the unstable feature (#3403)
 - Update `defmt` to 1.0 (#3416)
 - `spi::master::Spi::transfer` no longer returns the received data as a slice (#?)
+- esp-hal no longer clears the GPIO interrupt status bits by default. (#3408)
 
 ### Fixed
 
@@ -78,6 +79,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Passing an invalid seven bit I2C address is now rejected (#3343)
 - PARL_IO: Use correct max transfer size (#3346)
 - `OneShot` timer now returns an InvalidTimeout from `schedule` instead of panicking (#3433)
+- GPIO interrupt handling no longer causes infinite looping if a task at higher priority is awaiting on a pin event (#3408)
+- `esp_hal::gpio::Input::is_interrupt_set` can now return true (#3408)
 
 ### Removed
 

esp-hal/MIGRATING-1.0.0-beta.0.md

@@ -147,6 +147,36 @@ Normally you only need to configure your pin once, after which changing modes ca
 + flex.set_output_enable(true);
 ```
 
+### Interrupt handling changes
+
+The interrupt status bits are no longer cleared automatically. Depending on your use case, you will
+need to either do this yourself, or disable the pin's interrupt.
+
+If you want your interrupt to keep firing, clear the interrupt status. Keep in mind that
+this affects `is_interrupt_set`.
+
+```diff
+ #[handler]
+ pub fn interrupt_handler() {
+     critical_section::with(|cs| {
+         let pin = INPUT_PIN.borrow_ref_mut(cs).as_mut().unwrap();
++        pin.clear_interrupt();
+     });
+ }
+```
+
+If you want your interrupt to fire once per `listen` call, disable the interrupt.
+
+```diff
+ #[handler]
+ pub fn interrupt_handler() {
+     critical_section::with(|cs| {
+         let pin = INPUT_PIN.borrow_ref_mut(cs).as_mut().unwrap();
++        pin.unlisten();
+     });
+ }
+```
+
 ## I2S driver now takes `DmaDescriptor`s later in construction
 
 ```diff

esp-hal/src/gpio/asynch.rs

@@ -0,0 +1,214 @@
+use core::{
+    sync::atomic::Ordering,
+    task::{Context, Poll},
+};
+
+use procmacros::ram;
+
+use crate::{
+    asynch::AtomicWaker,
+    gpio::{Event, Flex, GpioBank, Input, NUM_PINS},
+};
+
+#[ram]
+pub(super) static PIN_WAKERS: [AtomicWaker; NUM_PINS] = [const { AtomicWaker::new() }; NUM_PINS];
+
+impl Flex<'_> {
+    /// Wait until the pin experiences a particular [`Event`].
+    ///
+    /// The GPIO driver will disable listening for the event once it occurs,
+    /// or if the `Future` is dropped - which also means this method is **not**
+    /// cancellation-safe, it will always wait for a future event.
+    ///
+    /// Note that calling this function will overwrite previous
+    /// [`listen`][Self::listen] operations for this pin.
+    #[inline]
+    #[instability::unstable]
+    pub async fn wait_for(&mut self, event: Event) {
+        // Make sure this pin is not being processed by an interrupt handler. We need to
+        // always take a critical section even if the pin is not listening, because the
+        // interrupt handler may be running on another core and the interrupt handler
+        // may be in the process of processing the pin if the interrupt status is set -
+        // regardless of the pin actually listening or not.
+        if self.is_listening() || self.is_interrupt_set() {
+            self.unlisten_and_clear();
+        }
+
+        // At this point the pin is no longer listening, and not being processed, so we
+        // can safely do our setup.
+
+        // Mark pin as async. The interrupt handler clears this bit before processing a
+        // pin and unlistens it, so this call will not race with the interrupt
+        // handler (because it must have finished before `unlisten` above, or the
+        // handler no longer )
+        self.pin
+            .bank()
+            .async_operations()
+            .fetch_or(self.pin.mask(), Ordering::Relaxed);
+
+        // Start listening for the event. We only need to do this once, as disabling
+        // the interrupt will signal the future to complete.
+        self.listen(event);
+
+        PinFuture { pin: self }.await
+    }
+
+    /// Wait until the pin is high.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    #[instability::unstable]
+    pub async fn wait_for_high(&mut self) {
+        self.wait_for(Event::HighLevel).await
+    }
+
+    /// Wait until the pin is low.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    #[instability::unstable]
+    pub async fn wait_for_low(&mut self) {
+        self.wait_for(Event::LowLevel).await
+    }
+
+    /// Wait for the pin to undergo a transition from low to high.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    #[instability::unstable]
+    pub async fn wait_for_rising_edge(&mut self) {
+        self.wait_for(Event::RisingEdge).await
+    }
+
+    /// Wait for the pin to undergo a transition from high to low.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    #[instability::unstable]
+    pub async fn wait_for_falling_edge(&mut self) {
+        self.wait_for(Event::FallingEdge).await
+    }
+
+    /// Wait for the pin to undergo any transition, i.e low to high OR high
+    /// to low.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    #[instability::unstable]
+    pub async fn wait_for_any_edge(&mut self) {
+        self.wait_for(Event::AnyEdge).await
+    }
+}
+
+impl Input<'_> {
+    /// Wait until the pin experiences a particular [`Event`].
+    ///
+    /// The GPIO driver will disable listening for the event once it occurs,
+    /// or if the `Future` is dropped - which also means this method is **not**
+    /// cancellation-safe, it will always wait for a future event.
+    ///
+    /// Note that calling this function will overwrite previous
+    /// [`listen`][Self::listen] operations for this pin.
+    #[inline]
+    pub async fn wait_for(&mut self, event: Event) {
+        self.pin.wait_for(event).await
+    }
+
+    /// Wait until the pin is high.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    pub async fn wait_for_high(&mut self) {
+        self.pin.wait_for_high().await
+    }
+
+    /// Wait until the pin is low.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    pub async fn wait_for_low(&mut self) {
+        self.pin.wait_for_low().await
+    }
+
+    /// Wait for the pin to undergo a transition from low to high.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    pub async fn wait_for_rising_edge(&mut self) {
+        self.pin.wait_for_rising_edge().await
+    }
+
+    /// Wait for the pin to undergo a transition from high to low.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    pub async fn wait_for_falling_edge(&mut self) {
+        self.pin.wait_for_falling_edge().await
+    }
+
+    /// Wait for the pin to undergo any transition, i.e low to high OR high
+    /// to low.
+    ///
+    /// See [Self::wait_for] for more information.
+    #[inline]
+    pub async fn wait_for_any_edge(&mut self) {
+        self.pin.wait_for_any_edge().await
+    }
+}
+
+#[must_use = "futures do nothing unless you `.await` or poll them"]
+struct PinFuture<'f, 'd> {
+    pin: &'f mut Flex<'d>,
+}
+
+impl PinFuture<'_, '_> {
+    fn number(&self) -> u8 {
+        self.pin.number()
+    }
+
+    fn bank(&self) -> GpioBank {
+        self.pin.pin.bank()
+    }
+
+    fn mask(&self) -> u32 {
+        self.pin.pin.mask()
+    }
+
+    fn is_done(&self) -> bool {
+        // Only the interrupt handler should clear the async bit, and only if the
+        // specific pin is handling an interrupt. This way the user may clear the
+        // interrupt status without worrying about the async bit being cleared.
+        self.bank().async_operations().load(Ordering::Acquire) & self.mask() == 0
+    }
+}
+
+impl core::future::Future for PinFuture<'_, '_> {
+    type Output = ();
+
+    fn poll(self: core::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
+        PIN_WAKERS[self.number() as usize].register(cx.waker());
+
+        if self.is_done() {
+            Poll::Ready(())
+        } else {
+            Poll::Pending
+        }
+    }
+}
+
+impl Drop for PinFuture<'_, '_> {
+    fn drop(&mut self) {
+        // If the future has completed, unlistening and removing the async bit will have
+        // been done by the interrupt handler.
+
+        if !self.is_done() {
+            self.pin.unlisten_and_clear();
+
+            // Unmark pin as async so that a future listen call doesn't wake a waker for no
+            // reason.
+            self.bank()
+                .async_operations()
+                .fetch_and(!self.mask(), Ordering::Relaxed);
+        }
+    }
+}

esp-hal/src/gpio/embedded_hal_impls.rs

@@ -0,0 +1,141 @@
+use embedded_hal::digital;
+use embedded_hal_async::digital::Wait;
+
+#[cfg(feature = "unstable")]
+use super::Flex;
+use super::{Input, Output};
+
+impl digital::ErrorType for Input<'_> {
+    type Error = core::convert::Infallible;
+}
+
+impl digital::InputPin for Input<'_> {
+    fn is_high(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_high(self))
+    }
+
+    fn is_low(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_low(self))
+    }
+}
+
+impl digital::ErrorType for Output<'_> {
+    type Error = core::convert::Infallible;
+}
+
+impl digital::OutputPin for Output<'_> {
+    fn set_low(&mut self) -> Result<(), Self::Error> {
+        Self::set_low(self);
+        Ok(())
+    }
+
+    fn set_high(&mut self) -> Result<(), Self::Error> {
+        Self::set_high(self);
+        Ok(())
+    }
+}
+
+impl digital::StatefulOutputPin for Output<'_> {
+    fn is_set_high(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_set_high(self))
+    }
+
+    fn is_set_low(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_set_low(self))
+    }
+}
+
+#[instability::unstable]
+impl digital::InputPin for Flex<'_> {
+    fn is_high(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_high(self))
+    }
+
+    fn is_low(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_low(self))
+    }
+}
+
+#[instability::unstable]
+impl digital::ErrorType for Flex<'_> {
+    type Error = core::convert::Infallible;
+}
+
+#[instability::unstable]
+impl digital::OutputPin for Flex<'_> {
+    fn set_low(&mut self) -> Result<(), Self::Error> {
+        Self::set_low(self);
+        Ok(())
+    }
+
+    fn set_high(&mut self) -> Result<(), Self::Error> {
+        Self::set_high(self);
+        Ok(())
+    }
+}
+
+#[instability::unstable]
+impl digital::StatefulOutputPin for super::Flex<'_> {
+    fn is_set_high(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_set_high(self))
+    }
+
+    fn is_set_low(&mut self) -> Result<bool, Self::Error> {
+        Ok(Self::is_set_low(self))
+    }
+}
+
+#[instability::unstable]
+impl Wait for Flex<'_> {
+    async fn wait_for_high(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_high(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_low(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_low(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_rising_edge(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_rising_edge(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_falling_edge(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_falling_edge(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_any_edge(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_any_edge(self).await;
+        Ok(())
+    }
+}
+
+impl Wait for Input<'_> {
+    async fn wait_for_high(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_high(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_low(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_low(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_rising_edge(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_rising_edge(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_falling_edge(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_falling_edge(self).await;
+        Ok(())
+    }
+
+    async fn wait_for_any_edge(&mut self) -> Result<(), Self::Error> {
+        Self::wait_for_any_edge(self).await;
+        Ok(())
+    }
+}

esp-hal/src/gpio/interrupt.rs

@@ -0,0 +1,297 @@
+//! GPIO interrupt handling
+//!
+//! ## Requirements
+//!
+//! - On devices other than the P4, there is a single interrupt handler. GPIO
+//!   interrupt handling must not interfere with the async API in this single
+//!   handler.
+//! - Async operations take pins by `&mut self`, so they can only be accessed
+//!   after the operation is complete, or cancelled. They may be defined to
+//!   overwrite the configuration of the manual interrupt API, but not affect
+//!   the interrupt handler.
+//! - Manual `listen` operations don't need to be prepared for async operations,
+//!   but async operations need to be prepared to handle cases where the pin was
+//!   configured to listen for an event - or even that the user unlistened the
+//!   pin but left the interrupt status set.
+//!
+//! The user should be careful when using the async API and the manual interrupt
+//! API together. For performance reasons, we will not prevent the user handler
+//! from running in response to an async event.
+//!
+//! ## Single-shot interaction with user interrupt handlers
+//!
+//! The async API disables the pin's interrupt when triggered. This makes async
+//! operations single-shot. If there is no user handler, the other GPIO
+//! interrupts are also single-shot. This is because the user has no way to
+//! handle multiple events in this case, the API only allows querying whether
+//! the interrupt has fired or not. Disabling the interrupt also means that the
+//! interrupt status bits are not cleared, so the `is_interrupt_set` works by
+//! default as expected.
+//!
+//! When the user sets a custom interrupt handler, the built-in interrupt
+//! handler will only disable the async interrupts. The user handler is
+//! responsible for clearing the interrupt status bits or disabling the
+//! interrupts, based on their needs. This is communicated to the user in the
+//! documentation of the `Io::set_interrupt_handler` function.
+//!
+//! ## Critical sections
+//!
+//! The interrupt handler runs in a GPIO-specific critical section. The critical
+//! section is required because interrupts are disabled by modifying the pin
+//! register, which is not an atomic operation. The critical section also
+//! ensures that a higher priority task waken by an async pin event (or the user
+//! handler) will only run after the interrupt handler has finished.
+//!
+//! ## Signaling async completion
+//!
+//! The completion is signalled by clearing a flag in an AtomicU32. This flag is
+//! set at the start of the async operation, and cleared when the interrupt
+//! handler is called. The flag is not accessible by the user, so they can't
+//! force-complete an async operation accidentally from the interrupt handler.
+//!
+//! We could technically use the interrupt status on single-core chips, but it
+//! would be slightly more complicated to prevent the user from breaking things.
+//! (If the user were to clear the interrupt status, we would need to re-enable
+//! it, for PinFuture to detect the completion).
+//!
+//! TODO: currently, direct-binding a GPIO interrupt handler will completely
+//! break the async API. We will need to expose a way to handle async events.
+
+use portable_atomic::{AtomicPtr, Ordering};
+use procmacros::ram;
+use strum::EnumCount;
+
+use crate::{
+    gpio::{GpioBank, InterruptStatusRegisterAccess, asynch, set_int_enable},
+    interrupt::{self, DEFAULT_INTERRUPT_HANDLER, Priority},
+    peripherals::Interrupt,
+    sync::RawMutex,
+};
+
+/// Convenience constant for `Option::None` pin
+pub(super) static USER_INTERRUPT_HANDLER: CFnPtr = CFnPtr::new();
+
+pub(super) static GPIO_LOCK: RawMutex = RawMutex::new();
+
+pub(super) struct CFnPtr(AtomicPtr<()>);
+impl CFnPtr {
+    pub const fn new() -> Self {
+        Self(AtomicPtr::new(core::ptr::null_mut()))
+    }
+
+    pub fn store(&self, f: extern "C" fn()) {
+        self.0.store(f as *mut (), Ordering::Relaxed);
+    }
+
+    pub fn call(&self) {
+        let ptr = self.0.load(Ordering::Relaxed);
+        if !ptr.is_null() {
+            unsafe { (core::mem::transmute::<*mut (), extern "C" fn()>(ptr))() };
+        }
+    }
+}
+
+pub(crate) fn bind_default_interrupt_handler() {
+    // We first check if a handler is set in the vector table.
+    if let Some(handler) = interrupt::bound_handler(Interrupt::GPIO) {
+        let handler = handler as *const unsafe extern "C" fn();
+
+        // We only allow binding the default handler if nothing else is bound.
+        // This prevents silently overwriting RTIC's interrupt handler, if using GPIO.
+        if !core::ptr::eq(handler, DEFAULT_INTERRUPT_HANDLER.handler() as _) {
+            // The user has configured an interrupt handler they wish to use.
+            info!("Not using default GPIO interrupt handler: already bound in vector table");
+            return;
+        }
+    }
+
+    // The vector table doesn't contain a custom entry. Still, the
+    // peripheral interrupt may already be bound to something else.
+    for cpu in cores() {
+        if interrupt::bound_cpu_interrupt_for(cpu, Interrupt::GPIO).is_some() {
+            info!("Not using default GPIO interrupt handler: peripheral interrupt already in use");
+            return;
+        }
+    }
+
+    unsafe { interrupt::bind_interrupt(Interrupt::GPIO, default_gpio_interrupt_handler) };
+
+    // By default, we use lowest priority
+    set_interrupt_priority(Interrupt::GPIO, Priority::min());
+}
+
+cfg_if::cfg_if! {
+    if #[cfg(esp32)] {
+        // On ESP32, the interrupt fires on the core that started listening for a pin event.
+        fn cores() -> impl Iterator<Item = crate::system::Cpu> {
+            crate::system::Cpu::all()
+        }
+    } else {
+        fn cores() -> [crate::system::Cpu; 1] {
+            [crate::system::Cpu::current()]
+        }
+    }
+}
+
+pub(super) fn set_interrupt_priority(interrupt: Interrupt, priority: Priority) {
+    for cpu in cores() {
+        unwrap!(crate::interrupt::enable_on_cpu(cpu, interrupt, priority));
+    }
+}
+
+/// The default GPIO interrupt handler, when the user has not set one.
+///
+/// This handler will disable all pending interrupts and leave the interrupt
+/// status bits unchanged. This enables functions like `is_interrupt_set` to
+/// work correctly.
+#[ram]
+extern "C" fn default_gpio_interrupt_handler() {
+    GPIO_LOCK.lock(|| {
+        let banks = interrupt_status();
+
+        // Handle the async interrupts
+        for (bank, intrs) in banks {
+            // Get the mask of active async pins and clear the relevant bits to signal
+            // completion. This way the user may clear the interrupt status
+            // without worrying about the async bit being cleared.
+            let async_pins = bank.async_operations().load(Ordering::Relaxed);
+
+            // Wake up the tasks
+            handle_async_pins(bank, async_pins, intrs);
+
+            // Disable the remaining interrupts.
+            let mut intrs = intrs & !async_pins;
+            while intrs != 0 {
+                let pin_pos = intrs.trailing_zeros();
+                intrs -= 1 << pin_pos;
+
+                let pin_nr = pin_pos as u8 + bank.offset();
+
+                // The remaining interrupts are not async, we treat them as single-shot.
+                set_int_enable(pin_nr, Some(0), 0, false);
+            }
+        }
+    });
+}
+
+/// The user GPIO interrupt handler, when the user has set one.
+///
+/// This handler only disables interrupts associated with async pins. The user
+/// handler is responsible for clearing the interrupt status bits or disabling
+/// the interrupts.
+#[ram]
+pub(super) extern "C" fn user_gpio_interrupt_handler() {
+    GPIO_LOCK.lock(|| {
+        // Read interrupt status before the user has a chance to modify them.
+        let banks = interrupt_status();
+
+        // Call the user handler before clearing interrupts. The user can use the enable
+        // bits to determine which interrupts they are interested in. Clearing the
+        // interupt status or enable bits have no effect on the rest of the
+        // interrupt handler.
+        USER_INTERRUPT_HANDLER.call();
+
+        // Handle the async interrupts
+        for (bank, intrs) in banks {
+            // Get the mask of active async pins and clear the relevant bits to signal
+            // completion. This way the user may clear the interrupt status
+            // without worrying about the async bit being cleared.
+            let async_pins = bank.async_operations().load(Ordering::Relaxed);
+
+            // Wake up the tasks
+            handle_async_pins(bank, async_pins, intrs);
+        }
+    });
+}
+
+fn interrupt_status() -> [(GpioBank, u32); GpioBank::COUNT] {
+    let intrs_bank0 = InterruptStatusRegisterAccess::Bank0.interrupt_status_read();
+
+    #[cfg(gpio_bank_1)]
+    let intrs_bank1 = InterruptStatusRegisterAccess::Bank1.interrupt_status_read();
+
+    [
+        (GpioBank::_0, intrs_bank0),
+        #[cfg(gpio_bank_1)]
+        (GpioBank::_1, intrs_bank1),
+    ]
+}
+
+// We have separate variants for single-core and multi-core async pin handling.
+// Single core can be much simpler because no code is running in parallel, so we
+// don't have to be so careful with the order of operations. On multi-core,
+// however, the order can actually break things, regardless of the critical
+// section (because tasks on the other core may be waken in inappropriate
+// times).
+
+#[cfg(single_core)]
+fn handle_async_pins(bank: GpioBank, async_pins: u32, intrs: u32) {
+    let mut async_intrs = async_pins & intrs;
+    while async_intrs != 0 {
+        let pin_pos = async_intrs.trailing_zeros();
+        async_intrs -= 1 << pin_pos;
+
+        let pin_nr = pin_pos as u8 + bank.offset();
+
+        // Disable the interrupt for this pin.
+        set_int_enable(pin_nr, Some(0), 0, false);
+
+        asynch::PIN_WAKERS[pin_nr as usize].wake();
+    }
+
+    // This is an optimization (in case multiple pin interrupts are handled at once)
+    // so that PinFuture doesn't have to clear interrupt status bits one by one
+    // for each pin. We need to clear after disabling the interrupt, so that a pin
+    // event can't re-set the bit.
+    bank.write_interrupt_status_clear(async_pins & intrs);
+
+    // On a single-core chip, the lock around `handle_async_pins` ensures
+    // that the interrupt handler will not be interrupted by other code,
+    // so we can safely write back without an atomic CAS.
+    bank.async_operations()
+        .store(async_pins & !intrs, Ordering::Relaxed);
+}
+
+#[cfg(multi_core)]
+fn handle_async_pins(bank: GpioBank, async_pins: u32, intrs: u32) {
+    // First, disable pin interrupts. If we were to do this after clearing the async
+    // flags, the PinFuture destructor may try to take a critical section which
+    // isn't necessary.
+    let mut async_intrs = async_pins & intrs;
+    while async_intrs != 0 {
+        let pin_pos = async_intrs.trailing_zeros();
+        async_intrs -= 1 << pin_pos;
+
+        let pin_nr = pin_pos as u8 + bank.offset();
+
+        // Disable the interrupt for this pin.
+        set_int_enable(pin_nr, Some(0), 0, false);
+    }
+
+    // This is an optimization (in case multiple pin interrupts are handled at once)
+    // so that PinFuture doesn't have to clear interrupt status bits one by one
+    // for each pin. We need to clear after disabling the interrupt, so that a pin
+    // event can't re-set the bit.
+    bank.write_interrupt_status_clear(async_pins & intrs);
+
+    // Clearing the async bit needs to be the last state change, as this signals
+    // completion.
+    // On multi-core chips, we need to use a CAS to ensure that only
+    // the handled async bits are cleared.
+    bank.async_operations().fetch_and(!intrs, Ordering::Relaxed);
+
+    // Now we can wake the tasks. This needs to happen after completion - waking an
+    // already running task isn't a big issue, but not waking a waiting one is.
+    // Doing it sooner would mean that on a multi-core chip a task could be
+    // waken, but the Future wouldn't actually be resolved, and so it might
+    // never wake again.
+    let mut async_intrs = async_pins & intrs;
+    while async_intrs != 0 {
+        let pin_pos = async_intrs.trailing_zeros();
+        async_intrs -= 1 << pin_pos;
+
+        let pin_nr = pin_pos as u8 + bank.offset();
+
+        asynch::PIN_WAKERS[pin_nr as usize].wake();
+    }
+}

esp-hal/src/gpio/mod.rs

@@ -51,25 +51,6 @@
 //! [embedded-hal-async]: embedded_hal_async
 //! [`split`]: crate::peripherals::GPIO0::split
 
-use core::fmt::Display;
-
-use portable_atomic::{AtomicU32, Ordering};
-use procmacros::ram;
-use strum::EnumCount;
-
-#[cfg(any(lp_io, rtc_cntl))]
-use crate::peripherals::{handle_rtcio, handle_rtcio_with_resistors};
-pub use crate::soc::gpio::*;
-use crate::{
-    interrupt::{self, DEFAULT_INTERRUPT_HANDLER, InterruptHandler, Priority},
-    peripherals::{GPIO, IO_MUX, Interrupt, handle_gpio_input, handle_gpio_output},
-    private::{self, Sealed},
-};
-
-mod placeholder;
-
-pub use placeholder::NoPin;
-
 crate::unstable_module! {
     pub mod interconnect;
 
@@ -83,36 +64,26 @@ crate::unstable_module! {
     pub mod rtc_io;
 }
 
-mod user_irq {
+mod asynch;
-    use portable_atomic::{AtomicPtr, Ordering};
+mod embedded_hal_impls;
-    use procmacros::ram;
+pub(crate) mod interrupt;
+mod placeholder;
 
-    /// Convenience constant for `Option::None` pin
+use core::fmt::Display;
-    pub(super) static USER_INTERRUPT_HANDLER: CFnPtr = CFnPtr::new();
 
-    pub(super) struct CFnPtr(AtomicPtr<()>);
+use interrupt::*;
-    impl CFnPtr {
+pub use placeholder::NoPin;
-        pub const fn new() -> Self {
+use portable_atomic::AtomicU32;
-            Self(AtomicPtr::new(core::ptr::null_mut()))
+use strum::EnumCount;
-        }
 
-        pub fn store(&self, f: extern "C" fn()) {
+#[cfg(any(lp_io, rtc_cntl))]
-            self.0.store(f as *mut (), Ordering::Relaxed);
+use crate::peripherals::{handle_rtcio, handle_rtcio_with_resistors};
-        }
+pub use crate::soc::gpio::*;
-
+use crate::{
-        pub fn call(&self) {
+    interrupt::{InterruptHandler, Priority},
-            let ptr = self.0.load(Ordering::Relaxed);
+    peripherals::{GPIO, IO_MUX, Interrupt, handle_gpio_input, handle_gpio_output},
-            if !ptr.is_null() {
+    private::{self, Sealed},
-                unsafe { (core::mem::transmute::<*mut (), extern "C" fn()>(ptr))() };
+};
-            }
-        }
-    }
-
-    #[ram]
-    pub(super) extern "C" fn user_gpio_interrupt_handler() {
-        super::handle_pin_interrupts(|| USER_INTERRUPT_HANDLER.call());
-    }
-}
 
 /// Represents a pin-peripheral connection that, when dropped, disconnects the
 /// peripheral from the pin.
@@ -637,32 +608,6 @@ fn disable_usb_pads(gpionum: u8) {
     }
 }
 
-pub(crate) fn bind_default_interrupt_handler() {
-    // We first check if a handler is set in the vector table.
-    if let Some(handler) = interrupt::bound_handler(Interrupt::GPIO) {
-        let handler = handler as *const unsafe extern "C" fn();
-
-        // We only allow binding the default handler if nothing else is bound.
-        // This prevents silently overwriting RTIC's interrupt handler, if using GPIO.
-        if !core::ptr::eq(handler, DEFAULT_INTERRUPT_HANDLER.handler() as _) {
-            // The user has configured an interrupt handler they wish to use.
-            info!("Not using default GPIO interrupt handler: already bound in vector table");
-            return;
-        }
-    }
-    // The vector table doesn't contain a custom entry.Still, the
-    // peripheral interrupt may already be bound to something else.
-    if interrupt::bound_cpu_interrupt_for(crate::system::Cpu::current(), Interrupt::GPIO).is_some()
-    {
-        info!("Not using default GPIO interrupt handler: peripheral interrupt already in use");
-        return;
-    }
-
-    unsafe { interrupt::bind_interrupt(Interrupt::GPIO, default_gpio_interrupt_handler) };
-    // By default, we use lowest priority
-    unwrap!(interrupt::enable(Interrupt::GPIO, Priority::min()));
-}
-
 /// General Purpose Input/Output driver
 #[derive(Debug)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
@@ -686,7 +631,7 @@ impl<'d> Io<'d> {
     /// `None`)
     #[instability::unstable]
     pub fn set_interrupt_priority(&self, prio: Priority) {
-        unwrap!(interrupt::enable(Interrupt::GPIO, prio));
+        interrupt::set_interrupt_priority(Interrupt::GPIO, prio);
     }
 
     #[cfg_attr(
@@ -698,10 +643,23 @@ impl<'d> Io<'d> {
         doc = "Registers an interrupt handler for all GPIO pins on the current core."
     )]
     #[doc = ""]
-    /// Note that when using interrupt handlers registered by this function,
+    /// Note that when using interrupt handlers registered by this function, or
-    /// we clear the interrupt status register for you. This is NOT the case
+    /// by defining a `#[no_mangle] unsafe extern "C" fn GPIO()` function, we do
-    /// if you register the interrupt handler directly, by defining a
+    /// **not** clear the interrupt status register or the interrupt enable
-    /// `#[no_mangle] unsafe extern "C" fn GPIO()` function.
+    /// setting for you. Based on your use case, you need to do one of this
+    /// yourself:
+    ///
+    /// - Disabling the interrupt enable setting for the GPIO pin allows you to
+    ///   handle an event once per call to [`listen()`]. Using this method, the
+    ///   [`is_interrupt_set()`] method will return `true` if the interrupt is
+    ///   set even after your handler has finished running.
+    /// - Clearing the interrupt status register allows you to handle an event
+    ///   repeatedly after [`listen()`] is called. Using this method,
+    ///   [`is_interrupt_set()`] will return `false` after your handler has
+    ///   finished running.
+    ///
+    /// [`listen()`]: Input::listen
+    /// [`is_interrupt_set()`]: Input::is_interrupt_set
     ///
     /// # Panics
     ///
@@ -713,10 +671,8 @@ impl<'d> Io<'d> {
             crate::interrupt::disable(core, Interrupt::GPIO);
         }
         self.set_interrupt_priority(handler.priority());
-        unsafe {
+        unsafe { crate::interrupt::bind_interrupt(Interrupt::GPIO, user_gpio_interrupt_handler) };
-            interrupt::bind_interrupt(Interrupt::GPIO, user_irq::user_gpio_interrupt_handler)
+        USER_INTERRUPT_HANDLER.store(handler.handler());
-        };
-        user_irq::USER_INTERRUPT_HANDLER.store(handler.handler());
     }
 }
 
@@ -729,48 +685,6 @@ impl crate::interrupt::InterruptConfigurable for Io<'_> {
     }
 }
 
-#[ram]
-extern "C" fn default_gpio_interrupt_handler() {
-    handle_pin_interrupts(|| ());
-}
-
-#[ram]
-fn handle_pin_interrupts(user_handler: fn()) {
-    let intrs_bank0 = InterruptStatusRegisterAccess::Bank0.interrupt_status_read();
-
-    #[cfg(gpio_bank_1)]
-    let intrs_bank1 = InterruptStatusRegisterAccess::Bank1.interrupt_status_read();
-
-    user_handler();
-
-    let banks = [
-        (GpioBank::_0, intrs_bank0),
-        #[cfg(gpio_bank_1)]
-        (GpioBank::_1, intrs_bank1),
-    ];
-
-    for (bank, intrs) in banks {
-        // Get the mask of active async pins and also unmark them in the same go.
-        let async_pins = bank.async_operations().fetch_and(!intrs, Ordering::Relaxed);
-
-        // Wake up the tasks
-        let mut intr_bits = intrs & async_pins;
-        while intr_bits != 0 {
-            let pin_pos = intr_bits.trailing_zeros();
-            intr_bits -= 1 << pin_pos;
-
-            let pin_nr = pin_pos as u8 + bank.offset();
-
-            crate::interrupt::free(|| {
-                asynch::PIN_WAKERS[pin_nr as usize].wake();
-                set_int_enable(pin_nr, Some(0), 0, false);
-            });
-        }
-
-        bank.write_interrupt_status_clear(intrs);
-    }
-}
-
 #[doc(hidden)]
 #[macro_export]
 macro_rules! if_output_pin {
@@ -1586,32 +1500,6 @@ impl<'d> Flex<'d> {
         self.pin.is_input_high().into()
     }
 
-    fn listen_with_options(
-        &self,
-        event: Event,
-        int_enable: bool,
-        nmi_enable: bool,
-        wake_up_from_light_sleep: bool,
-    ) -> Result<(), WakeConfigError> {
-        if wake_up_from_light_sleep {
-            match event {
-                Event::AnyEdge | Event::RisingEdge | Event::FallingEdge => {
-                    return Err(WakeConfigError::EdgeTriggeringNotSupported);
-                }
-                _ => {}
-            }
-        }
-
-        set_int_enable(
-            self.pin.number(),
-            Some(gpio_intr_enable(int_enable, nmi_enable)),
-            event as u8,
-            wake_up_from_light_sleep,
-        );
-
-        Ok(())
-    }
-
     /// Listen for interrupts.
     ///
     /// See [`Input::listen`] for more information and an example.
@@ -1620,14 +1508,23 @@ impl<'d> Flex<'d> {
     pub fn listen(&mut self, event: Event) {
         // Unwrap can't fail currently as listen_with_options is only supposed to return
         // an error if wake_up_from_light_sleep is true.
-        unwrap!(self.listen_with_options(event, true, false, false));
+        unwrap!(self.pin.listen_with_options(event, true, false, false));
     }
 
     /// Stop listening for interrupts.
     #[inline]
     #[instability::unstable]
     pub fn unlisten(&mut self) {
+        GPIO_LOCK.lock(|| {
             set_int_enable(self.pin.number(), Some(0), 0, false);
+        });
+    }
+
+    fn unlisten_and_clear(&mut self) {
+        GPIO_LOCK.lock(|| {
+            set_int_enable(self.pin.number(), Some(0), 0, false);
+            self.clear_interrupt();
+        });
     }
 
     /// Check if the pin is listening for interrupts.
@@ -1664,7 +1561,8 @@ impl<'d> Flex<'d> {
     #[inline]
     #[instability::unstable]
     pub fn wakeup_enable(&mut self, enable: bool, event: WakeEvent) -> Result<(), WakeConfigError> {
-        self.listen_with_options(event.into(), false, false, enable)
+        self.pin
+            .listen_with_options(event.into(), false, false, enable)
     }
 
     // Output functions
@@ -2054,6 +1952,55 @@ impl<'lt> AnyPin<'lt> {
         });
     }
 
+    fn clear_interrupt(&self) {
+        self.bank().write_interrupt_status_clear(self.mask());
+    }
+
+    fn with_gpio_lock<F, R>(&self, f: F) -> R
+    where
+        F: FnOnce() -> R,
+    {
+        // If the pin is listening, we need to take a critical section to prevent racing
+        // with the interrupt handler.
+        if is_int_enabled(self.number()) {
+            GPIO_LOCK.lock(f)
+        } else {
+            f()
+        }
+    }
+
+    fn listen_with_options(
+        &self,
+        event: Event,
+        int_enable: bool,
+        nmi_enable: bool,
+        wake_up_from_light_sleep: bool,
+    ) -> Result<(), WakeConfigError> {
+        if wake_up_from_light_sleep {
+            match event {
+                Event::AnyEdge | Event::RisingEdge | Event::FallingEdge => {
+                    return Err(WakeConfigError::EdgeTriggeringNotSupported);
+                }
+                _ => {}
+            }
+        }
+
+        self.with_gpio_lock(|| {
+            // Clear the interrupt status bit for this Pin, just in case the user forgot.
+            // Since we disabled the interrupt in the handler, it's not possible to
+            // trigger a new interrupt before we re-enable it here.
+            self.clear_interrupt();
+
+            set_int_enable(
+                self.number(),
+                Some(gpio_intr_enable(int_enable, nmi_enable)),
+                event as u8,
+                wake_up_from_light_sleep,
+            );
+        });
+        Ok(())
+    }
+
     #[inline]
     fn apply_output_config(&self, config: &OutputConfig) {
         let pull_up = config.pull == Pull::Up;
@@ -2069,12 +2016,12 @@ impl<'lt> AnyPin<'lt> {
             w
         });
 
-        let gpio = GPIO::regs();
+        self.with_gpio_lock(|| {
-
+            GPIO::regs().pin(self.number() as usize).modify(|_, w| {
-        gpio.pin(self.number() as usize).modify(|_, w| {
                 w.pad_driver()
                     .bit(config.drive_mode == DriveMode::OpenDrain)
             });
+        });
     }
 
     #[inline]
@@ -2188,378 +2135,3 @@ fn set_int_enable(gpio_num: u8, int_ena: Option<u8>, int_type: u8, wake_up_from_
 fn is_int_enabled(gpio_num: u8) -> bool {
     GPIO::regs().pin(gpio_num as usize).read().int_ena().bits() != 0
 }
-
-mod asynch {
-    use core::{
-        future::poll_fn,
-        task::{Context, Poll},
-    };
-
-    use super::*;
-    use crate::asynch::AtomicWaker;
-
-    #[ram]
-    pub(super) static PIN_WAKERS: [AtomicWaker; NUM_PINS] =
-        [const { AtomicWaker::new() }; NUM_PINS];
-
-    impl Flex<'_> {
-        /// Wait until the pin experiences a particular [`Event`].
-        ///
-        /// The GPIO driver will disable listening for the event once it occurs,
-        /// or if the `Future` is dropped.
-        ///
-        /// Note that calling this function will overwrite previous
-        /// [`listen`][Self::listen] operations for this pin.
-        #[inline]
-        #[instability::unstable]
-        pub async fn wait_for(&mut self, event: Event) {
-            // We construct the Future first, because its `Drop` implementation
-            // is load-bearing if `wait_for` is dropped during the initialization.
-            let future = PinFuture { pin: self };
-
-            // Make sure this pin is not being processed by an interrupt handler.
-            if future.pin.is_listening() {
-                set_int_enable(
-                    future.pin.number(),
-                    None, // Do not disable handling pending interrupts.
-                    0,    // Disable generating new events
-                    false,
-                );
-                poll_fn(|cx| {
-                    if future.pin.is_interrupt_set() {
-                        cx.waker().wake_by_ref();
-                        Poll::Pending
-                    } else {
-                        Poll::Ready(())
-                    }
-                })
-                .await;
-            }
-
-            // At this point the pin is no longer listening, we can safely
-            // do our setup.
-
-            // Mark pin as async.
-            future
-                .bank()
-                .async_operations()
-                .fetch_or(future.mask(), Ordering::Relaxed);
-
-            future.pin.listen(event);
-
-            future.await
-        }
-
-        /// Wait until the pin is high.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        #[instability::unstable]
-        pub async fn wait_for_high(&mut self) {
-            self.wait_for(Event::HighLevel).await
-        }
-
-        /// Wait until the pin is low.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        #[instability::unstable]
-        pub async fn wait_for_low(&mut self) {
-            self.wait_for(Event::LowLevel).await
-        }
-
-        /// Wait for the pin to undergo a transition from low to high.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        #[instability::unstable]
-        pub async fn wait_for_rising_edge(&mut self) {
-            self.wait_for(Event::RisingEdge).await
-        }
-
-        /// Wait for the pin to undergo a transition from high to low.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        #[instability::unstable]
-        pub async fn wait_for_falling_edge(&mut self) {
-            self.wait_for(Event::FallingEdge).await
-        }
-
-        /// Wait for the pin to undergo any transition, i.e low to high OR high
-        /// to low.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        #[instability::unstable]
-        pub async fn wait_for_any_edge(&mut self) {
-            self.wait_for(Event::AnyEdge).await
-        }
-    }
-
-    impl Input<'_> {
-        /// Wait until the pin experiences a particular [`Event`].
-        ///
-        /// The GPIO driver will disable listening for the event once it occurs,
-        /// or if the `Future` is dropped.
-        ///
-        /// Note that calling this function will overwrite previous
-        /// [`listen`][Self::listen] operations for this pin.
-        #[inline]
-        pub async fn wait_for(&mut self, event: Event) {
-            self.pin.wait_for(event).await
-        }
-
-        /// Wait until the pin is high.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        pub async fn wait_for_high(&mut self) {
-            self.pin.wait_for_high().await
-        }
-
-        /// Wait until the pin is low.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        pub async fn wait_for_low(&mut self) {
-            self.pin.wait_for_low().await
-        }
-
-        /// Wait for the pin to undergo a transition from low to high.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        pub async fn wait_for_rising_edge(&mut self) {
-            self.pin.wait_for_rising_edge().await
-        }
-
-        /// Wait for the pin to undergo a transition from high to low.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        pub async fn wait_for_falling_edge(&mut self) {
-            self.pin.wait_for_falling_edge().await
-        }
-
-        /// Wait for the pin to undergo any transition, i.e low to high OR high
-        /// to low.
-        ///
-        /// See [Self::wait_for] for more information.
-        #[inline]
-        pub async fn wait_for_any_edge(&mut self) {
-            self.pin.wait_for_any_edge().await
-        }
-    }
-
-    #[must_use = "futures do nothing unless you `.await` or poll them"]
-    struct PinFuture<'f, 'd> {
-        pin: &'f mut Flex<'d>,
-    }
-
-    impl PinFuture<'_, '_> {
-        fn number(&self) -> u8 {
-            self.pin.number()
-        }
-
-        fn bank(&self) -> GpioBank {
-            self.pin.pin.bank()
-        }
-
-        fn mask(&self) -> u32 {
-            self.pin.pin.mask()
-        }
-
-        fn is_done(&self) -> bool {
-            // Only the interrupt handler should clear the async bit, and only if the
-            // specific pin is handling an interrupt.
-            self.bank().async_operations().load(Ordering::Acquire) & self.mask() == 0
-        }
-    }
-
-    impl core::future::Future for PinFuture<'_, '_> {
-        type Output = ();
-
-        fn poll(self: core::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
-            PIN_WAKERS[self.number() as usize].register(cx.waker());
-
-            if self.is_done() {
-                Poll::Ready(())
-            } else {
-                Poll::Pending
-            }
-        }
-    }
-
-    impl Drop for PinFuture<'_, '_> {
-        fn drop(&mut self) {
-            // If the pin isn't listening, the future has either been dropped before setup,
-            // or the interrupt has already been handled.
-            if self.pin.is_listening() {
-                // Make sure the future isn't dropped while the interrupt is being handled.
-                // This prevents tricky drop-and-relisten scenarios.
-
-                set_int_enable(
-                    self.number(),
-                    None, // Do not disable handling pending interrupts.
-                    0,    // Disable generating new events
-                    false,
-                );
-
-                while self.pin.is_interrupt_set() {}
-
-                // Unmark pin as async
-                self.bank()
-                    .async_operations()
-                    .fetch_and(!self.mask(), Ordering::Relaxed);
-            }
-        }
-    }
-}
-
-mod embedded_hal_impls {
-    use embedded_hal::digital;
-
-    use super::*;
-
-    impl digital::ErrorType for Input<'_> {
-        type Error = core::convert::Infallible;
-    }
-
-    impl digital::InputPin for Input<'_> {
-        fn is_high(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_high(self))
-        }
-
-        fn is_low(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_low(self))
-        }
-    }
-
-    impl digital::ErrorType for Output<'_> {
-        type Error = core::convert::Infallible;
-    }
-
-    impl digital::OutputPin for Output<'_> {
-        fn set_low(&mut self) -> Result<(), Self::Error> {
-            Self::set_low(self);
-            Ok(())
-        }
-
-        fn set_high(&mut self) -> Result<(), Self::Error> {
-            Self::set_high(self);
-            Ok(())
-        }
-    }
-
-    impl digital::StatefulOutputPin for Output<'_> {
-        fn is_set_high(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_set_high(self))
-        }
-
-        fn is_set_low(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_set_low(self))
-        }
-    }
-
-    #[instability::unstable]
-    impl digital::InputPin for Flex<'_> {
-        fn is_high(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_high(self))
-        }
-
-        fn is_low(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_low(self))
-        }
-    }
-
-    #[instability::unstable]
-    impl digital::ErrorType for Flex<'_> {
-        type Error = core::convert::Infallible;
-    }
-
-    #[instability::unstable]
-    impl digital::OutputPin for Flex<'_> {
-        fn set_low(&mut self) -> Result<(), Self::Error> {
-            Self::set_low(self);
-            Ok(())
-        }
-
-        fn set_high(&mut self) -> Result<(), Self::Error> {
-            Self::set_high(self);
-            Ok(())
-        }
-    }
-
-    #[instability::unstable]
-    impl digital::StatefulOutputPin for Flex<'_> {
-        fn is_set_high(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_set_high(self))
-        }
-
-        fn is_set_low(&mut self) -> Result<bool, Self::Error> {
-            Ok(Self::is_set_low(self))
-        }
-    }
-}
-
-mod embedded_hal_async_impls {
-    use embedded_hal_async::digital::Wait;
-
-    use super::*;
-
-    #[instability::unstable]
-    impl Wait for Flex<'_> {
-        async fn wait_for_high(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_high(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_low(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_low(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_rising_edge(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_rising_edge(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_falling_edge(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_falling_edge(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_any_edge(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_any_edge(self).await;
-            Ok(())
-        }
-    }
-
-    impl Wait for Input<'_> {
-        async fn wait_for_high(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_high(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_low(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_low(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_rising_edge(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_rising_edge(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_falling_edge(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_falling_edge(self).await;
-            Ok(())
-        }
-
-        async fn wait_for_any_edge(&mut self) -> Result<(), Self::Error> {
-            Self::wait_for_any_edge(self).await;
-            Ok(())
-        }
-    }
-}

esp-hal/src/interrupt/riscv.rs

@@ -13,7 +13,6 @@
 //! ```
 
 pub use esp_riscv_rt::TrapFrame;
-pub(crate) use esp_riscv_rt::riscv::interrupt::free;
 use riscv::register::{mcause, mtvec};
 
 #[cfg(not(plic))]
@@ -424,6 +423,14 @@ mod vectored {
     /// Note that interrupts still need to be enabled globally for interrupts
     /// to be serviced.
     pub fn enable(interrupt: Interrupt, level: Priority) -> Result<(), Error> {
+        enable_on_cpu(Cpu::current(), interrupt, level)
+    }
+
+    pub(crate) fn enable_on_cpu(
+        cpu: Cpu,
+        interrupt: Interrupt,
+        level: Priority,
+    ) -> Result<(), Error> {
         if matches!(level, Priority::None) {
             return Err(Error::InvalidInterruptPriority);
         }
@@ -431,7 +438,7 @@ mod vectored {
             let cpu_interrupt = core::mem::transmute::<u32, CpuInterrupt>(
                 PRIORITY_TO_INTERRUPT[(level as usize) - 1] as u32,
             );
-            map(Cpu::current(), interrupt, cpu_interrupt);
+            map(cpu, interrupt, cpu_interrupt);
             enable_cpu_interrupt(cpu_interrupt);
         }
         Ok(())

esp-hal/src/interrupt/xtensa.rs

@@ -1,6 +1,7 @@
 //! Interrupt handling
 
 use xtensa_lx::interrupt;
+#[cfg(esp32)]
 pub(crate) use xtensa_lx::interrupt::free;
 use xtensa_lx_rt::exception::Context;
 
@@ -496,11 +497,19 @@ mod vectored {
 
     /// Enable the given peripheral interrupt
     pub fn enable(interrupt: Interrupt, level: Priority) -> Result<(), Error> {
+        enable_on_cpu(Cpu::current(), interrupt, level)
+    }
+
+    pub(crate) fn enable_on_cpu(
+        cpu: Cpu,
+        interrupt: Interrupt,
+        level: Priority,
+    ) -> Result<(), Error> {
         let cpu_interrupt =
             interrupt_level_to_cpu_interrupt(level, chip_specific::interrupt_is_edge(interrupt))?;
 
         unsafe {
-            map(Cpu::current(), interrupt, cpu_interrupt);
+            map(cpu, interrupt, cpu_interrupt);
 
             xtensa_lx::interrupt::enable_mask(
                 xtensa_lx::interrupt::get_mask() | (1 << cpu_interrupt as u32),

esp-hal/src/lib.rs

@@ -665,7 +665,7 @@ pub fn init(config: Config) -> Peripherals {
     #[cfg(esp32)]
     crate::time::time_init();
 
-    crate::gpio::bind_default_interrupt_handler();
+    crate::gpio::interrupt::bind_default_interrupt_handler();
 
     #[cfg(feature = "psram")]
     crate::psram::init_psram(config.psram);

esp-hal/src/soc/esp32/gpio.rs

@@ -113,8 +113,6 @@ pub(crate) fn io_mux_reg(gpio_num: u8) -> &'static io_mux::GPIO0 {
 pub(crate) fn gpio_intr_enable(int_enable: bool, nmi_enable: bool) -> u8 {
     match Cpu::current() {
         Cpu::AppCpu => int_enable as u8 | ((nmi_enable as u8) << 1),
-        // this should be bits 3 & 4 respectively, according to the TRM, but it doesn't seem to
-        // work. This does though.
         Cpu::ProCpu => ((int_enable as u8) << 2) | ((nmi_enable as u8) << 3),
     }
 }

hil-test/.cargo/config.toml

@@ -17,7 +17,7 @@ rustflags = [
 ]
 
 [env]
-DEFMT_LOG = "info"
+DEFMT_LOG = "info,embedded_test=warn"
 ESP_HAL_EMBASSY_CONFIG_TIMER_QUEUE="multiple-integrated"
 
 [unstable]

hil-test/src/lib.rs

@@ -83,6 +83,16 @@ macro_rules! unconnected_pin {
     }};
 }
 
+#[macro_export]
+macro_rules! mk_static {
+    ($t:ty,$val:expr) => {{
+        static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
+        #[deny(unused_attributes)]
+        let x = STATIC_CELL.uninit().write(($val));
+        x
+    }};
+}
+
 // A simple looping executor to test async code without esp-hal-embassy (which
 // needs `esp-hal/unstable`).
 #[cfg(not(feature = "embassy"))]

hil-test/tests/embassy_interrupt_executor.rs

@@ -26,16 +26,7 @@ use esp_hal::{
 #[cfg(multi_core)]
 use esp_hal_embassy::Executor;
 use esp_hal_embassy::InterruptExecutor;
-use hil_test as _;
+use hil_test::mk_static;
-
-macro_rules! mk_static {
-    ($t:ty,$val:expr) => {{
-        static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
-        #[deny(unused_attributes)]
-        let x = STATIC_CELL.uninit().write(($val));
-        x
-    }};
-}
 
 #[embassy_executor::task]
 async fn responder_task(

hil-test/tests/embassy_interrupt_spi_dma.rs

@@ -24,18 +24,9 @@ use esp_hal::{
     timer::AnyTimer,
 };
 use esp_hal_embassy::InterruptExecutor;
-use hil_test as _;
+use hil_test::mk_static;
 use portable_atomic::AtomicBool;
 
-macro_rules! mk_static {
-    ($t:ty,$val:expr) => {{
-        static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
-        #[deny(unused_attributes)]
-        let x = STATIC_CELL.uninit().write(($val));
-        x
-    }};
-}
-
 static STOP_INTERRUPT_TASK: AtomicBool = AtomicBool::new(false);
 static INTERRUPT_TASK_WORKING: AtomicBool = AtomicBool::new(false);
 

hil-test/tests/embassy_timers_executors.rs

@@ -21,17 +21,7 @@ use esp_hal::{
 };
 #[cfg(not(feature = "esp32"))]
 use esp_hal_embassy::InterruptExecutor;
-use hil_test as _;
+use hil_test::mk_static;
-
-#[cfg(not(feature = "esp32"))]
-macro_rules! mk_static {
-    ($t:ty,$val:expr) => {{
-        static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
-        #[deny(unused_attributes)]
-        let x = STATIC_CELL.uninit().write(($val));
-        x
-    }};
-}
 
 // List of the functions that are ACTUALLY TESTS but are called in the invokers
 mod test_helpers {

hil-test/tests/esp_wifi_init.rs

@@ -21,18 +21,9 @@ use esp_hal::{
 };
 use esp_hal_embassy::InterruptExecutor;
 use esp_wifi::InitializationError;
-use hil_test as _;
+use hil_test::mk_static;
 use static_cell::StaticCell;
 
-macro_rules! mk_static {
-    ($t:ty,$val:expr) => {{
-        static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
-        #[deny(unused_attributes)]
-        let x = STATIC_CELL.uninit().write(($val));
-        x
-    }};
-}
-
 #[embassy_executor::task]
 async fn try_init(
     signal: &'static Signal<CriticalSectionRawMutex, Option<InitializationError>>,

hil-test/tests/gpio.rs

@@ -7,36 +7,41 @@
 #![no_std]
 #![no_main]
 
-#[cfg(feature = "unstable")] // unused in stable build
-use core::cell::RefCell;
-
-#[cfg(feature = "unstable")] // unused in stable build
-use critical_section::Mutex;
-#[cfg(feature = "unstable")]
-use embassy_time::{Duration, Timer};
 use esp_hal::gpio::{AnyPin, Input, InputConfig, Level, Output, OutputConfig, Pin, Pull};
-#[cfg(feature = "unstable")]
+use hil_test as _;
-use esp_hal::{
+
+cfg_if::cfg_if! {
+    if #[cfg(feature = "unstable")] {
+        use core::cell::RefCell;
+        use critical_section::Mutex;
+        use embassy_time::{Duration, Timer};
+        use esp_hal::{
             // OutputOpenDrain is here because will be unused otherwise
             delay::Delay,
             gpio::{DriveMode, Event, Flex, Io},
             handler,
             timer::timg::TimerGroup,
-};
+        };
-use hil_test as _;
+        use portable_atomic::{AtomicUsize, Ordering};
-#[cfg(feature = "unstable")]
-use portable_atomic::{AtomicUsize, Ordering};
 
-#[cfg(feature = "unstable")] // unused in stable build
+        static COUNTER: Mutex<RefCell<u32>> = Mutex::new(RefCell::new(0));
-static COUNTER: Mutex<RefCell<u32>> = Mutex::new(RefCell::new(0));
+        static INPUT_PIN: Mutex<RefCell<Option<Input>>> = Mutex::new(RefCell::new(None));
-#[cfg(feature = "unstable")] // unused in stable build
+    }
-static INPUT_PIN: Mutex<RefCell<Option<Input>>> = Mutex::new(RefCell::new(None));
+}
+
+cfg_if::cfg_if! {
+    if #[cfg(all(multi_core, feature = "unstable"))] {
+        use embassy_sync::{blocking_mutex::raw::CriticalSectionRawMutex, signal::Signal};
+    }
+}
 
 struct Context {
     test_gpio1: AnyPin<'static>,
     test_gpio2: AnyPin<'static>,
     #[cfg(feature = "unstable")]
     delay: Delay,
+    #[cfg(feature = "unstable")]
+    io: Io<'static>,
 }
 
 #[cfg_attr(feature = "unstable", handler)]
@@ -46,7 +51,7 @@ pub fn interrupt_handler() {
         *COUNTER.borrow_ref_mut(cs) += 1;
         INPUT_PIN
             .borrow_ref_mut(cs)
-            .as_mut() // we can't unwrap as the handler may get called for async operations
+            .as_mut()
             .map(|pin| pin.clear_interrupt());
     });
 }
@@ -78,6 +83,29 @@ fn _gpios_can_be_reused() {
     }
 }
 
+#[cfg(all(multi_core, feature = "unstable"))]
+#[embassy_executor::task]
+async fn edge_counter_task(
+    mut in_pin: Input<'static>,
+    signal: &'static Signal<CriticalSectionRawMutex, u32>,
+) {
+    let mut edge_count = 0;
+    loop {
+        // This join will:
+        // - first set up the pin to listen
+        // - then poll the pin future once (which will return Pending)
+        // - then signal that the pin is listening, which enables the other core to
+        //   toggle the matching OutputPin
+        // - then will wait for the pin future to resolve.
+        embassy_futures::join::join(in_pin.wait_for_any_edge(), async {
+            signal.signal(edge_count);
+        })
+        .await;
+
+        edge_count += 1;
+    }
+}
+
 #[cfg(test)]
 #[embedded_test::tests(default_timeout = 3, executor = hil_test::Executor::new())]
 mod tests {
@@ -92,12 +120,12 @@ mod tests {
 
         let (gpio1, gpio2) = hil_test::common_test_pins!(peripherals);
 
+        // Interrupts are unstable
+        #[cfg(feature = "unstable")]
+        let io = Io::new(peripherals.IO_MUX);
+
         #[cfg(feature = "unstable")]
         {
-            // Interrupts are unstable
-            let mut io = Io::new(peripherals.IO_MUX);
-            io.set_interrupt_handler(interrupt_handler);
-
             // Timers are unstable
             let timg0 = TimerGroup::new(peripherals.TIMG0);
             esp_hal_embassy::init(timg0.timer0);
@@ -108,6 +136,8 @@ mod tests {
             test_gpio2: gpio2.degrade(),
             #[cfg(feature = "unstable")]
             delay,
+            #[cfg(feature = "unstable")]
+            io,
         }
     }
 
@@ -155,6 +185,22 @@ mod tests {
         .await;
     }
 
+    #[test]
+    #[cfg(feature = "unstable")] // Interrupts are unstable
+    async fn a_pin_can_wait_with_custom_handler(mut ctx: Context) {
+        ctx.io.set_interrupt_handler(interrupt_handler);
+
+        let mut first = Input::new(ctx.test_gpio1, InputConfig::default().with_pull(Pull::Down));
+
+        embassy_futures::select::select(
+            first.wait_for_rising_edge(),
+            // Other futures won't return, this one will, make sure its last so all other futures
+            // are polled first
+            embassy_futures::yield_now(),
+        )
+        .await;
+    }
+
     #[test]
     fn gpio_input(ctx: Context) {
         let test_gpio1 = Input::new(ctx.test_gpio1, InputConfig::default().with_pull(Pull::Down));
@@ -241,7 +287,9 @@ mod tests {
 
     #[test]
     #[cfg(feature = "unstable")] // Interrupts are unstable
-    fn gpio_interrupt(ctx: Context) {
+    fn gpio_interrupt(mut ctx: Context) {
+        ctx.io.set_interrupt_handler(interrupt_handler);
+
         let mut test_gpio1 =
             Input::new(ctx.test_gpio1, InputConfig::default().with_pull(Pull::Down));
         let mut test_gpio2 = Output::new(ctx.test_gpio2, Level::Low, OutputConfig::default());
@@ -437,4 +485,81 @@ mod tests {
 
         loop {}
     }
+
+    #[test]
+    #[cfg(feature = "unstable")]
+    async fn pending_interrupt_does_not_cause_future_to_resolve_immediately(ctx: Context) {
+        use embassy_futures::{
+            select::{Either, select},
+            yield_now,
+        };
+
+        let mut out_pin = Output::new(ctx.test_gpio2, Level::Low, OutputConfig::default());
+        let mut in_pin = Input::new(ctx.test_gpio1, InputConfig::default().with_pull(Pull::Down));
+
+        in_pin.listen(Event::RisingEdge);
+
+        out_pin.set_high();
+
+        assert!(in_pin.is_interrupt_set());
+
+        let should_timeout = select(in_pin.wait_for_falling_edge(), async {
+            // Give the future a bit of time, don't rely on the first poll resolving
+            for _ in 0..5 {
+                yield_now().await;
+            }
+        })
+        .await;
+
+        assert!(matches!(should_timeout, Either::Second(_)));
+    }
+
+    #[test]
+    #[cfg(all(multi_core, feature = "unstable"))]
+    async fn pin_waits_on_core_different_from_interrupt_handler(ctx: Context) {
+        // This test exercises cross-core pin events. Core 1 will wait for edge events
+        // that Core 0 generates. Interrupts are handled on Core 0. A signal is used to
+        // throttle the toggling, so that Core 1 will be expected to count the
+        // exact number of edge transitions.
+
+        use esp_hal::{
+            peripherals::CPU_CTRL,
+            system::{CpuControl, Stack},
+        };
+        use esp_hal_embassy::Executor;
+        use hil_test::mk_static;
+
+        let mut out_pin = Output::new(ctx.test_gpio2, Level::Low, OutputConfig::default());
+        let in_pin = Input::new(ctx.test_gpio1, InputConfig::default().with_pull(Pull::Down));
+
+        // `edge_counter_task` also returns the edge count as part of this signal
+        let input_pin_listening = &*mk_static!(Signal<CriticalSectionRawMutex, u32>, Signal::new());
+
+        // No need to thread this through `Context` for one test case
+        let cpu_ctrl = unsafe { CPU_CTRL::steal() };
+        const CORE1_STACK_SIZE: usize = 8192;
+        let app_core_stack = mk_static!(Stack<CORE1_STACK_SIZE>, Stack::new());
+        let _second_core = CpuControl::new(cpu_ctrl)
+            .start_app_core(app_core_stack, {
+                move || {
+                    let executor = mk_static!(Executor, Executor::new());
+                    executor.run(|spawner| {
+                        spawner.must_spawn(edge_counter_task(in_pin, input_pin_listening));
+                    });
+                }
+            })
+            .unwrap();
+
+        // Now drive the OutputPin and assert that the other core saw exactly as many
+        // edges as we generated here.
+        const EDGE_COUNT: u32 = 10_000;
+        for _ in 0..EDGE_COUNT {
+            input_pin_listening.wait().await;
+            out_pin.toggle();
+        }
+        // wait for signal
+        let edge_counter = input_pin_listening.wait().await;
+
+        assert_eq!(edge_counter, EDGE_COUNT);
+    }
 }

hil-test/tests/gpio_custom_handler.rs

@@ -11,13 +11,30 @@
 #![no_std]
 #![no_main]
 
+use embassy_executor::task;
+use embassy_sync::{blocking_mutex::raw::CriticalSectionRawMutex, signal::Signal};
 use embassy_time::{Duration, Timer};
 use esp_hal::{
-    gpio::{Flex, Input, InputConfig, InputPin, Io, Level, Output, OutputConfig, OutputPin, Pull},
+    gpio::{
+        AnyPin,
+        Flex,
+        Input,
+        InputConfig,
+        InputPin,
+        Io,
+        Level,
+        Output,
+        OutputConfig,
+        OutputPin,
+        Pin,
+        Pull,
+    },
     handler,
+    interrupt::{Priority, software::SoftwareInterruptControl},
     timer::timg::TimerGroup,
 };
-use hil_test as _;
+use esp_hal_embassy::InterruptExecutor;
+use hil_test::mk_static;
 use portable_atomic::{AtomicUsize, Ordering};
 
 #[unsafe(no_mangle)]
@@ -28,10 +45,11 @@ unsafe extern "C" fn GPIO() {
 
     let (gpio1, _) = hil_test::common_test_pins!(peripherals);
 
-    // Using flex will not mutate the pin.
+    // Using flex will reinitialize the pin, but it's okay here since we access an
+    // Input.
     let mut gpio1 = Flex::new(gpio1);
 
-    gpio1.clear_interrupt();
+    gpio1.unlisten();
 }
 
 #[handler]
@@ -64,8 +82,30 @@ async fn drive_pins(gpio1: impl InputPin, gpio2: impl OutputPin) -> usize {
     counter.load(Ordering::SeqCst)
 }
 
+#[task]
+async fn drive_pin(gpio: AnyPin<'static>) {
+    let mut test_gpio = Output::new(gpio, Level::Low, OutputConfig::default());
+    for _ in 0..5 {
+        test_gpio.set_high();
+        Timer::after(Duration::from_millis(25)).await;
+        test_gpio.set_low();
+        Timer::after(Duration::from_millis(25)).await;
+    }
+}
+
+#[task]
+async fn sense_pin(gpio: AnyPin<'static>, done: &'static Signal<CriticalSectionRawMutex, ()>) {
+    let mut test_gpio = Input::new(gpio, InputConfig::default().with_pull(Pull::Down));
+    test_gpio.wait_for_rising_edge().await;
+    test_gpio.wait_for_rising_edge().await;
+    test_gpio.wait_for_rising_edge().await;
+    test_gpio.wait_for_rising_edge().await;
+    test_gpio.wait_for_rising_edge().await;
+    done.signal(());
+}
+
 #[cfg(test)]
-#[embedded_test::tests(executor = hil_test::Executor::new())]
+#[embedded_test::tests(executor = hil_test::Executor::new(), default_timeout = 3)]
 mod tests {
 
     use super::*;
@@ -79,14 +119,6 @@ mod tests {
         let timg0 = TimerGroup::new(peripherals.TIMG0);
         esp_hal_embassy::init(timg0.timer0);
 
-        // We need to enable the GPIO interrupt, otherwise the async Future's
-        // setup or Drop implementation hangs.
-        esp_hal::interrupt::enable(
-            esp_hal::peripherals::Interrupt::GPIO,
-            esp_hal::interrupt::Priority::Priority1,
-        )
-        .unwrap();
-
         let counter = drive_pins(gpio1, gpio2).await;
 
         // GPIO is bound to something else, so we don't expect the async API to work.
@@ -110,4 +142,36 @@ mod tests {
         // We expect the async API to keep working even if a user handler is set.
         assert_eq!(counter, 5);
     }
+
+    #[test]
+    async fn task_that_runs_at_handlers_priority_is_not_locked_up() {
+        let peripherals = esp_hal::init(esp_hal::Config::default());
+
+        // Register an interrupt handler. Since we are not dealing with raw interrupts
+        // here, it's okay to do nothing. Handling async GPIO events will
+        // disable the corresponding interrupts.
+        let mut io = Io::new(peripherals.IO_MUX);
+        io.set_interrupt_handler(interrupt_handler);
+
+        let (gpio1, gpio2) = hil_test::common_test_pins!(peripherals);
+
+        let timg0 = TimerGroup::new(peripherals.TIMG0);
+        esp_hal_embassy::init(timg0.timer0);
+
+        let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
+        let interrupt_executor = mk_static!(
+            InterruptExecutor<1>,
+            InterruptExecutor::new(sw_ints.software_interrupt1)
+        );
+        // Run the executor at interrupt priority 1, which is the same as the default
+        // interrupt priority of the GPIO interrupt handler.
+        let interrupt_spawner = interrupt_executor.start(Priority::Priority1);
+
+        let done = mk_static!(Signal<CriticalSectionRawMutex, ()>, Signal::new());
+
+        interrupt_spawner.must_spawn(sense_pin(gpio1.degrade(), done));
+        interrupt_spawner.must_spawn(drive_pin(gpio2.degrade()));
+
+        done.wait().await;
+    }
 }