From c0c00725c0b9e7ee65b374dcd6526fbfc1a592a3 Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Thu, 5 Sep 2024 16:28:22 -0700 Subject: [PATCH 1/9] test: simple multi alarm test An intentionally simple test for multiple alarms that only sets exactly two repeating alarms that should overlap frequently (one is twice the other's frequency). --- .../tests/multi_alarm_simple_test/Makefile | 11 +++++++++ .../tests/multi_alarm_simple_test/README.md | 7 ++++++ examples/tests/multi_alarm_simple_test/main.c | 23 +++++++++++++++++++ 3 files changed, 41 insertions(+) create mode 100644 examples/tests/multi_alarm_simple_test/Makefile create mode 100644 examples/tests/multi_alarm_simple_test/README.md create mode 100644 examples/tests/multi_alarm_simple_test/main.c diff --git a/examples/tests/multi_alarm_simple_test/Makefile b/examples/tests/multi_alarm_simple_test/Makefile new file mode 100644 index 000000000..54d6a7969 --- /dev/null +++ b/examples/tests/multi_alarm_simple_test/Makefile @@ -0,0 +1,11 @@ +# Makefile for user application + +# Specify this directory relative to the current application. +TOCK_USERLAND_BASE_DIR = ../../.. + +# Which files to compile. +C_SRCS := $(wildcard *.c) + +# Include userland master makefile. Contains rules and flags for actually +# building the application. +include $(TOCK_USERLAND_BASE_DIR)/AppMakefile.mk diff --git a/examples/tests/multi_alarm_simple_test/README.md b/examples/tests/multi_alarm_simple_test/README.md new file mode 100644 index 000000000..d71c0f623 --- /dev/null +++ b/examples/tests/multi_alarm_simple_test/README.md @@ -0,0 +1,7 @@ +# Test Multiple Alarms (Simple) + +This tests the virtual alarms available to userspace. It sets two +repeating alarms, at 500ms and 1s respectively, each prints the alarm +index (1 or 2), current tick and alarm expiration to the console. When +successful, both alarms should fire and alarm 1 should fire twice as +often as alarm 2. diff --git a/examples/tests/multi_alarm_simple_test/main.c b/examples/tests/multi_alarm_simple_test/main.c new file mode 100644 index 000000000..6e9d2379d --- /dev/null +++ b/examples/tests/multi_alarm_simple_test/main.c @@ -0,0 +1,23 @@ +#include +#include + +#include + +static void event_cb(__attribute__ ((unused)) uint32_t now, + __attribute__ ((unused)) uint32_t expiration, + void* ud) { + int i = (int)ud; + printf("%d %ld %ld\n", i, now, expiration); +} + +int main(void) { + libtock_alarm_t t1; + libtock_alarm_t t2; + + libtock_alarm_repeating_every_ms(500, event_cb, (void*)1, &t1); + libtock_alarm_repeating_every_ms(1000, event_cb, (void*)2, &t2); + + while (1) { + yield(); + } +} From f30488c6eee64b5b61e77c48ae8fa2e0b22a83e1 Mon Sep 17 00:00:00 2001 From: Amit Levy Date: Fri, 6 Sep 2024 00:34:52 -0700 Subject: [PATCH 2/9] Update examples/tests/multi_alarm_simple_test/main.c Co-authored-by: Pat Pannuto --- examples/tests/multi_alarm_simple_test/main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/examples/tests/multi_alarm_simple_test/main.c b/examples/tests/multi_alarm_simple_test/main.c index 6e9d2379d..a0dbb1708 100644 --- a/examples/tests/multi_alarm_simple_test/main.c +++ b/examples/tests/multi_alarm_simple_test/main.c @@ -3,9 +3,7 @@ #include -static void event_cb(__attribute__ ((unused)) uint32_t now, - __attribute__ ((unused)) uint32_t expiration, - void* ud) { +static void event_cb(uint32_t now, uint32_t expiration, void* ud) { int i = (int)ud; printf("%d %ld %ld\n", i, now, expiration); } From 3237e14305d77a885d0d38fe19a37711066a4b66 Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Fri, 13 Sep 2024 21:12:46 -0700 Subject: [PATCH 3/9] alarm: fix new alarm insertion when one of the alarms overflows Fixes #466 When inserting an alarm into the virtual alarm queue, comparing two alarms needs to account for alarms that may wrap beyond the clock overflow. Simply comparing computed expirations against the new alarm's reference isn't sufficient, as the current alarm may overflow, resulting in an expiration that is earlies by absolute value, but should still expire later. --- .../multi_alarm_simple_overflow_test/Makefile | 11 ++++++ .../README.md | 13 +++++++ .../multi_alarm_simple_overflow_test/main.c | 26 +++++++++++++ examples/tests/multi_alarm_simple_test/main.c | 2 +- libtock/services/alarm.c | 38 ++++++++++++++----- libtock/services/alarm.h | 4 ++ 6 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 examples/tests/multi_alarm_simple_overflow_test/Makefile create mode 100644 examples/tests/multi_alarm_simple_overflow_test/README.md create mode 100644 examples/tests/multi_alarm_simple_overflow_test/main.c diff --git a/examples/tests/multi_alarm_simple_overflow_test/Makefile b/examples/tests/multi_alarm_simple_overflow_test/Makefile new file mode 100644 index 000000000..54d6a7969 --- /dev/null +++ b/examples/tests/multi_alarm_simple_overflow_test/Makefile @@ -0,0 +1,11 @@ +# Makefile for user application + +# Specify this directory relative to the current application. +TOCK_USERLAND_BASE_DIR = ../../.. + +# Which files to compile. +C_SRCS := $(wildcard *.c) + +# Include userland master makefile. Contains rules and flags for actually +# building the application. +include $(TOCK_USERLAND_BASE_DIR)/AppMakefile.mk diff --git a/examples/tests/multi_alarm_simple_overflow_test/README.md b/examples/tests/multi_alarm_simple_overflow_test/README.md new file mode 100644 index 000000000..34b41cec1 --- /dev/null +++ b/examples/tests/multi_alarm_simple_overflow_test/README.md @@ -0,0 +1,13 @@ +# Test Multiple Alarms (With Overflow) + +This tests the virtual alarms available to userspace. It sets two +alarms, first one that overflows the alarm, such that it's expiration +is small in absolute value (but should shouldn't fire until after the +clock overflows) and one after 1s. When successful, the second (1s) +alarm should fire after 1 second, while the second alarm should fire +after the clock overflows (approximately 7 minutes if the clock is at +32kHz). + +If the virtual alarm library is buggy, this test might fail by +scheduling the 1 second alarm _after_ the longer wrapping alarm, and +it won't fire immediately. diff --git a/examples/tests/multi_alarm_simple_overflow_test/main.c b/examples/tests/multi_alarm_simple_overflow_test/main.c new file mode 100644 index 000000000..e1d104492 --- /dev/null +++ b/examples/tests/multi_alarm_simple_overflow_test/main.c @@ -0,0 +1,26 @@ +#include +#include + +#include + +static void event_cb(uint32_t now, uint32_t expiration, void* ud) { + int i = (int)ud; + printf("%d %lu %lu\n", i, now, expiration); +} + +int main(void) { + libtock_alarm_t t1; + libtock_alarm_t t2; + + uint32_t now; + libtock_alarm_command_read(&now); + + uint32_t overflow_ms = libtock_alarm_ticks_to_ms(UINT32_MAX); + + libtock_alarm_in_ms(overflow_ms, event_cb, (void*)1, &t1); + libtock_alarm_in_ms(1000, event_cb, (void*)2, &t2); + + while (1) { + yield(); + } +} diff --git a/examples/tests/multi_alarm_simple_test/main.c b/examples/tests/multi_alarm_simple_test/main.c index a0dbb1708..049356884 100644 --- a/examples/tests/multi_alarm_simple_test/main.c +++ b/examples/tests/multi_alarm_simple_test/main.c @@ -5,7 +5,7 @@ static void event_cb(uint32_t now, uint32_t expiration, void* ud) { int i = (int)ud; - printf("%d %ld %ld\n", i, now, expiration); + printf("%d %lu %lu\n", i, now, expiration); } int main(void) { diff --git a/libtock/services/alarm.c b/libtock/services/alarm.c index feca19987..e86fb10bb 100644 --- a/libtock/services/alarm.c +++ b/libtock/services/alarm.c @@ -5,11 +5,6 @@ #define MAX_TICKS UINT32_MAX -// Returns 0 if a <= b < c, 1 otherwise -static int within_range(uint32_t a, uint32_t b, uint32_t c) { - return (b - a) < (b - c); -} - /** \brief Convert milliseconds to clock ticks * * WARNING: This function will assert if the output @@ -40,7 +35,7 @@ static uint32_t ms_to_ticks(uint32_t ms) { return ticks; } -static uint32_t ticks_to_ms(uint32_t ticks) { +uint32_t libtock_alarm_ticks_to_ms(uint32_t ticks) { // `ticks_to_ms`'s conversion will be accurate to within the range // 0 to 1 milliseconds less than the exact conversion // (true millisecond conversion - [0,1) milliseconds). @@ -75,6 +70,13 @@ static uint32_t ticks_to_ms(uint32_t ticks) { static libtock_alarm_ticks_t* root = NULL; static void root_insert(libtock_alarm_ticks_t* alarm) { + // We want to insert the new alarm just before an existing alarm + // that should expire next after it, as `alarm_upcall` breaks as + // soon as it finds an alarm that should not fire yet. To do so, we + // need to account for an empty list, a non-empty list where the new + // alarm should be last, as well as computing relative expirations + // when alarm expirations may overflow the clock 0 or 1 times. + if (root == NULL) { root = alarm; root->next = NULL; @@ -82,12 +84,30 @@ static void root_insert(libtock_alarm_ticks_t* alarm) { return; } + // Compute the tick value at which the new alarm will expire. + uint32_t new_expiration = alarm->reference + alarm->dt; + // Determine whether the clock overflows before the new alarm + // expires. Because ticks are 32-bit, a clock can overflow at most + // once before a 32-bit alarm fires. + bool new_overflows = alarm->reference > UINT32_MAX - alarm->dt; + libtock_alarm_ticks_t** cur = &root; libtock_alarm_ticks_t* prev = NULL; while (*cur != NULL) { + // Compute the tick value at which this alarm will expire. uint32_t cur_expiration = (*cur)->reference + (*cur)->dt; - uint32_t new_expiration = alarm->reference + alarm->dt; - if (!within_range(alarm->reference, cur_expiration, new_expiration)) { + // Determine whether the clock overflows before this alarm + // expires. + bool cur_overflows = (*cur)->reference > UINT32_MAX - (*cur)->dt; + + // This alarm happens after the new alarm if: + // - neither expirations overflow and this expiration value is larger than the new expiration + // - both overflow and this expiration value is larger than the new expiration + // - or, this alarm overflows but the new one doesn't + // + // If the new alarm overflows and this alarm doesn't, this alarm + // happens _before_ the new alarm. + if (!(!cur_overflows && new_overflows) && ((cur_overflows && !new_overflows) || cur_expiration > new_expiration)) { // insert before libtock_alarm_ticks_t* tmp = *cur; *cur = alarm; @@ -231,7 +251,7 @@ int libtock_alarm_in_ms(uint32_t ms, libtock_alarm_callback cb, void* opaque, li // schedule multiple alarms to reach the full length. We calculate the number of full overflows // and the remainder ticks to reach the target length of time. The overflows use the // `overflow_callback` for each intermediate overflow. - const uint32_t max_ticks_in_ms = ticks_to_ms(MAX_TICKS); + const uint32_t max_ticks_in_ms = libtock_alarm_ticks_to_ms(MAX_TICKS); if (ms > max_ticks_in_ms) { // overflows_left is the number of intermediate alarms that need to be scheduled to reach the target // dt_ms. After the alarm in this block is scheduled, we have this many overflows left (hence the reason diff --git a/libtock/services/alarm.h b/libtock/services/alarm.h index 29ddfbb15..285abca7e 100644 --- a/libtock/services/alarm.h +++ b/libtock/services/alarm.h @@ -31,6 +31,10 @@ extern "C" { // - `arg3` (`opaque`): An arbitrary user pointer passed back to the callback. typedef void (*libtock_alarm_callback)(uint32_t, uint32_t, void*); +/** \brief Utility function for converting hardware-specific tics to milliseconds + */ +uint32_t libtock_alarm_ticks_to_ms(uint32_t); + /** \brief Opaque handle to a single-shot alarm. * * An opaque handle to an alarm created by `alarm_at` or `alarm_in`. Memory From 7ecf0302a925987e708e028cb4bd731592ff7c65 Mon Sep 17 00:00:00 2001 From: Amit Levy Date: Fri, 20 Sep 2024 11:02:36 -0700 Subject: [PATCH 4/9] Apply suggestions from code review Co-authored-by: Pat Pannuto Co-authored-by: Branden Ghena --- examples/tests/multi_alarm_simple_overflow_test/README.md | 4 ++-- libtock/services/alarm.c | 4 ++-- libtock/services/alarm.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/tests/multi_alarm_simple_overflow_test/README.md b/examples/tests/multi_alarm_simple_overflow_test/README.md index 34b41cec1..4375d00eb 100644 --- a/examples/tests/multi_alarm_simple_overflow_test/README.md +++ b/examples/tests/multi_alarm_simple_overflow_test/README.md @@ -1,10 +1,10 @@ # Test Multiple Alarms (With Overflow) This tests the virtual alarms available to userspace. It sets two -alarms, first one that overflows the alarm, such that it's expiration +alarms, first one that overflows the alarm, such that its expiration is small in absolute value (but should shouldn't fire until after the clock overflows) and one after 1s. When successful, the second (1s) -alarm should fire after 1 second, while the second alarm should fire +alarm should fire after 1 second, while the first alarm should wait to fire until after the clock overflows (approximately 7 minutes if the clock is at 32kHz). diff --git a/libtock/services/alarm.c b/libtock/services/alarm.c index e86fb10bb..5bce5b4a9 100644 --- a/libtock/services/alarm.c +++ b/libtock/services/alarm.c @@ -89,7 +89,7 @@ static void root_insert(libtock_alarm_ticks_t* alarm) { // Determine whether the clock overflows before the new alarm // expires. Because ticks are 32-bit, a clock can overflow at most // once before a 32-bit alarm fires. - bool new_overflows = alarm->reference > UINT32_MAX - alarm->dt; + bool new_overflows = alarm->reference > (UINT32_MAX - alarm->dt); libtock_alarm_ticks_t** cur = &root; libtock_alarm_ticks_t* prev = NULL; @@ -98,7 +98,7 @@ static void root_insert(libtock_alarm_ticks_t* alarm) { uint32_t cur_expiration = (*cur)->reference + (*cur)->dt; // Determine whether the clock overflows before this alarm // expires. - bool cur_overflows = (*cur)->reference > UINT32_MAX - (*cur)->dt; + bool cur_overflows = (*cur)->reference > (UINT32_MAX - (*cur)->dt); // This alarm happens after the new alarm if: // - neither expirations overflow and this expiration value is larger than the new expiration diff --git a/libtock/services/alarm.h b/libtock/services/alarm.h index 285abca7e..6487e3140 100644 --- a/libtock/services/alarm.h +++ b/libtock/services/alarm.h @@ -31,7 +31,7 @@ extern "C" { // - `arg3` (`opaque`): An arbitrary user pointer passed back to the callback. typedef void (*libtock_alarm_callback)(uint32_t, uint32_t, void*); -/** \brief Utility function for converting hardware-specific tics to milliseconds +/** \brief Utility function for converting hardware-specific ticks to milliseconds */ uint32_t libtock_alarm_ticks_to_ms(uint32_t); From 3b11c22a387711b16b4e2fa6a2a67ceea839f734 Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Fri, 20 Sep 2024 11:05:44 -0700 Subject: [PATCH 5/9] Typo --- .../tests/multi_alarm_simple_overflow_test/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/tests/multi_alarm_simple_overflow_test/README.md b/examples/tests/multi_alarm_simple_overflow_test/README.md index 4375d00eb..63e28db28 100644 --- a/examples/tests/multi_alarm_simple_overflow_test/README.md +++ b/examples/tests/multi_alarm_simple_overflow_test/README.md @@ -2,11 +2,11 @@ This tests the virtual alarms available to userspace. It sets two alarms, first one that overflows the alarm, such that its expiration -is small in absolute value (but should shouldn't fire until after the -clock overflows) and one after 1s. When successful, the second (1s) -alarm should fire after 1 second, while the first alarm should wait to fire until -after the clock overflows (approximately 7 minutes if the clock is at -32kHz). +is small in absolute value (but shouldn't fire until after the clock +overflows) and one after 1s. When successful, the second (1s) alarm +should fire after 1 second, while the first alarm should wait to fire +until after the clock overflows (approximately 7 minutes if the clock +is at 32kHz). If the virtual alarm library is buggy, this test might fail by scheduling the 1 second alarm _after_ the longer wrapping alarm, and From 3a9c33a47593765bdf0750ba8ed19ef82274bb0e Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Fri, 20 Sep 2024 11:06:12 -0700 Subject: [PATCH 6/9] Add example output to multi_alarm_simple_test README --- .../tests/multi_alarm_simple_test/README.md | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/examples/tests/multi_alarm_simple_test/README.md b/examples/tests/multi_alarm_simple_test/README.md index d71c0f623..04e71b7e7 100644 --- a/examples/tests/multi_alarm_simple_test/README.md +++ b/examples/tests/multi_alarm_simple_test/README.md @@ -5,3 +5,36 @@ repeating alarms, at 500ms and 1s respectively, each prints the alarm index (1 or 2), current tick and alarm expiration to the console. When successful, both alarms should fire and alarm 1 should fire twice as often as alarm 2. + +## Example Output + +Correct: + +```sh +1 6316032 6314240 +2 10512384 10510592 +1 10512384 10511104 +1 14722560 14720768 +2 18903552 18901760 +1 18919680 18917632 +1 23116544 23114752 +2 27294720 27292928 +... +``` + +(Note that timestamps, the second and third column, and exact ordering of `1` and `2` will differ by execution) + +Incorrect: + +```sh +1 7254784 7252992 +2 11451136 11449344 +2 19842304 19840512 +2 28233472 28231680 +2 36624640 36622848 +2 45015808 45014016 +2 53406976 53405184 +... +``` + +Note that only alarm `2` appears after the first `1`. From ffc7f8bae4ec9f858ae16bcaa58e313468bebe13 Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Fri, 20 Sep 2024 11:14:26 -0700 Subject: [PATCH 7/9] alarm.c: clarify overflow logic --- libtock/services/alarm.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/libtock/services/alarm.c b/libtock/services/alarm.c index 5bce5b4a9..7d0727d39 100644 --- a/libtock/services/alarm.c +++ b/libtock/services/alarm.c @@ -100,14 +100,15 @@ static void root_insert(libtock_alarm_ticks_t* alarm) { // expires. bool cur_overflows = (*cur)->reference > (UINT32_MAX - (*cur)->dt); - // This alarm happens after the new alarm if: - // - neither expirations overflow and this expiration value is larger than the new expiration - // - both overflow and this expiration value is larger than the new expiration - // - or, this alarm overflows but the new one doesn't + // This alarm (`cur`) happens after the new alarm (`alarm`) if: + // - both overflow or neither overflow, and cur expiration is + // larger than the new expiration + // - or, only cur overflows // // If the new alarm overflows and this alarm doesn't, this alarm // happens _before_ the new alarm. - if (!(!cur_overflows && new_overflows) && ((cur_overflows && !new_overflows) || cur_expiration > new_expiration)) { + if ((cur_overflows == new_overflows && cur_expiration > new_expiration) || + cur_overflows) { // insert before libtock_alarm_ticks_t* tmp = *cur; *cur = alarm; From 099ae64ad3230c5aa5b58ffbbf385520f64419a8 Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Fri, 20 Sep 2024 11:23:49 -0700 Subject: [PATCH 8/9] Add example output to multi_alarm_simple_overflow README --- .../multi_alarm_simple_overflow_test/README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/examples/tests/multi_alarm_simple_overflow_test/README.md b/examples/tests/multi_alarm_simple_overflow_test/README.md index 63e28db28..bb5d8209d 100644 --- a/examples/tests/multi_alarm_simple_overflow_test/README.md +++ b/examples/tests/multi_alarm_simple_overflow_test/README.md @@ -11,3 +11,21 @@ is at 32kHz). If the virtual alarm library is buggy, this test might fail by scheduling the 1 second alarm _after_ the longer wrapping alarm, and it won't fire immediately. + +# Example Output + +Correct: + +``` +2 10512380 10511329 +1 3 1 +``` + +Incorrect: + +(after no output for an entire clock overflow, e.g. ~7 minutes) + +``` +1 3 1 +2 10512341 10511329 +``` From 702b4c14de38b0901a30cb95b3b032cdff2e1230 Mon Sep 17 00:00:00 2001 From: Pat Pannuto Date: Mon, 23 Sep 2024 15:30:58 -0700 Subject: [PATCH 9/9] alarm: parentheses are cheap, precedence is hard --- libtock/services/alarm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libtock/services/alarm.c b/libtock/services/alarm.c index 7d0727d39..296b936e7 100644 --- a/libtock/services/alarm.c +++ b/libtock/services/alarm.c @@ -107,7 +107,7 @@ static void root_insert(libtock_alarm_ticks_t* alarm) { // // If the new alarm overflows and this alarm doesn't, this alarm // happens _before_ the new alarm. - if ((cur_overflows == new_overflows && cur_expiration > new_expiration) || + if (((cur_overflows == new_overflows) && (cur_expiration > new_expiration)) || cur_overflows) { // insert before libtock_alarm_ticks_t* tmp = *cur;