GPIO: do not overwrite interrupt handler (#2486)

* Prefer defmt unwrap

* Auto-bind the GPIO interrupt handler

* Changelog

* Bind interrupt handler when initializing the hal

* Remove user handler overhead unless user handler is bound

* Add tests

* Try to preserve direct-bound handlers

* Fix match

* Don't rely on the vector table to check direct binding

* Fix comment

esp-hal/CHANGELOG.md

@@ -97,6 +97,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The `GpioEtmEventRising`, `GpioEtmEventFalling`, `GpioEtmEventAny` types have been replaced with `Event` (#2427)
 - The `TaskSet`, `TaskClear`, `TaskToggle` types have been replaced with `Task` (#2427)
 - `{Spi, SpiDma, SpiDmaBus}` configuration methods (#2448)
+- `Io::new_with_priority` and `Io::new_no_bind_interrupt`. (#2486)
 
 ## [0.21.1]
 

esp-hal/src/gpio/mod.rs

@@ -73,11 +73,12 @@ use procmacros::ram;
 
 pub use crate::soc::gpio::*;
 use crate::{
-    interrupt::InterruptHandler,
+    interrupt::{self, InterruptHandler, Priority},
     peripheral::{Peripheral, PeripheralRef},
-    peripherals::{GPIO, IO_MUX},
+    peripherals::{Interrupt, GPIO, IO_MUX},
     private::{self, Sealed},
     InterruptConfigurable,
+    DEFAULT_INTERRUPT_HANDLER,
 };
 
 pub mod interconnect;
@@ -784,6 +785,31 @@ where
 
 impl<const GPIONUM: u8> private::Sealed for GpioPin<GPIONUM> {}
 
+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::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
 pub struct Io {
     _io_mux: IO_MUX,
@@ -793,36 +819,17 @@ pub struct Io {
 
 impl Io {
     /// Initialize the I/O driver.
-    pub fn new(gpio: GPIO, io_mux: IO_MUX) -> Self {
-        Self::new_with_priority(gpio, io_mux, crate::interrupt::Priority::min())
-    }
-
-    /// Initialize the I/O driver with a interrupt priority.
-    ///
-    /// This decides the priority for the interrupt when using async.
-    pub fn new_with_priority(
-        mut gpio: GPIO,
-        io_mux: IO_MUX,
-        prio: crate::interrupt::Priority,
-    ) -> Self {
-        gpio.bind_gpio_interrupt(gpio_interrupt_handler);
-        crate::interrupt::enable(crate::peripherals::Interrupt::GPIO, prio).unwrap();
-
-        Self::new_no_bind_interrupt(gpio, io_mux)
-    }
-
-    /// Initialize the I/O driver without enabling the GPIO interrupt or
-    /// binding an interrupt handler to it.
-    ///
-    /// *Note:* You probably don't want to use this, it is intended to be used
-    /// in very specific use cases. Async GPIO functionality will not work
-    /// when instantiating `Io` using this constructor.
-    pub fn new_no_bind_interrupt(_gpio: GPIO, _io_mux: IO_MUX) -> Self {
+    pub fn new(_gpio: GPIO, _io_mux: IO_MUX) -> Self {
         Io {
             _io_mux,
             pins: unsafe { Pins::steal() },
         }
     }
+
+    /// Set the interrupt priority for GPIO interrupts.
+    pub fn set_interrupt_priority(&self, prio: Priority) {
+        unwrap!(interrupt::enable(Interrupt::GPIO, prio));
+    }
 }
 
 impl crate::private::Sealed for Io {}
@@ -840,15 +847,24 @@ impl InterruptConfigurable for Io {
     ///   corresponding pin's async API.
     /// - You will not be notified if you make a mistake.
     fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
-        crate::interrupt::enable(crate::peripherals::Interrupt::GPIO, handler.priority()).unwrap();
+        for core in crate::Cpu::other() {
+            crate::interrupt::disable(core, Interrupt::GPIO);
+        }
+        self.set_interrupt_priority(handler.priority());
+        unsafe { interrupt::bind_interrupt(Interrupt::GPIO, user_gpio_interrupt_handler) };
         USER_INTERRUPT_HANDLER.store(handler.handler());
     }
 }
 
 #[ram]
-extern "C" fn gpio_interrupt_handler() {
+extern "C" fn user_gpio_interrupt_handler() {
     USER_INTERRUPT_HANDLER.call();
 
+    default_gpio_interrupt_handler();
+}
+
+#[ram]
+extern "C" fn default_gpio_interrupt_handler() {
     handle_pin_interrupts(on_pin_irq);
 }
 

esp-hal/src/interrupt/riscv.rs

@@ -346,7 +346,7 @@ unsafe fn get_assigned_cpu_interrupt(interrupt: Interrupt) -> Option<CpuInterrup
     let intr_map_base = crate::soc::registers::INTERRUPT_MAP_BASE as *mut u32;
 
     let cpu_intr = intr_map_base.offset(interrupt_number).read_volatile();
-    if cpu_intr > 0 {
+    if cpu_intr > 0 && cpu_intr != DISABLED_CPU_INTERRUPT {
         Some(core::mem::transmute::<u32, CpuInterrupt>(
             cpu_intr - EXTERNAL_INTERRUPT_OFFSET,
         ))
@@ -355,6 +355,10 @@ unsafe fn get_assigned_cpu_interrupt(interrupt: Interrupt) -> Option<CpuInterrup
     }
 }
 
+pub(crate) fn bound_cpu_interrupt_for(_cpu: Cpu, interrupt: Interrupt) -> Option<CpuInterrupt> {
+    unsafe { get_assigned_cpu_interrupt(interrupt) }
+}
+
 mod vectored {
     use procmacros::ram;
 
@@ -423,17 +427,29 @@ mod vectored {
         Ok(())
     }
 
-    /// Bind the given interrupt to the given handler
+    /// Binds the given interrupt to the given handler.
     ///
     /// # Safety
     ///
     /// This will replace any previously bound interrupt handler
-    pub unsafe fn bind_interrupt(interrupt: Interrupt, handler: unsafe extern "C" fn() -> ()) {
+    pub unsafe fn bind_interrupt(interrupt: Interrupt, handler: unsafe extern "C" fn()) {
         let ptr = &peripherals::__EXTERNAL_INTERRUPTS[interrupt as usize]._handler as *const _
-            as *mut unsafe extern "C" fn() -> ();
+            as *mut unsafe extern "C" fn();
         ptr.write_volatile(handler);
     }
 
+    /// Returns the currently bound interrupt handler.
+    pub fn bound_handler(interrupt: Interrupt) -> Option<unsafe extern "C" fn()> {
+        unsafe {
+            let addr = peripherals::__EXTERNAL_INTERRUPTS[interrupt as usize]._handler;
+            if addr as usize == 0 {
+                return None;
+            }
+
+            Some(addr)
+        }
+    }
+
     #[no_mangle]
     #[ram]
     unsafe fn handle_interrupts(cpu_intr: CpuInterrupt, context: &mut TrapFrame) {

esp-hal/src/interrupt/xtensa.rs

@@ -94,6 +94,32 @@ pub enum CpuInterrupt {
     Interrupt31EdgePriority5,
 }
 
+impl CpuInterrupt {
+    fn from_u32(n: u32) -> Option<Self> {
+        if n < 32 {
+            Some(unsafe { core::mem::transmute::<u32, Self>(n) })
+        } else {
+            None
+        }
+    }
+
+    fn is_internal(self) -> bool {
+        matches!(
+            self,
+            Self::Interrupt6Timer0Priority1
+                | Self::Interrupt7SoftwarePriority1
+                | Self::Interrupt11ProfilingPriority3
+                | Self::Interrupt15Timer1Priority3
+                | Self::Interrupt16Timer2Priority5
+                | Self::Interrupt29SoftwarePriority3
+        )
+    }
+
+    fn is_peripheral(self) -> bool {
+        !self.is_internal()
+    }
+}
+
 /// The interrupts reserved by the HAL
 #[cfg_attr(place_switch_tables_in_ram, link_section = ".rwtext")]
 pub static RESERVED_INTERRUPTS: &[usize] = &[
@@ -160,6 +186,25 @@ pub unsafe fn map(core: Cpu, interrupt: Interrupt, which: CpuInterrupt) {
         .write_volatile(cpu_interrupt_number as u32);
 }
 
+/// Get cpu interrupt assigned to peripheral interrupt
+pub(crate) fn bound_cpu_interrupt_for(cpu: Cpu, interrupt: Interrupt) -> Option<CpuInterrupt> {
+    let interrupt_number = interrupt as isize;
+
+    let intr_map_base = match cpu {
+        Cpu::ProCpu => unsafe { (*core0_interrupt_peripheral()).pro_mac_intr_map().as_ptr() },
+        #[cfg(multi_core)]
+        Cpu::AppCpu => unsafe { (*core1_interrupt_peripheral()).app_mac_intr_map().as_ptr() },
+    };
+    let cpu_intr = unsafe { intr_map_base.offset(interrupt_number).read_volatile() };
+    let cpu_intr = CpuInterrupt::from_u32(cpu_intr)?;
+
+    if cpu_intr.is_peripheral() {
+        Some(cpu_intr)
+    } else {
+        None
+    }
+}
+
 /// Disable the given peripheral interrupt
 pub fn disable(core: Cpu, interrupt: Interrupt) {
     unsafe {
@@ -415,17 +460,29 @@ mod vectored {
         Ok(())
     }
 
-    /// Bind the given interrupt to the given handler
+    /// Binds the given interrupt to the given handler.
     ///
     /// # Safety
     ///
     /// This will replace any previously bound interrupt handler
-    pub unsafe fn bind_interrupt(interrupt: Interrupt, handler: unsafe extern "C" fn() -> ()) {
+    pub unsafe fn bind_interrupt(interrupt: Interrupt, handler: unsafe extern "C" fn()) {
         let ptr = &peripherals::__INTERRUPTS[interrupt as usize]._handler as *const _
-            as *mut unsafe extern "C" fn() -> ();
+            as *mut unsafe extern "C" fn();
         ptr.write_volatile(handler);
     }
 
+    /// Returns the currently bound interrupt handler.
+    pub fn bound_handler(interrupt: Interrupt) -> Option<unsafe extern "C" fn()> {
+        unsafe {
+            let addr = peripherals::__INTERRUPTS[interrupt as usize]._handler;
+            if addr as usize == 0 {
+                return None;
+            }
+
+            Some(addr)
+        }
+    }
+
     fn interrupt_level_to_cpu_interrupt(
         level: Priority,
         is_edge: bool,

esp-hal/src/lib.rs

@@ -560,5 +560,7 @@ pub fn init(config: Config) -> Peripherals {
     #[cfg(esp32)]
     crate::time::time_init();
 
+    crate::gpio::bind_default_interrupt_handler();
+
     peripherals
 }

hil-test/Cargo.toml

@@ -55,6 +55,10 @@ harness = false
 name    = "gpio"
 harness = false
 
+[[test]]
+name    = "gpio_custom_handler"
+harness = false
+
 [[test]]
 name    = "interrupt"
 harness = false

hil-test/tests/gpio_custom_handler.rs

@@ -0,0 +1,99 @@
+//! GPIO interrupt handler tests
+//!
+//! This test checks that during HAL initialization we do not overwrite custom
+//! GPIO interrupt handlers. We also check that binding a custom interrupt
+//! handler explicitly overwrites the handler set by the user, as well as the
+//! async API works for user handlers automatically.
+
+//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
+//% FEATURES: integrated-timers
+
+#![no_std]
+#![no_main]
+
+use embassy_time::{Duration, Timer};
+use esp_hal::{
+    gpio::{AnyPin, Input, Io, Level, Output, Pull},
+    macros::handler,
+    timer::timg::TimerGroup,
+    InterruptConfigurable,
+};
+use hil_test as _;
+use portable_atomic::{AtomicUsize, Ordering};
+
+#[no_mangle]
+unsafe extern "C" fn GPIO() {
+    // do nothing, prevents binding the default handler
+}
+
+#[handler]
+pub fn interrupt_handler() {
+    // Do nothing
+}
+
+async fn drive_pins(gpio1: impl Into<AnyPin>, gpio2: impl Into<AnyPin>) -> usize {
+    let counter = AtomicUsize::new(0);
+    let mut test_gpio1 = Input::new(gpio1.into(), Pull::Down);
+    let mut test_gpio2 = Output::new(gpio2.into(), Level::Low);
+    embassy_futures::select::select(
+        async {
+            loop {
+                test_gpio1.wait_for_rising_edge().await;
+                counter.fetch_add(1, Ordering::SeqCst);
+            }
+        },
+        async {
+            for _ in 0..5 {
+                test_gpio2.set_high();
+                Timer::after(Duration::from_millis(25)).await;
+                test_gpio2.set_low();
+                Timer::after(Duration::from_millis(25)).await;
+            }
+        },
+    )
+    .await;
+
+    counter.load(Ordering::SeqCst)
+}
+
+#[cfg(test)]
+#[embedded_test::tests(executor = esp_hal_embassy::Executor::new())]
+mod tests {
+
+    use super::*;
+
+    #[test]
+    async fn default_handler_does_not_run_because_gpio_is_defined() {
+        let peripherals = esp_hal::init(esp_hal::Config::default());
+
+        let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
+
+        let (gpio1, gpio2) = hil_test::common_test_pins!(io);
+
+        let timg0 = TimerGroup::new(peripherals.TIMG0);
+        esp_hal_embassy::init(timg0.timer0);
+
+        let counter = drive_pins(gpio1, gpio2).await;
+
+        // GPIO is bound to something else, so we don't expect the async API to work.
+        assert_eq!(counter, 0);
+    }
+
+    #[test]
+    async fn default_handler_runs_because_handler_is_set() {
+        let peripherals = esp_hal::init(esp_hal::Config::default());
+
+        let mut io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
+        io.set_interrupt_handler(interrupt_handler);
+
+        let (gpio1, gpio2) = hil_test::common_test_pins!(io);
+
+        let timg0 = TimerGroup::new(peripherals.TIMG0);
+        esp_hal_embassy::init(timg0.timer0);
+
+        let counter = drive_pins(gpio1, gpio2).await;
+
+        // We expect the async API to keep working even if a user handler is set.
+        assert_eq!(counter, 5);
+    }
+}