From 352879abce20f791b66eec9ccca803ccee3dc8e8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=A1niel=20Buga?= <bugadani@gmail.com>
Date: Tue, 13 Aug 2024 14:51:26 +0200
Subject: [PATCH] Fix an infinite loop in interrupt executors (#1936)

* 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
---
 esp-hal-embassy/CHANGELOG.md                 |  1 +
 esp-hal-embassy/src/time_driver.rs           |  7 ++-
 hil-test/Cargo.toml                          |  9 +++
 hil-test/tests/embassy_interrupt_executor.rs | 66 ++++++++++++++++++++
 4 files changed, 80 insertions(+), 3 deletions(-)
 create mode 100644 hil-test/tests/embassy_interrupt_executor.rs

diff --git a/esp-hal-embassy/CHANGELOG.md b/esp-hal-embassy/CHANGELOG.md
index 63019c77c..41c4502d4 100644
--- a/esp-hal-embassy/CHANGELOG.md
+++ b/esp-hal-embassy/CHANGELOG.md
@@ -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
 
diff --git a/esp-hal-embassy/src/time_driver.rs b/esp-hal-embassy/src/time_driver.rs
index 365cfe393..6c0c3e9ec 100644
--- a/esp-hal-embassy/src/time_driver.rs
+++ b/esp-hal-embassy/src/time_driver.rs
@@ -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
diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml
index d3631cb31..b1585ddc9 100644
--- a/hil-test/Cargo.toml
+++ b/hil-test/Cargo.toml
@@ -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"] }
@@ -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.
diff --git a/hil-test/tests/embassy_interrupt_executor.rs b/hil-test/tests/embassy_interrupt_executor.rs
new file mode 100644
index 000000000..e82d5fcc2
--- /dev/null
+++ b/hil-test/tests/embassy_interrupt_executor.rs
@@ -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");
+    }
+}