diff --git a/examples/teensy4_blinky/Cargo.lock b/examples/teensy4_blinky/Cargo.lock index daec54f7e5..408bfe7dd4 100644 --- a/examples/teensy4_blinky/Cargo.lock +++ b/examples/teensy4_blinky/Cargo.lock @@ -440,7 +440,7 @@ dependencies = [ [[package]] name = "rtic-monotonics" -version = "1.2.1" +version = "1.3.0" dependencies = [ "atomic-polyfill", "cfg-if", diff --git a/rtic-monotonics/CHANGELOG.md b/rtic-monotonics/CHANGELOG.md index d030bb0234..4dea3fc529 100644 --- 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 index 441e772f8b..4d9dc0b863 100644 --- 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 index 39448291a0..97ba73f8ac 100644 --- 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 { } } + rtic_time::embedded_hal_delay_impl_fugit64!($mono_name); + #[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; - } - } - - 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 {} - } - } + rtic_time::embedded_hal_async_delay_impl_fugit64!($mono_name); impl Monotonic for $mono_name { type Instant = fugit::TimerInstantU64; type Duration = fugit::TimerDurationU64; 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 index 94913071d8..1f4e34f5ac 100644 --- 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 { } } + + rtic_time::embedded_hal_delay_impl_fugit64!($mono_name); + #[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; - } - } - - 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 {} - } - } + 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 index 589cc6fdfb..0488ca9b22 100644 --- 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 { } } + rtic_time::embedded_hal_delay_impl_fugit64!($mono_name); + #[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; - } - } - - 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 {} - } - } + 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 index 130c7d3e60..998b53251e 100644 --- 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() {} } +rtic_time::embedded_hal_delay_impl_fugit64!(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; - } -} - -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 {} - } -} +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 index 2676a34627..254f1302fe 100644 --- 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 { } } + rtic_time::embedded_hal_delay_impl_fugit64!($mono_name); + #[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; - } - } - - 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 {} - } - } + rtic_time::embedded_hal_async_delay_impl_fugit64!($mono_name); impl Monotonic for $mono_name { type Instant = fugit::TimerInstantU64; type Duration = fugit::TimerDurationU64; 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 index 265ca9a0ec..9bd056ce97 100644 --- 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 index 39f62dc8f7..8172e2629c 100644 --- 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 index 99d4174829..f0dbc4cdb3 100644 --- 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, 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 index 2e7bebc317..cf312ac15f 100644 --- 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 index ef7635e319..a8379e4a53 100644 --- 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" diff --git a/rtic-time/src/lib.rs b/rtic-time/src/lib.rs index 0c3e349580..4c89d52c93 100644 --- a/rtic-time/src/lib.rs +++ b/rtic-time/src/lib.rs @@ -181,22 +181,36 @@ impl TimerQueue { } } - /// Timeout after a specific duration. + /// Timeout after at least a specific duration. #[inline] pub async fn timeout_after( &self, duration: Mono::Duration, future: F, ) -> Result { - self.timeout_at(Mono::now() + duration, future).await + let now = Mono::now(); + let mut timeout = now + duration; + if now != timeout { + timeout = timeout + Mono::TICK_PERIOD; + } + + // Wait for one period longer, because by definition timers have an uncertainty + // of one period, so waiting for 'at least' needs to compensate for that. + self.timeout_at(timeout, future).await } - /// Delay for some duration of time. + /// Delay for at least some duration of time. #[inline] pub async fn delay(&self, duration: Mono::Duration) { let now = Mono::now(); + let mut timeout = now + duration; + if now != timeout { + timeout = timeout + Mono::TICK_PERIOD; + } - self.delay_until(now + duration).await; + // Wait for one period longer, because by definition timers have an uncertainty + // of one period, so waiting for 'at least' needs to compensate for that. + self.delay_until(timeout).await; } /// Delay to some specific time instant. diff --git a/rtic-time/src/monotonic.rs b/rtic-time/src/monotonic.rs index ad79bf2053..0c0d6f352a 100644 --- a/rtic-time/src/monotonic.rs +++ b/rtic-time/src/monotonic.rs @@ -10,6 +10,9 @@ pub trait Monotonic { /// The time at time zero. const ZERO: Self::Instant; + /// The duration between two timer ticks. + const TICK_PERIOD: Self::Duration; + /// The type for instant, defining an instant in time. /// /// **Note:** In all APIs in RTIC that use instants from this monotonic, this type will be used. @@ -65,3 +68,153 @@ pub trait Monotonic { /// NOTE: This may be called more than once. fn disable_timer() {} } + +/// Creates impl blocks for `embedded_hal::delay::DelayUs`, +/// based on `fugit::ExtU64Ceil`. +#[macro_export] +macro_rules! embedded_hal_delay_impl_fugit64 { + ($t:ty) => { + impl ::embedded_hal::delay::DelayNs for $t { + fn delay_ns(&mut self, ns: u32) { + use ::fugit::ExtU64Ceil; + + let now = Self::now(); + let mut done = now + u64::from(ns).nanos_at_least(); + if now != done { + // Compensate for sub-tick uncertainty + done += Self::TICK_PERIOD; + } + + while Self::now() < done {} + } + + fn delay_us(&mut self, us: u32) { + use ::fugit::ExtU64Ceil; + + let now = Self::now(); + let mut done = now + u64::from(us).micros_at_least(); + if now != done { + // Compensate for sub-tick uncertainty + done += Self::TICK_PERIOD; + } + + while Self::now() < done {} + } + + fn delay_ms(&mut self, ms: u32) { + use ::fugit::ExtU64Ceil; + + let now = Self::now(); + let mut done = now + u64::from(ms).millis_at_least(); + if now != done { + // Compensate for sub-tick uncertainty + done += Self::TICK_PERIOD; + } + + while Self::now() < done {} + } + } + }; +} + +/// Creates impl blocks for `embedded_hal_async::delay::DelayUs`, +/// based on `fugit::ExtU64Ceil`. +#[macro_export] +macro_rules! embedded_hal_async_delay_impl_fugit64 { + ($t:ty) => { + impl ::embedded_hal_async::delay::DelayNs for $t { + #[inline] + async fn delay_ns(&mut self, ns: u32) { + use ::fugit::ExtU64Ceil; + Self::delay(u64::from(ns).nanos_at_least()).await; + } + + #[inline] + async fn delay_us(&mut self, us: u32) { + use ::fugit::ExtU64Ceil; + Self::delay(u64::from(us).micros_at_least()).await; + } + + #[inline] + async fn delay_ms(&mut self, ms: u32) { + use ::fugit::ExtU64Ceil; + Self::delay(u64::from(ms).millis_at_least()).await; + } + } + }; +} + +/// Creates impl blocks for `embedded_hal::delay::DelayUs`, +/// based on `fugit::ExtU32Ceil`. +#[macro_export] +macro_rules! embedded_hal_delay_impl_fugit32 { + ($t:ty) => { + impl ::embedded_hal::delay::DelayNs for $t { + fn delay_ns(&mut self, ns: u32) { + use ::fugit::ExtU32Ceil; + + let now = Self::now(); + let mut done = now + ns.nanos_at_least(); + if now != done { + // Compensate for sub-tick uncertainty + done += Self::TICK_PERIOD; + } + + while Self::now() < done {} + } + + fn delay_us(&mut self, us: u32) { + use ::fugit::ExtU32Ceil; + + let now = Self::now(); + let mut done = now + us.micros_at_least(); + if now != done { + // Compensate for sub-tick uncertainty + done += Self::TICK_PERIOD; + } + + while Self::now() < done {} + } + + fn delay_ms(&mut self, ms: u32) { + use ::fugit::ExtU32Ceil; + + let now = Self::now(); + let mut done = now + ms.millis_at_least(); + if now != done { + // Compensate for sub-tick uncertainty + done += Self::TICK_PERIOD; + } + + while Self::now() < done {} + } + } + }; +} + +/// Creates impl blocks for `embedded_hal_async::delay::DelayUs`, +/// based on `fugit::ExtU32Ceil`. +#[macro_export] +macro_rules! embedded_hal_async_delay_impl_fugit32 { + ($t:ty) => { + impl ::embedded_hal_async::delay::DelayNs for $t { + #[inline] + async fn delay_ns(&mut self, ns: u32) { + use ::fugit::ExtU32Ceil; + Self::delay(ns.nanos_at_least()).await; + } + + #[inline] + async fn delay_us(&mut self, us: u32) { + use ::fugit::ExtU32Ceil; + Self::delay(us.micros_at_least()).await; + } + + #[inline] + async fn delay_ms(&mut self, ms: u32) { + use ::fugit::ExtU32Ceil; + Self::delay(ms.millis_at_least()).await; + } + } + }; +} diff --git a/rtic-time/tests/delay_precision_subtick.rs b/rtic-time/tests/delay_precision_subtick.rs new file mode 100644 index 0000000000..4db889c858 --- /dev/null +++ b/rtic-time/tests/delay_precision_subtick.rs @@ -0,0 +1,313 @@ +//! A test that verifies the sub-tick correctness of the [`TimerQueue`]'s `delay` functionality. +//! +//! To run this test, you need to activate the `critical-section/std` feature. + +use std::{ + fmt::Debug, + future::Future, + pin::Pin, + sync::{ + atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}, + Arc, + }, + task::Context, + thread::sleep, + time::Duration, +}; + +use ::fugit::ExtU64Ceil; +use cooked_waker::{IntoWaker, WakeRef}; +use parking_lot::Mutex; +use rtic_time::{Monotonic, TimeoutError, TimerQueue}; + +const SUBTICKS_PER_TICK: u32 = 10; +struct SubtickTestTimer; +static TIMER_QUEUE: TimerQueue = TimerQueue::new(); +static NOW_SUBTICKS: AtomicU64 = AtomicU64::new(0); +static COMPARE_TICKS: Mutex> = Mutex::new(None); + +impl Monotonic for SubtickTestTimer { + const ZERO: Self::Instant = Self::Instant::from_ticks(0); + const TICK_PERIOD: Self::Duration = Self::Duration::from_ticks(1); + + type Instant = fugit::Instant; + type Duration = fugit::Duration; + + fn now() -> Self::Instant { + Self::Instant::from_ticks( + NOW_SUBTICKS.load(Ordering::Relaxed) / u64::from(SUBTICKS_PER_TICK), + ) + } + + fn set_compare(instant: Self::Instant) { + *COMPARE_TICKS.lock() = Some(instant.ticks()); + } + + fn clear_compare_flag() {} + + fn pend_interrupt() { + unsafe { + Self::__tq().on_monotonic_interrupt(); + } + } +} + +impl SubtickTestTimer { + pub fn init() { + Self::__tq().initialize(Self) + } + + pub fn tick() -> u64 { + let now = NOW_SUBTICKS.fetch_add(1, Ordering::Relaxed) + 1; + let ticks = now / u64::from(SUBTICKS_PER_TICK); + let subticks = now % u64::from(SUBTICKS_PER_TICK); + + let compare = COMPARE_TICKS.lock(); + + // println!( + // "ticks: {ticks}, subticks: {subticks}, compare: {:?}", + // *compare + // ); + if subticks == 0 && Some(ticks) == *compare { + unsafe { + Self::__tq().on_monotonic_interrupt(); + } + } + + subticks + } + + pub fn forward_to_subtick(subtick: u64) { + assert!(subtick < u64::from(SUBTICKS_PER_TICK)); + while Self::tick() != subtick {} + } + + pub fn now_subticks() -> u64 { + NOW_SUBTICKS.load(Ordering::Relaxed) + } + + fn __tq() -> &'static TimerQueue { + &TIMER_QUEUE + } + + /// Delay for some duration of time. + #[inline] + pub async fn delay(duration: ::Duration) { + Self::__tq().delay(duration).await; + } + + /// Timeout after a specific duration. + #[inline] + pub async fn timeout_after( + duration: ::Duration, + future: F, + ) -> Result { + Self::__tq().timeout_after(duration, future).await + } +} + +rtic_time::embedded_hal_delay_impl_fugit64!(SubtickTestTimer); +rtic_time::embedded_hal_async_delay_impl_fugit64!(SubtickTestTimer); + +// A simple struct that counts the number of times it is awoken. Can't +// be awoken by value (because that would discard the counter), so we +// must instead wrap it in an Arc. +#[derive(Debug, Default)] +struct WakeCounter { + count: AtomicUsize, +} + +impl WakeCounter { + fn get(&self) -> usize { + self.count.load(Ordering::SeqCst) + } +} + +impl WakeRef for WakeCounter { + fn wake_by_ref(&self) { + let _prev = self.count.fetch_add(1, Ordering::SeqCst); + } +} + +struct OnDrop(Option); +impl OnDrop { + pub fn new(f: F) -> Self { + Self(Some(f)) + } +} +impl Drop for OnDrop { + fn drop(&mut self) { + (self.0.take().unwrap())(); + } +} + +macro_rules! subtick_test { + (@run $start:expr, $actual_duration:expr, $delay_fn:expr) => {{ + // forward clock to $start + SubtickTestTimer::forward_to_subtick($start); + + // call wait function + let delay_fn = $delay_fn; + let mut future = std::pin::pin!(delay_fn); + + let wakecounter = Arc::new(WakeCounter::default()); + let waker = Arc::clone(&wakecounter).into_waker(); + let mut context = Context::from_waker(&waker); + + let mut finished_after: Option = None; + for i in 0..10 * u64::from(SUBTICKS_PER_TICK) { + if Future::poll(Pin::new(&mut future), &mut context).is_ready() { + if finished_after.is_none() { + finished_after = Some(i); + } + break; + }; + + assert_eq!(wakecounter.get(), 0); + SubtickTestTimer::tick(); + } + + let expected_wakeups = { + if $actual_duration == 0 { + 0 + } else { + 1 + } + }; + assert_eq!(wakecounter.get(), expected_wakeups); + + // Tick again to test that we don't get a second wake + SubtickTestTimer::tick(); + assert_eq!(wakecounter.get(), expected_wakeups); + + assert_eq!( + Some($actual_duration), + finished_after, + "Expected to wait {} ticks, but waited {:?} ticks.", + $actual_duration, + finished_after, + ); + }}; + + (@run_blocking $start:expr, $actual_duration:expr, $delay_fn:expr) => {{ + // forward clock to $start + SubtickTestTimer::forward_to_subtick($start); + + let t_start = SubtickTestTimer::now_subticks(); + + let finished = AtomicBool::new(false); + std::thread::scope(|s|{ + s.spawn(||{ + let _finished_guard = OnDrop::new(|| finished.store(true, Ordering::Relaxed)); + ($delay_fn)(); + }); + s.spawn(||{ + sleep(Duration::from_millis(10)); + while !finished.load(Ordering::Relaxed) { + SubtickTestTimer::tick(); + sleep(Duration::from_millis(10)); + } + }); + }); + + let t_end = SubtickTestTimer::now_subticks(); + let measured_duration = t_end - t_start; + assert_eq!( + $actual_duration, + measured_duration, + "Expected to wait {} ticks, but waited {:?} ticks.", + $actual_duration, + measured_duration, + ); + }}; + + + + + ($start:expr, $min_duration:expr, $actual_duration:expr) => {{ + subtick_test!(@run $start, $actual_duration, async { + let mut timer = SubtickTestTimer; + embedded_hal_async::delay::DelayNs::delay_ms(&mut timer, $min_duration).await; + }); + subtick_test!(@run $start, $actual_duration, async { + let mut timer = SubtickTestTimer; + embedded_hal_async::delay::DelayNs::delay_us(&mut timer, 1_000 * $min_duration).await; + }); + subtick_test!(@run $start, $actual_duration, async { + let mut timer = SubtickTestTimer; + embedded_hal_async::delay::DelayNs::delay_ns(&mut timer, 1_000_000 * $min_duration).await; + }); + subtick_test!(@run $start, $actual_duration, async { + SubtickTestTimer::delay($min_duration.millis_at_least()).await; + }); + subtick_test!(@run $start, $actual_duration, async { + let _ = SubtickTestTimer::timeout_after($min_duration.millis_at_least(), std::future::pending::<()>()).await; + }); + + // Those are slow and unreliable; enable them when needed. + const ENABLE_BLOCKING_TESTS: bool = false; + if ENABLE_BLOCKING_TESTS { + subtick_test!(@run_blocking $start, $actual_duration, || { + let mut timer = SubtickTestTimer; + embedded_hal::delay::DelayNs::delay_ms(&mut timer, $min_duration); + }); + subtick_test!(@run_blocking $start, $actual_duration, || { + let mut timer = SubtickTestTimer; + embedded_hal::delay::DelayNs::delay_us(&mut timer, 1_000 * $min_duration); + }); + subtick_test!(@run_blocking $start, $actual_duration, || { + let mut timer = SubtickTestTimer; + embedded_hal::delay::DelayNs::delay_ns(&mut timer, 1_000_000 * $min_duration); + }); + } + }}; +} + +#[test] +fn timer_queue_subtick_precision() { + SubtickTestTimer::init(); + + // subtick_test!(a, b, c) tests the following thing: + // + // If we start at subtick a and we need to wait b subticks, + // then we will actually wait c subticks. + // The important part is that c is never smaller than b, + // in all cases, as that would violate the contract of + // embedded-hal's DelayUs. + + subtick_test!(0, 0, 0); + subtick_test!(0, 1, 20); + subtick_test!(0, 10, 20); + subtick_test!(0, 11, 30); + subtick_test!(0, 12, 30); + + subtick_test!(1, 0, 0); + subtick_test!(1, 1, 19); + subtick_test!(1, 10, 19); + subtick_test!(1, 11, 29); + subtick_test!(1, 12, 29); + + subtick_test!(2, 0, 0); + subtick_test!(2, 1, 18); + subtick_test!(2, 10, 18); + subtick_test!(2, 11, 28); + subtick_test!(2, 12, 28); + + subtick_test!(3, 0, 0); + subtick_test!(3, 1, 17); + subtick_test!(3, 10, 17); + subtick_test!(3, 11, 27); + subtick_test!(3, 12, 27); + + subtick_test!(8, 0, 0); + subtick_test!(8, 1, 12); + subtick_test!(8, 10, 12); + subtick_test!(8, 11, 22); + subtick_test!(8, 12, 22); + + subtick_test!(9, 0, 0); + subtick_test!(9, 1, 11); + subtick_test!(9, 10, 11); + subtick_test!(9, 11, 21); + subtick_test!(9, 12, 21); +} diff --git a/rtic-time/tests/timer_queue.rs b/rtic-time/tests/timer_queue.rs index 9ad717574f..8bae385355 100644 --- a/rtic-time/tests/timer_queue.rs +++ b/rtic-time/tests/timer_queue.rs @@ -17,7 +17,7 @@ static NOW: Mutex> = Mutex::new(None); pub struct Duration(u64); impl Duration { - pub fn from_ticks(millis: u64) -> Self { + pub const fn from_ticks(millis: u64) -> Self { Self(millis) } @@ -161,6 +161,7 @@ impl TestMono { impl Monotonic for TestMono { const ZERO: Self::Instant = Instant::ZERO; + const TICK_PERIOD: Self::Duration = Duration::from_ticks(1); type Instant = Instant; @@ -210,7 +211,8 @@ fn timer_queue() { let elapsed = start.elapsed().as_ticks(); println!("{total_millis} ticks delay reached after {elapsed} ticks"); - if elapsed != total_millis { + // Expect a delay of one longer, to compensate for timer uncertainty + if elapsed != total_millis + 1 { panic!( "{total_millis} ticks delay was not on time ({elapsed} ticks passed instead)" ); @@ -263,25 +265,25 @@ fn timer_queue() { if Instant::now() == 0.into() { // First, we want to be waiting for our 300 tick delay - assert_eq!(TestMono::compare(), Some(300.into())); + assert_eq!(TestMono::compare(), Some(301.into())); } if Instant::now() == 100.into() { // After 100 ticks, we enqueue a new delay that is supposed to last // until the 200-tick-mark - assert_eq!(TestMono::compare(), Some(200.into())); + assert_eq!(TestMono::compare(), Some(201.into())); } - if Instant::now() == 200.into() { + if Instant::now() == 201.into() { // After 200 ticks, we dequeue the 200-tick-mark delay and // requeue the 300 tick delay - assert_eq!(TestMono::compare(), Some(300.into())); + assert_eq!(TestMono::compare(), Some(301.into())); } - if Instant::now() == 300.into() { + if Instant::now() == 301.into() { // After 300 ticks, we dequeue the 300-tick-mark delay and // go to the 400 tick delay that is already enqueued - assert_eq!(TestMono::compare(), Some(400.into())); + assert_eq!(TestMono::compare(), Some(401.into())); } } diff --git a/rtic/CHANGELOG.md b/rtic/CHANGELOG.md index db90aa3ecb..815b1bb0df 100644 --- a/rtic/CHANGELOG.md +++ b/rtic/CHANGELOG.md @@ -8,10 +8,14 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top! ## [Unreleased] ### Added + - Unstable support for ESP32-C3 ### Fixed +- **Soundness fix:** Monotonics did not wait long enough in `Duration` based delays. + This is not directly a change for `rtic`, but required bumping the minimal version of `rtic-monotonics`. + ### Changed ## [v2.0.1] - 2023-07-25 diff --git a/rtic/ci/expected/async-timeout.run b/rtic/ci/expected/async-timeout.run index dea8e5d2cc..6b4c0898b1 100644 --- a/rtic/ci/expected/async-timeout.run +++ b/rtic/ci/expected/async-timeout.run @@ -5,12 +5,12 @@ the hal takes a duration of Duration { ticks: 45 } hal returned 5 the hal takes a duration of Duration { ticks: 45 } hal returned 5 -now is Instant { ticks: 210 }, timeout at Instant { ticks: 260 } +now is Instant { ticks: 213 }, timeout at Instant { ticks: 263 } the hal takes a duration of Duration { ticks: 35 } -hal returned 5 at time Instant { ticks: 245 } -now is Instant { ticks: 310 }, timeout at Instant { ticks: 360 } +hal returned 5 at time Instant { ticks: 249 } +now is Instant { ticks: 313 }, timeout at Instant { ticks: 363 } the hal takes a duration of Duration { ticks: 45 } -hal returned 5 at time Instant { ticks: 355 } -now is Instant { ticks: 410 }, timeout at Instant { ticks: 460 } +hal returned 5 at time Instant { ticks: 359 } +now is Instant { ticks: 413 }, timeout at Instant { ticks: 463 } the hal takes a duration of Duration { ticks: 55 } timeout