Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update our time driver for upcoming embassy changes #2701

Merged
merged 44 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
c25c78a
Add a timer-driven task
bugadani Nov 26, 2024
31a1c8a
Spawn another timer
bugadani Nov 28, 2024
54315a4
Log
bugadani Nov 28, 2024
b9ee98e
foo
bugadani Nov 30, 2024
6cded9d
Do not access current time on each schedule
bugadani Dec 1, 2024
154cb86
Update generic queue
bugadani Dec 2, 2024
9a8af6e
Minimize alarm priorities
bugadani Dec 6, 2024
d000187
Point to github with patches
bugadani Dec 6, 2024
cc49f37
Fix build without any queue impl selected
bugadani Dec 6, 2024
9c02e41
Remove explicit generic-queue features
bugadani Dec 6, 2024
68fdcf9
Define cfgs, fix calling something uninitialized
bugadani Dec 6, 2024
0786f35
Clean up RefCell+generic queue
bugadani Dec 6, 2024
476c996
Fix arg order
bugadani Dec 6, 2024
8689ce7
Feature
bugadani Dec 7, 2024
1ba1c78
Fix single integrated-timer queue
bugadani Dec 7, 2024
3f0d4d7
Fix next expiration when arming
bugadani Dec 7, 2024
74570a6
Add note
bugadani Dec 8, 2024
b65294d
Adjust impl to latest changes
bugadani Dec 8, 2024
b8f10cd
Local patch
bugadani Dec 7, 2024
51b2034
Refactor the refactor refactor
bugadani Dec 10, 2024
60bf774
Track the timer item's owner
bugadani Dec 16, 2024
928a1d1
Clear owner on dequeue
bugadani Dec 16, 2024
58f74da
Clean up
bugadani Dec 17, 2024
4b801d1
Point at the right branch
bugadani Dec 17, 2024
6aadf1f
Fix panic message
bugadani Dec 17, 2024
2202349
Hide private function
bugadani Dec 17, 2024
891708d
Remove integrated-timer references
bugadani Dec 17, 2024
9e1c63a
Point at upstream embassy
bugadani Dec 17, 2024
84f58ae
Configure via esp-config
bugadani Dec 17, 2024
518fa05
Document, clean up, fix
bugadani Dec 18, 2024
19e9a23
Hack
bugadani Dec 18, 2024
5e07273
Remove patches
bugadani Jan 6, 2025
9477fb2
Update config separator, test the complex variant
bugadani Jan 6, 2025
6295fbd
Undo esp-config hack
bugadani Jan 6, 2025
0899f2c
Remove trouble example, update edge-net
bugadani Jan 7, 2025
532db7e
Update test deps
bugadani Jan 7, 2025
a373732
Document
bugadani Jan 7, 2025
b1f8f53
Update bt-hci.
Dirbaio Jan 8, 2025
e44e109
Fix generic queue
bugadani Jan 9, 2025
088597e
Fix panic message
bugadani Jan 9, 2025
47f0c6c
Fix UB
bugadani Jan 9, 2025
5433780
Fix rebase
bugadani Jan 9, 2025
2326283
Resolve UB
bugadani Jan 14, 2025
bd7032c
Avoid mutable reference in interrupt executor
bugadani Jan 14, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions esp-hal-embassy/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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
Expand Down
26 changes: 14 additions & 12 deletions esp-hal-embassy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand All @@ -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"
21 changes: 21 additions & 0 deletions esp-hal-embassy/MIGRATING-0.5.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
64 changes: 59 additions & 5 deletions esp-hal-embassy/build.rs
Original file line number Diff line number Diff line change
@@ -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!(
Expand Down Expand Up @@ -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>",
MabezDev marked this conversation as resolved.
Show resolved Hide resolved
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(())
}
29 changes: 18 additions & 11 deletions esp-hal-embassy/src/executor/interrupt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand All @@ -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 {
Expand All @@ -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) };
}
}
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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());
}
Expand All @@ -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
Expand All @@ -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()
}
}
37 changes: 37 additions & 0 deletions esp-hal-embassy/src/executor/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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());
}
}
17 changes: 12 additions & 5 deletions esp-hal-embassy/src/executor/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 ()>,
}

Expand All @@ -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,
}
}
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading