Merge pull request #466 from tock/multi_alarm

Fix multi-alarm overlapping bug

Author: ppannuto

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

examples/tests/multi_alarm_simple_overflow_test/README.md

@@ -0,0 +1,31 @@
+# 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 its expiration
+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
+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
+```

examples/tests/multi_alarm_simple_overflow_test/main.c

@@ -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();
+  }
+}

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

examples/tests/multi_alarm_simple_test/README.md

@@ -0,0 +1,40 @@
+# 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.
+
+## 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`.

examples/tests/multi_alarm_simple_test/main.c

@@ -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();
+  }
+}

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,19 +70,45 @@ 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);
+
   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 (`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_expiration > new_expiration)) ||
+        cur_overflows) {
       // insert before
       libtock_alarm_ticks_t* tmp = *cur;
       *cur        = alarm;
@@ -231,7 +252,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

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 ticks 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