diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 3f12da94ae0..06a7b6b40d8 100644 --- a/esp-hal/CHANGELOG.md +++ b/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] diff --git a/esp-hal/src/gpio/mod.rs b/esp-hal/src/gpio/mod.rs index 4186dc4d719..672021f9df0 100644 --- a/esp-hal/src/gpio/mod.rs +++ b/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 private::Sealed for GpioPin {} +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); } diff --git a/esp-hal/src/interrupt/riscv.rs b/esp-hal/src/interrupt/riscv.rs index 151fa622471..de55b303e8f 100644 --- a/esp-hal/src/interrupt/riscv.rs +++ b/esp-hal/src/interrupt/riscv.rs @@ -346,7 +346,7 @@ unsafe fn get_assigned_cpu_interrupt(interrupt: Interrupt) -> Option 0 { + if cpu_intr > 0 && cpu_intr != DISABLED_CPU_INTERRUPT { Some(core::mem::transmute::( cpu_intr - EXTERNAL_INTERRUPT_OFFSET, )) @@ -355,6 +355,10 @@ unsafe fn get_assigned_cpu_interrupt(interrupt: Interrupt) -> Option Option { + 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 { + 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) { diff --git a/esp-hal/src/interrupt/xtensa.rs b/esp-hal/src/interrupt/xtensa.rs index 5ed4138c440..c83d1e75011 100644 --- a/esp-hal/src/interrupt/xtensa.rs +++ b/esp-hal/src/interrupt/xtensa.rs @@ -94,6 +94,32 @@ pub enum CpuInterrupt { Interrupt31EdgePriority5, } +impl CpuInterrupt { + fn from_u32(n: u32) -> Option { + if n < 32 { + Some(unsafe { core::mem::transmute::(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 { + 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 { + 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, diff --git a/esp-hal/src/lib.rs b/esp-hal/src/lib.rs index aa8dda2a86c..f0c28132f68 100644 --- a/esp-hal/src/lib.rs +++ b/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 } diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml index 112d87941bb..0915ffed01e 100644 --- a/hil-test/Cargo.toml +++ b/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 diff --git a/hil-test/tests/gpio_custom_handler.rs b/hil-test/tests/gpio_custom_handler.rs new file mode 100644 index 00000000000..334c9f53101 --- /dev/null +++ b/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, gpio2: impl Into) -> 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); + } +}