From 5982222a40ef5a2b773ffb608c238ec228132526 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 8 Aug 2025 12:09:41 +0200 Subject: [PATCH] Make mutexes reentrant by default (#3913) * Make mutexes reentrant by default * Remove redundant function --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/sync.rs | 36 +++++++++++++++++------------------ esp-hal/src/timer/systimer.rs | 8 ++++---- esp-hal/src/timer/timg.rs | 4 ++-- hil-test/tests/gpio.rs | 31 ++++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 1293ad61d..424c7eb0b 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - WDT now allows configuring longer timeouts (#3816) - `ADC2` now cannot be used simultaneously with `radio` on ESP32 (#3876) - Switched GPIO32 and GPIO33 ADC channel numbers (#3908, #3911) +- Calling `Input::unlisten` in a GPIO interrupt handler no longer panics (#3913) ### Removed diff --git a/esp-hal/src/sync.rs b/esp-hal/src/sync.rs index 6ab06c925..205d52ba1 100644 --- a/esp-hal/src/sync.rs +++ b/esp-hal/src/sync.rs @@ -321,8 +321,14 @@ impl GenericRawMutex { /// /// Note that this function is not reentrant, calling it reentrantly will /// panic. + pub fn lock_non_reentrant(&self, f: impl FnOnce() -> R) -> R { + let _token = LockGuard::new_non_reentrant(self); + f() + } + + /// Runs the callback with this lock locked. pub fn lock(&self, f: impl FnOnce() -> R) -> R { - let _token = LockGuard::new(self); + let _token = LockGuard::new_reentrant(self); f() } } @@ -381,6 +387,11 @@ impl RawMutex { /// /// Note that this function is not reentrant, calling it reentrantly will /// panic. + pub fn lock_non_reentrant(&self, f: impl FnOnce() -> R) -> R { + self.inner.lock_non_reentrant(f) + } + + /// Runs the callback with this lock locked. pub fn lock(&self, f: impl FnOnce() -> R) -> R { self.inner.lock(f) } @@ -391,9 +402,7 @@ unsafe impl embassy_sync::blocking_mutex::raw::RawMutex for RawMutex { const INIT: Self = Self::new(); fn lock(&self, f: impl FnOnce() -> R) -> R { - // embassy_sync semantics allow reentrancy. - let _token = LockGuard::new_reentrant(&self.inner); - f() + self.inner.lock(f) } } @@ -414,9 +423,6 @@ impl RawPriorityLimitedMutex { } /// Runs the callback with this lock locked. - /// - /// Note that this function is not reentrant, calling it reentrantly will - /// panic. pub fn lock(&self, f: impl FnOnce() -> R) -> R { self.inner.lock(f) } @@ -427,19 +433,10 @@ unsafe impl embassy_sync::blocking_mutex::raw::RawMutex for RawPriorityLimitedMu const INIT: Self = Self::new(Priority::max()); fn lock(&self, f: impl FnOnce() -> R) -> R { - // embassy_sync semantics allow reentrancy. - let _token = LockGuard::new_reentrant(&self.inner); - f() + self.inner.lock(f) } } -// Prefer this over a critical-section as this allows you to have multiple -// locks active at the same time rather than using the global mutex that is -// critical-section. -pub(crate) fn lock(lock: &RawMutex, f: impl FnOnce() -> T) -> T { - lock.lock(f) -} - /// Data protected by a [RawMutex]. /// /// This is largely equivalent to a `Mutex>`, but accessing the inner @@ -462,7 +459,8 @@ impl Locked { /// /// Calling this reentrantly will panic. pub fn with(&self, f: impl FnOnce(&mut T) -> R) -> R { - lock(&self.lock_state, || f(unsafe { &mut *self.data.get() })) + self.lock_state + .lock_non_reentrant(|| f(unsafe { &mut *self.data.get() })) } } @@ -474,7 +472,7 @@ struct LockGuard<'a, L: single_core::RawLock> { } impl<'a, L: single_core::RawLock> LockGuard<'a, L> { - fn new(lock: &'a GenericRawMutex) -> Self { + fn new_non_reentrant(lock: &'a GenericRawMutex) -> Self { let this = Self::new_reentrant(lock); assert!(!this.token.is_reentry(), "lock is not reentrant"); this diff --git a/esp-hal/src/timer/systimer.rs b/esp-hal/src/timer/systimer.rs index f6cb3c7c7..8003d389f 100644 --- a/esp-hal/src/timer/systimer.rs +++ b/esp-hal/src/timer/systimer.rs @@ -24,7 +24,7 @@ use crate::{ asynch::AtomicWaker, interrupt::{self, InterruptHandler}, peripherals::{Interrupt, SYSTIMER}, - sync::{RawMutex, lock}, + sync::RawMutex, system::{Cpu, Peripheral as PeripheralEnable, PeripheralClockControl}, time::{Duration, Instant}, }; @@ -168,7 +168,7 @@ impl Unit { #[cfg(not(esp32s2))] fn configure(&self, config: UnitConfig) { - lock(&CONF_LOCK, || { + CONF_LOCK.lock(|| { SYSTIMER::regs().conf().modify(|_, w| match config { UnitConfig::Disabled => match self.channel() { 0 => w.timer_unit0_work_en().clear_bit(), @@ -313,7 +313,7 @@ impl Alarm<'_> { /// Enables/disables the comparator. If enabled, this means /// it will generate interrupt based on its configuration. fn set_enable(&self, enable: bool) { - lock(&CONF_LOCK, || { + CONF_LOCK.lock(|| { #[cfg(not(esp32s2))] SYSTIMER::regs().conf().modify(|_, w| match self.channel() { 0 => w.target0_work_en().bit(enable), @@ -566,7 +566,7 @@ impl super::Timer for Alarm<'_> { } fn enable_interrupt(&self, state: bool) { - lock(&INT_ENA_LOCK, || { + INT_ENA_LOCK.lock(|| { SYSTIMER::regs() .int_ena() .modify(|_, w| w.target(self.channel()).bit(state)); diff --git a/esp-hal/src/timer/timg.rs b/esp-hal/src/timer/timg.rs index f948b6786..ab9b7988c 100644 --- a/esp-hal/src/timer/timg.rs +++ b/esp-hal/src/timer/timg.rs @@ -93,7 +93,7 @@ cfg_if::cfg_if! { // and S2 where the effective interrupt enable register (config) is not shared between // the timers. if #[cfg(all(timergroup_timg_has_timer1, not(any(esp32, esp32s2))))] { - use crate::sync::{lock, RawMutex}; + use crate::sync::RawMutex; static INT_ENA_LOCK: [RawMutex; NUM_TIMG] = [const { RawMutex::new() }; NUM_TIMG]; } } @@ -582,7 +582,7 @@ impl Timer<'_> { .config() .modify(|_, w| w.level_int_en().bit(state)); } else if #[cfg(timergroup_timg_has_timer1)] { - lock(&INT_ENA_LOCK[self.timer_group() as usize], || { + INT_ENA_LOCK[self.timer_group() as usize].lock(|| { self.register_block() .int_ena() .modify(|_, w| w.t(self.timer_number()).bit(state)); diff --git a/hil-test/tests/gpio.rs b/hil-test/tests/gpio.rs index 048923f1e..ef05c57f6 100644 --- a/hil-test/tests/gpio.rs +++ b/hil-test/tests/gpio.rs @@ -58,6 +58,18 @@ pub fn interrupt_handler() { }); } +#[cfg_attr(feature = "unstable", handler)] +#[cfg(feature = "unstable")] +pub fn interrupt_handler_unlisten() { + critical_section::with(|cs| { + INPUT_PIN + .borrow_ref_mut(cs) + .as_mut() + .map(|pin| pin.unlisten()); + *COUNTER.borrow_ref_mut(cs) += 1; + }); +} + // Compile-time test to check that GPIOs can be passed by reference. fn _gpios_can_be_reused() { let p = esp_hal::init(esp_hal::Config::default()); @@ -328,6 +340,25 @@ mod tests { test_gpio1.unlisten(); } + #[test] + #[cfg(feature = "unstable")] // Interrupts are unstable + async fn unlisten_in_interrupt_handler_does_not_panic(mut ctx: Context) { + ctx.io.set_interrupt_handler(interrupt_handler_unlisten); + + 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()); + + critical_section::with(|cs| { + *COUNTER.borrow_ref_mut(cs) = 0; + test_gpio1.listen(Event::AnyEdge); + INPUT_PIN.borrow_ref_mut(cs).replace(test_gpio1); + }); + test_gpio2.set_high(); + + while critical_section::with(|cs| *COUNTER.borrow_ref(cs)) == 0 {} + } + #[test] #[cfg(feature = "unstable")] // delay is unstable fn gpio_od(ctx: Context) {