diff --git a/rtic-monotonics/CHANGELOG.md b/rtic-monotonics/CHANGELOG.md index 041cff974c..e1a0f83baa 100644 --- a/rtic-monotonics/CHANGELOG.md +++ b/rtic-monotonics/CHANGELOG.md @@ -13,6 +13,7 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top! - Fix race condition in `nrf::rtc`. - Fix errata in `nrf::rtc`. - Add internal counter integrity check to all half-period based monotonics. +- Apply race condition fixes from `rtic-time`. ## v1.4.0 - 2023-12-04 diff --git a/rtic-monotonics/src/imxrt.rs b/rtic-monotonics/src/imxrt.rs index ecf9129b70..2299beac46 100644 --- a/rtic-monotonics/src/imxrt.rs +++ b/rtic-monotonics/src/imxrt.rs @@ -212,7 +212,7 @@ macro_rules! make_timer { let gpt = unsafe{ $timer::instance() }; Self::Instant::from_ticks(calculate_now( - $period.load(Ordering::Relaxed), + || $period.load(Ordering::Relaxed), || ral::read_reg!(ral::gpt, gpt, CNT) )) } diff --git a/rtic-monotonics/src/nrf/rtc.rs b/rtic-monotonics/src/nrf/rtc.rs index 643d8bdb1c..d425b11481 100644 --- a/rtic-monotonics/src/nrf/rtc.rs +++ b/rtic-monotonics/src/nrf/rtc.rs @@ -224,7 +224,7 @@ macro_rules! make_rtc { fn now() -> Self::Instant { let rtc = unsafe { &*$rtc::PTR }; Self::Instant::from_ticks(calculate_now( - $overflow.load(Ordering::Relaxed), + || $overflow.load(Ordering::Relaxed), || TimerValueU24(rtc.counter.read().bits()) )) } diff --git a/rtic-monotonics/src/nrf/timer.rs b/rtic-monotonics/src/nrf/timer.rs index 2f83f40b68..7b760e4c2b 100644 --- a/rtic-monotonics/src/nrf/timer.rs +++ b/rtic-monotonics/src/nrf/timer.rs @@ -242,7 +242,7 @@ macro_rules! make_timer { let timer = unsafe { &*$timer::PTR }; Self::Instant::from_ticks(calculate_now( - $overflow.load(Ordering::Relaxed), + || $overflow.load(Ordering::Relaxed), || { timer.tasks_capture[3].write(|w| unsafe { w.bits(1) }); timer.cc[3].read().bits() diff --git a/rtic-monotonics/src/stm32.rs b/rtic-monotonics/src/stm32.rs index 601196a3d5..68f95a25d3 100644 --- a/rtic-monotonics/src/stm32.rs +++ b/rtic-monotonics/src/stm32.rs @@ -234,7 +234,7 @@ macro_rules! make_timer { fn now() -> Self::Instant { Self::Instant::from_ticks(calculate_now( - $overflow.load(Ordering::Relaxed), + || $overflow.load(Ordering::Relaxed), || $timer.cnt().read().cnt() )) } diff --git a/rtic-time/CHANGELOG.md b/rtic-time/CHANGELOG.md index 8a99633aa8..24b9322717 100644 --- a/rtic-time/CHANGELOG.md +++ b/rtic-time/CHANGELOG.md @@ -16,6 +16,9 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top! ### Fixed +- Fix race condition in `half_period_counter::calculate_now`. + This sadly required a minor API change. + ## v1.1.0 - 2023-12-04 ### Added diff --git a/rtic-time/src/half_period_counter.rs b/rtic-time/src/half_period_counter.rs index 0aaec5e2f8..753b75aba1 100644 --- a/rtic-time/src/half_period_counter.rs +++ b/rtic-time/src/half_period_counter.rs @@ -108,7 +108,7 @@ //! //! fn now() -> u64 { //! rtic_time::half_period_counter::calculate_now( -//! HALF_PERIOD_COUNTER.load(Ordering::Relaxed), +//! || HALF_PERIOD_COUNTER.load(Ordering::Relaxed), //! || timer_get_value(), //! ) //! } @@ -191,19 +191,27 @@ impl_timer_ops!(u128); /// /// # Arguments /// -/// * `half_periods` - The period counter value. If read from an atomic, can use `Ordering::Relaxed`. +/// * `half_periods` - A closure/function that when called produces the period counter value. If read from an atomic, can use `Ordering::Relaxed`. /// * `timer_value` - A closure/function that when called produces the current timer value. -pub fn calculate_now(half_periods: P, timer_value: F) -> O +pub fn calculate_now(half_periods: F1, timer_value: F2) -> O where T: TimerValue, O: From

+ From + TimerOps, - F: FnOnce() -> T, + F1: FnOnce() -> P, + F2: FnOnce() -> T, { - // Important: half_period **must** be read first. - // Otherwise we have another mathematical race condition. - let half_periods = O::from(half_periods); - compiler_fence(Ordering::Acquire); - let timer_value = O::from(timer_value()); + // This is timing critical; for fast-overflowing timers (like the 1MHz 16-bit timers on STM32), + // it could lead to erroneous behavior if preempted in between the two reads. + // Hence the critical section. + let (half_periods, timer_value) = critical_section::with(|_| { + // Important: half_periods **must** be read first. + // Otherwise the mathematical principle that prevents + // the race condition does not work. + let half_periods = O::from(half_periods()); + compiler_fence(Ordering::Acquire); + let timer_value = O::from(timer_value()); + (half_periods, timer_value) + }); // Credits to the `time-driver` of `embassy-stm32`. // diff --git a/rtic-time/tests/half_period_time_counter.rs b/rtic-time/tests/half_period_time_counter.rs index ceffbea2bc..ee501b431e 100644 --- a/rtic-time/tests/half_period_time_counter.rs +++ b/rtic-time/tests/half_period_time_counter.rs @@ -5,7 +5,7 @@ macro_rules! do_test_u8 { let periods: u32 = $periods; let counter: u8 = $counter; let expected: u32 = $expected; - let actual: u32 = calculate_now(periods, || counter); + let actual: u32 = calculate_now(|| periods, || counter); assert_eq!( actual, expected, "Expected: ({} | {}) => {}, got: {}", @@ -19,7 +19,7 @@ macro_rules! do_test_u16 { let periods: u16 = $periods; let counter: u16 = $counter; let expected: u32 = $expected; - let actual: u32 = calculate_now(periods, || counter); + let actual: u32 = calculate_now(|| periods, || counter); assert_eq!( actual, expected, "Expected: ({} | {}) => {}, got: {}",