Implement `embedded_hal_async::delay::DelayNs` for `TIMGx` timers #2084

jessebraham · 2024-09-04T18:25:21Z

Both SYSTIMER alarms and TIMGx timers now implement this trait, and I've added some simple tests to cover the implementations.

I guess this closes #1864 because I am not sure there is a non-hacky and portable way to implement this trait for our Delay struct.

hil-test/tests/delay_async.rs

jessebraham · 2024-09-04T18:40:54Z

The HIL failure for the S2 is a bit perplexing. I will have to find my programmer and try to run that locally.

bjoernQ · 2024-09-05T07:25:15Z

hil-test/tests/delay_async.rs

+    async fn test_systimer_async_delay_ns(ctx: Context) {
+        let mut alarms = SystemTimer::new(ctx.peripherals.SYSTIMER);
+        let unit = FrozenUnit::new(&mut alarms.unit0);
+        let alarm0 = Alarm::new_async(alarms.comparator0, &unit).into_periodic();


not related to this PR but isn't it a bit surprising that Delay is implemented for Periodic? - I would have expected it to be available for Target

jessebraham · 2024-09-06T15:48:22Z

I've rebased this and made the necessary updates. Interestingly the ESP32 is failing in addition to the ESP32-S2, so maybe that will help me hunt down this issue.

esp-hal/src/timer/timg.rs

Dominaezzz · 2024-09-06T22:40:47Z

esp-hal/src/timer/timg.rs

+        pub(crate) fn new(timer: &'a Timer<T, crate::Async>) -> Self {
+            use crate::timer::Timer as _;
+
+            timer.clear_interrupt();


This is racey. You could end up clearing the interrupt bit that just fired for this delay causing this to hang forever.

This should use the semantics introduced in #1835

jessebraham · 2024-09-09T14:28:45Z

I'm moving soon and accidentally packed my programmer already 😅 So @JurajSadel has offered to wrap this up (thanks!)

bugadani · 2024-09-10T19:49:24Z

Do we ever want to support type-erased timers that provide async delays? If so, I'm not sure this direction is the way forward - I'd probably not invest time in trying to provide async mode at all for the lower level timers (both Systimer and Timg).

At this point, our API supports creating OneShotTimer out of periodic systimers, and PeriodicTimer out of Target systimers (through ErasedTimer). Without this PR, Async mode in TimerGroup has absolutely no use and systimer is just a complex heap of... something. What's the design behind all this and where are we headed?

bjoernQ · 2024-09-11T06:40:45Z

Do we ever want to support type-erased timers that provide async delays? If so, I'm not sure this direction is the way forward - I'd probably not invest time in trying to provide async mode at all for the lower level timers (both Systimer and Timg).

That is something I was wondering about, too. Should we care about implementing foreign traits on the low-level timers at all?
(My gut feeling is no but maybe there are good reasons to do so)

bugadani · 2024-09-11T07:11:30Z

I'm leaning towards no, the low-level timers should be, where possible, only backends for higher level abstractions. Ideally we shouldn't provide multiple, drastically different ways to do the same thing, and where possible I'd love to group timer functionality behind PeriodicTimer/OneShotTimer. Doing so allows us to expose a single API regardless of the backend. A single API reduces the barrier of entry for new users as they don't have to consider details. And we have lots of details :)

I'd like to see an ergonimics-first approach to our APIs, with optional complexity if the user needs it. We can provide the first one by prioritising AnyTimer and PeriodicTimer/OneShotTimer. The latter is more tricky with timers, but not impossible if our timers have functionality that can't be abstracted over.

bjoernQ · 2024-09-12T11:02:54Z

I think we agreed we don't want to have the traits implemented on low-level timers but on the abstractions - nevertheless the failing tests are suspicious

I had a look and apparently on ESP32 and ESP32-S2 disabling INT_ENA doesn't de-assert the pending interrupt - i.e. after returning from the handler, it gets immediately called again

You can verify this by changing the timer_interrupt example accordingly

That's easy to fix by clearing the interrupt pending flag via INT_CLR

The tests then pass for me locally

Patch

diff --git a/esp-hal/src/timer/timg.rs b/esp-hal/src/timer/timg.rs
index 9d1f8157..77083926 100644
--- a/esp-hal/src/timer/timg.rs
+++ b/esp-hal/src/timer/timg.rs
@@ -1180,6 +1180,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(0).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG0::PTR }
+            .int_clr()
+            .write(|w| w.t(0).clear_bit_by_one());
+
         WAKERS[0].wake();
     }
 
@@ -1190,6 +1194,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(0).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG1::PTR }
+            .int_clr()
+            .write(|w| w.t(0).clear_bit_by_one());
+
         WAKERS[1].wake();
     }
 
@@ -1200,6 +1208,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(1).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG0::PTR }
+            .int_clr()
+            .write(|w| w.t(1).clear_bit_by_one());
+
         WAKERS[2].wake();
     }
 
@@ -1210,6 +1222,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(1).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG1::PTR }
+            .int_clr()
+            .write(|w| w.t(1).clear_bit_by_one());
+
         WAKERS[3].wake();
     }
 }

Dominaezzz · 2024-09-12T11:36:21Z

I had a look and apparently on ESP32 and ESP32-S2 disabling INT_ENA doesn't de-assert the pending interrupt - i.e. after returning from the handler, it gets immediately called again

That's really strange.
Perhaps it's due to

esp-hal/esp-hal/src/timer/timg.rs

Lines 483 to 488 in 562c891

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

?

Clearing the interrupt bit without setting some kind of flag is a little dangerous, in terms of race conditions.

JurajSadel · 2024-09-12T13:12:15Z

I think we agreed we don't want to have the traits implemented on low-level timers but on the abstractions - nevertheless the failing tests are suspicious

I had a look and apparently on ESP32 and ESP32-S2 disabling INT_ENA doesn't de-assert the pending interrupt - i.e. after returning from the handler, it gets immediately called again

You can verify this by changing the timer_interrupt example accordingly

That's easy to fix by clearing the interrupt pending flag via INT_CLR

The tests then pass for me locally

Patch

diff --git a/esp-hal/src/timer/timg.rs b/esp-hal/src/timer/timg.rs
index 9d1f8157..77083926 100644
--- a/esp-hal/src/timer/timg.rs
+++ b/esp-hal/src/timer/timg.rs
@@ -1180,6 +1180,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(0).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG0::PTR }
+            .int_clr()
+            .write(|w| w.t(0).clear_bit_by_one());
+
         WAKERS[0].wake();
     }
 
@@ -1190,6 +1194,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(0).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG1::PTR }
+            .int_clr()
+            .write(|w| w.t(0).clear_bit_by_one());
+
         WAKERS[1].wake();
     }
 
@@ -1200,6 +1208,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(1).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG0::PTR }
+            .int_clr()
+            .write(|w| w.t(1).clear_bit_by_one());
+
         WAKERS[2].wake();
     }
 
@@ -1210,6 +1222,10 @@ mod asynch {
             .int_ena()
             .modify(|_, w| w.t(1).clear_bit());
 
+        unsafe { &*crate::peripherals::TIMG1::PTR }
+            .int_clr()
+            .write(|w| w.t(1).clear_bit_by_one());
+
         WAKERS[3].wake();
     }
 }

When I looked into this issue on Tuesday, tests were timing out with this exact change for me 🤔

…ay with `SYSTIMER`

…s, move interrupt binds from Future constructor into new_async constructor

esp-hal/src/timer/timg.rs

hil-test/tests/delay_async.rs

…delay_us and delay_ms

Dominaezzz · 2024-09-18T12:25:10Z

esp-hal/src/timer/timg.rs

+        }
+    }
+
+    impl<'a, T> core::future::Future for TimerFuture<'a, T>


When this future is dropped (early) it should disable the interrupt.

Dominaezzz · 2024-09-18T12:28:05Z

esp-hal/src/timer/timg.rs

+                .modify(|_, w| w.t(0).clear_bit())
+        });
+
+        unsafe { &*crate::peripherals::TIMG0::PTR }


Would you mind adding a comment here explaining why this INT_CLR is necessary even though the INT_ENA has been cleared above.

Dominaezzz · 2024-09-18T12:29:06Z

esp-hal/src/timer/timg.rs

+        type Output = ();
+
+        fn poll(self: Pin<&mut Self>, ctx: &mut Context<'_>) -> Poll<Self::Output> {
+            let index = (self.timer.timer_number() << 1) | self.timer.timer_group();


Mind making a (const) function for this math? Then it can be used in the interrupt handlers as well, just so we know the indexing is consistent/correct.

Dominaezzz · 2024-09-18T12:30:58Z

esp-hal/src/timer/timg.rs

+            self.start();
+            self.listen();


Probably not super important for this PR but if the user cancels the future returned by this delay function, the call to start and listen should be undone. I'm guessing there's a stop and unlisten somewhere.

Dominaezzz · 2024-09-18T12:32:18Z

hil-test/tests/delay_async.rs

+    peripherals: Peripherals,
+}
+
+async fn test_async_delay_ns(mut timer: impl DelayNs, duration: u32) {


Mind adding a loop in here to ensure repeated calls to delay_ns work as expected.
It happens quite often in drivers that the first one works but the next ones don't.

jessebraham commented Sep 4, 2024

View reviewed changes

hil-test/tests/delay_async.rs Outdated Show resolved Hide resolved

bjoernQ reviewed Sep 5, 2024

View reviewed changes

jessebraham force-pushed the feature/async-timers branch from f3b203a to 1f8fdad Compare September 6, 2024 15:13

Dominaezzz reviewed Sep 6, 2024

View reviewed changes

JurajSadel self-assigned this Sep 9, 2024

JurajSadel force-pushed the feature/async-timers branch from ab22e3f to 816c47d Compare September 17, 2024 14:11

jessebraham and others added 8 commits September 17, 2024 07:48

Update hil-test package dependencies, add simple test for async del…

f615e97

…ay with `SYSTIMER`

Implement embedded_hal_async::delay::DelayNs for the TIMGx timers

b188b57

Improve tests slightly

bc1065e

Update CHANGELOG.md

206367f

Enable delay and delay_async tests for the ESP32-H2

715b61b

Fix error in delay_async test after rebasing

7d7d630

ESP32 does not have SYSTIMER, so don't try to test it :)

2de7745

Protect int_ena modifications with INT_ENA_LOCK, clear int_clr in ISR…

de9c2ac

…s, move interrupt binds from Future constructor into new_async constructor

jessebraham force-pushed the feature/async-timers branch from 816c47d to de9c2ac Compare September 17, 2024 14:49

Fix wrong imports

68a9d40

bjoernQ reviewed Sep 18, 2024

View reviewed changes

esp-hal/src/timer/timg.rs Outdated Show resolved Hide resolved