Fix race condition in calculate_now (#860)

* Fix race condition in calculate_now

* Add changelog

* Update changelog

* Refine comment

* More comment fixes

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
 

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)
                 ))
             }

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())
                 ))
             }

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()

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()
                 ))
             }

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

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<P, T, F, O>(half_periods: P, timer_value: F) -> O
+pub fn calculate_now<P, T, F1, F2, O>(half_periods: F1, timer_value: F2) -> O
 where
     T: TimerValue,
     O: From<P> + From<T> + 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`.
     //

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: {}",