[riot-notifications] [RIOT-OS/RIOT] periph/timer: add timer_set_periodic() (#13902)

Marian Buschsieweke notifications at github.com
Wed May 27 21:06:21 CEST 2020


@maribu commented on this pull request.

Looks quite good. Some remarks inline

> +    set_oneshot(tim, channel);
+
+    return 0;
+}
+
+int timer_set_periodic(tim_t tim, int channel, unsigned int value, unsigned flags)
+{
+    if (channel >= TIMER_CHANNELS) {
+        return -1;
+    }
+
+    if (flags & TIM_FLAG_RESET_ON_SET) {
+        ctx[tim].dev->CNT = 0;
+    }
+
+    ctx[tim].dev->OCR[channel] = (uint16_t)value;

Needs to be guarded by `irq_disable()`...`irq_restore()`.

(Note: I'm aware that for 16 bit registers the ATmega platform internally uses a temporary 8 bit register that buffers the first write, so that the whole 16 bit are stored atomically with the second write. However, there is only one temporary register for achieving these atomic accesses. If during an ISR any 16 bit registers is accesses, the content of the temporary registers are changed and the second store results in corrupted data being written to OCR.)

> @@ -89,6 +106,32 @@ static uint8_t _get_prescaler(unsigned long freq_out, unsigned long freq_in)
     return scale;
 }
 
+/* TOP value is CC0 */
+static inline void _set_MFRQ(tim_t tim)

```suggestion
static inline void _set_mfrq(tim_t tim)
```

> @@ -89,6 +106,32 @@ static uint8_t _get_prescaler(unsigned long freq_out, unsigned long freq_in)
     return scale;
 }
 
+/* TOP value is CC0 */
+static inline void _set_MFRQ(tim_t tim)
+{
+    timer_stop(tim);
+    wait_synchronization(tim);
+#ifdef TC_WAVE_WAVEGEN_MFRQ
+    dev(tim)->WAVE.reg = TC_WAVE_WAVEGEN_MFRQ;
+#else
+    dev(tim)->CTRLA.bit.WAVEGEN = TC_CTRLA_WAVEGEN_MFRQ_Val;
+#endif
+    timer_start(tim);
+}
+
+/* TOP value is MAX timer value */
+static inline void _set_NFRQ(tim_t tim)

```suggestion
static inline void _set_nfrq(tim_t tim)
```

> +    ctx[tim].dev->OCR[channel] = (uint16_t)value;
+    *ctx[tim].flag &= ~(1 << (channel + OCF1A));
+    *ctx[tim].mask |= (1 << (channel + OCIE1A));
+    clear_oneshot(tim, channel);
+
+    /* only OCR0 can be use to set TOP */
+    if (channel == 0) {
+        if (flags & TIM_FLAG_RESET_ON_MATCH) {
+            /* enable CTC mode */
+            ctx[tim].dev->CRB |= (1 << 3);
+        } else {
+            /* disable CTC mode */
+            ctx[tim].dev->CRB &= (1 << 3);
+        }
+    } else {
+        assert((flags & TIM_FLAG_RESET_ON_MATCH) == 0);

```suggestion
        assert((flags & TIM_FLAG_RESET_ON_MATCH) == 0);
        return -1;
```

With `NDEBUG`, it would be IMO good if this still fails. Or [`expect()`](http://api.riot-os.org/group__test__utils__expect.html) could be used instead of `assert()`, but `return -1` should be more lightweight.

> +
+    *succeeded = false;
+    return "ERROR";
+}
+
+int main(void)
+{
+    mutex_t lock = MUTEX_INIT_LOCKED;
+    const unsigned timer_hz = 62500;
+    const unsigned steps = (250UL * timer_hz) / 1000; /* 250 ms */
+
+    printf("\nRunning Timer %d at %u Hz.\n", TIMER_CYCL, timer_hz);
+    printf("One counter cycle is %u ticks or 250 ms\n", steps);
+    puts("Will print 'tick' every second / every 4 cycles.\n");
+
+    timer_init(TIMER_CYCL, timer_hz, cb, &lock);

```suggestion
    expect(0 == timer_init(TIMER_CYCL, timer_hz, cb, &lock));
```

The return value must be checked. E.g. the 62500 Hz cannot be used for the Waspmote Pro. (I am aware that this is just the very frequency that board has configured for the xtimer. But I double checked that there is just no matching prescaler available for the configured core clock to reach this frequency. That broken timer config only highlights that there are apparently no users of that board, otherwise we would have an issue open for that.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/13902#pullrequestreview-419514626
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200527/f3f3013a/attachment.htm>


More information about the notifications mailing list