ta_set_timeout can fail to set an alarm (#2127)

* 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 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

* ta_disable_irq_handler should unarm its timer

Author: peterharperuk

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;
     }
 }
@@ -75,6 +76,7 @@ static inline void ta_enable_irq_handler(alarm_pool_timer_t *timer, uint alarm_n
 
 static inline void ta_disable_irq_handler(alarm_pool_timer_t *timer, uint alarm_num, irq_handler_t irq_handler) {
     uint irq_num = timer_hardware_alarm_get_irq_num(timer, alarm_num);
+    timer_hw_from_timer(timer)->armed = 1u << alarm_num; // disarm the timer
     hw_clear_bits(&timer_hw_from_timer(timer)->inte, 1u << alarm_num);
     irq_set_enabled(irq_num, true);
     irq_remove_handler(irq_num, irq_handler);

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");
+
+    alarm_pool_destroy(pool);
+    hard_assert(timer_hw->armed == 0); // check destroying the pool unarms its timer
+
+    set_sys_clock_hz(SYS_CLK_HZ, true);
+    setup_default_uart();
+
+    PICOTEST_END_SECTION();
+    return 0;
+}