diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index ac319cef4a..fe5c3ee224 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 where asynchronous Timer waits on the ESP32 and S2 may have resulted in the futures never resolving. (#?) ### Removed diff --git a/esp-hal/src/timer/mod.rs b/esp-hal/src/timer/mod.rs index 12e7dcda49..ddb1e36118 100644 --- a/esp-hal/src/timer/mod.rs +++ b/esp-hal/src/timer/mod.rs @@ -188,8 +188,12 @@ impl OneShotTimer<'_, Async> { } async fn delay_async(&mut self, us: Duration) { + self.stop(); + + self.inner.enable_interrupt(true); unwrap!(self.schedule(us)); self.inner.wait().await; + self.stop(); self.clear_interrupt(); } @@ -227,9 +231,7 @@ where /// Start counting until the given timeout and raise an interrupt pub fn schedule(&mut self, timeout: Duration) -> Result<(), Error> { - if self.inner.is_running() { - self.inner.stop(); - } + self.inner.stop(); self.inner.clear_interrupt(); self.inner.reset(); @@ -311,9 +313,7 @@ where { /// Start a new count down. pub fn start(&mut self, period: Duration) -> Result<(), Error> { - if self.inner.is_running() { - self.inner.stop(); - } + self.inner.stop(); self.inner.clear_interrupt(); self.inner.reset(); diff --git a/esp-hal/src/timer/systimer.rs b/esp-hal/src/timer/systimer.rs index 11d9b775ba..4051b7a692 100644 --- a/esp-hal/src/timer/systimer.rs +++ b/esp-hal/src/timer/systimer.rs @@ -620,7 +620,8 @@ mod asynch { const NUM_ALARMS: usize = 3; - static WAKERS: [AtomicWaker; NUM_ALARMS] = [const { AtomicWaker::new() }; NUM_ALARMS]; + pub(super) 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> { @@ -629,18 +630,8 @@ mod asynch { impl<'a> AlarmFuture<'a> { pub(crate) fn new(alarm: &'a Alarm) -> Self { - alarm.enable_interrupt(true); - Self { alarm } } - - 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<'_> { @@ -649,7 +640,7 @@ mod asynch { 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() { + if self.alarm.is_interrupt_set() { Poll::Ready(()) } else { Poll::Pending @@ -657,6 +648,13 @@ mod asynch { } } + impl Drop for AlarmFuture<'_> { + fn drop(&mut self) { + self.alarm.enable_interrupt(false); + self.alarm.clear_interrupt(); + } + } + #[handler] pub(crate) fn target0_handler() { lock(&INT_ENA_LOCK, || { diff --git a/esp-hal/src/timer/timg.rs b/esp-hal/src/timer/timg.rs index edfe8b4100..3f92dc42a2 100644 --- a/esp-hal/src/timer/timg.rs +++ b/esp-hal/src/timer/timg.rs @@ -69,6 +69,8 @@ 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::{ @@ -178,7 +180,7 @@ impl TimerGroupInstance for TIMG0 { } #[cfg(timg1)] -impl TimerGroupInstance for crate::peripherals::TIMG1 { +impl TimerGroupInstance for TIMG1 { fn id() -> u8 { 1 } @@ -245,6 +247,21 @@ where T::configure_src_clk(); + // always use level interrupt + #[cfg(any(esp32, esp32s2))] + { + let regs = unsafe { &*T::register_block() }; + regs.t(0).config().modify(|_, w| { + w.edge_int_en().clear_bit(); + w.level_int_en().set_bit() + }); + #[cfg(timg_timer1)] + regs.t(1).config().modify(|_, w| { + w.edge_int_en().clear_bit(); + w.level_int_en().set_bit() + }); + } + Self { _timer_group: PhantomData, timer0: Timer { @@ -300,13 +317,6 @@ 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() @@ -791,7 +801,7 @@ mod asynch { use procmacros::handler; use super::*; - use crate::asynch::AtomicWaker; + use crate::{asynch::AtomicWaker, timer::Timer as _}; cfg_if::cfg_if! { if #[cfg(all(timg1, timg_timer1))] { @@ -803,22 +813,25 @@ mod asynch { } } - static WAKERS: [AtomicWaker; NUM_WAKERS] = [const { AtomicWaker::new() }; NUM_WAKERS]; + pub(super) static WAKERS: [AtomicWaker; NUM_WAKERS] = + [const { AtomicWaker::new() }; NUM_WAKERS]; + // Note that this future relies on the interrupt being already enabled. This is + // because on ESP32 and S2 it seems a clear interrupt enable doesn't + // actually prevent the interrupt from firing. Since we use the interrupt + // enable bit to signal completion, we need it to be enabled before starting + // the timer. + #[must_use = "futures do nothing unless you `.await` or poll them"] pub(crate) struct TimerFuture<'a> { timer: &'a Timer, } 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 { + fn is_done(&self) -> bool { self.timer .register_block() .int_ena() @@ -833,9 +846,9 @@ mod asynch { 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()); + asynch::WAKERS[index as usize].register(ctx.waker()); - if self.event_bit_is_clear() { + if self.is_done() { Poll::Ready(()) } else { Poll::Pending @@ -845,26 +858,18 @@ mod asynch { impl Drop for TimerFuture<'_> { fn drop(&mut self) { + self.timer.enable_interrupt(false); self.timer.clear_interrupt(); } } - // INT_ENA means that when the interrupt occurs, it will show up in the - // INT_ST. Clearing INT_ENA that it won't show up on INT_ST but if - // interrupt is already there, it won't clear it - that's why we need to - // 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()) + TIMG0::regs().int_ena().modify(|_, w| w.t(0).clear_bit()); + TIMG0::regs().int_clr().write(|w| w.t(0).clear_bit_by_one()); }); - crate::peripherals::TIMG0::regs() - .int_clr() - .write(|w| w.t(0).clear_bit_by_one()); - WAKERS[0].wake(); } @@ -872,13 +877,9 @@ mod asynch { #[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()) + TIMG1::regs().int_ena().modify(|_, w| w.t(0).clear_bit()); + TIMG1::regs().int_clr().write(|w| w.t(0).clear_bit_by_one()); }); - crate::peripherals::TIMG1::regs() - .int_clr() - .write(|w| w.t(0).clear_bit_by_one()); WAKERS[1].wake(); } @@ -887,13 +888,9 @@ mod asynch { #[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()) + TIMG0::regs().int_ena().modify(|_, w| w.t(1).clear_bit()); + TIMG0::regs().int_clr().write(|w| w.t(1).clear_bit_by_one()); }); - crate::peripherals::TIMG0::regs() - .int_clr() - .write(|w| w.t(1).clear_bit_by_one()); WAKERS[2].wake(); } @@ -902,15 +899,10 @@ mod asynch { #[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()) + TIMG1::regs().int_ena().modify(|_, w| w.t(1).clear_bit()); + TIMG1::regs().int_clr().write(|w| w.t(1).clear_bit_by_one()); }); - 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; } }