Merge pull request #3779 from geky/ncs36510-timing

NCS36510: Fix the sporadic semaphore timing issue

Author: sg-

TESTS/events/queue/main.cpp

@@ -70,7 +70,7 @@ SIMPLE_POSTS_TEST(0)
 
 
 void time_func(Timer *t, int ms) {
-    TEST_ASSERT_INT_WITHIN(2, ms, t->read_ms());
+    TEST_ASSERT_INT_WITHIN(5, ms, t->read_ms());
     t->reset();
 }
 

TESTS/events/timing/main.cpp

@@ -0,0 +1,122 @@
+#include "mbed_events.h"
+#include "mbed.h"
+#include "rtos.h"
+#include "greentea-client/test_env.h"
+#include "unity.h"
+#include "utest.h"
+#include <cstdlib>
+#include <cmath>
+
+using namespace utest::v1;
+
+
+// Test delay
+#ifndef TEST_EVENTS_TIMING_TIME
+#define TEST_EVENTS_TIMING_TIME 20000
+#endif
+
+#ifndef TEST_EVENTS_TIMING_MEAN
+#define TEST_EVENTS_TIMING_MEAN 25
+#endif
+
+#ifndef M_PI
+#define M_PI 3.14159265358979323846264338327950288
+#endif
+
+// Random number generation to skew timing values
+float gauss(float mu, float sigma) {
+    float x = (float)rand() / ((float)RAND_MAX+1);
+    float y = (float)rand() / ((float)RAND_MAX+1);
+    float x2pi = x*2.0*M_PI;
+    float g2rad = sqrt(-2.0 * log(1.0-y));
+    float z = cos(x2pi) * g2rad;
+    return mu + z*sigma;
+}
+
+float chisq(float sigma) {
+    return pow(gauss(0, sqrt(sigma)), 2);
+}
+
+
+Timer timer;
+DigitalOut led(LED1);
+
+equeue_sema_t sema;
+
+// Timer timing test
+void timer_timing_test() {
+    timer.reset();
+    timer.start();
+    int prev = timer.read_us();
+
+    while (prev < TEST_EVENTS_TIMING_TIME*1000) {
+        int next = timer.read_us();
+        if (next < prev) {
+            printf("backwards drift %d -> %d (%08x -> %08x)\r\n",
+                prev, next, prev, next);
+        }
+        TEST_ASSERT(next >= prev);
+        prev = next;
+    }
+}
+
+// equeue tick timing test
+void tick_timing_test() {
+    unsigned start = equeue_tick();
+    int prev = 0;
+
+    while (prev < TEST_EVENTS_TIMING_TIME) {
+        int next = equeue_tick() - start;
+        if (next < prev) {
+            printf("backwards drift %d -> %d (%08x -> %08x)\r\n",
+                prev, next, prev, next);
+        }
+        TEST_ASSERT(next >= prev);
+        prev = next;
+    }
+}
+
+// equeue semaphore timing test
+void semaphore_timing_test() {
+    srand(0);
+    timer.reset();
+    timer.start();
+
+    int err = equeue_sema_create(&sema);
+    TEST_ASSERT_EQUAL(0, err);
+
+    while (timer.read_ms() < TEST_EVENTS_TIMING_TIME) {
+        int delay = chisq(TEST_EVENTS_TIMING_MEAN);
+
+        int start = timer.read_us();
+        equeue_sema_wait(&sema, delay);
+        int taken = timer.read_us() - start;
+
+        printf("delay %dms => error %dus\r\n", delay, abs(1000*delay - taken));
+        TEST_ASSERT_INT_WITHIN(5000, taken, delay * 1000);
+
+        led = !led;
+    }
+
+    equeue_sema_destroy(&sema);
+}
+
+
+// Test setup
+utest::v1::status_t test_setup(const size_t number_of_cases) {
+    GREENTEA_SETUP((number_of_cases+1)*TEST_EVENTS_TIMING_TIME, "default_auto");
+    return verbose_test_setup_handler(number_of_cases);
+}
+
+const Case cases[] = {
+    Case("Testing accuracy of timer", timer_timing_test),
+    Case("Testing accuracy of equeue tick", tick_timing_test),
+    Case("Testing accuracy of equeue semaphore", semaphore_timing_test),
+};
+
+Specification specification(test_setup, cases);
+
+int main() {
+    return !Harness::run(specification);
+}
+

events/equeue/equeue_mbed.cpp

@@ -26,15 +26,15 @@
 
 // Ticker operations
 static bool equeue_tick_inited = false;
-static unsigned equeue_minutes = 0;
+static volatile unsigned equeue_minutes = 0;
 static unsigned equeue_timer[
         (sizeof(Timer)+sizeof(unsigned)-1)/sizeof(unsigned)];
 static unsigned equeue_ticker[
         (sizeof(Ticker)+sizeof(unsigned)-1)/sizeof(unsigned)];
 
 static void equeue_tick_update() {
+    equeue_minutes += reinterpret_cast<Timer*>(equeue_timer)->read_ms();
     reinterpret_cast<Timer*>(equeue_timer)->reset();
-    equeue_minutes += 1;
 }
 
 static void equeue_tick_init() {
@@ -48,7 +48,7 @@ static void equeue_tick_init() {
     equeue_minutes = 0;
     reinterpret_cast<Timer*>(equeue_timer)->start();
     reinterpret_cast<Ticker*>(equeue_ticker)
-            ->attach_us(equeue_tick_update, (1 << 16)*1000);
+            ->attach_us(equeue_tick_update, 1000 << 16);
 
     equeue_tick_inited = true;
 }
@@ -58,8 +58,15 @@ unsigned equeue_tick() {
         equeue_tick_init();
     }
 
-    unsigned equeue_ms = reinterpret_cast<Timer*>(equeue_timer)->read_ms();
-    return (equeue_minutes << 16) + equeue_ms;
+    unsigned minutes;
+    unsigned ms;
+
+    do {
+        minutes = equeue_minutes;
+        ms = reinterpret_cast<Timer*>(equeue_timer)->read_ms();
+    } while (minutes != equeue_minutes);
+
+    return minutes + ms;
 }
 
 

targets/TARGET_ONSEMI/TARGET_NCS36510/ncs36510_us_ticker_api.c

@@ -37,7 +37,7 @@ static int us_ticker_inited = 0;
 
 static void us_timer_init(void);
 
-static uint32_t us_ticker_int_counter = 0;
+static uint32_t us_ticker_target = 0;
 static volatile uint32_t msb_counter = 0;
 
 void us_ticker_init(void)
@@ -168,20 +168,25 @@ extern void us_ticker_isr(void)
     /* Clear IRQ flag */
     TIM1REG->CLEAR = 0;
 
-    /* If this is a longer timer it will take multiple full hw counter cycles */
-    if (us_ticker_int_counter > 0) {
-        ticker_set(0xFFFF);
-        us_ticker_int_counter--;
-    } else {
+    int32_t delta = us_ticker_target - us_ticker_read();
+    if (delta <= 0) {
         TIM1REG->CONTROL.BITS.ENABLE = False;
         us_ticker_irq_handler();
+    } else {
+        // Clamp at max value of timer
+        if (delta > 0xFFFF) {
+            delta = 0xFFFF;
+        }
+
+        ticker_set(delta);
     }
 }
 
 /* Set timer 1 ticker interrupt */
 void us_ticker_set_interrupt(timestamp_t timestamp)
 {
-    int32_t delta = (uint32_t)timestamp - us_ticker_read();
+    us_ticker_target = (uint32_t)timestamp;
+    int32_t delta = us_ticker_target - us_ticker_read();
 
     if (delta <= 0) {
         /* This event was in the past */
@@ -195,10 +200,10 @@ void us_ticker_set_interrupt(timestamp_t timestamp)
         return;
     }
 
-    /* Calculate how much delta falls outside the 16-bit counter range. */
-    /* You will have to perform a full timer overflow for each bit above */
-    /* that range. */
-    us_ticker_int_counter = (uint32_t)(delta >> 16);
+    // Clamp at max value of timer
+    if (delta > 0xFFFF) {
+        delta = 0xFFFF;
+    }
 
     ticker_set(delta);
 }