Remove Async/Blocking and Periodic/Target modes from systimer and timg #2542

MabezDev · 2024-11-14T15:11:47Z

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs API guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

This PR removes much of the logic out of the lower level timers, and instead moves the logic to the OneShot and Periodic timer drivers.

cc: #2321

I will do a follow up PR which adds the mode to the timer drivers, and implement into_async and the eh traits there, as this PR got surprising large surprisingly quick.

…Cull examples

MabezDev · 2024-11-14T15:12:10Z

esp-hal/Cargo.toml

@@ -22,7 +22,7 @@ cfg-if                   = "1.0.0"
 chrono                   = { version = "0.4.38", default-features = false }
 critical-section         = "1.1.3"
 defmt                    = { version = "0.3.8", optional = true }
-delegate                 = "0.12.0"
+delegate                 = "0.13.0"


Add's support for delegating async fns

MabezDev · 2024-11-14T15:12:59Z

esp-hal/src/timer/mod.rs

+    ///
+    /// Requires the correct `InterruptHandler` to be installed to function
+    /// correctly.
+    async fn wait(&self);


I tried an API where it just returned a future, but it was noisy and I had trouble getting delegate to play ball with the types.

I think this should be fine, but let me know.

MabezDev · 2024-11-14T15:14:04Z

esp-hal/src/timer/mod.rs

@@ -105,22 +104,40 @@ pub trait Timer: 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


I deliberately decoupled the async fn from installing the correct interrupt handler, as this is a low level trait and if a user is reaching for it they probably want to register there own handler which could also call the async one once they're done doing what they need to do with it.

bugadani · 2024-11-14T15:14:09Z

esp-hal/src/timer/mod.rs

-        Self(AnyTimerInner::SystimerAlarmTarget(value))
+crate::any_peripheral! {
+    /// Any Timer peripheral.
+    pub peripheral AnyTimer {


huh this works for timers, too? 🤣

MabezDev · 2024-11-14T15:14:29Z

esp-hal/src/timer/mod.rs

-impl From<systimer::Alarm<'static, systimer::Target, Blocking>> for AnyTimer {
-    fn from(value: systimer::Alarm<'static, systimer::Target, Blocking>) -> Self {
-        Self(AnyTimerInner::SystimerAlarmTarget(value))
+crate::any_peripheral! {


This macro is very handy :D

MabezDev · 2024-11-14T15:56:18Z

hil-test/tests/delay_async.rs

@@ -1,173 +0,0 @@
-//! Async Delay Test


I will re-add this test in the next PR, for now the functionality is removed.

MabezDev · 2024-11-14T16:03:00Z

hil-test/tests/embassy_timers_executors.rs

-        let mut alarm: Alarm<'_, Target, _, _, _> = Alarm::new(systimer.comparator0, &unit);
-        run_test_oneshot_timer(&mut alarm);
-    }
+    // FIXME (mabez)


This needs some work to get going again, will do it in the follow up PR

bugadani · 2024-11-14T19:54:11Z

esp-hal-embassy/src/lib.rs

-    Self: Into<AnyTimer>,
-{
-}
+impl<T: esp_hal::timer::timg::Instance> IntoAnyTimer for TimgTimer<T> where Self: Into<AnyTimer> {}


In the other peripherals, Instance implies Into<AnyInstance> (+ 'static). I think we can do the same to timers as well. That may also mean IntoAnyTimer is meaningless, though I don't quite remember why I needed it in the first place - probably blanket impl'ing TimerCollection runs into some coherence rules.

bugadani · 2024-11-14T19:59:49Z

esp-hal/src/timer/systimer.rs

-            }
-
-            alarm.set_interrupt_handler(handler);
+            alarm.set_interrupt_handler(match alarm.comparator.channel() {


This is unfortunately problematic because we can overwrite the user's interrupt handler without notice. If we offer an async API OR configurable interrupt handlers, we should have the Async/Blocking distinction.

If not, we should at least make sure we don't expose the functionality to users, and only them properly in the public API. I think this (adding a private::internal to the async fn in the trait), along with the Mode on Periodic/OneShot should suffice.

Dominaezzz · 2024-11-14T23:06:41Z

esp-hal/src/timer/systimer.rs

@@ -7,74 +7,14 @@
 //! timer which can be used, for example, to generate tick interrupts for an
 //! operating system, or simply as a general-purpose timer.
 //!
-//! ## Configuration


Did you accidentally delete too much? Some of it still looks relevant.

Dominaezzz · 2024-11-14T23:32:40Z

esp-hal/src/timer/systimer.rs

-    pub unit0: SpecificUnit<'d, 0>,
+    pub unit0: AnyUnit<'d>,


Any reason not to keep the specific ones here and accept impl Into<AnyUnit> in Alarm::new(....)?

Dominaezzz · 2024-11-14T23:33:43Z

esp-hal/src/timer/systimer.rs

+        asynch::AlarmFuture::new(self).await
+    }
+
+    fn async_interrupt_handler(&self) -> InterruptHandler {


(Long term) Should the interrupt priority be decided down here?

MabezDev added 4 commits November 14, 2024 12:00

Remove Async/Blocking and Periodic/Target modes from systimer and timg

8849ed6

Remove generics from Alarm

d156826

Default to AnyTimer, added new_typed to get typed timer drivers. …

7eb50ed

…Cull examples

Update all examples

9fe6a53

MabezDev commented Nov 14, 2024

View reviewed changes

bugadani reviewed Nov 14, 2024

View reviewed changes

MabezDev commented Nov 14, 2024

View reviewed changes

make CI green

40c5fe9

MabezDev commented Nov 14, 2024

View reviewed changes

bugadani reviewed Nov 14, 2024

View reviewed changes

Dominaezzz reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Async/Blocking and Periodic/Target modes from systimer and timg #2542

Remove Async/Blocking and Periodic/Target modes from systimer and timg #2542

MabezDev commented Nov 14, 2024

MabezDev Nov 14, 2024

MabezDev Nov 14, 2024 •

edited

Loading

MabezDev Nov 14, 2024

bugadani Nov 14, 2024

MabezDev Nov 14, 2024

MabezDev Nov 14, 2024

MabezDev Nov 14, 2024

bugadani Nov 14, 2024

bugadani Nov 14, 2024 •

edited

Loading

Dominaezzz Nov 14, 2024

Dominaezzz Nov 14, 2024

Dominaezzz Nov 14, 2024

Remove Async/Blocking and Periodic/Target modes from systimer and timg #2542

Are you sure you want to change the base?

Remove Async/Blocking and Periodic/Target modes from systimer and timg #2542

Conversation

MabezDev commented Nov 14, 2024

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Choose a reason for hiding this comment

MabezDev Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bugadani Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MabezDev Nov 14, 2024 •

edited

Loading

bugadani Nov 14, 2024 •

edited

Loading