Fix mono delay (#843)

* rtic-time: Compenstate for timer uncertainty * Update changelog and incorrect cargo.lock in an example * Fix Monotonic impls * Fix tests * Fix other monotonics, again * Update changelog * Fix example * Fix DelayUs and DelayMs impls * Minor coding style fix in u64 conversions * Fix all changelogs * Fix changelog * Fix blocking DelayUs * Minor monotonic rework * Add delay precision test * Add more tests * Add rust-version tags to Cargo.toml * Fix imxrt, rp2040 and systick timer * Fix more monotonics * Fix systick monotonic * Some reverts * Fix imxrt * Fix nrf * Fix rp2040 * Fix stm32 * Fix systick * Fix rtic-time tests * Bump to e-h.rc2 * Apply e-h.rc2 fixes to rtic-time * Apply fixes from arbiter * Fix clippy warning * Minor beautification * Revert previous changes * Fix variable name * Add blocking tests, but disable them by default
rtic-rs · Dec 1, 2023 · 612a47e · 612a47e
1 parent 9f5820d
commit 612a47e
Show file tree

Hide file tree

Showing 19 changed files with 559 additions and 140 deletions.
diff --git a/examples/teensy4_blinky/Cargo.lock b/examples/teensy4_blinky/Cargo.lock
diff --git a/rtic-monotonics/CHANGELOG.md b/rtic-monotonics/CHANGELOG.md
@@ -7,6 +7,10 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top!
 
 ## Unreleased
 
+### Fixed
+
+- **Soundness fix:** Monotonics did not wait long enough in `Duration` based delays.
+
 ## v1.3.0 - 2023-11-08
 
 ### Added

diff --git a/rtic-monotonics/Cargo.toml b/rtic-monotonics/Cargo.toml
@@ -28,8 +28,8 @@ rustdoc-flags = ["--cfg", "docsrs"]
 
 [dependencies]
 rtic-time = { version = "1.0.0", path = "../rtic-time" }
-embedded-hal = { version = "1.0.0-rc.1" }
-embedded-hal-async = { version = "1.0.0-rc.1", optional = true }
+embedded-hal = { version = "1.0.0-rc.2" }
+embedded-hal-async = { version = "1.0.0-rc.2", optional = true }
 fugit = { version = "0.3.6" }
 atomic-polyfill = "1"
 cfg-if = "1.0.0"

diff --git a/rtic-monotonics/src/imxrt.rs b/rtic-monotonics/src/imxrt.rs
@@ -31,7 +31,7 @@
 
 use crate::{Monotonic, TimeoutError, TimerQueue};
 use atomic_polyfill::{compiler_fence, AtomicU32, Ordering};
-pub use fugit::{self, ExtU64};
+pub use fugit::{self, ExtU64, ExtU64Ceil};
 
 use imxrt_ral as ral;
 
@@ -216,31 +216,17 @@ macro_rules! make_timer {
             }
         }
 
-        #[cfg(feature = "embedded-hal-async")]
-        impl embedded_hal_async::delay::DelayUs for $mono_name {
-            #[inline]
-            async fn delay_us(&mut self, us: u32) {
-                Self::delay((us as u64).micros()).await;
-            }
-
-            #[inline]
-            async fn delay_ms(&mut self, ms: u32) {
-                Self::delay((ms as u64).millis()).await;
-            }
-        }
+        rtic_time::embedded_hal_delay_impl_fugit64!($mono_name);
 
-        impl embedded_hal::delay::DelayUs for $mono_name {
-            fn delay_us(&mut self, us: u32) {
-                let done = Self::now() + (us as u64).micros();
-                while Self::now() < done {}
-            }
-        }
+        #[cfg(feature = "embedded-hal-async")]
+        rtic_time::embedded_hal_async_delay_impl_fugit64!($mono_name);
 
         impl Monotonic for $mono_name {
             type Instant = fugit::TimerInstantU64<TIMER_HZ>;
             type Duration = fugit::TimerDurationU64<TIMER_HZ>;
 
             const ZERO: Self::Instant = Self::Instant::from_ticks(0);
+            const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1);
 
             fn now() -> Self::Instant {
                 let gpt = unsafe{ $timer::instance() };

diff --git a/rtic-monotonics/src/nrf/rtc.rs b/rtic-monotonics/src/nrf/rtc.rs
@@ -43,7 +43,7 @@ use nrf9160_pac::{self as pac, Interrupt, RTC0_NS as RTC0, RTC1_NS as RTC1};
 use crate::{Monotonic, TimeoutError, TimerQueue};
 use atomic_polyfill::{AtomicU32, Ordering};
 use core::future::Future;
-pub use fugit::{self, ExtU64};
+pub use fugit::{self, ExtU64, ExtU64Ceil};
 
 #[doc(hidden)]
 #[macro_export]
@@ -167,28 +167,15 @@ macro_rules! make_rtc {
             }
         }
 
-        #[cfg(feature = "embedded-hal-async")]
-        impl embedded_hal_async::delay::DelayUs for $mono_name {
-            #[inline]
-            async fn delay_us(&mut self, us: u32) {
-               Self::delay((us as u64).micros()).await;
-            }
 
-            #[inline]
-            async fn delay_ms(&mut self, ms: u32) {
-                Self::delay((ms as u64).millis()).await;
-            }
-        }
+        rtic_time::embedded_hal_delay_impl_fugit64!($mono_name);
 
-        impl embedded_hal::delay::DelayUs for $mono_name {
-            fn delay_us(&mut self, us: u32) {
-                let done = Self::now() + u64::from(us).micros();
-                while Self::now() < done {}
-            }
-        }
+        #[cfg(feature = "embedded-hal-async")]
+        rtic_time::embedded_hal_async_delay_impl_fugit64!($mono_name);
 
         impl Monotonic for $mono_name {
             const ZERO: Self::Instant = Self::Instant::from_ticks(0);
+            const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1);
 
             type Instant = fugit::TimerInstantU64<32_768>;
             type Duration = fugit::TimerDurationU64<32_768>;

diff --git a/rtic-monotonics/src/nrf/timer.rs b/rtic-monotonics/src/nrf/timer.rs
@@ -29,7 +29,7 @@
 use crate::{Monotonic, TimeoutError, TimerQueue};
 use atomic_polyfill::{AtomicU32, Ordering};
 use core::future::Future;
-pub use fugit::{self, ExtU64};
+pub use fugit::{self, ExtU64, ExtU64Ceil};
 
 #[cfg(feature = "nrf52810")]
 use nrf52810_pac::{self as pac, Interrupt, TIMER0, TIMER1, TIMER2};
@@ -203,28 +203,14 @@ macro_rules! make_timer {
             }
         }
 
-        #[cfg(feature = "embedded-hal-async")]
-        impl embedded_hal_async::delay::DelayUs for $mono_name {
-            #[inline]
-            async fn delay_us(&mut self, us: u32) {
-                Self::delay((us as u64).micros()).await;
-            }
-
-            #[inline]
-            async fn delay_ms(&mut self, ms: u32) {
-                Self::delay((ms as u64).millis()).await;
-            }
-        }
+        rtic_time::embedded_hal_delay_impl_fugit64!($mono_name);
 
-        impl embedded_hal::delay::DelayUs for $mono_name {
-            fn delay_us(&mut self, us: u32) {
-                let done = Self::now() + (us as u64).micros();
-                while Self::now() < done {}
-            }
-        }
+        #[cfg(feature = "embedded-hal-async")]
+        rtic_time::embedded_hal_async_delay_impl_fugit64!($mono_name);
 
         impl Monotonic for $mono_name {
             const ZERO: Self::Instant = Self::Instant::from_ticks(0);
+            const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1);
 
             type Instant = fugit::TimerInstantU64<1_000_000>;
             type Duration = fugit::TimerDurationU64<1_000_000>;

diff --git a/rtic-monotonics/src/rp2040.rs b/rtic-monotonics/src/rp2040.rs
@@ -28,7 +28,7 @@ use super::Monotonic;
 
 pub use super::{TimeoutError, TimerQueue};
 use core::future::Future;
-pub use fugit::{self, ExtU64};
+pub use fugit::{self, ExtU64, ExtU64Ceil};
 use rp2040_pac::{timer, Interrupt, NVIC, RESETS, TIMER};
 
 /// Timer implementing [`Monotonic`] which runs at 1 MHz.
@@ -104,6 +104,7 @@ impl Monotonic for Timer {
     type Duration = fugit::TimerDurationU64<1_000_000>;
 
     const ZERO: Self::Instant = Self::Instant::from_ticks(0);
+    const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1);
 
     fn now() -> Self::Instant {
         let timer = Self::timer();
@@ -151,23 +152,10 @@ impl Monotonic for Timer {
     fn disable_timer() {}
 }
 
-#[cfg(feature = "embedded-hal-async")]
-impl embedded_hal_async::delay::DelayUs for Timer {
-    async fn delay_us(&mut self, us: u32) {
-        Self::delay((us as u64).micros()).await;
-    }
-
-    async fn delay_ms(&mut self, ms: u32) {
-        Self::delay((ms as u64).millis()).await;
-    }
-}
+rtic_time::embedded_hal_delay_impl_fugit64!(Timer);
 
-impl embedded_hal::delay::DelayUs for Timer {
-    fn delay_us(&mut self, us: u32) {
-        let done = Self::now() + u64::from(us).micros();
-        while Self::now() < done {}
-    }
-}
+#[cfg(feature = "embedded-hal-async")]
+rtic_time::embedded_hal_async_delay_impl_fugit64!(Timer);
 
 /// Register the Timer interrupt for the monotonic.
 #[macro_export]

diff --git a/rtic-monotonics/src/stm32.rs b/rtic-monotonics/src/stm32.rs
@@ -36,7 +36,7 @@
 
 use crate::{Monotonic, TimeoutError, TimerQueue};
 use atomic_polyfill::{compiler_fence, AtomicU64, Ordering};
-pub use fugit::{self, ExtU64};
+pub use fugit::{self, ExtU64, ExtU64Ceil};
 use stm32_metapac as pac;
 
 mod _generated {
@@ -218,31 +218,17 @@ macro_rules! make_timer {
             }
         }
 
-        #[cfg(feature = "embedded-hal-async")]
-        impl embedded_hal_async::delay::DelayUs for $mono_name {
-            #[inline]
-            async fn delay_us(&mut self, us: u32) {
-                Self::delay((us as u64).micros()).await;
-            }
-
-            #[inline]
-            async fn delay_ms(&mut self, ms: u32) {
-                Self::delay((ms as u64).millis()).await;
-            }
-        }
+        rtic_time::embedded_hal_delay_impl_fugit64!($mono_name);
 
-        impl embedded_hal::delay::DelayUs for $mono_name {
-            fn delay_us(&mut self, us: u32) {
-                let done = Self::now() + (us as u64).micros();
-                while Self::now() < done {}
-            }
-        }
+        #[cfg(feature = "embedded-hal-async")]
+        rtic_time::embedded_hal_async_delay_impl_fugit64!($mono_name);
 
         impl Monotonic for $mono_name {
             type Instant = fugit::TimerInstantU64<TIMER_HZ>;
             type Duration = fugit::TimerDurationU64<TIMER_HZ>;
 
             const ZERO: Self::Instant = Self::Instant::from_ticks(0);
+            const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1);
 
             fn now() -> Self::Instant {
                 // Credits to the `time-driver` of `embassy-stm32`.

diff --git a/rtic-monotonics/src/systick.rs b/rtic-monotonics/src/systick.rs
@@ -40,11 +40,11 @@ use cortex_m::peripheral::SYST;
 pub use fugit;
 cfg_if::cfg_if! {
     if #[cfg(feature = "systick-64bit")] {
-        pub use fugit::ExtU64;
+        pub use fugit::{ExtU64, ExtU64Ceil};
         use atomic_polyfill::AtomicU64;
         static SYSTICK_CNT: AtomicU64 = AtomicU64::new(0);
     } else {
-        pub use fugit::ExtU32;
+        pub use fugit::{ExtU32, ExtU32Ceil};
         use atomic_polyfill::AtomicU32;
         static SYSTICK_CNT: AtomicU32 = AtomicU32::new(0);
     }
@@ -156,6 +156,7 @@ impl Monotonic for Systick {
     }
 
     const ZERO: Self::Instant = Self::Instant::from_ticks(0);
+    const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1);
 
     fn now() -> Self::Instant {
         if Self::systick().has_wrapped() {
@@ -188,27 +189,17 @@ impl Monotonic for Systick {
     fn disable_timer() {}
 }
 
-#[cfg(feature = "embedded-hal-async")]
-impl embedded_hal_async::delay::DelayUs for Systick {
-    async fn delay_us(&mut self, us: u32) {
-        #[cfg(feature = "systick-64bit")]
-        let us = u64::from(us);
-        Self::delay(us.micros()).await;
-    }
+cfg_if::cfg_if! {
+    if #[cfg(feature = "systick-64bit")] {
+        rtic_time::embedded_hal_delay_impl_fugit64!(Systick);
 
-    async fn delay_ms(&mut self, ms: u32) {
-        #[cfg(feature = "systick-64bit")]
-        let ms = u64::from(ms);
-        Self::delay(ms.millis()).await;
-    }
-}
+        #[cfg(feature = "embedded-hal-async")]
+        rtic_time::embedded_hal_async_delay_impl_fugit64!(Systick);
+    } else {
+        rtic_time::embedded_hal_delay_impl_fugit32!(Systick);
 
-impl embedded_hal::delay::DelayUs for Systick {
-    fn delay_us(&mut self, us: u32) {
-        #[cfg(feature = "systick-64bit")]
-        let us = u64::from(us);
-        let done = Self::now() + us.micros();
-        while Self::now() < done {}
+        #[cfg(feature = "embedded-hal-async")]
+        rtic_time::embedded_hal_async_delay_impl_fugit32!(Systick);
     }
 }
 

diff --git a/rtic-sync/Cargo.toml b/rtic-sync/Cargo.toml
@@ -21,9 +21,9 @@ heapless = "0.7"
 critical-section = "1"
 rtic-common = { version = "1.0.0", path = "../rtic-common" }
 portable-atomic = { version = "1", default-features = false }
-embedded-hal = { version = "1.0.0-rc.1", optional = true }
-embedded-hal-async = { version = "1.0.0-rc.1", optional = true }
-embedded-hal-bus = { version = "0.1.0-rc.1", optional = true, features = ["async"] }
+embedded-hal = { version = "1.0.0-rc.2", optional = true }
+embedded-hal-async = { version = "1.0.0-rc.2", optional = true }
+embedded-hal-bus = { version = "0.1.0-rc.2", optional = true, features = ["async"] }
 
 [dev-dependencies]
 tokio = { version = "1", features = ["rt", "macros", "time"] }

diff --git a/rtic-sync/src/arbiter.rs b/rtic-sync/src/arbiter.rs
@@ -197,7 +197,7 @@ pub mod spi {
     use super::Arbiter;
     use embedded_hal::digital::OutputPin;
     use embedded_hal_async::{
-        delay::DelayUs,
+        delay::DelayNs,
         spi::{ErrorType, Operation, SpiBus, SpiDevice},
     };
     use embedded_hal_bus::spi::DeviceError;
@@ -229,7 +229,7 @@ pub mod spi {
         Word: Copy + 'static,
         BUS: SpiBus<Word>,
         CS: OutputPin,
-        D: DelayUs,
+        D: DelayNs,
     {
         async fn transaction(
             &mut self,
@@ -246,10 +246,10 @@ pub mod spi {
                         Operation::Write(buf) => bus.write(buf).await,
                         Operation::Transfer(read, write) => bus.transfer(read, write).await,
                         Operation::TransferInPlace(buf) => bus.transfer_in_place(buf).await,
-                        Operation::DelayUs(us) => match bus.flush().await {
+                        Operation::DelayNs(ns) => match bus.flush().await {
                             Err(e) => Err(e),
                             Ok(()) => {
-                                self.delay.delay_us(*us).await;
+                                self.delay.delay_ns(*ns).await;
                                 Ok(())
                             }
                         },

diff --git a/rtic-time/CHANGELOG.md b/rtic-time/CHANGELOG.md
@@ -15,6 +15,7 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top!
 
 ### Fixed
 
+- **Soundness fix:** `TimerQueue` did not wait long enough in `Duration` based delays. Fixing this sadly required adding a `const TICK_PERIOD` to the `Monotonic` trait, which requires updating all existing implementations.
 - If the queue was non-empty and a new instant was added that was earlier than `head`, then the queue would no pend the monotonic handler. This would cause the new `head` to be dequeued at the wrong time.
 
 ## [v1.0.0] - 2023-05-31
diff --git a/rtic-time/Cargo.toml b/rtic-time/Cargo.toml
@@ -22,5 +22,9 @@ futures-util = { version = "0.3.25", default-features = false }
 rtic-common = { version = "1.0.0", path = "../rtic-common" }
 
 [dev-dependencies]
+embedded-hal = { version = "1.0.0-rc.2" }
+embedded-hal-async = { version = "1.0.0-rc.2" }
+fugit = "0.3.7"
 parking_lot = "0.12"
 cassette = "0.2"
+cooked-waker = "5.0.0"