Skip to content

Commit

Permalink
Fix an infinite loop in interrupt executors (#1936)
Browse files Browse the repository at this point in the history
* Add failing test

Fix the name of the test fn

* Fix interrupt executor looping

* Fix formatting

* Fix changelog reference

* Move changelog to the right crate

* Remove dead code
  • Loading branch information
bugadani authored Aug 13, 2024
1 parent a6e1406 commit 352879a
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 3 deletions.
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;
}

// 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");
}
}

0 comments on commit 352879a

Please sign in to comment.