fix minor bug in add_repeating_timer_us, and add some comments (#621)

src/common/pico_time/include/pico/time.h

@@ -416,9 +416,12 @@ void alarm_pool_destroy(alarm_pool_t *pool);
  * @param time the timestamp when (after which) the callback should fire
  * @param callback the callback function
  * @param user_data user data to pass to the callback function
- * @param fire_if_past if true, this method will call the callback itself before returning 0 if the timestamp happens before or during this method call
+ * @param fire_if_past if true, and the alarm time falls before or during this call before the alarm can be set,
- * @return >0 the alarm id
+ *                     then the callback should be called during (by) this function instead 
- * @return 0 the target timestamp was during or before this method call (whether the callback was called depends on fire_if_past)
+ * @return >0 the alarm id for an active (at the time of return) alarm
+ * @return 0 if the alarm time passed before or during the call AND there is no active alarm to return the id of.
+ *           The latter can either happen because fire_if_past was false (i.e. no timer was ever created),
+ *           or if the callback <i>was</i> called during this method but the callback cancelled itself by returning 0
  * @return -1 if there were no alarm slots available
  */
 alarm_id_t alarm_pool_add_alarm_at(alarm_pool_t *pool, absolute_time_t time, alarm_callback_t callback, void *user_data, bool fire_if_past);
@@ -438,9 +441,12 @@ alarm_id_t alarm_pool_add_alarm_at(alarm_pool_t *pool, absolute_time_t time, ala
  * @param us the delay (from now) in microseconds when (after which) the callback should fire
  * @param callback the callback function
  * @param user_data user data to pass to the callback function
- * @param fire_if_past if true, this method will call the callback itself before returning 0 if the timestamp happens before or during this method call
+ * @param fire_if_past if true, and the alarm time falls during this call before the alarm can be set,
+ *                     then the callback should be called during (by) this function instead 
  * @return >0 the alarm id
- * @return 0 the target timestamp was during or before this method call (whether the callback was called depends on fire_if_past)
+ * @return 0 if the alarm time passed before or during the call AND there is no active alarm to return the id of.
+ *           The latter can either happen because fire_if_past was false (i.e. no timer was ever created),
+ *           or if the callback <i>was</i> called during this method but the callback cancelled itself by returning 0
  * @return -1 if there were no alarm slots available
  */
 static inline alarm_id_t alarm_pool_add_alarm_in_us(alarm_pool_t *pool, uint64_t us, alarm_callback_t callback, void *user_data, bool fire_if_past) {
@@ -462,9 +468,12 @@ static inline alarm_id_t alarm_pool_add_alarm_in_us(alarm_pool_t *pool, uint64_t
  * @param ms the delay (from now) in milliseconds when (after which) the callback should fire
  * @param callback the callback function
  * @param user_data user data to pass to the callback function
- * @param fire_if_past if true, this method will call the callback itself before returning 0 if the timestamp happens before or during this method call
+ * @param fire_if_past if true, and the alarm time falls before or during this call before the alarm can be set,
+ *                     then the callback should be called during (by) this function instead 
  * @return >0 the alarm id
- * @return 0 the target timestamp was during or before this method call (whether the callback was called depends on fire_if_past)
+ * @return 0 if the alarm time passed before or during the call AND there is no active alarm to return the id of.
+ *           The latter can either happen because fire_if_past was false (i.e. no timer was ever created),
+ *           or if the callback <i>was</i> called during this method but the callback cancelled itself by returning 0
  * @return -1 if there were no alarm slots available
  */
 static inline alarm_id_t alarm_pool_add_alarm_in_ms(alarm_pool_t *pool, uint32_t ms, alarm_callback_t callback, void *user_data, bool fire_if_past) {
@@ -496,9 +505,12 @@ bool alarm_pool_cancel_alarm(alarm_pool_t *pool, alarm_id_t alarm_id);
  * @param time the timestamp when (after which) the callback should fire
  * @param callback the callback function
  * @param user_data user data to pass to the callback function
- * @param fire_if_past if true, this method will call the callback itself before returning 0 if the timestamp happens before or during this method call
+ * @param fire_if_past if true, and the alarm time falls before or during this call before the alarm can be set,
+ *                     then the callback should be called during (by) this function instead 
  * @return >0 the alarm id
- * @return 0 the target timestamp was during or before this method call (whether the callback was called depends on fire_if_past)
+ * @return 0 if the alarm time passed before or during the call AND there is no active alarm to return the id of.
+ *           The latter can either happen because fire_if_past was false (i.e. no timer was ever created),
+ *           or if the callback <i>was</i> called during this method but the callback cancelled itself by returning 0
  * @return -1 if there were no alarm slots available
  */
 static inline alarm_id_t add_alarm_at(absolute_time_t time, alarm_callback_t callback, void *user_data, bool fire_if_past) {
@@ -519,9 +531,12 @@ static inline alarm_id_t add_alarm_at(absolute_time_t time, alarm_callback_t cal
  * @param us the delay (from now) in microseconds when (after which) the callback should fire
  * @param callback the callback function
  * @param user_data user data to pass to the callback function
- * @param fire_if_past if true, this method will call the callback itself before returning 0 if the timestamp happens before or during this method call
+ * @param fire_if_past if true, and the alarm time falls during this call before the alarm can be set,
+ *                     then the callback should be called during (by) this function instead 
  * @return >0 the alarm id
- * @return 0 the target timestamp was during or before this method call (whether the callback was called depends on fire_if_past)
+ * @return 0 if the alarm time passed before or during the call AND there is no active alarm to return the id of.
+ *           The latter can either happen because fire_if_past was false (i.e. no timer was ever created),
+ *           or if the callback <i>was</i> called during this method but the callback cancelled itself by returning 0
  * @return -1 if there were no alarm slots available
  */
 static inline alarm_id_t add_alarm_in_us(uint64_t us, alarm_callback_t callback, void *user_data, bool fire_if_past) {
@@ -542,9 +557,12 @@ static inline alarm_id_t add_alarm_in_us(uint64_t us, alarm_callback_t callback,
  * @param ms the delay (from now) in milliseconds when (after which) the callback should fire
  * @param callback the callback function
  * @param user_data user data to pass to the callback function
- * @param fire_if_past if true, this method will call the callback itself before returning 0 if the timestamp happens before or during this method call
+ * @param fire_if_past if true, and the alarm time falls during this call before the alarm can be set,
+ *                     then the callback should be called during (by) this function instead 
  * @return >0 the alarm id
- * @return 0 the target timestamp was during or before this method call (whether the callback was called depends on fire_if_past)
+ * @return 0 if the alarm time passed before or during the call AND there is no active alarm to return the id of.
+ *           The latter can either happen because fire_if_past was false (i.e. no timer was ever created),
+ *           or if the callback <i>was</i> called during this method but the callback cancelled itself by returning 0
  * @return -1 if there were no alarm slots available
  */
 static inline alarm_id_t add_alarm_in_ms(uint32_t ms, alarm_callback_t callback, void *user_data, bool fire_if_past) {

src/common/pico_time/time.c

@@ -219,18 +219,26 @@ alarm_id_t alarm_pool_add_alarm_at(alarm_pool_t *pool, absolute_time_t time, ala
     do {
         uint8_t id_high = 0;
         uint32_t save = spin_lock_blocking(pool->lock);
+
         pheap_node_id_t id = add_alarm_under_lock(pool, time, callback, user_data, 0, false, &missed);
         if (id) id_high = *get_entry_id_high(pool, id);
 
         spin_unlock(pool->lock, save);
 
         if (!id) {
+            // no space in pheap to allocate an alarm
             return -1;
         }
 
+        // note that if missed was true, then the id was never added to the pheap (because we
+        // passed false for create_if_past arg above)
         public_id = missed ? 0 : make_public_id(id_high, id);
         if (missed && fire_if_past) {
+            // ... so if fire_if_past == true we call the callback
             int64_t repeat = callback(public_id, user_data);
+            // if not repeated we have no id to return so set public_id to 0,
+            // otherwise we need to repeat, but will assign a new id next time
+            // todo arguably this does mean that the id passed to the first callback may differ from subsequent calls
             if (!repeat) {
                 public_id = 0;
                 break;
@@ -240,6 +248,10 @@ alarm_id_t alarm_pool_add_alarm_at(alarm_pool_t *pool, absolute_time_t time, ala
                 time = delayed_by_us(get_absolute_time(), (uint64_t)repeat);
             }
         } else {
+            // either:
+            // a) missed == false && public_id is > 0
+            // b) missed == true && fire_if_past == false && public_id = 0
+            // but we are done in either case
             break;
         }
     } while (true);
@@ -302,7 +314,9 @@ bool alarm_pool_add_repeating_timer_us(alarm_pool_t *pool, int64_t delay_us, rep
     out->user_data = user_data;
     out->alarm_id = alarm_pool_add_alarm_at(pool, make_timeout_time_us((uint64_t)(delay_us >= 0 ? delay_us : -delay_us)),
                                             repeating_timer_callback, out, true);
-    return out->alarm_id > 0;
+    // note that if out->alarm_id is 0, then the callback was called during the above call (fire_if_past == true)
+    // and then the callback removed itself.
+    return out->alarm_id >= 0;
 }
 
 bool cancel_repeating_timer(repeating_timer_t *timer) {