Merge pull request #2300 from RobertTDowling/stm32-fix-time-driver-race

STM32: Fix race in alarm setting, which impacted scheduling.

embassy-stm32/src/time_driver.rs

@@ -474,16 +474,29 @@ impl Driver for RtcDriver {
                 return false;
             }
 
-            let safe_timestamp = timestamp.max(t + 3);
-
             // Write the CCR value regardless of whether we're going to enable it now or not.
             // This way, when we enable it later, the right value is already set.
-            r.ccr(n + 1).write(|w| w.set_ccr(safe_timestamp as u16));
+            r.ccr(n + 1).write(|w| w.set_ccr(timestamp as u16));
 
             // Enable it if it'll happen soon. Otherwise, `next_period` will enable it.
             let diff = timestamp - t;
             r.dier().modify(|w| w.set_ccie(n + 1, diff < 0xc000));
 
+            // Reevaluate if the alarm timestamp is still in the future
+            let t = self.now();
+            if timestamp <= t {
+                // If alarm timestamp has passed since we set it, we have a race condition and
+                // the alarm may or may not have fired.
+                // Disarm the alarm and return `false` to indicate that.
+                // It is the caller's responsibility to handle this ambiguity.
+                r.dier().modify(|w| w.set_ccie(n + 1, false));
+
+                alarm.timestamp.set(u64::MAX);
+
+                return false;
+            }
+
+            // We're confident the alarm will ring in the future.
             true
         })
     }

embassy-time/src/driver.rs

@@ -108,6 +108,10 @@ pub trait Driver: Send + Sync + 'static {
     /// The `Driver` implementation should guarantee that the alarm callback is never called synchronously from `set_alarm`.
     /// Rather - if `timestamp` is already in the past - `false` should be returned and alarm should not be set,
     /// or alternatively, the driver should return `true` and arrange to call the alarm callback as soon as possible, but not synchronously.
+    /// There is a rare third possibility that the alarm was barely in the future, and by the time it was enabled, it had slipped into the
+    /// past.  This is can be detected by double-checking that the alarm is still in the future after enabling it; if it isn't, `false`
+    /// should also be returned to indicate that the callback may have been called already by the alarm, but it is not guaranteed, so the
+    /// caller should also call the callback, just like in the more common `false` case. (Note: This requires idempotency of the callback.)
     ///
     /// When callback is called, it is guaranteed that now() will return a value greater or equal than timestamp.
     ///