Fix waiting for short periods (#3093)

* Shorten test delays

* Fix TIMG

* Fix systimer

* Changelog

* Use a single future implementation

* Explain level interrupts

esp-hal/CHANGELOG.md

@@ -45,6 +45,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)
 
 - ESP32-S2: Fixed linker script (#3096)
 

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<AnyTimer> + '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<AnyTimer> + '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<P = AnyTimer> + '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<Self::Output> {
+        // 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<Dm> 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;
         }
     }
 }

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<Self::Output> {
-            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);
     }
 }