From e012e1385814647e1c17cb64175a19745c18bb8f Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Wed, 1 Jan 2025 19:47:39 +0100 Subject: [PATCH 1/5] Ensure alarm is re-scheduled if timetamp is in the past Fixes #3672 --- embassy-nrf/src/time_driver.rs | 82 ++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/embassy-nrf/src/time_driver.rs b/embassy-nrf/src/time_driver.rs index ade6fd2a1..01079e26a 100644 --- a/embassy-nrf/src/time_driver.rs +++ b/embassy-nrf/src/time_driver.rs @@ -209,47 +209,53 @@ impl RtcDriver { let r = rtc(); - let t = self.now(); - if timestamp <= t { - // If alarm timestamp has passed the alarm will not fire. - // Disarm the alarm and return `false` to indicate that. - r.intenclr().write(|w| w.0 = compare_n(n)); + loop { + let t = self.now(); + if timestamp <= t { + // If alarm timestamp has passed the alarm will not fire. + // Disarm the alarm and return `false` to indicate that. + r.intenclr().write(|w| w.0 = compare_n(n)); - alarm.timestamp.set(u64::MAX); + alarm.timestamp.set(u64::MAX); - return false; + return false; + } + + // If it hasn't triggered yet, setup it in the compare channel. + + // Write the CC 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. + + // nrf52 docs say: + // If the COUNTER is N, writing N or N+1 to a CC register may not trigger a COMPARE event. + // To workaround this, we never write a timestamp smaller than N+3. + // N+2 is not safe because rtc can tick from N to N+1 between calling now() and writing cc. + // + // It is impossible for rtc to tick more than once because + // - this code takes less time than 1 tick + // - it runs with interrupts disabled so nothing else can preempt it. + // + // This means that an alarm can be delayed for up to 2 ticks (from t+1 to t+3), but this is allowed + // by the Alarm trait contract. What's not allowed is triggering alarms *before* their scheduled time, + // and we don't do that here. + let safe_timestamp = timestamp.max(t + 3); + r.cc(n).write(|w| w.set_compare(safe_timestamp as u32 & 0xFFFFFF)); + + let diff = timestamp - t; + if diff < 0xc00000 { + r.intenset().write(|w| w.0 = compare_n(n)); + } else { + // If it's too far in the future, don't setup the compare channel yet. + // It will be setup later by `next_period`. + r.intenclr().write(|w| w.0 = compare_n(n)); + } + + // If we have not passed the safe timestamp, we can be sure the alarm will be invoked. Otherwise, + // we need to retry setting the alarm. + if self.now() <= safe_timestamp { + return true; + } } - - // If it hasn't triggered yet, setup it in the compare channel. - - // Write the CC 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. - - // nrf52 docs say: - // If the COUNTER is N, writing N or N+1 to a CC register may not trigger a COMPARE event. - // To workaround this, we never write a timestamp smaller than N+3. - // N+2 is not safe because rtc can tick from N to N+1 between calling now() and writing cc. - // - // It is impossible for rtc to tick more than once because - // - this code takes less time than 1 tick - // - it runs with interrupts disabled so nothing else can preempt it. - // - // This means that an alarm can be delayed for up to 2 ticks (from t+1 to t+3), but this is allowed - // by the Alarm trait contract. What's not allowed is triggering alarms *before* their scheduled time, - // and we don't do that here. - let safe_timestamp = timestamp.max(t + 3); - r.cc(n).write(|w| w.set_compare(safe_timestamp as u32 & 0xFFFFFF)); - - let diff = timestamp - t; - if diff < 0xc00000 { - r.intenset().write(|w| w.0 = compare_n(n)); - } else { - // If it's too far in the future, don't setup the compare channel yet. - // It will be setup later by `next_period`. - r.intenclr().write(|w| w.0 = compare_n(n)); - } - - true } } From 9c7d508bcf3384ab0883c7ef710e485106ccbb6c Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 2 Jan 2025 11:59:57 +0100 Subject: [PATCH 2/5] Fix case where a short interrupt caused wrong alarm set --- embassy-nrf/src/time_driver.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/embassy-nrf/src/time_driver.rs b/embassy-nrf/src/time_driver.rs index 01079e26a..ad61f5ca7 100644 --- a/embassy-nrf/src/time_driver.rs +++ b/embassy-nrf/src/time_driver.rs @@ -250,9 +250,9 @@ impl RtcDriver { r.intenclr().write(|w| w.0 = compare_n(n)); } - // If we have not passed the safe timestamp, we can be sure the alarm will be invoked. Otherwise, + // If we have not passed the timestamp, we can be sure the alarm will be invoked. Otherwise, // we need to retry setting the alarm. - if self.now() <= safe_timestamp { + if self.now() + 3 <= timestamp { return true; } } From 06901c3279430a40a449e5944e8ba3788388deb7 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 2 Jan 2025 12:03:03 +0100 Subject: [PATCH 3/5] Update comment to reflect reality --- embassy-nrf/src/time_driver.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/embassy-nrf/src/time_driver.rs b/embassy-nrf/src/time_driver.rs index ad61f5ca7..b8c19014e 100644 --- a/embassy-nrf/src/time_driver.rs +++ b/embassy-nrf/src/time_driver.rs @@ -231,9 +231,9 @@ impl RtcDriver { // To workaround this, we never write a timestamp smaller than N+3. // N+2 is not safe because rtc can tick from N to N+1 between calling now() and writing cc. // - // It is impossible for rtc to tick more than once because - // - this code takes less time than 1 tick - // - it runs with interrupts disabled so nothing else can preempt it. + // Since the critical section does not guarantee that a higher prio interrupt causes + // this to be delayed, we need to re-check how much time actually passed after setting the + // alarm, and retry if we are within the unsafe interval still. // // This means that an alarm can be delayed for up to 2 ticks (from t+1 to t+3), but this is allowed // by the Alarm trait contract. What's not allowed is triggering alarms *before* their scheduled time, From 422938745a03ae36ffc058ff28011fe15dff2d00 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 2 Jan 2025 12:53:55 +0100 Subject: [PATCH 4/5] Move safeguard if compare irq is enabled --- embassy-nrf/src/time_driver.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/embassy-nrf/src/time_driver.rs b/embassy-nrf/src/time_driver.rs index b8c19014e..61ae96718 100644 --- a/embassy-nrf/src/time_driver.rs +++ b/embassy-nrf/src/time_driver.rs @@ -244,15 +244,16 @@ impl RtcDriver { let diff = timestamp - t; if diff < 0xc00000 { r.intenset().write(|w| w.0 = compare_n(n)); + + // If we have not passed the timestamp, we can be sure the alarm will be invoked. Otherwise, + // we need to retry setting the alarm. + if self.now() + 3 <= timestamp { + return true; + } } else { // If it's too far in the future, don't setup the compare channel yet. // It will be setup later by `next_period`. r.intenclr().write(|w| w.0 = compare_n(n)); - } - - // If we have not passed the timestamp, we can be sure the alarm will be invoked. Otherwise, - // we need to retry setting the alarm. - if self.now() + 3 <= timestamp { return true; } } From 92d67c3ccda06fecf5864a90fe1a74be8495bd36 Mon Sep 17 00:00:00 2001 From: Ulf Lilleengen Date: Thu, 2 Jan 2025 13:35:55 +0100 Subject: [PATCH 5/5] Relax timestamp check --- embassy-nrf/src/time_driver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/embassy-nrf/src/time_driver.rs b/embassy-nrf/src/time_driver.rs index 61ae96718..03f4c2e2b 100644 --- a/embassy-nrf/src/time_driver.rs +++ b/embassy-nrf/src/time_driver.rs @@ -247,7 +247,7 @@ impl RtcDriver { // If we have not passed the timestamp, we can be sure the alarm will be invoked. Otherwise, // we need to retry setting the alarm. - if self.now() + 3 <= timestamp { + if self.now() + 2 <= timestamp { return true; } } else {