Shorten wait times in delay_async test

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
 

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();

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,14 +640,21 @@ mod asynch {
         fn poll(self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll<Self::Output> {
             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
             }
         }
     }
 
+    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, || {

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<Self::Output> {
             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,40 +858,28 @@ 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();
     }
 
     #[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())
+            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();
     }
 }

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,79 +89,79 @@ 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)]
     #[test]
     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)]
     #[test]
     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)]
     #[test]
     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)]
     #[test]
     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)]
     #[test]
     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;
     }
 }