From eeb359e80c3f6cb37ab5dfedf00d08c9948d4e06 Mon Sep 17 00:00:00 2001
From: Joshua DeWeese <jdeweese@primecontrols.com>
Date: Tue, 7 Mar 2023 11:29:17 -0500
Subject: [PATCH] cpu/stm32/periph/timer: fix execution flow

The implmentation of `timer_set_absolute()` has The following problems.
First, it attempts to restore the auto reload register (ARR) to it's
default if the ARR was previosly set by `timer_set_periodic()` by
comparing it to the channel's capture compare (CC) register _after_ it
has already set the CC register. Secondly, it clears spurious IRQs
_after_ the CC register has been set. If the value being set is equal to
the timer's current count (or the two become equal before the supurios
IRQ clearing happens), this could cause a legitimate IRQ to be cleared.

The implmentation of `timer_set()` has the same error in handling the
ARR as described above.

This patch reorders the operations of both functions to do:

1. handle ARR
2. clear spurious IRQs
3. set channel's CC
4. enable IRQ

Additionally, the calulation of `value` in `timer_set()` is moved
earlier in the function's exec path as a pedantic measure.
---
 cpu/stm32/periph/timer.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/cpu/stm32/periph/timer.c b/cpu/stm32/periph/timer.c
index 35eaab276b..6f5004177a 100644
--- a/cpu/stm32/periph/timer.c
+++ b/cpu/stm32/periph/timer.c
@@ -128,8 +128,6 @@ int timer_set_absolute(tim_t tim, int channel, unsigned int value)
     unsigned irqstate = irq_disable();
     set_oneshot(tim, channel);
 
-    TIM_CHAN(tim, channel) = (value & timer_config[tim].max);
-
 #ifdef MODULE_PERIPH_TIMER_PERIODIC
     if (dev(tim)->ARR == TIM_CHAN(tim, channel)) {
         dev(tim)->ARR = timer_config[tim].max;
@@ -139,6 +137,8 @@ int timer_set_absolute(tim_t tim, int channel, unsigned int value)
     /* clear spurious IRQs */
     dev(tim)->SR &= ~(TIM_SR_CC1IF << channel);
 
+    TIM_CHAN(tim, channel) = (value & timer_config[tim].max);
+
     /* enable IRQ */
     dev(tim)->DIER |= (TIM_DIER_CC1IE << channel);
     irq_restore(irqstate);
@@ -148,6 +148,8 @@ int timer_set_absolute(tim_t tim, int channel, unsigned int value)
 
 int timer_set(tim_t tim, int channel, unsigned int timeout)
 {
+    unsigned value = (dev(tim)->CNT + timeout) & timer_config[tim].max;
+
     if (channel >= (int)TIMER_CHANNEL_NUMOF) {
         return -1;
     }
@@ -155,21 +157,20 @@ int timer_set(tim_t tim, int channel, unsigned int timeout)
     unsigned irqstate = irq_disable();
     set_oneshot(tim, channel);
 
-    /* clear spurious IRQs */
-    dev(tim)->SR &= ~(TIM_SR_CC1IF << channel);
-
-    unsigned value = (dev(tim)->CNT + timeout) & timer_config[tim].max;
-    TIM_CHAN(tim, channel) = value;
-
-    /* enable IRQ */
-    dev(tim)->DIER |= (TIM_DIER_CC1IE << channel);
-
 #ifdef MODULE_PERIPH_TIMER_PERIODIC
     if (dev(tim)->ARR == TIM_CHAN(tim, channel)) {
         dev(tim)->ARR = timer_config[tim].max;
     }
 #endif
 
+    /* clear spurious IRQs */
+    dev(tim)->SR &= ~(TIM_SR_CC1IF << channel);
+
+    TIM_CHAN(tim, channel) = value;
+
+    /* enable IRQ */
+    dev(tim)->DIER |= (TIM_DIER_CC1IE << channel);
+
     /* calculate time till timeout */
     value = (value - dev(tim)->CNT) & timer_config[tim].max;