From b857334f92fc188004567edb93e0d1dfce4c259e Mon Sep 17 00:00:00 2001
From: RobertTDowling <roberttdowling@gmail.com>
Date: Sun, 17 Dec 2023 15:11:03 -0800
Subject: [PATCH] STM32: Fix race in alarm setting, which impacted scheduling.

Detect potential race condition (should be rare) and return false back
to caller, allowing them to handle the possibility that either the
alarm was never set because it was in the past (old meaning of false),
or that in fact the alarm was set and may have fired within the race
window (new meaning of false). In either case, the caller needs to
make sure the callback got called.
---
 embassy-stm32/src/time_driver.rs | 19 ++++++++++++++++---
 embassy-time/src/driver.rs       |  4 ++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/embassy-stm32/src/time_driver.rs b/embassy-stm32/src/time_driver.rs
index ea9c22d8..9981800b 100644
--- a/embassy-stm32/src/time_driver.rs
+++ b/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
         })
     }
diff --git a/embassy-time/src/driver.rs b/embassy-time/src/driver.rs
index 5fe7beca..81ee1b0f 100644
--- a/embassy-time/src/driver.rs
+++ b/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.
     ///