hardware_timer: fix race condition whem a new timer being added becomes missed thus obviating the need for an IRQ but there is an IRQ already pending for another timer (#243)

src/rp2_common/hardware_timer/timer.c

@@ -166,8 +166,9 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) {
         // 1) actually set the hardware timer
         spin_lock_t *lock = spin_lock_instance(PICO_SPINLOCK_ID_TIMER);
         uint32_t save = spin_lock_blocking(lock);
-        timer_hw->intr = 1u << alarm_num;
+        uint8_t old_timer_callbacks_pending = timer_callbacks_pending;
         timer_callbacks_pending |= (uint8_t)(1u << alarm_num);
+        timer_hw->intr = 1u << alarm_num; // clear any IRQ
         timer_hw->alarm[alarm_num] = (uint32_t) t;
         // Set the alarm. Writing time should arm it
         target_hi[alarm_num] = (uint32_t)(t >> 32u);
@@ -178,18 +179,26 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) {
             assert(timer_hw->ints & 1u << alarm_num);
         } else {
             if (time_us_64() >= t) {
-                // ok well it is time now; the irq isn't being handled yet because of the spin lock
+                // we are already at or past the right time; there is no point in us racing against the IRQ
-                // however the other core might be in the IRQ handler itself about to do a callback
+                // we are about to generate. note however that, if there was already a timer pending before,
-                // we do the firing ourselves (and indicate to the IRQ handler if any that it shouldn't
+                // then we still let the IRQ fire, as whatever it was, is not handled by our setting missed=true here
                 missed = true;
+                if (timer_callbacks_pending != old_timer_callbacks_pending) {
                     // disarm the timer
                     timer_hw->armed = 1u << alarm_num;
-                timer_hw->intr = 1u << alarm_num; // clear the IRQ too
+                    // clear the IRQ...
-                // and set flag in case we're already in the IRQ handler waiting on the spinlock (on the other core)
+                    timer_hw->intr = 1u << alarm_num;
-                timer_callbacks_pending &= (uint8_t)~(1u << alarm_num);
+                    // ... including anything pending on the processor - perhaps unnecessary, but
+                    // our timer flag says we aren't expecting anything.
+                    irq_clear(harware_alarm_irq_number(alarm_num));
+                    // and clear our flag so that if the IRQ handler is already active (because it is on
+                    // the other core) it will also skip doing anything
+                    timer_callbacks_pending = old_timer_callbacks_pending;
+                }
             }
         }
         spin_unlock(lock, save);
+        // note at this point any pending timer IRQ can likely run
     }
     return missed;
 }

test/pico_time_test/pico_time_test.c

@@ -64,12 +64,13 @@ static bool repeating_timer_callback(struct repeating_timer *t) {
 #define RESOLUTION_ALLOWANCE PICO_HARDWARE_TIMER_RESOLUTION_US
 #endif
 
+int issue_195_test(void);
+
 int main() {
     setup_default_uart();
     alarm_pool_init_default();
 
     PICOTEST_START();
-
     struct alarm_pool *pools[NUM_TIMERS];
     for(uint i=0; i<NUM_TIMERS; i++) {
         if (i == alarm_pool_hardware_alarm_num(alarm_pool_get_default())) {
@@ -215,6 +216,35 @@ int main() {
     PICOTEST_CHECK(absolute_time_diff_us(near_the_end_of_time, at_the_end_of_time) > 0, "near the end of time should be before the end of time")
     PICOTEST_END_SECTION();
 
+    if (issue_195_test()) {
+        return -1;
+    }
+
     PICOTEST_END_TEST();
 }
 
+#define ISSUE_195_TIMER_DELAY 50
+volatile int issue_195_counter;
+int64_t issue_195_callback(alarm_id_t id, void *user_data) {
+    issue_195_counter++;
+    return -ISSUE_195_TIMER_DELAY;
+}
+
+int issue_195_test(void) {
+    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");
+    absolute_time_t t1 = get_absolute_time();
+    int id = add_alarm_in_us(ISSUE_195_TIMER_DELAY, issue_195_callback, NULL, true);
+    for(uint i=0;i<5000;i++) {
+        sleep_us(100);
+        sleep_us(100);
+        uint delay = 9; // 9 seems to be the magic number (at least for reproducing on 10.2.1)
+        sleep_us(delay);
+    }
+    absolute_time_t t2 = get_absolute_time();
+    cancel_alarm(id);
+    int expected_count = absolute_time_diff_us(t1, t2) / ISSUE_195_TIMER_DELAY;
+    printf("Timer fires approx_expected=%d actual=%d\n", expected_count, issue_195_counter);
+    PICOTEST_END_SECTION();
+    return 0;
+}
+