Fix best_effort_wfe_or_timeout further by not having the IRQ ever move the...

Fix best_effort_wfe_or_timeout further by not having the IRQ ever move the alarm target backwards; needs a new test case to make sure though

src/common/pico_time/time.c

@@ -136,7 +136,9 @@ static void alarm_pool_irq_handler(void);
 static void alarm_pool_irq_handler(void) {
     // This IRQ handler does the main work, as it always (assuming the IRQ hasn't been enabled on both cores
     // which is unsupported) run on the alarm pool's core, and can't be preempted by itself, meaning
-    // that it doesn't need locks except to protect against linked list access
+    // that it doesn't need locks except to protect against linked list, or other state access.
+    // This simplifies the code considerably, and makes it much faster in general, even though we are forced to take
+    // two IRQs per alarm.
     uint timer_alarm_num;
     alarm_pool_timer_t *timer = ta_from_current_irq(&timer_alarm_num);
     uint timer_num = ta_timer_num(timer);
@@ -268,11 +270,14 @@ static void alarm_pool_irq_handler(void) {
         // we are leaving a timeout every 2^32 microseconds anyway if there is no valid target, so we can choose any value.
         // best_effort_wfe_or_timeout now relies on it being the last value set, and arguably this is the
         // best value anyway, as it is the furthest away from the last fire.
-        if (earliest_target != -1) {
+        if (earliest_target != -1) { // cancelled alarm has target of -1
             ta_set_timeout(timer, timer_alarm_num, earliest_target);
         }
-        // check we haven't now past the target time; if not we don't want to loop again
+        // check we haven't now passed the target time; if not we don't want to loop again
     } while ((earliest_target - now) <= 0);
+    // We always want the timer IRQ to wake a WFE so that best_effort_wfe_or_timeout() will wake up. It will wake
+    // a WFE on its own core by nature of having taken an IRQ, but we do an explicit SEV so it wakes the other core
+    __sev();
 }
 
 void alarm_pool_post_alloc_init(alarm_pool_t *pool, alarm_pool_timer_t *timer, uint hardware_alarm_num, uint max_timers) {
@@ -444,7 +449,7 @@ bool best_effort_wfe_or_timeout(absolute_time_t timeout_timestamp) {
         return time_reached(timeout_timestamp);
     } else {
         alarm_id_t id;
-        // note that as of SDK2.0.0 calling add_alarm_at always causes a SEV. Whwat we really
+        // note that as of SDK 2.0.0 calling add_alarm_at always causes a SEV. What we really
         // want to do is cause an IRQ at the specified time in the future if there is not
         // an IRQ already happening before then. The problem is that the IRQ may be happening on the
         // other core, so taking an IRQ is the only way to get the state protection.
@@ -452,9 +457,22 @@ bool best_effort_wfe_or_timeout(absolute_time_t timeout_timestamp) {
         // Therefore, we make a compromise; we will set the alarm, if we won't wake up before the right time
         // already. This means that repeated calls to this function with the same timeout will work correctly
         // after the first one! This is fine, because we ask callers to use a polling loop on another
-        // event variable when using this function
+        // event variable when using this function.
+        //
+        // For this to work, we require that once we have set an alarm, an SEV happens no later than that, even
+        // if we cancel the alarm as we do below. Therefore, the IRQ handler (which is always enabled) will
+        // never set its wakeup time to a later value, but instead wake up once and then wake up again.
+        //
+        // This overhead when canceling alarms is a small price to pay for the much simpler/faster/cleaner
+        // implementation that relies on the IRQ handler (on a single core) being the only state accessor.
+        //
+        // Note also, that the use of software spin locks on RP2350 to access state would always cause a SEV
+        // due to use of LDREX etc., so actually using spin locks to protect the state would be worse.
         if (ta_wakes_up_on_or_before(alarm_pool_get_default()->timer, alarm_pool_get_default()->timer_alarm_num,
                                      (int64_t)to_us_since_boot(timeout_timestamp))) {
+            // we already are waking up at or before when we want to (possibly due to us having been called
+            // before in a loop), so we can do an actual WFE. Note we rely on the fact that the alarm pool IRQ
+            // handler always does an explicit SEV, since it may be on the other core.
             __wfe();
             return time_reached(timeout_timestamp);
         } else {

src/rp2_common/pico_time_adapter/include/pico/time_adapter.h

@@ -36,8 +36,22 @@ static inline alarm_pool_timer_t *ta_from_current_irq(uint *alarm_num) {
 }
 
 static inline void ta_set_timeout(alarm_pool_timer_t *timer, uint alarm_num, int64_t target) {
+    // We never want to set the timeout to be later than our current one.
+    uint32_t current = timer_time_us_32(timer_hw_from_timer(timer));
+    uint32_t time_til_target = (uint32_t) target - current;
+    uint32_t time_til_alarm = timer_hw_from_timer(timer)->alarm[alarm_num] - current;
+    // Note: we are only dealing with the low 32 bits of the timer values,
+    // so there is some opportunity to make wrap-around errors.
+    //
+    // 1. If we just passed the alarm time, then time_til_alarm will be high, meaning we will
+    //    likely do the update, but this is OK since the alarm will have just fired
+    // 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) {
         timer_hw_from_timer(timer)->alarm[alarm_num] = (uint32_t) target;
     }
+}
 
 static inline bool ta_wakes_up_on_or_before(alarm_pool_timer_t *timer, uint alarm_num, int64_t target) {
     uint32_t current = timer_time_us_32(timer_hw_from_timer(timer));