ta_set_timeout can fail to set an alarm

If alarm_pool_irq_handler takes <1us between handling an alarm and calling ta_set_timeout then no alarms will be set as 8000 it will appear as if an earlier alarm is already armedi before the target time. Make sure ta_set_timeout always leaves with an alarm set by checking the armed status. Fixes #2118
raspberrypi · kilograham · Dec 6, 2024 · Dec 5, 2024 · Dec 6, 2024 · Dec 5, 2024
commit 4b1b18fafdf7373d8300d0dd97bd63a408f36afc
diff --git a/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h b/src/rp2_common/pico_time_adapter/include/pico/time_adapter.h
@@ -48,7 +48,8 @@ static inline void ta_set_timeout(alarm_pool_timer_t *timer, uint alarm_num, int
     // 2. If we just passed the target time, then time_til_target will be high, meaning we will
     //    likely not do the update, but this is OK since the caller who has the full 64 bits
     //    must check if the target time has passed when we return anyway to avoid races.
-    if (time_til_target < time_til_alarm) {
+    // 3. We should never leave here without an alarm being armed
+    if (time_til_target < time_til_alarm || (timer_hw_from_timer(timer)->armed & (1 << alarm_num)) == 0) {
         timer_hw_from_timer(timer)->alarm[alarm_num] = (uint32_t) target;
     }
 }

diff --git a/test/pico_time_test/pico_time_test.c b/test/pico_time_test/pico_time_test.c
@@ -8,6 +8,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <hardware/sync.h>
+#include "hardware/clocks.h"
 #include "pico/stdlib.h"
 #include "pico/test.h"
 // Include sys/types.h before inttypes.h to work around issue with
@@ -74,6 +75,7 @@ static bool repeating_timer_callback(struct repeating_timer *t) {
 int issue_195_test(void);
 int issue_1812_test(void);
 int issue_1953_test(void);
+int issue_2118_test(void);
 
 int main() {
     setup_default_uart();
@@ -246,6 +248,8 @@ int main() {
 
     issue_1953_test();
 
+    issue_2118_test();
+
     PICOTEST_END_TEST();
 }
 
@@ -325,3 +329,38 @@ int issue_1953_test(void) {
     PICOTEST_END_SECTION();
     return 0;
 }
+
+static int counter_2118;
+static bool timer_callback_issue_2118(repeating_timer_t *rt) {
+    counter_2118++;
+    return true;
+}
+
+int issue_2118_test(void) {
+    PICOTEST_START_SECTION("Issue #2118 defect - failure to set an alarm");
+
+    // this problem only happens when running the clock fast as it requires the time between
+    // alarm_pool_irq_handler handling an alarm and setting the next alarm to be <1us
+    set_sys_clock_hz(200 * MHZ, true);
+    setup_default_uart();
+
+    alarm_pool_t *pool = alarm_pool_create(2, 1);
+    repeating_timer_t timer;
+    alarm_pool_add_repeating_timer_ms(pool, -20, timer_callback_issue_2118, NULL, &timer);
+
+    int iterations = 0;
+    while(iterations < 100) {
+        iterations++;
+        sleep_ms(20);
+    }
+    PICOTEST_CHECK(counter_2118 >= 100, "Repeating timer failure");
+
+    cancel_repeating_timer(&timer);
+    alarm_pool_destroy(pool);
+
+    set_sys_clock_hz(SYS_CLK_HZ, true);
+    setup_default_uart();
+
+    PICOTEST_END_SECTION();
+    return 0;
+}