Handle race conditions preventing timers to be set correctly on MSP430 MCUs

when the counter is incrementing and/or overflowing

cpu/cc430/hwtimer_cc430.c

@@ -27,18 +27,16 @@
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
-static uint32_t ticks = 0;
 
 extern void (*int_handler)(int);
 extern void timer_unset(short timer);
-extern uint16_t overflow_interrupt[HWTIMER:_MAXTIMERS+1];
+extern volatile uint16_t overflow_interrupt[HWTIMER_MAXTIMERS+1];
-extern uint16_t timer_round;
+extern volatile uint16_t timer_round;
 
 void timerA_init(void)
 {
     volatile unsigned int *ccr = &TA0CCR0;
     volatile unsigned int *ctl = &TA0CCTL0;
-    ticks = 0;                               /* Set tick counter value to 0 */
     timer_round = 0;                         /* Set to round 0 */
     TA0CTL = TASSEL_1 + TACLR;               /* Clear the timer counter, set ACLK */
     TA0CTL &= ~TAIFG;                        /* Clear the IFG */
@@ -56,10 +54,12 @@ void timerA_init(void)
 interrupt(TIMER0_A0_VECTOR) __attribute__((naked)) timer0_a0_isr(void)
 {
     __enter_isr();
+
     if (overflow_interrupt[0] == timer_round) {
         timer_unset(0);
         int_handler(0);
     }
+
     __exit_isr();
 }
 
@@ -67,18 +67,34 @@ interrupt(TIMER0_A1_VECTOR) __attribute__((naked)) timer0_a1_5_isr(void)
 {
     __enter_isr();
 
-    short taiv = TA0IV;
+    short taiv_reg = TA0IV;
-    short timer = taiv / 2;
+    if (taiv_reg == 0x0E) {
-    /* TAIV = 0x0E means overflow */
+        /* TAIV = 0x0E means overflow */
-    if (taiv == 0x0E) {
         DEBUG("Overflow\n");
-        timer_round += 1;
+        timer_round++;
     }
-    /* check which CCR has been hit and if the overflow counter for this timer
+    else {
-     * has been reached */
+        short timer = taiv_reg >> 1;
-    else if (overflow_interrupt[timer] == timer_round) {
+        /* check which CCR has been hit and if the overflow counter
-        timer_unset(timer);
+           for this timer has been reached (or exceeded);
-        int_handler(timer);
+           there is indeed a race condition where an hwtimer
+           due to fire in the next round can be set after
+           the timer's counter has overflowed but *before*
+           timer_round incrementation has occured (when
+           interrupts are disabled for any reason), thus
+           effectively setting the timer one round in the past! */
+        int16_t round_delta = overflow_interrupt[timer] - timer_round;
+        /* in order to correctly handle timer_round overflow,
+           we must fire the timer when, for example,
+           timer_round == 0 and overflow_interrupt[timer] == 65535;
+           to that end, we compute the difference between the two
+           on a 16-bit signed integer: any difference >= +32768 will
+           thus overload to a negative number; we should then
+           correctly fire "overdue" timers whenever the case */
+        if (round_delta <= 0) {
+            timer_unset(timer);
+            int_handler(timer);
+        }
     }
 
     __exit_isr();

cpu/msp430-common/hwtimer_cpu.c

@@ -22,8 +22,8 @@ See the file LICENSE in the top level directory for more details.
 
 void (*int_handler)(int);
 extern void timerA_init(void);
-uint16_t overflow_interrupt[HWTIMER_MAXTIMERS+1];
+volatile uint16_t overflow_interrupt[HWTIMER_MAXTIMERS+1];
-uint16_t timer_round;
+volatile uint16_t timer_round;
 
 #ifdef CC430
   /* CC430 have "TimerA0", "TimerA1" and so on... */
@@ -52,15 +52,20 @@ static void timer_enable_interrupt(short timer)
     *ptr &= ~(CCIFG);
 }
 
-static void timer_set_nostart(unsigned long value, short timer)
+static void timer_set_nostart(uint32_t value, short timer)
 {
     volatile unsigned int *ptr = &CNT_COMP_BASE_REG + (timer);
-    *ptr = value;
+    /* ensure we won't set the timer to a "past" tick */
+    if (value <= hwtimer_arch_now()) {
+        value = hwtimer_arch_now() + 2;
+    }
+    overflow_interrupt[timer] = (uint16_t)(value >> 16);
+    *ptr = (value & 0xFFFF);
 }
 
-static void timer_set(unsigned long value, short timer)
+static void timer_set(uint32_t value, short timer)
 {
-    DEBUG("Setting timer %u to %u overflows and %lu\n", timer, overflow_interrupt[timer], value);
+    DEBUG("Setting timer %u to %lu\n", timer, value);
     timer_set_nostart(value, timer);
     timer_enable_interrupt(timer);
 }
@@ -74,7 +79,7 @@ void timer_unset(short timer)
 
 unsigned long hwtimer_arch_now()
 {
-    return ((uint32_t)timer_round << 16)+TIMER_VAL_REG;
+    return ((uint32_t)timer_round << 16) + TIMER_VAL_REG;
 }
 
 void hwtimer_arch_init(void (*handler)(int), uint32_t fcpu)
@@ -106,9 +111,7 @@ void hwtimer_arch_set(unsigned long offset, short timer)
 
 void hwtimer_arch_set_absolute(unsigned long value, short timer)
 {
-    uint16_t small_value = value & 0xFFFF;
+    timer_set(value, timer);
-    overflow_interrupt[timer] = (uint16_t)(value >> 16);
-    timer_set(small_value, timer);
 }
 
 void hwtimer_arch_unset(short timer)

cpu/msp430x16x/hwtimer_msp430.c

@@ -27,18 +27,15 @@
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
-static uint32_t ticks = 0;
-
 extern void (*int_handler)(int);
 extern void timer_unset(short timer);
-extern uint16_t overflow_interrupt[HWTIMER_MAXTIMERS+1];
+extern volatile uint16_t overflow_interrupt[HWTIMER_MAXTIMERS+1];
-extern uint16_t timer_round;
+extern volatile uint16_t timer_round;
 
 void timerA_init(void)
 {
     volatile unsigned int *ccr;
     volatile unsigned int *ctl;
-    ticks = 0;                               /* Set tick counter value to 0 */
     timer_round = 0;                         /* Set to round 0 */
     TACTL = TASSEL_1 + TACLR;                /* Clear the timer counter, set ACLK */
     TACTL &= ~TAIFG;                         /* Clear the IFG */
@@ -58,10 +55,12 @@ void timerA_init(void)
 interrupt(TIMERA0_VECTOR) __attribute__((naked)) timer_isr_ccr0(void)
 {
     __enter_isr();
+
     if (overflow_interrupt[0] == timer_round) {
         timer_unset(0);
         int_handler(0);
     }
+
     __exit_isr();
 }
 
@@ -69,18 +68,34 @@ interrupt(TIMERA1_VECTOR) __attribute__((naked)) timer_isr(void)
 {
     __enter_isr();
 
-    short taiv = TAIV;
+    short taiv_reg = TAIV;
-    short timer = taiv / 2;
+    if (taiv_reg == 0x0A) {
-    /* TAIV = 0x0A means overflow */
+        /* TAIV = 0x0A means overflow */
-    if (taiv == 0x0A) {
         DEBUG("Overflow\n");
-        timer_round += 1;
+        timer_round++;
     }
-    /* check which CCR has been hit and if the overflow counter for this timer
+    else {
-     * has been reached */
+        short timer = taiv_reg >> 1;
-    else if (overflow_interrupt[timer] == timer_round) {
+        /* check which CCR has been hit and if the overflow counter
-        timer_unset(timer);
+           for this timer has been reached (or exceeded);
-        int_handler(timer);
+            there is indeed a race condition where an hwtimer
+            due to fire in the next round can be set after
+            the timer's counter has overflowed but *before*
+            timer_round incrementation has occured (when
+            interrupts are disabled for any reason), thus
+            effectively setting the timer one round in the past! */
+        int16_t round_delta = overflow_interrupt[timer] - timer_round;
+        /* in order to correctly handle timer_round overflow,
+           we must fire the timer when, for example,
+           timer_round == 0 and overflow_interrupt[timer] == 65535;
+           to that end, we compute the difference between the two
+           on a 16-bit signed integer: any difference >= +32768 will
+           thus overload to a negative number; we should then
+           correctly fire "overdue" timers whenever the case */
+        if (round_delta <= 0) {
+            timer_unset(timer);
+            int_handler(timer);
+        }
     }
 
     __exit_isr();