-
Notifications
You must be signed in to change notification settings - Fork 2.1k
tests/periph_timer_timeout0: Regression test for timer callbacks setting new timers #10019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jnohlgard
wants to merge
2
commits into
RIOT-OS:master
Choose a base branch
from
jnohlgard:pr/test-periph-timer-timeout0
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,5 +1,7 @@ | ||||
| /* | ||||
| * Copyright (C) 2015 Freie Universität Berlin | ||||
| * 2018 Eistec AB | ||||
| * 2024 HAW Hamburg | ||||
| * | ||||
| * This file is subject to the terms and conditions of the GNU Lesser | ||||
| * General Public License v2.1. See the file LICENSE in the top level | ||||
|
|
@@ -14,6 +16,8 @@ | |||
| * @brief Peripheral timer test application | ||||
| * | ||||
| * @author Hauke Petersen <[email protected]> | ||||
| * Joakim Nohlgård <[email protected]> | ||||
| * Bennet Blischke <[email protected]> | ||||
| * | ||||
| * @} | ||||
| */ | ||||
|
|
@@ -25,6 +29,7 @@ | |||
| #include "atomic_utils.h" | ||||
| #include "architecture.h" | ||||
| #include "clk.h" | ||||
| #include "mutex.h" | ||||
| #include "periph/timer.h" | ||||
| #include "test_utils/expect.h" | ||||
| #include "time_units.h" | ||||
|
|
@@ -51,11 +56,36 @@ | |||
| * e.g. when the timer was about to tick anyway */ | ||||
| #define MINIMUM_TICKS 2 | ||||
|
|
||||
| #ifndef TEST_ITERATIONS | ||||
| #define TEST_ITERATIONS (10000ul) | ||||
| #endif | ||||
|
|
||||
| static uint8_t fired; | ||||
| static uint32_t sw_count; | ||||
| static uint32_t timeouts[TIMER_CHANNEL_NUMOF]; | ||||
| static unsigned args[TIMER_CHANNEL_NUMOF]; | ||||
|
|
||||
| typedef struct { | ||||
| unsigned long counter; | ||||
| tim_t dev; | ||||
| mutex_t mtx; | ||||
| } test_ctx_t; | ||||
|
|
||||
| static void cb_incr(void *arg, int chan) | ||||
| { | ||||
| (void)chan; | ||||
| test_ctx_t *ctx = arg; | ||||
|
|
||||
| ctx->counter++; | ||||
| if (ctx->counter < TEST_ITERATIONS) { | ||||
| /* Rescheduling the timer like this will trigger a bug in the lptmr | ||||
| * implementation in Kinetis */ | ||||
| timer_set(ctx->dev, chan, 20000u); | ||||
| timer_set(ctx->dev, chan, 0); | ||||
| } | ||||
| mutex_unlock(&ctx->mtx); | ||||
| } | ||||
|
|
||||
| static void cb(void *arg, int chan) | ||||
| { | ||||
| timeouts[chan] = sw_count; | ||||
|
|
@@ -84,6 +114,7 @@ static unsigned milliseconds_to_ticks(uint32_t timer_freq, unsigned millisecs) | |||
| { | ||||
| /* Use 64 bit arithmetic to avoid overflows for high frequencies. */ | ||||
| unsigned result = ((uint64_t)millisecs * US_PER_MS * timer_freq) / US_PER_SEC; | ||||
|
|
||||
| /* Never return less than MINIMUM_TICKS ticks */ | ||||
| return (result >= MINIMUM_TICKS) ? result : MINIMUM_TICKS; | ||||
| } | ||||
|
|
@@ -102,7 +133,7 @@ static int test_timer(unsigned num, uint32_t timer_freq) | |||
| } | ||||
|
|
||||
| printf(" - Calling timer_init(%u, %" PRIu32 ")\n ", | ||||
| num, timer_freq); | ||||
| num, timer_freq); | ||||
| /* initialize and halt timer */ | ||||
| if (timer_init(TIMER_DEV(num), timer_freq, cb, (void *)(COOKIE * num)) != 0) { | ||||
| printf("ERROR: timer_init() failed\n\n"); | ||||
|
|
@@ -177,6 +208,7 @@ static int test_timer(unsigned num, uint32_t 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); | ||||
|
|
@@ -207,6 +239,53 @@ static int test_timer(unsigned num, uint32_t timer_freq) | |||
| return 1; | ||||
| } | ||||
|
|
||||
|
|
||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| /* This test is designed to catch an implementation bug where a timer callback is | ||||
| * called directly from inside timer_set if the given timeout=0, leading to a | ||||
| * stack overflow if timer_set is called from within the callback of the same | ||||
| * timer. | ||||
| * | ||||
| * The test will attempt to initialize each timer in the system and set a non-zero | ||||
| * timeout at first. The callback function provided will then attempt to set a new | ||||
| * timeout=0 until we have called the callback TEST_ITERATIONS times (default 10000). | ||||
| * The expected behavior is that the timer will trigger again as soon as the timer | ||||
| * callback function returns. If the timer driver implementation is broken, then | ||||
| * the callback will be called again by timer_set, causing a stack overflow after a | ||||
| * number of iterations. */ | ||||
| static int test_timer_timeout(unsigned num, uint32_t timer_freq) | ||||
| { | ||||
| /* initialize and halt timer */ | ||||
| unsigned long switches = 0; | ||||
| test_ctx_t ctx = { | ||||
| .counter = 0, | ||||
| .dev = TIMER_DEV(num), | ||||
| .mtx = MUTEX_INIT_LOCKED | ||||
| }; | ||||
|
|
||||
| printf(" - Testing timeout=0 in callback:\n"); | ||||
|
|
||||
| if (timer_init(ctx.dev, timer_freq, cb_incr, &ctx) < 0) { | ||||
| printf(" TIMER_DEV(%u) init failed.\n", num); | ||||
| return 0; | ||||
| } | ||||
| /* Send the initial trigger for the timer */ | ||||
| timer_set(ctx.dev, 0, 100); | ||||
| /* Wait until we have executed the zero timeout callback enough times */ | ||||
| while (ctx.counter < TEST_ITERATIONS) { | ||||
| mutex_lock(&ctx.mtx); | ||||
| ++switches; | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
| } | ||||
|
|
||||
| /* verify results */ | ||||
| if (ctx.counter != TEST_ITERATIONS) { | ||||
| printf(" TIMER_DEV(%u) counter mismatch, expected: %lu, actual: %lu\n", | ||||
| num, TEST_ITERATIONS, ctx.counter); | ||||
| return 0; | ||||
| } | ||||
| printf(" OK (timer timeout successful)\n"); | ||||
| return 1; | ||||
| } | ||||
|
|
||||
| static uword_t query_freq_numof(tim_t dev) | ||||
| { | ||||
| if (IS_USED(MODULE_PERIPH_TIMER_QUERY_FREQS)) { | ||||
|
|
@@ -239,7 +318,8 @@ static void print_supported_frequencies(tim_t dev) | |||
| } | ||||
|
|
||||
| uword_t end = query_freq_numof(dev); | ||||
| printf(" - supported frequencies:\n"); | ||||
|
|
||||
| printf(" - supported frequencies:\n"); | ||||
| for (uword_t i = 0; i < MIN(end, 3); i++) { | ||||
| printf(" %u: %" PRIu32 "\n", (unsigned)i, timer_query_freqs(dev, i)); | ||||
| } | ||||
|
|
@@ -258,10 +338,11 @@ int main(void) | |||
| printf("Available timers: %i\n", TIMER_NUMOF); | ||||
|
|
||||
| int failed = 0; | ||||
|
|
||||
| /* test all configured timers */ | ||||
| for (unsigned i = 0; i < TIMER_NUMOF; i++) { | ||||
| printf("\nTIMER %u\n" | ||||
| "=======\n\n", i); | ||||
| "=======\n\n", i); | ||||
| print_supported_frequencies(TIMER_DEV(i)); | ||||
| uword_t end = query_freq_numof(TIMER_DEV(i)); | ||||
|
|
||||
|
|
@@ -271,7 +352,11 @@ int main(void) | |||
| * complete */ | ||||
| end = MIN(end, 3); | ||||
| for (uword_t j = 0; j < end; j++) { | ||||
| if (!test_timer(i, query_freq(TIMER_DEV(i), j))) { | ||||
| uint32_t freq = query_freq(TIMER_DEV(i), j); | ||||
| if (!test_timer(i, freq)) { | ||||
| failed = 1; | ||||
| } | ||||
| if (!test_timer_timeout(i, freq)) { | ||||
| failed = 1; | ||||
| } | ||||
| } | ||||
|
|
||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can someone explain to me the purpose of the mutex? It is locked in
test_timer_timeoutand unlocked in the timer callback?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a common pattern in RIOT for a thread to wait for an IRQ. Unlike a POSIX mutex, a RIOT mutex can be unlocked from a different context (and even from IRQ context).
The idea is that locking an already locked mutex will block. Unlocking that mutex from the ISR will unblock the thread.