tests/periph_timer: Cleanup

[maribu]

Contribution description

• (ab)use XTIMER_HZ instead of a manual list for timer frequencies
• Use C11 atomics instead of incorrectly using volatile

Testing procedure

Test should still work as previously. In fact, the manual list of timer frequencies seems to be identical to the definition of XTIMER_HZ for every board.

Issues/PRs references

Split out of #14799

[aabadie]

Just a minor style improvement.

Suggested change

	#define XTIMER_HZ MHZ(1)
	#define XTIMER_HZ MHZ(1)

I can try on usb-kw41z, remote, hifive1b.

[aabadie]

The test now fails on usb-kw41z (but works on master):

main(): This is RIOT! (Version: 2020.10-devel-874-g467996-review_tests_periph_timer)

Test for peripheral TIMERs

Available timers: 2

Testing TIMER_0:
TIMER_0: initialization successful
TIMER_0: stopped
TIMER_0: set channel 0 to 5000
TIMER_0: starting
TIMER_0: channel 0 fired at SW count     6858 - init:     6858

Testing TIMER_1:
TIMER_1: ERROR on initialization - skipping


TEST FAILED
Timeout in expect script at "child.expect_exact('TIMER_{}: initialization successful'.format(timer))" (tests/periph_timer/tests/01-run.py:18)

[aabadie]

Suggested change

	#include "macros/units.h"
	#include "board.h"
	#include "macros/units.h"

This fixes the failing test on usb-kw41z (and probably on other boards)

[maribu]

Thanks. I'll fix tonight. (Feel free to (force) push instead, of you want.)

[aabadie]

It's also broken on remote (cc2538) based boards: they don't define XTIMER_HZ but requires CLOCK_CORECLOCK instead of MHZ(1). We can set XTIMER_HZ for them, but I'm not sure if that makes sense to enable it globally this way.

fix for usb-kw41z, hifive1b and remote

diff --git a/boards/common/remote/include/board_common.h b/boards/common/remote/include/board_common.h
index acb1d8a6fa..6eef086471 100644
--- a/boards/common/remote/include/board_common.h
+++ b/boards/common/remote/include/board_common.h
@@ -78,6 +78,7 @@
  * @name xtimer configuration
  * @{
  */
+#define XTIMER_HZ           (CLOCK_CORECLOCK)
 #define XTIMER_WIDTH        (16)
 #define XTIMER_BACKOFF      (50)
 #define XTIMER_ISR_BACKOFF  (40)
diff --git a/tests/periph_timer/main.c b/tests/periph_timer/main.c
index 806ea8dc30..660661f0b4 100644
--- a/tests/periph_timer/main.c
+++ b/tests/periph_timer/main.c
@@ -23,6 +23,8 @@
 #include <stdatomic.h>
 #include <stdlib.h>
 
+#include "board.h"
+
 #include "macros/units.h"
 #include "periph/timer.h"
 

[aabadie]

@fjmolinas any opinion on #14808 (comment) ? (you know cc2538 boards quite well).

[fjmolinas]

I don't think I like 752c6da:

• using XTIMER_HZ in a test for timers is weird
• XTIMER_HZ will probably be 1MHZ, there are boards with timers that run at 32Khz, but that timer is not used for XTIMER. This change will make the test break for those timers.

[maribu]

using XTIMER_HZ in a test for timers is weird

I agree. But that would be consistent with the other timer tests. Manually maintaining lists for each test is IMO not an option either; at least we should deduplicate this to only maintain one list for all tests.

[maribu]

I wonder if CLOCK_CORECLOCK would work for all boards, except those using 32.678 kHz. If so, maybe the easiest would be to modify the test to just always use CLOCK_CORECLOCK and fall back to 32.678 kHz when timer_init() fails.

[fjmolinas]

BTW this should have been 250kHz :)

[aabadie]

BTW this should have been 250kHz :)

Also noticed that and even opened a PR until I noticed that #14799 was removing it...

[fjmolinas]

I wonder if CLOCK_CORECLOCK would work for all boards, except those using 32.678 kHz. If so, maybe the easiest would be to modify the test to just always use CLOCK_CORECLOCK and fall back to 32.678 kHz when timer_init() fails.

I'm unaware if for all platforms the timer speed can be the same as the core clock speed.

[fjmolinas]

I'm unaware if for all platforms the timer speed can be the same as the core clock speed.

If you want to try it out I can run the test on a series of boards.

[aabadie]

Needs rebase. What's the state of this PR btw ?

[maribu]

What's the state of this PR btw ?

I dislike the approach in this PR. I think it would be better to extend the API of periph_timer to allow querying supported frequencies. That might be useful for platform independent apps anyway, and would clean up this test quite a bit. (And without resorting to abuse XTIMER_HZ for selecting the test frequency.)

Most importantly however, this would allow to iterate over supported frequencies and verify that the timer implementation indeed works for every supported frequency. E.g. currently running periph_timer on CPU speed on the Nucleo-F767zi doesn't work at all, see #15072. Testing all frequencies would ease detecting those issues.