diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index ac319cef4a..a6ddaccf2b 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `DmaDescriptor` is now `#[repr(C)]` (#2988) - Fixed an issue that caused LCD_CAM drivers to turn off their clocks unexpectedly (#3007) - Fixed an issue where DMA-driver peripherals started transferring before the data was ready (#3003) +- Fixed an issue on ESP32 and S2 where short asynchronous Timer delays would never resolve (#3093) ### Removed diff --git a/esp-hal/src/timer/mod.rs b/esp-hal/src/timer/mod.rs index 12e7dcda49..c7b8cebb0c 100644 --- a/esp-hal/src/timer/mod.rs +++ b/esp-hal/src/timer/mod.rs @@ -41,9 +41,14 @@ //! # } //! ``` -use core::marker::PhantomData; +use core::{ + marker::PhantomData, + pin::Pin, + task::{Context, Poll}, +}; use crate::{ + asynch::AtomicWaker, interrupt::{InterruptConfigurable, InterruptHandler}, peripheral::{Peripheral, PeripheralRef}, peripherals::Interrupt, @@ -113,13 +118,6 @@ pub trait Timer: Into + 'static + crate::private::Sealed { /// Has the timer triggered? fn is_interrupt_set(&self) -> bool; - /// Asynchronously wait for the timer interrupt to fire. - /// - /// Requires the correct `InterruptHandler` to be installed to function - /// correctly. - #[doc(hidden)] - async fn wait(&self); - /// Returns the HAL provided async interrupt handler #[doc(hidden)] fn async_interrupt_handler(&self) -> InterruptHandler; @@ -130,6 +128,9 @@ pub trait Timer: Into + 'static + crate::private::Sealed { /// Configures the interrupt handler. #[doc(hidden)] fn set_interrupt_handler(&self, handler: InterruptHandler); + + #[doc(hidden)] + fn waker(&self) -> &AtomicWaker; } /// A one-shot timer. @@ -189,12 +190,56 @@ impl OneShotTimer<'_, Async> { async fn delay_async(&mut self, us: Duration) { unwrap!(self.schedule(us)); - self.inner.wait().await; + + WaitFuture::new(self.inner.reborrow()).await; + self.stop(); self.clear_interrupt(); } } +#[must_use = "futures do nothing unless you `.await` or poll them"] +struct WaitFuture<'d> { + timer: PeripheralRef<'d, AnyTimer>, +} + +impl<'d> WaitFuture<'d> { + fn new(timer: impl Peripheral

+ 'd) -> Self { + crate::into_ref!(timer); + // For some reason, on the S2 we need to enable the interrupt before we + // read its status. Doing so in the other order causes the interrupt + // request to never be fired. + timer.enable_interrupt(true); + Self { timer } + } + + fn is_done(&self) -> bool { + self.timer.is_interrupt_set() + } +} + +impl core::future::Future for WaitFuture<'_> { + type Output = (); + + fn poll(self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll { + // Interrupts are enabled, so we need to register the waker before we check for + // done. Otherwise we might miss the interrupt that would wake us. + self.timer.waker().register(ctx.waker()); + + if self.is_done() { + Poll::Ready(()) + } else { + Poll::Pending + } + } +} + +impl Drop for WaitFuture<'_> { + fn drop(&mut self) { + self.timer.enable_interrupt(false); + } +} + impl OneShotTimer<'_, Dm> where Dm: DriverMode, @@ -398,10 +443,10 @@ impl Timer for AnyTimer { fn enable_interrupt(&self, state: bool); fn clear_interrupt(&self); fn is_interrupt_set(&self) -> bool; - async fn wait(&self); fn async_interrupt_handler(&self) -> InterruptHandler; fn peripheral_interrupt(&self) -> Interrupt; fn set_interrupt_handler(&self, handler: InterruptHandler); + fn waker(&self) -> &AtomicWaker; } } } diff --git a/esp-hal/src/timer/systimer.rs b/esp-hal/src/timer/systimer.rs index 11d9b775ba..8c74637519 100644 --- a/esp-hal/src/timer/systimer.rs +++ b/esp-hal/src/timer/systimer.rs @@ -21,6 +21,7 @@ use core::fmt::Debug; use super::{Error, Timer as _}; use crate::{ + asynch::AtomicWaker, interrupt::{self, InterruptHandler}, peripheral::Peripheral, peripherals::{Interrupt, SYSTIMER}, @@ -562,10 +563,6 @@ impl super::Timer for Alarm { .bit_is_set() } - async fn wait(&self) { - asynch::AlarmFuture::new(self).await - } - fn async_interrupt_handler(&self) -> InterruptHandler { match self.channel() { 0 => asynch::target0_handler, @@ -587,6 +584,10 @@ impl super::Timer for Alarm { fn set_interrupt_handler(&self, handler: InterruptHandler) { self.set_interrupt_handler(handler) } + + fn waker(&self) -> &AtomicWaker { + asynch::waker(self) + } } impl Peripheral for Alarm { @@ -608,86 +609,42 @@ static INT_ENA_LOCK: RawMutex = RawMutex::new(); // Async functionality of the system timer. mod asynch { - use core::{ - pin::Pin, - task::{Context, Poll}, - }; - use procmacros::handler; use super::*; - use crate::{asynch::AtomicWaker, peripherals::SYSTIMER}; + use crate::asynch::AtomicWaker; const NUM_ALARMS: usize = 3; - static WAKERS: [AtomicWaker; NUM_ALARMS] = [const { AtomicWaker::new() }; NUM_ALARMS]; - #[must_use = "futures do nothing unless you `.await` or poll them"] - pub(crate) struct AlarmFuture<'a> { - alarm: &'a Alarm, + pub(super) fn waker(alarm: &Alarm) -> &AtomicWaker { + &WAKERS[alarm.channel() as usize] } - impl<'a> AlarmFuture<'a> { - pub(crate) fn new(alarm: &'a Alarm) -> Self { - alarm.enable_interrupt(true); - - Self { alarm } + #[inline] + fn handle_alarm(alarm: u8) { + Alarm { + comp: alarm, + unit: Unit::Unit0, } + .enable_interrupt(false); - fn event_bit_is_clear(&self) -> bool { - SYSTIMER::regs() - .int_ena() - .read() - .target(self.alarm.channel()) - .bit_is_clear() - } - } - - impl core::future::Future for AlarmFuture<'_> { - type Output = (); - - fn poll(self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll { - WAKERS[self.alarm.channel() as usize].register(ctx.waker()); - - if self.event_bit_is_clear() { - Poll::Ready(()) - } else { - Poll::Pending - } - } + WAKERS[alarm as usize].wake(); } #[handler] pub(crate) fn target0_handler() { - lock(&INT_ENA_LOCK, || { - SYSTIMER::regs() - .int_ena() - .modify(|_, w| w.target0().clear_bit()); - }); - - WAKERS[0].wake(); + handle_alarm(0); } #[handler] pub(crate) fn target1_handler() { - lock(&INT_ENA_LOCK, || { - SYSTIMER::regs() - .int_ena() - .modify(|_, w| w.target1().clear_bit()); - }); - - WAKERS[1].wake(); + handle_alarm(1); } #[handler] pub(crate) fn target2_handler() { - lock(&INT_ENA_LOCK, || { - SYSTIMER::regs() - .int_ena() - .modify(|_, w| w.target2().clear_bit()); - }); - - WAKERS[2].wake(); + handle_alarm(2); } } diff --git a/esp-hal/src/timer/timg.rs b/esp-hal/src/timer/timg.rs index edfe8b4100..0b546876b5 100644 --- a/esp-hal/src/timer/timg.rs +++ b/esp-hal/src/timer/timg.rs @@ -69,23 +69,33 @@ use core::marker::PhantomData; use super::Error; +#[cfg(timg1)] +use crate::peripherals::TIMG1; #[cfg(any(esp32c6, esp32h2))] use crate::soc::constants::TIMG_DEFAULT_CLK_SRC; use crate::{ + asynch::AtomicWaker, clock::Clocks, interrupt::{self, InterruptConfigurable, InterruptHandler}, pac::timg0::RegisterBlock, peripheral::Peripheral, peripherals::{Interrupt, TIMG0}, private::Sealed, - sync::{lock, RawMutex}, system::PeripheralClockControl, time::{Duration, Instant, Rate}, }; const NUM_TIMG: usize = 1 + cfg!(timg1) as usize; -static INT_ENA_LOCK: [RawMutex; NUM_TIMG] = [const { RawMutex::new() }; NUM_TIMG]; +cfg_if::cfg_if! { + // We need no locks when a TIMG has a single timer, and we don't need locks for ESP32 + // and S2 where the effective interrupt enable register (config) is not shared between + // the timers. + if #[cfg(all(timg_timer1, not(any(esp32, esp32s2))))] { + use crate::sync::{lock, RawMutex}; + static INT_ENA_LOCK: [RawMutex; NUM_TIMG] = [const { RawMutex::new() }; NUM_TIMG]; + } +} /// A timer group consisting of #[cfg_attr(not(timg_timer1), doc = "a general purpose timer")] @@ -300,18 +310,7 @@ impl super::Timer for Timer { } fn enable_interrupt(&self, state: bool) { - // always use level interrupt - #[cfg(any(esp32, esp32s2))] - self.register_block() - .t(self.timer_number().into()) - .config() - .modify(|_, w| w.level_int_en().set_bit()); - - lock(&INT_ENA_LOCK[self.timer_group() as usize], || { - self.register_block() - .int_ena() - .modify(|_, w| w.t(self.timer_number()).bit(state)); - }); + self.set_interrupt_enabled(state); } fn clear_interrupt(&self) { @@ -322,10 +321,6 @@ impl super::Timer for Timer { self.is_interrupt_set() } - async fn wait(&self) { - asynch::TimerFuture::new(self).await - } - fn async_interrupt_handler(&self) -> InterruptHandler { match (self.timer_group(), self.timer_number()) { (0, 0) => asynch::timg0_timer0_handler, @@ -355,6 +350,10 @@ impl super::Timer for Timer { fn set_interrupt_handler(&self, handler: InterruptHandler) { self.set_interrupt_handler(handler) } + + fn waker(&self) -> &AtomicWaker { + asynch::waker(self) + } } impl Peripheral for Timer { @@ -532,6 +531,30 @@ impl Timer { .t(self.timer) .bit_is_set() } + + fn set_interrupt_enabled(&self, state: bool) { + cfg_if::cfg_if! { + if #[cfg(any(esp32, esp32s2))] { + // On ESP32 and S2, the `int_ena` register is ineffective - interrupts fire even + // without int_ena enabling them. We use level interrupts so that we have a status + // bit available. + self.register_block() + .t(self.timer as usize) + .config() + .modify(|_, w| w.level_int_en().bit(state)); + } else if #[cfg(timg_timer1)] { + lock(&INT_ENA_LOCK[self.timer_group() as usize], || { + self.register_block() + .int_ena() + .modify(|_, w| w.t(self.timer_number()).bit(state)); + }); + } else { + self.register_block() + .int_ena() + .modify(|_, w| w.t(0).bit(state)); + } + } + } } fn ticks_to_timeout(ticks: u64, clock: Rate, divider: u32) -> u64 { @@ -783,70 +806,27 @@ where // Async functionality of the timer groups. mod asynch { - use core::{ - pin::Pin, - task::{Context, Poll}, - }; - use procmacros::handler; use super::*; use crate::asynch::AtomicWaker; - cfg_if::cfg_if! { - if #[cfg(all(timg1, timg_timer1))] { - const NUM_WAKERS: usize = 4; - } else if #[cfg(timg1)] { - const NUM_WAKERS: usize = 2; - } else { - const NUM_WAKERS: usize = 1; - } - } + const NUM_WAKERS: usize = { + let timer_per_group = 1 + cfg!(timg_timer1) as usize; + NUM_TIMG * timer_per_group + }; static WAKERS: [AtomicWaker; NUM_WAKERS] = [const { AtomicWaker::new() }; NUM_WAKERS]; - pub(crate) struct TimerFuture<'a> { - timer: &'a Timer, + pub(super) fn waker(timer: &Timer) -> &AtomicWaker { + let index = (timer.timer_number() << 1) | timer.timer_group(); + &WAKERS[index as usize] } - impl<'a> TimerFuture<'a> { - pub(crate) fn new(timer: &'a Timer) -> Self { - use crate::timer::Timer; - - timer.enable_interrupt(true); - - Self { timer } - } - - fn event_bit_is_clear(&self) -> bool { - self.timer - .register_block() - .int_ena() - .read() - .t(self.timer.timer_number()) - .bit_is_clear() - } - } - - impl core::future::Future for TimerFuture<'_> { - type Output = (); - - fn poll(self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll { - let index = (self.timer.timer_number() << 1) | self.timer.timer_group(); - WAKERS[index as usize].register(ctx.waker()); - - if self.event_bit_is_clear() { - Poll::Ready(()) - } else { - Poll::Pending - } - } - } - - impl Drop for TimerFuture<'_> { - fn drop(&mut self) { - self.timer.clear_interrupt(); - } + #[inline] + fn handle_irq(timer: Timer) { + timer.set_interrupt_enabled(false); + waker(&timer).wake(); } // INT_ENA means that when the interrupt occurs, it will show up in the @@ -855,63 +835,41 @@ mod asynch { // clear the INT_CLR as well. #[handler] pub(crate) fn timg0_timer0_handler() { - lock(&INT_ENA_LOCK[0], || { - crate::peripherals::TIMG0::regs() - .int_ena() - .modify(|_, w| w.t(0).clear_bit()) + handle_irq(Timer { + register_block: TIMG0::regs(), + timer: 0, + tg: 0, }); - - crate::peripherals::TIMG0::regs() - .int_clr() - .write(|w| w.t(0).clear_bit_by_one()); - - WAKERS[0].wake(); } #[cfg(timg1)] #[handler] pub(crate) fn timg1_timer0_handler() { - lock(&INT_ENA_LOCK[1], || { - crate::peripherals::TIMG1::regs() - .int_ena() - .modify(|_, w| w.t(0).clear_bit()) + handle_irq(Timer { + register_block: TIMG1::regs(), + timer: 0, + tg: 1, }); - crate::peripherals::TIMG1::regs() - .int_clr() - .write(|w| w.t(0).clear_bit_by_one()); - - WAKERS[1].wake(); } #[cfg(timg_timer1)] #[handler] pub(crate) fn timg0_timer1_handler() { - lock(&INT_ENA_LOCK[0], || { - crate::peripherals::TIMG0::regs() - .int_ena() - .modify(|_, w| w.t(1).clear_bit()) + handle_irq(Timer { + register_block: TIMG0::regs(), + timer: 1, + tg: 0, }); - crate::peripherals::TIMG0::regs() - .int_clr() - .write(|w| w.t(1).clear_bit_by_one()); - - WAKERS[2].wake(); } #[cfg(all(timg1, timg_timer1))] #[handler] pub(crate) fn timg1_timer1_handler() { - lock(&INT_ENA_LOCK[1], || { - crate::peripherals::TIMG1::regs() - .int_ena() - .modify(|_, w| w.t(1).clear_bit()) + handle_irq(Timer { + register_block: TIMG1::regs(), + timer: 1, + tg: 1, }); - - crate::peripherals::TIMG1::regs() - .int_clr() - .write(|w| w.t(1).clear_bit_by_one()); - - WAKERS[3].wake(); } } diff --git a/hil-test/tests/delay_async.rs b/hil-test/tests/delay_async.rs index cb21e1bc8a..56c19ddcb7 100644 --- a/hil-test/tests/delay_async.rs +++ b/hil-test/tests/delay_async.rs @@ -2,6 +2,9 @@ //! //! Specifically tests the various implementations of the //! `embedded_hal_async::delay::DelayNs` trait. +//! +//! This test does not configure embassy, as it doesn't need a timer queue +//! implementation or an embassy time driver. //% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3 //% FEATURES: unstable @@ -86,16 +89,16 @@ mod tests { async fn test_systimer_async_delay_ns(ctx: Context) { let alarms = SystemTimer::new(ctx.peripherals.SYSTIMER); - test_async_delay_ns(OneShotTimer::new(alarms.alarm0).into_async(), 10_000_000).await; + test_async_delay_ns(OneShotTimer::new(alarms.alarm0).into_async(), 10_000).await; } #[test] async fn test_timg0_async_delay_ns(ctx: Context) { let timg0 = TimerGroup::new(ctx.peripherals.TIMG0); - test_async_delay_ns(OneShotTimer::new(timg0.timer0).into_async(), 10_000_000).await; + test_async_delay_ns(OneShotTimer::new(timg0.timer0).into_async(), 10_000).await; #[cfg(timg_timer1)] - test_async_delay_ns(OneShotTimer::new(timg0.timer1).into_async(), 10_000_000).await; + test_async_delay_ns(OneShotTimer::new(timg0.timer1).into_async(), 10_000).await; } #[cfg(timg1)] @@ -103,9 +106,9 @@ mod tests { async fn test_timg1_async_delay_ns(ctx: Context) { let timg1 = TimerGroup::new(ctx.peripherals.TIMG1); - test_async_delay_ns(OneShotTimer::new(timg1.timer0).into_async(), 10_000_000).await; + test_async_delay_ns(OneShotTimer::new(timg1.timer0).into_async(), 10_000).await; #[cfg(timg_timer1)] - test_async_delay_ns(OneShotTimer::new(timg1.timer1).into_async(), 10_000_000).await; + test_async_delay_ns(OneShotTimer::new(timg1.timer1).into_async(), 10_000).await; } #[cfg(systimer)] @@ -113,16 +116,16 @@ mod tests { async fn test_systimer_async_delay_us(ctx: Context) { let alarms = SystemTimer::new(ctx.peripherals.SYSTIMER); - test_async_delay_us(OneShotTimer::new(alarms.alarm0).into_async(), 10_000).await; + test_async_delay_us(OneShotTimer::new(alarms.alarm0).into_async(), 10).await; } #[test] async fn test_timg0_async_delay_us(ctx: Context) { let timg0 = TimerGroup::new(ctx.peripherals.TIMG0); - test_async_delay_us(OneShotTimer::new(timg0.timer0).into_async(), 10_000).await; + test_async_delay_us(OneShotTimer::new(timg0.timer0).into_async(), 10).await; #[cfg(timg_timer1)] - test_async_delay_us(OneShotTimer::new(timg0.timer1).into_async(), 10_000).await; + test_async_delay_us(OneShotTimer::new(timg0.timer1).into_async(), 10).await; } #[cfg(timg1)] @@ -130,9 +133,9 @@ mod tests { async fn test_timg1_async_delay_us(ctx: Context) { let timg1 = TimerGroup::new(ctx.peripherals.TIMG1); - test_async_delay_us(OneShotTimer::new(timg1.timer0).into_async(), 10_000).await; + test_async_delay_us(OneShotTimer::new(timg1.timer0).into_async(), 10).await; #[cfg(timg_timer1)] - test_async_delay_us(OneShotTimer::new(timg1.timer1).into_async(), 10_000).await; + test_async_delay_us(OneShotTimer::new(timg1.timer1).into_async(), 10).await; } #[cfg(systimer)] @@ -140,16 +143,16 @@ mod tests { async fn test_systimer_async_delay_ms(ctx: Context) { let alarms = SystemTimer::new(ctx.peripherals.SYSTIMER); - test_async_delay_ms(OneShotTimer::new(alarms.alarm0).into_async(), 10).await; + test_async_delay_ms(OneShotTimer::new(alarms.alarm0).into_async(), 1).await; } #[test] async fn test_timg0_async_delay_ms(ctx: Context) { let timg0 = TimerGroup::new(ctx.peripherals.TIMG0); - test_async_delay_ms(OneShotTimer::new(timg0.timer0).into_async(), 10).await; + test_async_delay_ms(OneShotTimer::new(timg0.timer0).into_async(), 1).await; #[cfg(timg_timer1)] - test_async_delay_ms(OneShotTimer::new(timg0.timer1).into_async(), 10).await; + test_async_delay_ms(OneShotTimer::new(timg0.timer1).into_async(), 1).await; } #[cfg(timg1)] @@ -157,8 +160,8 @@ mod tests { async fn test_timg1_async_delay_ms(ctx: Context) { let timg1 = TimerGroup::new(ctx.peripherals.TIMG1); - test_async_delay_ms(OneShotTimer::new(timg1.timer0).into_async(), 10).await; + test_async_delay_ms(OneShotTimer::new(timg1.timer0).into_async(), 1).await; #[cfg(timg_timer1)] - test_async_delay_ms(OneShotTimer::new(timg1.timer1).into_async(), 10).await; + test_async_delay_ms(OneShotTimer::new(timg1.timer1).into_async(), 1).await; } }