Merge pull request #5612 from gebart/pr/xtimer-periodic-wakeup

xtimer: Refactor xtimer_usleep_until and rename to xtimer_periodic_wakeup

examples/timer_periodic_wakeup/main.c

@@ -20,17 +20,18 @@
 
 #include <stdio.h>
 #include "xtimer.h"
+#include "timex.h"
 
 /* set interval to 1 second */
-#define INTERVAL (1000000U)
+#define INTERVAL (1U * SEC_IN_USEC)
 
 int main(void)
 {
     uint32_t last_wakeup = xtimer_now();
 
     while(1) {
-        xtimer_usleep_until(&last_wakeup, INTERVAL);
-        printf("slept until %"PRIu32"\n", xtimer_now());
+        xtimer_periodic_wakeup(&last_wakeup, INTERVAL);
+        printf("slept until %" PRIu32 "\n", xtimer_now());
     }
 
     return 0;

sys/include/xtimer.h

@@ -144,25 +144,24 @@ static void xtimer_nanosleep(uint32_t nanoseconds);
  */
 static inline void xtimer_spin(uint32_t microseconds);
 
- /**
+/**
  * @brief will cause the calling thread to be suspended until the absolute
- * time (@p last_wakeup + @p interval).
+ * time (@p last_wakeup + @p period).
  *
  * When the function returns, @p last_wakeup is set to
- * (@p last_wakeup + @p interval).
+ * (@p last_wakeup + @p period).
  *
  * This function can be used to create periodic wakeups.
  * @c last_wakeup should be set to xtimer_now() before first call of the
  * function.
  *
- * If the result of (@p last_wakeup + usecs) would be in the past, the function
- * sets @p last_wakeup to @p last_wakeup + @p interval and returns immediately.
+ * If the result of (@p last_wakeup + @p period) would be in the past, the function
+ * sets @p last_wakeup to @p last_wakeup + @p period and returns immediately.
  *
- * @param[in] last_wakeup   base time for the wakeup
- * @param[in] usecs         time in microseconds that will be added to
- *                          last_wakeup
+ * @param[in] last_wakeup   base time stamp for the wakeup
+ * @param[in] period        time in microseconds that will be added to last_wakeup
  */
-void xtimer_usleep_until(uint32_t *last_wakeup, uint32_t usecs);
+void xtimer_periodic_wakeup(uint32_t *last_wakeup, uint32_t period);
 
 /**
  * @brief Set a timer that sends a message
@@ -326,6 +325,29 @@ int xtimer_msg_receive_timeout64(msg_t *msg, uint64_t us);
 #define XTIMER_ISR_BACKOFF 20
 #endif
 
+#ifndef XTIMER_PERIODIC_SPIN
+/**
+ * @brief   xtimer_periodic_wakeup spin cutoff
+ *
+ * If the difference between target time and now is less than this value, then
+ * xtimer_periodic_wakeup will use xtimer_spin instead of setting a timer.
+ */
+#define XTIMER_PERIODIC_SPIN (XTIMER_BACKOFF * 2)
+#endif
+
+#ifndef XTIMER_PERIODIC_RELATIVE
+/**
+ * @brief   xtimer_periodic_wakeup relative target cutoff
+ *
+ * If the difference between target time and now is less than this value, then
+ * xtimer_periodic_wakeup will set a relative target time in the future instead
+ * of the true target.
+ *
+ * This is done to prevent target time underflows.
+ */
+#define XTIMER_PERIODIC_RELATIVE (512)
+#endif
+
 #ifndef XTIMER_SHIFT
 /**
  * @brief   xtimer prescaler value
@@ -373,7 +395,6 @@ int xtimer_msg_receive_timeout64(msg_t *msg, uint64_t us);
 #define XTIMER_WIDTH (32)
 #endif
 
-#if XTIMER_WIDTH != 32
 /**
  * @brief xtimer timer mask
  *
@@ -383,6 +404,7 @@ int xtimer_msg_receive_timeout64(msg_t *msg, uint64_t us);
  * For a 16bit timer, the mask would be 0xFFFF0000, for a 24bit timer, the mask
  * would be 0xFF000000.
  */
+#if XTIMER_WIDTH != 32
 #define XTIMER_MASK ((0xffffffff >> XTIMER_WIDTH) << XTIMER_WIDTH)
 #else
 #define XTIMER_MASK (0)

sys/xtimer/xtimer.c

@@ -58,50 +58,66 @@ void _xtimer_sleep(uint32_t offset, uint32_t long_offset)
     mutex_lock(&mutex);
 }
 
-void xtimer_usleep_until(uint32_t *last_wakeup, uint32_t interval) {
+void xtimer_periodic_wakeup(uint32_t *last_wakeup, uint32_t period) {
     xtimer_t timer;
     mutex_t mutex = MUTEX_INIT;
 
     timer.callback = _callback_unlock_mutex;
     timer.arg = (void*) &mutex;
 
-    uint32_t target = *last_wakeup + interval;
-
+    uint32_t target = (*last_wakeup) + period;
     uint32_t now = xtimer_now();
     /* make sure we're not setting a value in the past */
-    if (now < *last_wakeup) {
-        /* base timer overflowed */
-        if (!((target < *last_wakeup) && (target > now))) {
+    if (now < (*last_wakeup)) {
+        /* base timer overflowed between last_wakeup and now */
+        if (!((now < target) && (target < (*last_wakeup)))) {
+            /* target time has already passed */
             goto out;
         }
     }
-    else if (! ((target < *last_wakeup) || (target > now))) {
-        goto out;
+    else {
+        /* base timer did not overflow */
+        if ((((*last_wakeup) <= target) && (target <= now))) {
+            /* target time has already passed */
+            goto out;
+        }
     }
 
-    /* For large offsets, set an absolute target time, as
-     * it is more exact.
-     *
-     * As that might cause an underflow, for small offsets,
-     * use a relative offset, as that can never underflow.
-     *
+    /*
+     * For large offsets, set an absolute target time.
+     * As that might cause an underflow, for small offsets, set a relative
+     * target time.
      * For very small offsets, spin.
      */
+    /*
+     * Note: last_wakeup _must never_ specify a time in the future after
+     * _xtimer_periodic_sleep returns.
+     * If this happens, last_wakeup may specify a time in the future when the
+     * next call to _xtimer_periodic_sleep is made, which in turn will trigger
+     * the overflow logic above and make the next timer fire too early, causing
+     * last_wakeup to point even further into the future, leading to a chain
+     * reaction.
+     *
+     * tl;dr Don't return too early!
+     */
     uint32_t offset = target - now;
-
-    if (offset > (XTIMER_BACKOFF * 2)) {
-        mutex_lock(&mutex);
-        if (offset >> 9) { /* >= 512 */
-            offset = target;
-        }
-        else {
-            offset += xtimer_now();
-        }
-        _xtimer_set_absolute(&timer, offset);
-        mutex_lock(&mutex);
+    DEBUG("xps, now: %9" PRIu32 ", tgt: %9" PRIu32 ", off: %9" PRIu32 "\n", now, target, offset);
+    if (offset < XTIMER_PERIODIC_SPIN) {
+        xtimer_spin(offset);
     }
     else {
-        xtimer_spin(offset);
+        if (offset < XTIMER_PERIODIC_RELATIVE) {
+            /* NB: This will overshoot the target by the amount of time it took
+             * to get here from the beginning of xtimer_periodic_wakeup()
+             *
+             * Since interrupts are normally enabled inside this function, this time may
+             * be undeterministic. */
+            target = xtimer_now() + offset;
+        }
+        mutex_lock(&mutex);
+        DEBUG("xps, abs: %" PRIu32 "\n", target);
+        _xtimer_set_absolute(&timer, target);
+        mutex_lock(&mutex);
     }
 
 out:

tests/driver_bh1750/main.c

@@ -40,7 +40,7 @@ int main(void)
     while(1) {
         uint16_t val = bh1750fvi_sample(&dev);
         printf("value: %5i lux\n", (int)val);
-        xtimer_usleep_until(&last, RATE);
+        xtimer_periodic_wakeup(&last, RATE);
     }
 
     return 0;

tests/driver_srf02/main.c

@@ -27,6 +27,7 @@
 
 #include "shell.h"
 #include "xtimer.h"
+#include "timex.h"
 #include "srf02.h"
 
 #ifndef TEST_SRF02_I2C
@@ -36,7 +37,7 @@
 #error "TEST_MODE not defined"
 #endif
 
-#define SAMPLE_PERIOD       (100LU * 1000U)
+#define SAMPLE_PERIOD       (100LU * MS_IN_USEC) /* 100 ms */
 
 static srf02_t dev;
 
@@ -78,7 +79,7 @@ static int cmd_sample(int argc, char **argv)
 
     while(1) {
         sample();
-        xtimer_usleep_until(&wakeup, SAMPLE_PERIOD);
+        xtimer_periodic_wakeup(&wakeup, SAMPLE_PERIOD);
     }
 
     return 0;

tests/periph_adc/main.c

@@ -21,11 +21,12 @@
 #include <stdio.h>
 
 #include "xtimer.h"
+#include "timex.h"
 #include "periph/adc.h"
 
 
 #define RES             ADC_RES_10BIT
-#define DELAY           (100LU * 1000U)
+#define DELAY           (100LU * MS_IN_USEC) /* 100 ms */
 
 
 int main(void)
@@ -56,7 +57,7 @@ int main(void)
                 printf("ADC_LINE(%i): %i\n", i, sample);
             }
         }
-        xtimer_usleep_until(&last, DELAY);
+        xtimer_periodic_wakeup(&last, DELAY);
     }
 
     return 0;

tests/periph_dac/main.c

@@ -56,7 +56,7 @@ int main(void)
             dac_set(DAC_LINE(i), val);
         }
         val += step;
-        xtimer_usleep_until(&last, DELAY);
+        xtimer_periodic_wakeup(&last, DELAY);
     }
 
     return 0;

tests/periph_pwm/main.c

@@ -27,9 +27,10 @@
 #include <stdio.h>
 
 #include "xtimer.h"
+#include "timex.h"
 #include "periph/pwm.h"
 
-#define INTERVAL    (10000U)
+#define INTERVAL    (10LU * MS_IN_USEC) /* 10 ms */
 #define STEP        (10)
 
 #define MODE        PWM_LEFT
@@ -71,7 +72,7 @@ int main(void)
             step = -step;
         }
 
-        xtimer_usleep_until(&last_wakeup, INTERVAL);
+        xtimer_periodic_wakeup(&last_wakeup, INTERVAL);
     }
 
     return 0;

tests/saul/main.c

@@ -26,7 +26,7 @@
 /**
  * @brief   Read th sensors every second
  */
-#define INTERVAL            (1000000LU)
+#define INTERVAL            (1LU * SEC_IN_USEC)
 
 
 int main(void)
@@ -50,7 +50,7 @@ int main(void)
             dev = dev->next;
         }
 
-        xtimer_usleep_until(&last, INTERVAL);
+        xtimer_periodic_wakeup(&last, INTERVAL);
     }
 
     return 0;

tests/xtimer_drift/main.c

@@ -192,7 +192,7 @@ int main(void)
 
     uint32_t last_wakeup = xtimer_now();
     while (1) {
-        xtimer_usleep_until(&last_wakeup, TEST_INTERVAL);
+        xtimer_periodic_wakeup(&last_wakeup, TEST_INTERVAL);
         msg_try_send(&m, pid3);
     }
 }

tests/xtimer_longterm/README.md

@@ -19,7 +19,7 @@ we consider this long-term...). The mid-term timers are set to 3 and 5 minutes.
 Both kind of timers have one that is using `xtimer_usleep` and one that is
 using `xtimer_set_msg`.
 
-The short-term timer is triggered every 50ms and is using `xtimer_sleep_until`.
+The short-term timer is triggered every 50ms and is using `xtimer_periodic_wakeup`.
 Each time this timer triggers, it increments a software counter, which triggers
 then a message every minute. A 50ms interval should be small enough, to trigger
 also for 16-bit wide timers at least once in every timer period.
@@ -28,4 +28,4 @@ On each mid- and long-term timer event, the output shows also the number of
 fast timer (1min timer) ticks, that have been triggered since a timer was
 triggered last. This number should be equal to the timers interval.
 
-For reasonable results, you should run this test at least for some ours...
+For reasonable results, you should run this test at least for some hours...

tests/xtimer_longterm/main.c

@@ -113,7 +113,7 @@ void *ticker(void *arg)
             msg_send(&msg, print_pid);
         }
 
-        xtimer_usleep_until(&base, INT_SHORT);
+        xtimer_periodic_wakeup(&base, INT_SHORT);
     }
 
     return NULL;

tests/xtimer_periodic_wakeup/main.c

@@ -30,23 +30,25 @@ int32_t res[NUMOF];
 
 int main(void)
 {
-    puts("xtimer_usleep_until test application.\n");
+    puts("xtimer_periodic_wakeup test application.\n");
 
     uint32_t interval = NUMOF;
     int32_t max_diff = INT32_MIN;
     int32_t min_diff = INT32_MAX;
 
     for (int i = 0; i < NUMOF; i++) {
-        printf("Testing interval %u... (now=%"PRIu32")\n", (unsigned)interval, xtimer_now());
+        uint32_t now = xtimer_now();
+        printf("Testing interval %" PRIu32 "... (now=%" PRIu32 ")\n", interval, now);
         uint32_t last_wakeup = xtimer_now();
         uint32_t before = last_wakeup;
-        xtimer_usleep_until(&last_wakeup, (unsigned)interval);
-        res[i] = (xtimer_now()-before)-interval;
+        xtimer_periodic_wakeup(&last_wakeup, interval);
+        now = xtimer_now();
+        res[i] = (now - before) - interval;
         interval -= 1;
     }
 
     for (int i = 0; i < NUMOF; i++) {
-        printf("%4d diff=%"PRIi32"\n", NUMOF-i, res[i]);
+        printf("%4d diff=%" PRIi32 "\n", NUMOF - i, res[i]);
 
         if (res[i] > max_diff) {
             max_diff = res[i];

tests/xtimer_reset/main.c

@@ -18,6 +18,8 @@
  * @}
  */
 
+#include <stdint.h>
+#include <inttypes.h>
 #include <stdio.h>
 
 #include "thread.h"
@@ -46,11 +48,11 @@ int main(void)
     xtimer_set_wakeup(&xtimer, 200000, me);
     xtimer_set_wakeup(&xtimer2, 100000, me);
 
-    printf("now=%u\n", (unsigned)xtimer_now());
+    printf("now=%" PRIu32 "\n", xtimer_now());
     thread_sleep();
-    printf("now=%u\n", (unsigned)xtimer_now());
+    printf("now=%" PRIu32 "\n", xtimer_now());
     thread_sleep();
-    printf("now=%u\n", (unsigned)xtimer_now());
+    printf("now=%" PRIu32 "\n", xtimer_now());
 
     printf("Test completed!\n");