Skip to content
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

Fix multi-alarm overlapping bug #466

Merged
merged 9 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions examples/tests/multi_alarm_simple_overflow_test/Makefile
Original file line number Diff line number Diff line change
@@ -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
13 changes: 13 additions & 0 deletions examples/tests/multi_alarm_simple_overflow_test/README.md
Original file line number Diff line number Diff line change
@@ -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
alevy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should or shouldn't? Pick one please

clock overflows) and one after 1s. When successful, the second (1s)
alarm should fire after 1 second, while the second alarm should fire
alevy marked this conversation as resolved.
Show resolved Hide resolved
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.
ppannuto marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 26 additions & 0 deletions examples/tests/multi_alarm_simple_overflow_test/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include <stdio.h>
#include <stdlib.h>

#include <libtock-sync/services/alarm.h>

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();
}
}
11 changes: 11 additions & 0 deletions examples/tests/multi_alarm_simple_test/Makefile
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions examples/tests/multi_alarm_simple_test/README.md
Original file line number Diff line number Diff line change
@@ -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.
ppannuto marked this conversation as resolved.
Show resolved Hide resolved
21 changes: 21 additions & 0 deletions examples/tests/multi_alarm_simple_test/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#include <stdio.h>
#include <stdlib.h>

#include <libtock-sync/services/alarm.h>

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;

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();
}
}
38 changes: 29 additions & 9 deletions libtock/services/alarm.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -75,19 +70,44 @@ 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;
root->prev = NULL;
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;
alevy marked this conversation as resolved.
Show resolved Hide resolved

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;
alevy marked this conversation as resolved.
Show resolved Hide resolved

// This alarm happens after the new alarm if:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "this alarm" and what is "the new alarm"? I think it's cur and alarm respectively? But I might have that backwards. In my opinion, this entire comment block should use the actual names for clarity.

// - 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double negations and nested operations make this somewhat tricky to parse. Perhaps altering the conditional's logic to match the 4 statements enumerated in the comment more closely would improve readability. Something to the effect of:

( case1 || case2 || case3 || case4)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly agree. I really can't parse line 110.

Based on my reading of the comment, the logic would be if ((cur_overflows && !new_overflows) || (cur_expiration > new_expiration)). I don't understand why the first part of the condition is there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't quite help myself from being a tiny bit clever still, but I believe the new version of both the comment and boolean logic are clear now.

// insert before
libtock_alarm_ticks_t* tmp = *cur;
*cur = alarm;
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions libtock/services/alarm.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
alevy marked this conversation as resolved.
Show resolved Hide resolved
*/
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
Expand Down
Loading