Update our time driver for upcoming embassy changes (#2701)

* Add a timer-driven task * Spawn another timer * Log * foo * Do not access current time on each schedule * Update generic queue * Minimize alarm priorities * Point to github with patches * Fix build without any queue impl selected * Remove explicit generic-queue features * Define cfgs, fix calling something uninitialized * Clean up RefCell+generic queue * Fix arg order * Feature * Fix single integrated-timer queue * Fix next expiration when arming * Add note * Adjust impl to latest changes * Local patch * Refactor the refactor refactor * Track the timer item's owner * Clear owner on dequeue * Clean up * Point at the right branch * Fix panic message * Hide private function * Remove integrated-timer references * Point at upstream embassy * Configure via esp-config * Document, clean up, fix * Hack * Remove patches * Update config separator, test the complex variant * Undo esp-config hack * Remove trouble example, update edge-net * Update test deps * Document * Update bt-hci. * Fix generic queue * Fix panic message * Fix UB * Fix rebase * Resolve UB * Avoid mutable reference in interrupt executor --------- Co-authored-by: Dario Nieuwenhuis <[email protected]>
esp-rs · Jan 14, 2025 · 5717608 · 5717608
1 parent 5e05402
commit 5717608
Show file tree

Hide file tree

Showing 54 changed files with 664 additions and 352 deletions.
diff --git a/esp-hal-embassy/CHANGELOG.md b/esp-hal-embassy/CHANGELOG.md
@@ -9,8 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- Added `ESP_HAL_EMBASSY_CONFIG_TIMER_QUEUE` (#2701)
+- Added `ESP_HAL_EMBASSY_CONFIG_GENERIC_QUEUE_SIZE` instead of using `embassy-time/generic-queue-*` (#2701)
+
 ### Changed
 
+- Bump MSRV to 1.83 (#2615)
+- Updated embassy-time to v0.4 (#2701)
 - Config: Crate prefixes and configuration keys are now separated by `_CONFIG_` (#2848)
 - Bump MSRV to 1.84 (#2951)
 
@@ -20,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Removed
 
+- The `integrated-timers` option has been replaced by configuration options. (#2701)
+
 ## 0.5.0 - 2024-11-20
 
 ### Added

diff --git a/esp-hal-embassy/Cargo.toml b/esp-hal-embassy/Cargo.toml
@@ -14,16 +14,20 @@ default-target = "riscv32imac-unknown-none-elf"
 features       = ["esp32c6"]
 
 [dependencies]
-critical-section    = "1.2.0"
-defmt               = { version = "0.3.8", optional = true }
-document-features   = "0.2.10"
-embassy-executor    = { version = "0.6.3", optional = true }
-embassy-time-driver = { version = "0.1.0", features = [ "tick-hz-1_000_000" ] }
-esp-hal             = { version = "0.22.0", path = "../esp-hal" }
-log                 = { version = "0.4.22", optional = true }
-macros              = { version = "0.15.0", features = ["embassy"], package = "esp-hal-procmacros", path = "../esp-hal-procmacros" }
-portable-atomic     = "1.9.0"
-static_cell         = "2.1.0"
+critical-section          = "1.2.0"
+defmt                     = { version = "0.3.8", optional = true }
+document-features         = "0.2.10"
+embassy-executor          = { version = "0.7.0", features = ["timer-item-payload-size-4"], optional = true }
+embassy-sync              = { version = "0.6.1" }
+embassy-time              = { version = "0.4.0" }
+embassy-time-driver       = { version = "0.2.0", features = [ "tick-hz-1_000_000" ] }
+embassy-time-queue-utils  = { version = "0.1.0", features = ["_generic-queue"] }
+esp-config                = { version = "0.2.0", path = "../esp-config" }
+esp-hal                   = { version = "0.22.0", path = "../esp-hal" }
+log                       = { version = "0.4.22", optional = true }
+macros                    = { version = "0.15.0", features = ["embassy"], package = "esp-hal-procmacros", path = "../esp-hal-procmacros" }
+portable-atomic           = "1.9.0"
+static_cell               = "2.1.0"
 
 [build-dependencies]
 esp-build    = { version = "0.1.0", path = "../esp-build" }
@@ -47,8 +51,6 @@ defmt = ["dep:defmt", "embassy-executor?/defmt", "esp-hal/defmt"]
 log = ["dep:log"]
 ## Provide `Executor` and `InterruptExecutor`
 executors = ["dep:embassy-executor", "esp-hal/__esp_hal_embassy"]
-## Use the executor-integrated `embassy-time` timer queue.
-integrated-timers = ["embassy-executor?/integrated-timers"]
 
 [lints.rust]
 unexpected_cfgs = "allow"
diff --git a/esp-hal-embassy/MIGRATING-0.5.md b/esp-hal-embassy/MIGRATING-0.5.md
@@ -13,3 +13,24 @@ configurations to match the new format.
 -ESP_HAL_EMBASSY_LOW_POWER_WAIT="false"
 +ESP_HAL_EMBASSY_CONFIG_LOW_POWER_WAIT="false"
 ```
+
+### Removal of `integrated-timers`
+
+The `integrated-timers` feature has been replaced by two new configuration options:
+
+- `ESP_HAL_EMBASSY_CONFIG_TIMER_QUEUE`: selects a timer queue implementation, which allows
+  tuning based on a few requirements. Possible options:
+  - `generic`: a generic queue implementation is used. This option does not depend on
+    embassy-executor, uses a global critical section to access the timer queue, and its
+    capacity determines how many tasks can wait for timers at the same time. You need to pass only a
+    single timer to `esp_hal_embassy::init`.
+  - `single-integrated`: This option requires embassy-executor and the `executors` feature, uses a
+    global critical section to access the timer queue. You need to pass only a single timer to
+    `esp_hal_embassy::init`. This is slightly faster than `generic` and does not need a capacity
+    configuration. This is the default option when the `executors` feature is enabled.
+  - `multiple-integrated`: This option requires embassy-executor and the `executors` feature, uses
+    more granular locks to access the timer queue. You need to pass one timer for each embassy
+    executor to `esp_hal_embassy::init` and the option does not need a capacity configuration.
+    This is the most performant option.
+- `ESP_HAL_EMBASSY_CONFIG_GENERIC_QUEUE_SIZE`: determines the capacity of the timer queue when using
+    the `generic` option. `esp_hal_embassy` ignores the `embassy-time/generic-queue-*` features.
diff --git a/esp-hal-embassy/build.rs b/esp-hal-embassy/build.rs
@@ -1,10 +1,10 @@
-use std::{error::Error, str::FromStr};
+use std::{error::Error as StdError, str::FromStr};
 
 use esp_build::assert_unique_used_features;
-use esp_config::{generate_config, Value};
+use esp_config::{generate_config, Error, Validator, Value};
 use esp_metadata::{Chip, Config};
 
-fn main() -> Result<(), Box<dyn Error>> {
+fn main() -> Result<(), Box<dyn StdError>> {
     // NOTE: update when adding new device support!
     // Ensure that exactly one chip has been specified:
     assert_unique_used_features!(
@@ -39,16 +39,70 @@ fn main() -> Result<(), Box<dyn Error>> {
     config.define_symbols();
 
     // emit config
-    generate_config(
+    let crate_config = generate_config(
         "esp_hal_embassy",
         &[(
             "low-power-wait",
             "Enables the lower-power wait if no tasks are ready to run on the thread-mode executor. This allows the MCU to use less power if the workload allows. Recommended for battery-powered systems. May impact analog performance.",
             Value::Bool(true),
             None
-        )],
+        ),
+        (
+            "timer-queue",
+            "<p>The flavour of the timer queue provided by this crate. Accepts one of `single-integrated`, `multiple-integrated` or `generic`. Integrated queues require the `executors` feature to be enabled.</p><p>If you use embassy-executor, the `single-integrated` queue is recommended for ease of use, while the `multiple-integrated` queue is recommended for performance. The `multiple-integrated` option needs one timer per executor.</p><p>The `generic` queue allows using embassy-time without the embassy executors.</p>",
+            Value::String(if cfg!(feature = "executors") {
+                String::from("single-integrated")
+            } else {
+                String::from("generic")
+            }),
+            Some(Validator::Custom(Box::new(|value| {
+                let Value::String(string) = value else {
+                    return Err(Error::Validation(String::from("Expected a string")));
+                };
+
+                if !cfg!(feature = "executors") {
+                    if string.as_str() != "generic" {
+                        return Err(Error::Validation(format!("Expected 'generic' because the `executors` feature is not enabled. Found {string}")));
+                    }
+                    return Ok(());
+                }
+
+                match string.as_str() {
+                    "single-integrated" => Ok(()), // preferred for ease of use
+                    "multiple-integrated" => Ok(()), // preferred for performance
+                    "generic" => Ok(()), // allows using embassy-time without the embassy executors
+                    _ => Err(Error::Validation(format!("Expected 'single-integrated', 'multiple-integrated' or 'generic', found {string}")))
+                }
+            })))
+        ),
+        (
+            "generic-queue-size",
+            "The capacity of the queue when the `generic` timer queue flavour is selected.",
+            Value::Integer(64),
+            Some(Validator::PositiveInteger),
+        ),
+    ],
         true,
     );
 
+    println!("cargo:rustc-check-cfg=cfg(integrated_timers)");
+    println!("cargo:rustc-check-cfg=cfg(single_queue)");
+    println!("cargo:rustc-check-cfg=cfg(generic_timers)");
+
+    match &crate_config["ESP_HAL_EMBASSY_CONFIG_TIMER_QUEUE"] {
+        Value::String(s) if s.as_str() == "single-integrated" => {
+            println!("cargo:rustc-cfg=integrated_timers");
+            println!("cargo:rustc-cfg=single_queue");
+        }
+        Value::String(s) if s.as_str() == "multiple-integrated" => {
+            println!("cargo:rustc-cfg=integrated_timers");
+        }
+        Value::String(s) if s.as_str() == "generic" => {
+            println!("cargo:rustc-cfg=generic_timers");
+            println!("cargo:rustc-cfg=single_queue");
+        }
+        _ => unreachable!(),
+    }
+
     Ok(())
 }
diff --git a/esp-hal-embassy/src/executor/interrupt.rs b/esp-hal-embassy/src/executor/interrupt.rs
@@ -2,13 +2,15 @@
 
 use core::{cell::UnsafeCell, mem::MaybeUninit};
 
-use embassy_executor::{raw, SendSpawner};
+use embassy_executor::SendSpawner;
 use esp_hal::{
     interrupt::{self, software::SoftwareInterrupt, InterruptHandler},
     Cpu,
 };
 use portable_atomic::{AtomicUsize, Ordering};
 
+use super::InnerExecutor;
+
 const COUNT: usize = 3 + cfg!(not(multi_core)) as usize;
 static mut EXECUTORS: [CallbackContext; COUNT] = [const { CallbackContext::new() }; COUNT];
 
@@ -19,15 +21,15 @@ static mut EXECUTORS: [CallbackContext; COUNT] = [const { CallbackContext::new()
 /// software.
 pub struct InterruptExecutor<const SWI: u8> {
     core: AtomicUsize,
-    executor: UnsafeCell<MaybeUninit<raw::Executor>>,
+    executor: UnsafeCell<MaybeUninit<InnerExecutor>>,
     interrupt: SoftwareInterrupt<SWI>,
 }
 
 unsafe impl<const SWI: u8> Send for InterruptExecutor<SWI> {}
 unsafe impl<const SWI: u8> Sync for InterruptExecutor<SWI> {}
 
 struct CallbackContext {
-    raw_executor: UnsafeCell<*mut raw::Executor>,
+    raw_executor: UnsafeCell<*mut InnerExecutor>,
 }
 
 impl CallbackContext {
@@ -37,11 +39,14 @@ impl CallbackContext {
         }
     }
 
-    fn get(&self) -> *mut raw::Executor {
-        unsafe { *self.raw_executor.get() }
+    /// # Safety:
+    ///
+    /// The caller must ensure `set` has been called before.
+    unsafe fn get(&self) -> &InnerExecutor {
+        unsafe { &**self.raw_executor.get() }
     }
 
-    fn set(&self, executor: *mut raw::Executor) {
+    fn set(&self, executor: *mut InnerExecutor) {
         unsafe { self.raw_executor.get().write(executor) };
     }
 }
@@ -51,8 +56,9 @@ extern "C" fn handle_interrupt<const NUM: u8>() {
     swi.reset();
 
     unsafe {
-        let executor = unwrap!(EXECUTORS[NUM as usize].get().as_mut());
-        executor.poll();
+        // SAFETY: The executor is always initialized before the interrupt is enabled.
+        let executor = EXECUTORS[NUM as usize].get();
+        executor.inner.poll();
     }
 }
 
@@ -99,7 +105,7 @@ impl<const SWI: u8> InterruptExecutor<SWI> {
         unsafe {
             (*self.executor.get())
                 .as_mut_ptr()
-                .write(raw::Executor::new((SWI as usize) as *mut ()));
+                .write(InnerExecutor::new(priority, (SWI as usize) as *mut ()));
 
             EXECUTORS[SWI as usize].set((*self.executor.get()).as_mut_ptr());
         }
@@ -117,7 +123,8 @@ impl<const SWI: u8> InterruptExecutor<SWI> {
             .set_interrupt_handler(InterruptHandler::new(swi_handler, priority));
 
         let executor = unsafe { (*self.executor.get()).assume_init_ref() };
-        executor.spawner().make_send()
+        executor.init();
+        executor.inner.spawner().make_send()
     }
 
     /// Get a SendSpawner for this executor
@@ -132,6 +139,6 @@ impl<const SWI: u8> InterruptExecutor<SWI> {
             panic!("InterruptExecutor::spawner() called on uninitialized executor.");
         }
         let executor = unsafe { (*self.executor.get()).assume_init_ref() };
-        executor.spawner().make_send()
+        executor.inner.spawner().make_send()
     }
 }
diff --git a/esp-hal-embassy/src/executor/mod.rs b/esp-hal-embassy/src/executor/mod.rs
@@ -1,4 +1,9 @@
+use embassy_executor::raw;
+use esp_hal::interrupt::Priority;
+
 pub use self::{interrupt::*, thread::*};
+#[cfg(not(single_queue))]
+use crate::timer_queue::TimerQueue;
 
 mod interrupt;
 mod thread;
@@ -22,3 +27,35 @@ fn __pender(context: *mut ()) {
         _ => unreachable!(),
     }
 }
+
+#[repr(C)]
+pub(crate) struct InnerExecutor {
+    inner: raw::Executor,
+    #[cfg(not(single_queue))]
+    pub(crate) timer_queue: TimerQueue,
+}
+
+impl InnerExecutor {
+    /// Create a new executor.
+    ///
+    /// When the executor has work to do, it will call the pender function and
+    /// pass `context` to it.
+    ///
+    /// See [`Executor`] docs for details on the pender.
+    pub(crate) fn new(_prio: Priority, context: *mut ()) -> Self {
+        Self {
+            inner: raw::Executor::new(context),
+            #[cfg(not(single_queue))]
+            timer_queue: TimerQueue::new(_prio),
+        }
+    }
+
+    pub(crate) fn init(&self) {
+        let inner_executor_ptr = self as *const _ as *mut InnerExecutor;
+        // Expose provenance so that casting back to InnerExecutor in the time driver is
+        // not UB.
+        _ = inner_executor_ptr.expose_provenance();
+        #[cfg(not(single_queue))]
+        self.timer_queue.set_context(inner_executor_ptr.cast());
+    }
+}
diff --git a/esp-hal-embassy/src/executor/thread.rs b/esp-hal-embassy/src/executor/thread.rs
@@ -2,13 +2,15 @@
 
 use core::marker::PhantomData;
 
-use embassy_executor::{raw, Spawner};
+use embassy_executor::Spawner;
 #[cfg(multi_core)]
 use esp_hal::interrupt::software::SoftwareInterrupt;
 use esp_hal::{interrupt::Priority, Cpu};
 #[cfg(low_power_wait)]
 use portable_atomic::{AtomicBool, Ordering};
 
+use super::InnerExecutor;
+
 pub(crate) const THREAD_MODE_CONTEXT: usize = 16;
 
 /// global atomic used to keep track of whether there is work to do since sev()
@@ -45,7 +47,7 @@ pub(crate) fn pend_thread_mode(_core: usize) {
 create one instance per core. The executors don't steal tasks from each other."
 )]
 pub struct Executor {
-    inner: raw::Executor,
+    inner: InnerExecutor,
     not_send: PhantomData<*mut ()>,
 }
 
@@ -59,7 +61,10 @@ This will use software-interrupt 3 which isn't available for anything else to wa
     )]
     pub fn new() -> Self {
         Self {
-            inner: raw::Executor::new((THREAD_MODE_CONTEXT + Cpu::current() as usize) as *mut ()),
+            inner: InnerExecutor::new(
+                Priority::Priority1,
+                (THREAD_MODE_CONTEXT + Cpu::current() as usize) as *mut (),
+            ),
             not_send: PhantomData,
         }
     }
@@ -90,13 +95,15 @@ This will use software-interrupt 3 which isn't available for anything else to wa
             Priority::min(),
         ));
 
-        init(self.inner.spawner());
+        self.inner.init();
+
+        init(self.inner.inner.spawner());
 
         #[cfg(low_power_wait)]
         let cpu = Cpu::current() as usize;
 
         loop {
-            unsafe { self.inner.poll() };
+            unsafe { self.inner.inner.poll() };
 
             #[cfg(low_power_wait)]
             Self::wait_impl(cpu);