From 09619bc9d93c2f1112be5adf57179c7a100d2fb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 18 Dec 2024 17:11:36 +0100 Subject: [PATCH] Document, clean up, fix --- esp-hal-embassy/Cargo.toml | 2 +- esp-hal-embassy/build.rs | 18 +++--- esp-hal-embassy/src/lib.rs | 18 +++++- esp-hal-embassy/src/time_driver.rs | 99 ++++++++++++++++++++++-------- esp-hal-embassy/src/timer_queue.rs | 6 ++ xtask/src/lib.rs | 3 + 6 files changed, 108 insertions(+), 38 deletions(-) diff --git a/esp-hal-embassy/Cargo.toml b/esp-hal-embassy/Cargo.toml index 629c79502ca..97aef6bb381 100644 --- a/esp-hal-embassy/Cargo.toml +++ b/esp-hal-embassy/Cargo.toml @@ -19,7 +19,7 @@ embassy-executor = { version = "0.6.3", features = ["timer-item-payload embassy-sync = { version = "0.6.1" } embassy-time = { version = "0.3.0" } embassy-time-driver = { version = "0.1.0", features = [ "tick-hz-1_000_000" ] } -embassy-time-queue-driver = { version = "0.1.0" } +embassy-time-queue-driver = { 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 } diff --git a/esp-hal-embassy/build.rs b/esp-hal-embassy/build.rs index 0d584ac0f37..2e9de630a0a 100644 --- a/esp-hal-embassy/build.rs +++ b/esp-hal-embassy/build.rs @@ -47,15 +47,9 @@ fn main() -> Result<(), Box> { Value::Bool(true), None ), - ( - "generic-queue-size", - "The size of the generic queue. Only used if `generic-queue` is enabled.", - Value::Integer(64), - Some(Validator::PositiveInteger), - ), ( "timer-queue", - "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.", + "

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.

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 'generic' queue allows using embassy-time without the embassy executors.

", Value::String(if cfg!(feature = "executors") { String::from("single-integrated") } else { @@ -73,7 +67,13 @@ fn main() -> Result<(), Box> { _ => Err(Error::Validation(format!("Expected 'single-integrated', 'multiple-integrated' or 'generic', found {string}"))) } }))) - ) + ), + ( + "generic-queue-size", + "The size of the generic queue. Only used if the `generic` timer-queue flavour is selected.", + Value::Integer(64), + Some(Validator::PositiveInteger), + ), ], true, ); @@ -91,7 +91,7 @@ fn main() -> Result<(), Box> { println!("cargo:rustc-cfg=integrated_timers"); } Value::String(s) if s.as_str() == "generic" => { - println!("cargo:rustc-cfg=generic-timers"); + println!("cargo:rustc-cfg=generic_timers"); println!("cargo:rustc-cfg=single_queue"); } _ => unreachable!(), diff --git a/esp-hal-embassy/src/lib.rs b/esp-hal-embassy/src/lib.rs index 159946ba789..4ded7d6c6a9 100644 --- a/esp-hal-embassy/src/lib.rs +++ b/esp-hal-embassy/src/lib.rs @@ -29,6 +29,19 @@ //! Embassy **must** be initialized by calling the [init] function. This //! initialization must be performed *prior* to spawning any tasks. //! +//! Initialization requires a number of timers to be passed in. The number of +//! timers required depends on the timer queue flavour used, as well as the +//! number of executors started. If you use the `multiple-integrated` timer +//! queue flavour, then you need to pass as many timers as you start executors. +//! In other cases, you can pass a single timer. +//! +//! ## Configuration +//! +//! You can configure the behaviour of the embassy runtime by using the +//! following environment variables: +#![doc = ""] +#![doc = include_str!(concat!(env!("OUT_DIR"), "/esp_hal_embassy_config_table.md"))] +#![doc = ""] //! ## Feature Flags #![doc = document_features::document_features!()] #![doc(html_logo_url = "https://avatars.githubusercontent.com/u/46717278")] @@ -143,8 +156,9 @@ impl_array!(4); /// - A mutable static array of `OneShotTimer` instances /// - A 2, 3, 4 element array of `AnyTimer` instances /// -/// Note that unless you use the `generic-queue` or the `single-queue` feature, -/// you need to pass as many timers as you start executors. +/// Note that if you use the `multiple-integrated` timer-queue flavour, then +/// you need to pass as many timers as you start executors. In other cases, +/// you can pass a single timer. /// /// # Examples /// diff --git a/esp-hal-embassy/src/time_driver.rs b/esp-hal-embassy/src/time_driver.rs index 196b26abc48..e130a69e761 100644 --- a/esp-hal-embassy/src/time_driver.rs +++ b/esp-hal-embassy/src/time_driver.rs @@ -1,3 +1,9 @@ +//! Embassy time driver implementation +//! +//! The time driver is responsible for keeping time, as well as to manage the +//! wait queue for embassy-time. + +#[cfg(not(single_queue))] use core::cell::Cell; use embassy_time_driver::Driver; @@ -53,12 +59,19 @@ impl AlarmState { } struct AlarmInner { - pub callback: Cell<*const ()>, + /// If multiple queues are used, we store the appropriate timer queue here. + // FIXME: we currently store the executor, but we could probably avoid an addition by actually + // storing a reference to the timer queue. + #[cfg(not(single_queue))] + pub context: Cell<*const ()>, pub state: AlarmState, } struct Alarm { + // FIXME: we should be able to use priority-limited locks here, but we can initialize alarms + // while running at an arbitrary priority level. We need to rework alarm allocation to only use + // a critical section to allocate an alarm, but not when using it. pub inner: Locked, } @@ -68,14 +81,34 @@ impl Alarm { pub const fn new(handler: extern "C" fn()) -> Self { Self { inner: Locked::new(AlarmInner { - callback: Cell::new(core::ptr::null_mut()), + #[cfg(not(single_queue))] + context: Cell::new(core::ptr::null_mut()), state: AlarmState::Created(handler), }), } } } +/// embassy requires us to implement the [embassy_time_driver::Driver] trait, +/// which we do here. This trait needs us to be able to tell the current time, +/// as well as to schedule a wake-up at a certain time. +/// +/// We are free to choose how we implement these features, and we provide three +/// options: +/// +/// - If the `generic` feature is enabled, we implement a single timer queue, +/// using the implementation provided by embassy-time-queue-driver. +/// - If the `single-integrated` feature is enabled, we implement a single timer +/// queue, using our own integrated timer implementation. Our implementation +/// is a copy of the embassy integrated timer queue, with the addition of +/// clearing the "owner" information upon dequeueing. +/// - If the `multiple-integrated` feature is enabled, we provide a separate +/// timer queue for each executor. We store a separate timer queue for each +/// executor, and we use the scheduled task's owner to determine which queue +/// to use. This mode allows us to use less disruptive locks around the timer +/// queue, but requires more timers - one per timer queue. pub(super) struct EmbassyTimer { + /// The timer queue, if we use a single one (single-integrated, or generic). #[cfg(single_queue)] pub(crate) inner: crate::timer_queue::TimerQueue, @@ -99,8 +132,13 @@ macro_rules! alarms { }; } +// TODO: we can reduce this to 1 for single_queue, but that would break current +// tests. Resolve when tests can use separate configuration sets, or update +// tests to always pass a single timer. const MAX_SUPPORTED_ALARM_COUNT: usize = 7; + embassy_time_driver::time_driver_impl!(static DRIVER: EmbassyTimer = EmbassyTimer { + // Single queue, needs maximum priority. #[cfg(single_queue)] inner: crate::timer_queue::TimerQueue::new(Priority::max()), alarms: alarms!(0, 1, 2, 3, 4, 5, 6), @@ -130,15 +168,18 @@ impl EmbassyTimer { #[cfg(not(single_queue))] pub(crate) fn set_callback_ctx(&self, alarm: AlarmHandle, ctx: *const ()) { self.alarms[alarm.id].inner.with(|alarm| { - alarm.callback.set(ctx.cast_mut()); + alarm.context.set(ctx.cast_mut()); }) } fn on_interrupt(&self, id: usize) { + // On interrupt, we clear the alarm that was triggered... + #[cfg_attr(single_queue, allow(clippy::let_unit_value))] let _ctx = self.alarms[id].inner.with(|alarm| { if let AlarmState::Initialized(timer) = &mut alarm.state { timer.clear_interrupt(); - alarm.callback.get() + #[cfg(not(single_queue))] + alarm.context.get() } else { unsafe { // SAFETY: `on_interrupt` is registered right when the alarm is initialized. @@ -147,12 +188,15 @@ impl EmbassyTimer { } }); + // ... and process the timer queue if we have one. For multiple queues, the + // timer queue is stored in the alarm's context. #[cfg(all(integrated_timers, not(single_queue)))] { let executor = unsafe { &*_ctx.cast::() }; executor.timer_queue.dispatch(); } + // If we have a single queue, it lives in this struct. #[cfg(single_queue)] self.inner.dispatch(); } @@ -175,6 +219,16 @@ impl EmbassyTimer { } } + /// Allocate an alarm, if possible. + /// + /// Returns `None` if there are no available alarms. + /// + /// When using multiple timer queues, the `priority` parameter indicates the + /// priority of the interrupt handler. It is 1 for thread-mode + /// executors, or equals to the priority of an interrupt executor. + /// + /// When using a single timer queue, the `priority` parameter is always the + /// highest value possible. pub(crate) unsafe fn allocate_alarm(&self, priority: Priority) -> Option { for (i, alarm) in self.alarms.iter().enumerate() { let handle = alarm.inner.with(|alarm| { @@ -183,9 +237,6 @@ impl EmbassyTimer { }; let timer = self.available_timers.with(|available_timers| { - // `allocate_alarm` may be called before `esp_hal_embassy::init()`. If - // `timers` is `None`, we return `None` to signal that the alarm cannot be - // initialized yet. These alarms will be initialized when `init` is called. if let Some(timers) = available_timers.take() { // If the driver is initialized, we can allocate a timer. // If this fails, we can't do anything about it. @@ -193,20 +244,16 @@ impl EmbassyTimer { not_enough_timers(); }; *available_timers = Some(rest); - Some(timer) + timer } else { - None + panic!("schedule_wake called before esp_hal_embassy::init()") } }); - alarm.state = match timer { - Some(timer) => AlarmState::initialize( - timer, - InterruptHandler::new(interrupt_handler, priority), - ), - - None => panic!(), - }; + alarm.state = AlarmState::initialize( + timer, + InterruptHandler::new(interrupt_handler, priority), + ); Some(AlarmHandle::new(i)) }); @@ -219,16 +266,12 @@ impl EmbassyTimer { None } - pub(crate) fn set_alarm(&self, alarm: AlarmHandle, timestamp: u64) -> bool { + /// Set an alarm to fire at a certain timestamp. + /// + /// Returns `false` if the timestamp is in the past. + fn set_alarm(&self, alarm: AlarmHandle, timestamp: u64) -> bool { let alarm = &self.alarms[alarm.id]; - // If integrated timers are used and there are no pending - // timers, embassy still calls `set_alarm` with `u64::MAX`. By returning - // `true` we signal that no re-polling is necessary. - if timestamp == u64::MAX { - return true; - } - // The hardware fires the alarm even if timestamp is lower than the current // time. In this case the interrupt handler will pend a wake-up when we exit the // critical section. @@ -241,7 +284,11 @@ impl EmbassyTimer { if let AlarmState::Initialized(timer) = &mut alarm.state { Self::arm(timer, timestamp) } else { - panic!("set_alarm called before esp_hal_embassy::init()") + unsafe { + // SAFETY: We only create `AlarmHandle` instances after the alarm is + // initialized. + core::hint::unreachable_unchecked() + } } }) } diff --git a/esp-hal-embassy/src/timer_queue.rs b/esp-hal-embassy/src/timer_queue.rs index 67e03549846..a48ca309406 100644 --- a/esp-hal-embassy/src/timer_queue.rs +++ b/esp-hal-embassy/src/timer_queue.rs @@ -1,3 +1,9 @@ +//! Timer waiter queue. +//! +//! This module implements the timer queue, which is managed by the time driver. +//! The timer queue contains wakers and their expiration times, and is used to +//! wake tasks at the correct time. + #[cfg(not(single_queue))] use core::cell::Cell; use core::cell::{RefCell, UnsafeCell}; diff --git a/xtask/src/lib.rs b/xtask/src/lib.rs index d2563bd0d40..a458e59b4aa 100644 --- a/xtask/src/lib.rs +++ b/xtask/src/lib.rs @@ -185,6 +185,9 @@ fn apply_feature_rules(package: &Package, config: &Config) -> Vec { features.push("coex".to_owned()); } } + Package::EspHalEmbassy => { + features.push("esp-hal/unstable".to_owned()); + } _ => {} } features