Merge pull request #10717 from keestux/refactor-evtimer-add-del

sys/evtimer: refactor evtimer add()/del()

sys/evtimer/evtimer.c

@@ -31,64 +31,63 @@
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
-/* XXX this function is intentionally non-static, since the optimizer can't
+static void _add_event_to_list(evtimer_t *evtimer, evtimer_event_t *event)
- * handle the pointer hack in this function */
-void evtimer_add_event_to_list(evtimer_t *evtimer, evtimer_event_t *event)
 {
     DEBUG("evtimer: new event offset %" PRIu32 " ms\n", event->offset);
-    /* we want list->next to point to the first list element. thus we take the
+    evtimer_event_t **list = &evtimer->events;
-     * *address* of evtimer->events, then cast it from (evtimer_event_t **) to
-     * (evtimer_event_t*). After that, list->next actually equals
-     * evtimer->events. */
-    evtimer_event_t *list = (evtimer_event_t *)&evtimer->events;
 
-    while (list->next) {
+    while (*list) {
-        evtimer_event_t *list_entry = list->next;
         /* Stop when new event time is nearer then next */
-        if (event->offset < list_entry->offset) {
+        if (event->offset < (*list)->offset) {
             DEBUG("evtimer: next %" PRIu32 " < %" PRIu32 " ms\n",
-                  event->offset, list_entry->offset);
+                  event->offset, (*list)->offset);
             break;
         }
         /* Set event offset relative to previous event */
-        event->offset -= list_entry->offset;
+        event->offset -= (*list)->offset;
-        list = list->next;
+        list = &(*list)->next;
     }
 
     DEBUG("evtimer: new event relativ offset %" PRIu32 " ms\n", event->offset);
 
     /* Set found next bigger event after new event */
-    event->next = list->next;
+    event->next = *list;
-    if (list->next) {
+    if (*list) {
         /* Set offset following event relative to new event */
-        evtimer_event_t *next_entry = list->next;
         DEBUG("evtimer: recalculate offset for %" PRIu32 " ms\n",
-              next_entry->offset);
+              (*list)->offset);
 
-        next_entry->offset -= event->offset;
+        (*list)->offset -= event->offset;
 
         DEBUG("evtimer: resulting new event offset %" PRIu32 " ms\n",
-              next_entry->offset);
+              (*list)->offset);
     }
 
-    list->next = event;
+    *list = event;
 }
 
 static void _del_event_from_list(evtimer_t *evtimer, evtimer_event_t *event)
 {
-    evtimer_event_t *list = (evtimer_event_t *) &evtimer->events;
+    evtimer_event_t **list = &evtimer->events;
 
-    while (list->next) {
+    /* Find the entry to delete from the list */
-        evtimer_event_t *list_entry = list->next;
+    while (*list) {
-        if (list_entry == event) {
+        if (*list == event) {
-            list->next = event->next;
-            if (list->next) {
-                list_entry = list->next;
-                list_entry->offset += event->offset;
-            }
             break;
         }
-        list = list->next;
+        list = &(*list)->next;
+    }
+
+    if (*list) {
+        /* This is the entry we want to remove */
+        *list = (*list)->next;
+        /* If the deleted entry was _not_ the last one
+         * then update 'offset' of the entry that
+         * followed.
+         */
+        if (*list) {
+            (*list)->offset += event->offset;
+        }
     }
 }
 
@@ -145,7 +144,7 @@ void evtimer_add(evtimer_t *evtimer, evtimer_event_t *event)
     DEBUG("evtimer_add(): adding event with offset %" PRIu32 "\n", event->offset);
 
     _update_head_offset(evtimer);
-    evtimer_add_event_to_list(evtimer, event);
+    _add_event_to_list(evtimer, event);
     if (evtimer->events == event) {
         _set_timer(&evtimer->timer, event->offset);
     }
@@ -210,10 +209,11 @@ void evtimer_init(evtimer_t *evtimer, evtimer_callback_t handler)
 void evtimer_print(const evtimer_t *evtimer)
 {
     evtimer_event_t *list = evtimer->events;
+    int nr = 0;
 
-    while (list->next) {
+    while (list) {
-        evtimer_event_t *list_entry = list->next;
+        nr++;
-        printf("ev offset=%u\n", (unsigned)list_entry->offset);
+        printf("ev #%d offset=%u\n", nr, (unsigned)list->offset);
         list = list->next;
     }
 }

tests/evtimer_msg/main.c

@@ -27,14 +27,20 @@
 
 static char worker_stack[THREAD_STACKSIZE_MAIN];
 static evtimer_t evtimer;
-static evtimer_msg_event_t events[] = {
+#define NEVENTS (unsigned)(4)
-    { .event = { .offset = 1000 }, .msg = { .content = { .ptr = "supposed to be 1000" } } },
+/*
-    { .event = { .offset = 1500 }, .msg = { .content = { .ptr = "supposed to be 1500" } } },
+ * The events (.offset) are modified by evtimer_add.
-    { .event = { .offset = 659 }, .msg = { .content = { .ptr = "supposed to be 659" } } },
+ * This list of offsets is used to populate the events
-    { .event = { .offset = 3954 }, .msg = { .content = { .ptr = "supposed to be 3954" } } },
+ * before adding (or re-adding).
+ */
+static uint32_t offsets[NEVENTS] = {
+        1000,
+        1500,
+        659,
+        3954,
 };
-
+static evtimer_msg_event_t events[NEVENTS];
-#define NEVENTS ((unsigned)(sizeof(events) / sizeof(evtimer_msg_event_t)))
+static char texts[NEVENTS][40];
 
 /* This thread will print the drift to stdout once per second */
 void *worker_thread(void *arg)
@@ -56,7 +62,7 @@ void *worker_thread(void *arg)
 
 int main(void)
 {
-    uint32_t now = xtimer_now_usec() / US_PER_MS;
+    uint32_t now;
 
     evtimer_init_msg(&evtimer);
 
@@ -65,11 +71,47 @@ int main(void)
                                      THREAD_PRIORITY_MAIN - 1,
                                      THREAD_CREATE_STACKTEST,
                                      worker_thread, NULL, "worker");
-    printf("Testing generic evtimer (start time = %" PRIu32 " ms)\n", now);
+
+    printf("Testing generic evtimer\n");
+
+    /* Add all the events */
     for (unsigned i = 0; i < NEVENTS; i++) {
+        events[i].event.offset = offsets[i];
+        now = xtimer_now_usec() / US_PER_MS;
+        snprintf(texts[i], sizeof(texts[i]) - 1, "#%u supposed to be %" PRIu32, i, now + events[i].event.offset);
+        events[i].msg.content.ptr = texts[i];
         evtimer_add_msg(&evtimer, &events[i], pid);
     }
+
+    /* Delete all the events */
+    /* First we delete the last, to test deleting the last */
+    /* Then we delete the first, to test deleting the first */
+    evtimer_del(&evtimer, &events[3].event);
+    evtimer_del(&evtimer, &events[2].event);
+    printf("This should list %u items\n", NEVENTS - 2);
+    evtimer_print(&evtimer);
+
+    /* Delete the remaining entries */
+    for (unsigned i = 0; i < NEVENTS; i++) {
+        evtimer_del(&evtimer, &events[i].event);
+    }
+
+    /* Add all the events, again */
+    for (unsigned i = 0; i < NEVENTS; i++) {
+        events[i].event.offset = offsets[i];
+        now = xtimer_now_usec() / US_PER_MS;
+        snprintf(texts[i], sizeof(texts[i]) - 1, "#%u supposed to be %" PRIu32, i, now + events[i].event.offset);
+        events[i].msg.content.ptr = texts[i];
+        evtimer_add_msg(&evtimer, &events[i], pid);
+    }
+    printf("This should list %u items\n", NEVENTS);
+    evtimer_print(&evtimer);
+
     printf("Are the reception times of all %u msgs close to the supposed values?\n",
            NEVENTS);
+
+    /* The last offset is the largest, wait for it and a tiny bit more */
+    xtimer_usleep((offsets[3] + 10) * US_PER_MS);
+    puts("By now all msgs should have been received");
     puts("If yes, the tests were successful");
 }

tests/evtimer_msg/tests/01-run.py

@@ -10,20 +10,20 @@ from __future__ import print_function
 import sys
 from testrunner import run
 
-ACCEPTED_ERROR = 20
+ACCEPTED_ERROR = 2
 
 
 def testfunc(child):
-    child.expect(r"Testing generic evtimer \(start time = (\d+) ms\)")
+    child.expect(r"Testing generic evtimer")
-    timer_offset = int(child.match.group(1))
     child.expect(r"Are the reception times of all (\d+) msgs close to the supposed values?")
     numof = int(child.match.group(1))
 
     for i in range(numof):
-        child.expect(r'At \s*(\d+) ms received msg %i: "supposed to be (\d+)"' % i)
+        child.expect(r'At \s*(\d+) ms received msg %i: "#\d+ supposed to be (\d+)"' % i)
         # check if output is correct
-        exp = int(child.match.group(2)) + timer_offset
+        expected = int(child.match.group(2))
-        assert(int(child.match.group(1)) in range(exp - ACCEPTED_ERROR, exp + ACCEPTED_ERROR + 1))
+        actual = int(child.match.group(1))
+        assert(actual in range(expected - ACCEPTED_ERROR, expected + ACCEPTED_ERROR))
         print(".", end="", flush=True)
     print("")
     print("All tests successful")