From 4449ba493350a3683d48a91934aa767409c42ec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Tue, 5 Jul 2016 23:01:26 +0200 Subject: [PATCH 1/3] xtimer: Refactor xtimer_usleep_until and rename to xtimer_periodic_wakeup Rewrote conditions for improved readability, and removed magic number 512 --- sys/include/xtimer.h | 42 ++++++++++++++++++++------- sys/xtimer/xtimer.c | 68 +++++++++++++++++++++++++++----------------- 2 files changed, 74 insertions(+), 36 deletions(-) diff --git a/sys/include/xtimer.h b/sys/include/xtimer.h index a5df8c3c6c..6b3168d08b 100644 --- a/sys/include/xtimer.h +++ b/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) diff --git a/sys/xtimer/xtimer.c b/sys/xtimer/xtimer.c index 87036dbdea..efe19d635e 100644 --- a/sys/xtimer/xtimer.c +++ b/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: From 71501de4c62e32cfbf245506fb2701c10fdb9d76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Tue, 5 Jul 2016 21:32:44 +0200 Subject: [PATCH 2/3] tests/xtimer_periodic_wakeup: Copy and adapt from xtimer_usleep_until --- .../Makefile | 0 .../main.c | 12 +++++++----- 2 files changed, 7 insertions(+), 5 deletions(-) rename tests/{xtimer_usleep_until => xtimer_periodic_wakeup}/Makefile (100%) rename tests/{xtimer_usleep_until => xtimer_periodic_wakeup}/main.c (77%) diff --git a/tests/xtimer_usleep_until/Makefile b/tests/xtimer_periodic_wakeup/Makefile similarity index 100% rename from tests/xtimer_usleep_until/Makefile rename to tests/xtimer_periodic_wakeup/Makefile diff --git a/tests/xtimer_usleep_until/main.c b/tests/xtimer_periodic_wakeup/main.c similarity index 77% rename from tests/xtimer_usleep_until/main.c rename to tests/xtimer_periodic_wakeup/main.c index 5ee77acf71..479c5d0b86 100644 --- a/tests/xtimer_usleep_until/main.c +++ b/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]; From c0ad83c167208793d8f98516b2ee40bb6038c983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joakim=20Nohlg=C3=A5rd?= Date: Thu, 7 Jul 2016 16:15:33 +0200 Subject: [PATCH 3/3] tests: Update xtimer_usleep_until usage to xtimer_periodic_wakeup --- examples/timer_periodic_wakeup/main.c | 7 ++++--- tests/driver_bh1750/main.c | 2 +- tests/driver_srf02/main.c | 5 +++-- tests/periph_adc/main.c | 5 +++-- tests/periph_dac/main.c | 2 +- tests/periph_pwm/main.c | 5 +++-- tests/saul/main.c | 4 ++-- tests/xtimer_drift/main.c | 2 +- tests/xtimer_longterm/README.md | 4 ++-- tests/xtimer_longterm/main.c | 2 +- tests/xtimer_reset/main.c | 8 +++++--- 11 files changed, 26 insertions(+), 20 deletions(-) diff --git a/examples/timer_periodic_wakeup/main.c b/examples/timer_periodic_wakeup/main.c index ce080f4e98..b2925f3309 100644 --- a/examples/timer_periodic_wakeup/main.c +++ b/examples/timer_periodic_wakeup/main.c @@ -20,17 +20,18 @@ #include #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; diff --git a/tests/driver_bh1750/main.c b/tests/driver_bh1750/main.c index 4380a5f667..0c276aa6bc 100644 --- a/tests/driver_bh1750/main.c +++ b/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; diff --git a/tests/driver_srf02/main.c b/tests/driver_srf02/main.c index 1e6df623f9..317f5a3950 100644 --- a/tests/driver_srf02/main.c +++ b/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; diff --git a/tests/periph_adc/main.c b/tests/periph_adc/main.c index b63d33bcaa..79146a04f0 100644 --- a/tests/periph_adc/main.c +++ b/tests/periph_adc/main.c @@ -21,11 +21,12 @@ #include #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; diff --git a/tests/periph_dac/main.c b/tests/periph_dac/main.c index a827eede30..59897a7a5f 100644 --- a/tests/periph_dac/main.c +++ b/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; diff --git a/tests/periph_pwm/main.c b/tests/periph_pwm/main.c index b20d253615..aee1a385c7 100644 --- a/tests/periph_pwm/main.c +++ b/tests/periph_pwm/main.c @@ -27,9 +27,10 @@ #include #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; diff --git a/tests/saul/main.c b/tests/saul/main.c index 7e228ea466..ff6d694c9d 100644 --- a/tests/saul/main.c +++ b/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; diff --git a/tests/xtimer_drift/main.c b/tests/xtimer_drift/main.c index 4af305c9f8..7bb5828d9c 100644 --- a/tests/xtimer_drift/main.c +++ b/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); } } diff --git a/tests/xtimer_longterm/README.md b/tests/xtimer_longterm/README.md index 6ad57d601a..d0ba6f47b7 100644 --- a/tests/xtimer_longterm/README.md +++ b/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... diff --git a/tests/xtimer_longterm/main.c b/tests/xtimer_longterm/main.c index 4fe44c49ca..e59fdd216f 100644 --- a/tests/xtimer_longterm/main.c +++ b/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; diff --git a/tests/xtimer_reset/main.c b/tests/xtimer_reset/main.c index 329e670a53..e591b646d6 100644 --- a/tests/xtimer_reset/main.c +++ b/tests/xtimer_reset/main.c @@ -18,6 +18,8 @@ * @} */ +#include +#include #include #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");