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

Fix an infinite loop in interrupt executors #1936

Merged
merged 6 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions esp-hal-embassy/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Fixed a bug where the timeout was huge whenever the timestamp at the time of scheduling was already in the past (#1875)
- Fixed interrupt executors looping endlessly when `integrated-timers` is used. (#1936)

### Removed

Expand Down
7 changes: 4 additions & 3 deletions esp-hal-embassy/src/time_driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,11 @@ impl Driver for EmbassyTimer {
}

fn set_alarm(&self, alarm: AlarmHandle, timestamp: u64) -> bool {
// we sometimes get called with `u64::MAX` for apparently not yet initialized
// timers which would fail later on
// If `embassy-executor/integrated-timers` is enabled 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 false;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting ... docs could be a bit better at explaining the meaning of the result 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, as far as I understand it's along the lines of "true: a timer has been set and will trigger a tick if needed; false: the next alarm is in the past, do a re-poll".

IMO set_alarm shouldn't even be called when I experience the error but I expect getting this changed in embassy is a larger task. And I don't know if the current state of things is caused because some time drivers may handle u64::MAX as a valid alarm value?

cc @Dirbaio do you have an opinion whether we can change embassy-executor to not call set_timer if there is no valid next expiration value?

Copy link
Contributor

@Dirbaio Dirbaio Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason it calls set_alarm(u64::MAX) is to cancel an existing timer from a previous poll if any. This can happen if on a previous poll a task was waiting on a timer, and then on this poll it's not anymore. This can happen if you cancel the timer, for example if it was before waiting on either a gpio event or a timer, and now is waiting on a gpio event only.

Not calling set_alarm(u64::MAX) would do one "useless" poll when the stale timer expires.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this piece of code would ideally unset the alarm, not just "do nothing, return true".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've posted embassy-rs/embassy#3255 which clarifies this, please let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an interesting assumption that we have any active alarms, but the explanation makes sense and I guess it points at the current PR being incomplete. Thank you.

}

// The hardware fires the alarm even if timestamp is lower than the current
Expand Down
9 changes: 9 additions & 0 deletions hil-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,18 @@ name = "embassy_timers_executors"
harness = false
required-features = ["async", "embassy"]

[[test]]
name = "embassy_interrupt_executor"
harness = false
required-features = ["async", "embassy", "integrated-timers"]

[dependencies]
cfg-if = "1.0.0"
critical-section = "1.1.2"
defmt = "0.3.8"
defmt-rtt = "0.4.1"
embassy-futures = "0.1.1"
embassy-sync = "0.6.0"
embassy-time = { version = "0.3.1", features = ["generic-queue-64"] }
embedded-hal = "1.0.0"
embedded-hal-02 = { version = "0.2.7", package = "embedded-hal", features = ["unproven"] }
Expand Down Expand Up @@ -180,6 +186,9 @@ embassy = [
"embedded-test/external-executor",
"dep:esp-hal-embassy",
]
integrated-timers = [
"embassy-executor/integrated-timers",
]

# https://doc.rust-lang.org/cargo/reference/profiles.html#test
# Test and bench profiles inherit from dev and release respectively.
Expand Down
66 changes: 66 additions & 0 deletions hil-test/tests/embassy_interrupt_executor.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
//! Test that the interrupt executor correctly gives back control to thread mode
//! code.

//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
//% FEATURES: integrated-timers

#![no_std]
#![no_main]

use embassy_sync::{blocking_mutex::raw::CriticalSectionRawMutex, signal::Signal};

macro_rules! mk_static {
($t:ty,$val:expr) => {{
static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
#[deny(unused_attributes)]
let x = STATIC_CELL.uninit().write(($val));
x
}};
}

#[embassy_executor::task]
async fn interrupt_driven_task(signal: &'static Signal<CriticalSectionRawMutex, ()>) {
loop {
signal.wait().await;
defmt::info!("Received");
}
}

#[cfg(test)]
#[embedded_test::tests]
mod test {
use defmt_rtt as _;
use esp_backtrace as _;
use esp_hal::{
clock::ClockControl,
interrupt::Priority,
peripherals::Peripherals,
system::{SoftwareInterrupt, SystemControl},
};
use esp_hal_embassy::InterruptExecutor;

use super::*;

#[init]
fn init() -> SoftwareInterrupt<1> {
let peripherals = Peripherals::take();
let system = SystemControl::new(peripherals.SYSTEM);
let _clocks = ClockControl::boot_defaults(system.clock_control).freeze();
system.software_interrupt_control.software_interrupt1
}

#[test]
#[timeout(3)]
fn run_interrupt_executor_test(interrupt: SoftwareInterrupt<1>) {
let interrupt_executor =
mk_static!(InterruptExecutor<1>, InterruptExecutor::new(interrupt));
let signal = mk_static!(Signal<CriticalSectionRawMutex, ()>, Signal::new());

let spawner = interrupt_executor.start(Priority::Priority3);

spawner.spawn(interrupt_driven_task(signal)).unwrap();

signal.signal(());
defmt::info!("Returned");
}
}