Ensure alarm is re-scheduled if timetamp is in the past #3705
Conversation
| // 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)); |
There was a problem hiding this comment.
Let's say an interrupt happens before this line, and when this code regains control self.now() equals to safe_timestamp-1. That means you'll set an alarm to a too-soon timestamp and you'll not know about it still.
There was a problem hiding this comment.
@bugadani Thanks for the review! I'm not sure if I'm understanding the scenario you describe here fully. I've tried to annotate the code with the situation you're describing (removing the timer intnset/intnclr lines), but it seems to me that the check at the end will catch this situation. Maybe my assumptions around now()/counter incrementing during the irq is wrong...
First is the case I think you are describing where self.now() == safe_timestamp - 1 (i.e. fast irq)
let t = self.now();
timestamp = 10
t = 0
ticks = 0
if timestamp <= t {
...
return false;
}
ticks = 1
let safe_timestamp = timestamp.max(t + 3);
safe_timestamp = 13
IRQ
ticks = 12
r.cc(n).write(|w| w.set_compare(safe_timestamp as u32 & 0xFFFFFF));
ticks = 12 or 13:
12 <= 13 => alarm will be invoked so we return
if self.now() <= safe_timestamp {
return true;
}
Second is the case where now() moves past safe_timestamp:
let t = self.now();
timestamp = 10
t = 0
ticks = 0
if timestamp <= t {
...
return false;
}
ticks = 1
let safe_timestamp = timestamp.max(t + 3);
safe_timestamp = 13
IRQ
ticks = 12
r.cc(n).write(|w| w.set_compare(safe_timestamp as u32 & 0xFFFFFF));
ticks >= 14
14 <= 13 => loop is retried
if self.now() <= safe_timestamp {
return true;
}
There was a problem hiding this comment.
Ok, we found the case:
let t = self.now();
timestamp = 10
t = 7
ticks = 7
if timestamp <= t {
...
return false;
}
let safe_timestamp = timestamp.max(t + 3);
safe_timestamp = 10
IRQ
ticks = 9
r.cc(n).write(|w| w.set_compare(safe_timestamp as u32 & 0xFFFFFF));
9 <= 10 =>
if self.now() <= safe_timestamp {
return true;
}
Fixes #3672
Verified that this fixes the reproducer in the reported issue. I considered creating a HIL test for it, but it's a bit quirky since a custom critical section would need to be used, so I was wondering if it's ok to create another 'category' tests/nrf-mpsl' or something that uses the CS from nrf-mpsl instead to verify the fix.
Wdyt?