From 197f4dc0db8f0008cfe365f121180298b9ecbd7c Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 7 Dec 2023 14:32:55 +0100 Subject: [PATCH] tests/periph/timer: fix timeout computation and overflow handling - the timeout computation for the spurious IRQ test confused numerator and denominator in a fraction - the timeout offset between timer channels was hardcoded to 5000 from when the timer was only tested with 1 MHz as frequency - This resulted in slooooow test runs when running at slow frequencies - fix overflow handling in the spinning wait - likely this would never overflow anyway assuming that `timer_init()` resets the counter value, but let's not rely on this and just fix the bug for good --- tests/periph/timer/main.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/tests/periph/timer/main.c b/tests/periph/timer/main.c index a3f189d08e9e9..d2f2bb4615c19 100644 --- a/tests/periph/timer/main.c +++ b/tests/periph/timer/main.c @@ -41,8 +41,15 @@ #define TIMER_CHANNEL_NUMOF 10U #endif -#define CHAN_OFFSET (5000U) /* fire every 5ms */ +#define CHAN_OFFSET_MS 5U /* fire channels with 5 ms offset */ +/* The minimum timeout to set and still being able to clear the timer + * before it fires. This should be conservative, as wasting a few milliseconds + * in the test is less annoying than false test failures */ +#define MINIMUM_TIMEOUT_MS 2 #define COOKIE (100U) /* for checking if arg is passed */ +/* Setting a timer for less than two ticks may cause it to fire right away, + * e.g. when the timer was about to tick anyway */ +#define MINIMUM_TICKS 2 static uint8_t fired; static uint32_t sw_count; @@ -73,6 +80,14 @@ static uword_t query_channel_numof(tim_t dev) return TIMER_CHANNEL_NUMOF; } +static unsigned milliseconds_to_ticks(uint32_t timer_freq, unsigned millisecs) +{ + /* Use 64 bit arithmetics and to avoid overflows for high frequencies. */ + unsigned result = (uint64_t)millisecs * US_PER_MS * timer_freq / US_PER_SEC; + /* Never return less than 2 ticks */ + return (result >= MINIMUM_TICKS) ? result : MINIMUM_TICKS; +} + static int test_timer(unsigned num, uint32_t timer_freq) { int set = 0; @@ -99,10 +114,11 @@ static int test_timer(unsigned num, uint32_t timer_freq) timer_stop(TIMER_DEV(num)); printf(" - timer_stop(%u): stopped\n", num); + unsigned chan_offset_ticks = milliseconds_to_ticks(timer_freq, CHAN_OFFSET_MS); /* set each available channel */ for (unsigned i = 0; i < query_channel_numof(TIMER_DEV(num)); i++) { - unsigned timeout = ((i + 1) * CHAN_OFFSET); + unsigned timeout = ((i + 1) * chan_offset_ticks); printf(" - timer_set(%u, %u, %u)\n ", num, i, timeout); if (timer_set(TIMER_DEV(num), i, timeout) < 0) { printf("ERROR: Couldn't set timeout %u for channel %u\n", @@ -159,12 +175,12 @@ static int test_timer(unsigned num, uint32_t timer_freq) printf(" - Validating no spurious IRQs are triggered:\n"); expect(0 == timer_init(TIMER_DEV(num), timer_freq, cb_not_to_be_executed, NULL)); - const unsigned duration = 2ULL * US_PER_MS * US_PER_SEC / timer_freq; + const unsigned duration = milliseconds_to_ticks(timer_freq, MINIMUM_TIMEOUT_MS); unsigned target = timer_read(TIMER_DEV(num)) + duration; expect(0 == timer_set_absolute(TIMER_DEV(num), 0, target)); expect(0 == timer_clear(TIMER_DEV(num), 0)); atomic_store_u8(&fired, 0); - while (timer_read(TIMER_DEV(num)) < target) { + while ((target - timer_read(TIMER_DEV(num))) <= duration) { /* busy waiting for the timer to reach it timeout. Timer must not fire, * it was cleared */ } @@ -176,10 +192,8 @@ static int test_timer(unsigned num, uint32_t timer_freq) /* checking again to make sure that any IRQ pending bit that may just was * mask doesn't trigger a timer IRQ on the next set */ target = timer_read(TIMER_DEV(num)) + duration; - timer_set_absolute(TIMER_DEV(num), 0, target); - timer_clear(TIMER_DEV(num), 0); - while (timer_read(TIMER_DEV(num)) < target) { + while ((target - timer_read(TIMER_DEV(num))) <= duration) { /* busy waiting for the timer to reach it timeout. Timer must not fire, * it was cleared */ }