From 68345293ed78a22b8c16b84f8b4768d3cf2a33f9 Mon Sep 17 00:00:00 2001 From: Juraj Sadel Date: Thu, 21 Aug 2025 11:46:04 +0200 Subject: [PATCH] More consistent naming of interrupt-related functions (#3933) * More consistent naming of interrupt-related functions * MG entry * changelog * use correct package for MG * fix hil * other drivers * address review comments --- esp-hal-embassy/src/time_driver.rs | 4 +- esp-hal/CHANGELOG.md | 1 + esp-hal/MIGRATING-1.0.0-rc.0.md | 7 +++ esp-hal/src/aes/mod.rs | 4 +- esp-hal/src/analog/adc/riscv.rs | 18 +++---- esp-hal/src/timer/mod.rs | 22 +++++--- esp-hal/src/touch.rs | 22 ++++---- esp-hal/src/twai/mod.rs | 54 +++++++++++++++---- esp-lp-hal/src/i2c.rs | 6 +-- esp-preempt/src/timer/mod.rs | 4 +- examples/peripheral/touch/src/main.rs | 6 +-- hil-test/tests/systimer.rs | 8 +-- qa-test/src/bin/embassy_executor_benchmark.rs | 2 +- 13 files changed, 104 insertions(+), 54 deletions(-) diff --git a/esp-hal-embassy/src/time_driver.rs b/esp-hal-embassy/src/time_driver.rs index 6a70598df..e1f3516e9 100644 --- a/esp-hal-embassy/src/time_driver.rs +++ b/esp-hal-embassy/src/time_driver.rs @@ -52,7 +52,7 @@ impl AlarmState { // timer. This ensures that alarms allocated after init are correctly // bound to the core that created the executor. timer.set_interrupt_handler(interrupt_handler); - timer.enable_interrupt(true); + timer.listen(); AlarmState::Initialized(timer) } } @@ -152,7 +152,7 @@ impl EmbassyTimer { // Reset timers timers.iter_mut().for_each(|timer| { - timer.enable_interrupt(false); + timer.unlisten(); timer.stop(); }); diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index f13e27efd..72f3627b0 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -90,6 +90,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Adjusted ESP32-S2 deep-sleep to hibernate for the Ext1WakeupSource (#3785) - Libraries depending on esp-hal should now disable default features, so that only the final binary crate enables the `rt` feature (#3706) - Changed `interrupt::RESERVED_INTERRUPTS` from `&[usize]` to `&[u32]` (#3798) +- Replace Timer's `pub fn enable_interrupt(&mut self, enable: bool)` with `pub fn listen(&mut self)` and `pub fn unlisten(&mut self)` #(3933) ### Fixed diff --git a/esp-hal/MIGRATING-1.0.0-rc.0.md b/esp-hal/MIGRATING-1.0.0-rc.0.md index 33b9f3b72..2a9d44c96 100644 --- a/esp-hal/MIGRATING-1.0.0-rc.0.md +++ b/esp-hal/MIGRATING-1.0.0-rc.0.md @@ -128,3 +128,10 @@ DMA buffer implementations. ``` If the `Final` type is not `Self`, `fn from_view()` will need to be updated to return `Self::Final`. + +## Timer interrupts +```diff +- pub fn enable_interrupt(&mut self, enable: bool) { ... } ++ pub fn listen(&mut self) { ... } ++ pub fn unlisten(&mut self) { ... } +``` diff --git a/esp-hal/src/aes/mod.rs b/esp-hal/src/aes/mod.rs index f13151045..fe080bd1d 100644 --- a/esp-hal/src/aes/mod.rs +++ b/esp-hal/src/aes/mod.rs @@ -475,7 +475,7 @@ pub mod dma { } self.enable_dma(true); - self.enable_interrupt(); + self.listen(); self.aes.write_mode(mode); self.write_key(key); @@ -539,7 +539,7 @@ pub mod dma { .write(|w| w.dma_enable().bit(enable)); } - fn enable_interrupt(&self) { + fn listen(&self) { self.aes.regs().int_ena().write(|w| w.int_ena().set_bit()); } diff --git a/esp-hal/src/analog/adc/riscv.rs b/esp-hal/src/analog/adc/riscv.rs index 25c24d29a..ca44a9d8d 100644 --- a/esp-hal/src/analog/adc/riscv.rs +++ b/esp-hal/src/analog/adc/riscv.rs @@ -551,16 +551,16 @@ pub(crate) fn adc_interrupt_handler() { fn handle_async(_instance: ADCI) { ADCI::waker().wake(); - ADCI::disable_interrupt(); + ADCI::unlisten(); } /// Enable asynchronous access. pub trait Instance: crate::private::Sealed { /// Enable the ADC interrupt - fn enable_interrupt(); + fn listen(); /// Disable the ADC interrupt - fn disable_interrupt(); + fn unlisten(); /// Clear the ADC interrupt fn clear_interrupt(); @@ -571,13 +571,13 @@ pub trait Instance: crate::private::Sealed { #[cfg(adc_adc1)] impl Instance for crate::peripherals::ADC1<'_> { - fn enable_interrupt() { + fn listen() { APB_SARADC::regs() .int_ena() .modify(|_, w| w.adc1_done().set_bit()); } - fn disable_interrupt() { + fn unlisten() { APB_SARADC::regs() .int_ena() .modify(|_, w| w.adc1_done().clear_bit()); @@ -598,13 +598,13 @@ impl Instance for crate::peripherals::ADC1<'_> { #[cfg(adc_adc2)] impl Instance for crate::peripherals::ADC2<'_> { - fn enable_interrupt() { + fn listen() { APB_SARADC::regs() .int_ena() .modify(|_, w| w.adc2_done().set_bit()); } - fn disable_interrupt() { + fn unlisten() { APB_SARADC::regs() .int_ena() .modify(|_, w| w.adc2_done().clear_bit()); @@ -645,7 +645,7 @@ impl core::future::Future for AdcFuture< Poll::Ready(()) } else { ADCI::waker().register(cx.waker()); - ADCI::enable_interrupt(); + ADCI::listen(); Poll::Pending } } @@ -653,6 +653,6 @@ impl core::future::Future for AdcFuture< impl Drop for AdcFuture { fn drop(&mut self) { - ADCI::disable_interrupt(); + ADCI::unlisten(); } } diff --git a/esp-hal/src/timer/mod.rs b/esp-hal/src/timer/mod.rs index a094e1a0b..73d84e395 100644 --- a/esp-hal/src/timer/mod.rs +++ b/esp-hal/src/timer/mod.rs @@ -296,9 +296,14 @@ where self.inner.set_interrupt_handler(handler); } - /// Enable listening for interrupts - pub fn enable_interrupt(&mut self, enable: bool) { - self.inner.enable_interrupt(enable); + /// Listen for interrupt + pub fn listen(&mut self) { + self.inner.enable_interrupt(true); + } + + /// Unlisten for interrupt + pub fn unlisten(&mut self) { + self.inner.enable_interrupt(false); } /// Clear the interrupt flag @@ -391,9 +396,14 @@ where self.inner.set_interrupt_handler(handler); } - /// Enable/disable listening for interrupts - pub fn enable_interrupt(&mut self, enable: bool) { - self.inner.enable_interrupt(enable); + /// Listen for interrupt + pub fn listen(&mut self) { + self.inner.enable_interrupt(true); + } + + /// Unlisten for interrupt + pub fn unlisten(&mut self) { + self.inner.enable_interrupt(false); } /// Clear the interrupt flag diff --git a/esp-hal/src/touch.rs b/esp-hal/src/touch.rs index b723cb771..f66b9cab6 100644 --- a/esp-hal/src/touch.rs +++ b/esp-hal/src/touch.rs @@ -404,7 +404,7 @@ impl TouchPad { self.pin.touch_measurement(Internal) } - /// Enables the touch_pad interrupt. + /// Listens for the touch_pad interrupt. /// /// The raised interrupt is actually /// [`RTC_CORE`](crate::peripherals::Interrupt::RTC_CORE). A handler can @@ -417,17 +417,17 @@ impl TouchPad { /// depends on the configuration of `touch` in [`new`](Self::new) (defaults to below). /// /// ## Example - pub fn enable_interrupt(&mut self, threshold: u16) { + pub fn listen(&mut self, threshold: u16) { self.pin.set_threshold(threshold, Internal); - internal_enable_interrupt(self.pin.touch_nr(Internal)) + listen(self.pin.touch_nr(Internal)) } - /// Disables the touch pad's interrupt. + /// Unlisten for the touch pad's interrupt. /// /// If no other touch pad interrupts are active, the touch interrupt is /// disabled completely. - pub fn disable_interrupt(&mut self) { - internal_disable_interrupt(self.pin.touch_nr(Internal)) + pub fn unlisten(&mut self) { + unlisten(self.pin.touch_nr(Internal)) } /// Clears a pending touch interrupt. @@ -448,7 +448,7 @@ impl TouchPad { } } -fn internal_enable_interrupt(touch_nr: u8) { +fn listen(touch_nr: u8) { // enable touch interrupts LPWR::regs().int_ena().write(|w| w.touch().set_bit()); @@ -458,7 +458,7 @@ fn internal_enable_interrupt(touch_nr: u8) { }); } -fn internal_disable_interrupt(touch_nr: u8) { +fn unlisten(touch_nr: u8) { SENS::regs().sar_touch_enable().modify(|r, w| unsafe { w.touch_pad_outen1() .bits(r.touch_pad_outen1().bits() & !(1 << touch_nr)) @@ -474,7 +474,7 @@ fn internal_disable_interrupt(touch_nr: u8) { } } -fn internal_disable_interrupts() { +fn internal_unlisten() { SENS::regs() .sar_touch_enable() .write(|w| unsafe { w.touch_pad_outen1().bits(0) }); @@ -565,7 +565,7 @@ mod asynch { } TOUCHED_PINS.store(touch_pads, Ordering::Relaxed); internal_clear_interrupt(); - internal_disable_interrupts(); + internal_unlisten(); } impl TouchPad { @@ -573,7 +573,7 @@ mod asynch { pub async fn wait_for_touch(&mut self, threshold: u16) { self.pin.set_threshold(threshold, Internal); let touch_nr = self.pin.touch_nr(Internal); - internal_enable_interrupt(touch_nr); + listen(touch_nr); TouchFuture::new(touch_nr).await; } } diff --git a/esp-hal/src/twai/mod.rs b/esp-hal/src/twai/mod.rs index 427213351..e19ebe951 100644 --- a/esp-hal/src/twai/mod.rs +++ b/esp-hal/src/twai/mod.rs @@ -122,6 +122,7 @@ use core::marker::PhantomData; +use enumset::{EnumSet, EnumSetType}; use procmacros::handler; use self::filter::{Filter, FilterType}; @@ -143,7 +144,6 @@ use crate::{ system::{Cpu, PeripheralGuard}, twai::filter::SingleStandardFilter, }; - pub mod filter; /// TWAI error kind @@ -1209,6 +1209,24 @@ where } } +/// List of TWAI events. +#[derive(Debug, EnumSetType)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[non_exhaustive] +#[instability::unstable] +pub enum TwaiInterrupt { + /// A frame has been received. + Receive, + /// A frame has been transmitted. + Transmit, + /// An error has occurred on the bus. + BusError, + /// An arbitration lost event has occurred. + ArbitrationLost, + /// The controller has entered an error passive state. + ErrorPassive, +} + /// Represents errors that can occur in the TWAI driver. /// This enum defines the possible errors that can be encountered when /// interacting with the TWAI peripheral. @@ -1313,17 +1331,31 @@ pub trait PrivateInstance: crate::private::Sealed { /// Returns a reference to the register block for TWAI instance. fn register_block(&self) -> &RegisterBlock; - /// Enables interrupts for the TWAI peripheral. - fn enable_interrupts(&self) { + /// Enables/disables interrupts for the TWAI peripheral based on the `enable` flag. + fn enable_interrupts(&self, interrupts: EnumSet, enable: bool) { self.register_block().int_ena().modify(|_, w| { - w.rx_int_ena().set_bit(); - w.tx_int_ena().set_bit(); - w.bus_err_int_ena().set_bit(); - w.arb_lost_int_ena().set_bit(); - w.err_passive_int_ena().set_bit() + for interrupt in interrupts { + match interrupt { + TwaiInterrupt::Receive => w.rx_int_ena().bit(enable), + TwaiInterrupt::Transmit => w.tx_int_ena().bit(enable), + TwaiInterrupt::BusError => w.bus_err_int_ena().bit(enable), + TwaiInterrupt::ArbitrationLost => w.arb_lost_int_ena().bit(enable), + TwaiInterrupt::ErrorPassive => w.err_passive_int_ena().bit(enable), + }; + } + w }); } + /// Listen for given interrupts. + fn listen(&mut self, interrupts: impl Into>) { + self.enable_interrupts(interrupts.into(), true); + } + + /// Unlisten the given interrupts. + fn unlisten(&mut self, interrupts: impl Into>) { + self.enable_interrupts(interrupts.into(), false); + } /// Returns a reference to the asynchronous state for this TWAI instance. fn async_state(&self) -> &asynch::TwaiAsyncState; } @@ -1705,11 +1737,11 @@ mod asynch { /// /// The transmission is aborted if the future is dropped. The technical /// reference manual does not specifiy if aborting the transmission also - /// stops it, in case it is activly transmitting. Therefor it could be + /// stops it, in case it is actively transmitting. Therefor it could be /// the case that even though the future is dropped, the frame was sent /// anyways. pub async fn transmit_async(&mut self, frame: &EspTwaiFrame) -> Result<(), EspTwaiError> { - self.twai.enable_interrupts(); + self.twai.listen(EnumSet::all()); TransmitFuture::new(self.twai.reborrow(), frame).await } } @@ -1717,7 +1749,7 @@ mod asynch { impl TwaiRx<'_, Async> { /// Receives an `EspTwaiFrame` asynchronously over the TWAI bus. pub async fn receive_async(&mut self) -> Result { - self.twai.enable_interrupts(); + self.twai.listen(EnumSet::all()); poll_fn(|cx| { self.twai.async_state().err_waker.register(cx.waker()); diff --git a/esp-lp-hal/src/i2c.rs b/esp-lp-hal/src/i2c.rs index 4f69e466f..b984cfbc4 100644 --- a/esp-lp-hal/src/i2c.rs +++ b/esp-lp-hal/src/i2c.rs @@ -219,7 +219,7 @@ impl LpI2c { }, )?; - self.enable_interrupts(I2C_LL_INTR_MASK); + self.enable_listen(I2C_LL_INTR_MASK); let mut data_idx = 0; let mut remaining_bytes = bytes.len() as u32; @@ -298,7 +298,7 @@ impl LpI2c { }, )?; - self.enable_interrupts( + self.enable_listen( (1 << LP_I2C_TRANS_COMPLETE_INT_ST_S) | (1 << LP_I2C_END_DETECT_INT_ST_S), ); @@ -442,7 +442,7 @@ impl LpI2c { Ok(()) } - fn enable_interrupts(&self, mask: u32) { + fn enable_listen(&self, mask: u32) { self.i2c.int_ena().write(|w| unsafe { w.bits(mask) }); } diff --git a/esp-preempt/src/timer/mod.rs b/esp-preempt/src/timer/mod.rs index 1e169b23d..512d53b18 100644 --- a/esp-preempt/src/timer/mod.rs +++ b/esp-preempt/src/timer/mod.rs @@ -32,7 +32,7 @@ pub(crate) fn setup_timebase(mut timer: TimeBase) { timer.set_interrupt_handler(handler); TIMER.with(|t| { - timer.enable_interrupt(true); + timer.listen(); t.replace(timer); }); } @@ -46,7 +46,7 @@ pub(crate) fn clear_timer_interrupt() { pub(crate) fn disable_timebase() { TIMER.with(|timer| { let mut timer = unwrap!(timer.take()); - timer.enable_interrupt(false); + timer.unlisten(); unwrap!(timer.cancel()); }); } diff --git a/examples/peripheral/touch/src/main.rs b/examples/peripheral/touch/src/main.rs index f0e7c4845..089f10382 100644 --- a/examples/peripheral/touch/src/main.rs +++ b/examples/peripheral/touch/src/main.rs @@ -41,7 +41,7 @@ fn interrupt_handler() { touch1.clear_interrupt(); // We disable the interrupt until the next loop iteration to avoid massive // retriggering. - touch1.disable_interrupt(); + touch1.unlisten(); } }); } @@ -72,7 +72,7 @@ fn main() -> ! { critical_section::with(|cs| { // A good threshold is 2/3 of the reading when the pad is not touched. - touch1.enable_interrupt(touch1_baseline * 2 / 3); + touch1.listen(touch1_baseline * 2 / 3); TOUCH1.borrow_ref_mut(cs).replace(touch1) }); @@ -86,7 +86,7 @@ fn main() -> ! { .borrow_ref_mut(cs) .as_mut() .unwrap() - .enable_interrupt(touch1_baseline * 2 / 3); + .listen(touch1_baseline * 2 / 3); }); } } diff --git a/hil-test/tests/systimer.rs b/hil-test/tests/systimer.rs index 9031eddb0..1fbd9c418 100644 --- a/hil-test/tests/systimer.rs +++ b/hil-test/tests/systimer.rs @@ -112,7 +112,7 @@ mod tests { critical_section::with(|cs| { alarm0.set_interrupt_handler(pass_test_if_called); - alarm0.enable_interrupt(true); + alarm0.listen(); alarm0.schedule(Duration::from_millis(10)).unwrap(); ALARM_TARGET.borrow_ref_mut(cs).replace(alarm0); @@ -131,11 +131,11 @@ mod tests { critical_section::with(|cs| { alarm0.set_interrupt_handler(target_fail_test_if_called_twice); - alarm0.enable_interrupt(true); + alarm0.listen(); alarm0.schedule(Duration::from_millis(10)).unwrap(); alarm1.set_interrupt_handler(handle_periodic_interrupt); - alarm1.enable_interrupt(true); + alarm1.listen(); alarm1.start(Duration::from_millis(100)).unwrap(); ALARM_TARGET.borrow_ref_mut(cs).replace(alarm0); @@ -154,7 +154,7 @@ mod tests { critical_section::with(|cs| { alarm1.set_interrupt_handler(pass_test_if_called_twice); - alarm1.enable_interrupt(true); + alarm1.listen(); alarm1.start(Duration::from_millis(100)).unwrap(); ALARM_PERIODIC.borrow_ref_mut(cs).replace(alarm1); diff --git a/qa-test/src/bin/embassy_executor_benchmark.rs b/qa-test/src/bin/embassy_executor_benchmark.rs index 0e30858fb..1d99aac20 100644 --- a/qa-test/src/bin/embassy_executor_benchmark.rs +++ b/qa-test/src/bin/embassy_executor_benchmark.rs @@ -91,6 +91,6 @@ async fn main(spawner: Spawner) { let mut timer = OneShotTimer::new(systimer.alarm1); timer.set_interrupt_handler(timer_handler); - timer.enable_interrupt(true); + timer.listen(); timer.schedule(Duration::from_millis(TEST_MILLIS)).unwrap(); }