Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timers: Increase test variation, increase EFM32 flexibility #18052

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented May 4, 2022

Contribution description

Timers appear to be written with a strong focus on providing for XTimer; requirements have changed with the advent of ZTimer. In particular, EFM32 timers encoded a few of these choices rather hardly.

This PR contains small changes for making them test- and usable in a more versatile manner:

  • Timer test: Run with all the typical frequencies. It's OK if some can not be configured on the timer; this is reflected both in the output text and the success assessment.
  • EFM32: Expose initialization's faillibility: An assert is turned into a negative return. (The same commit also returns an error on an invalid configuration, but that is later fully handled anyway; expecting things to work without a prescaler was an interesting debug journey).
  • EFM32: Allow timers without prescale timers. They have a relatively narrow band of frequencies they can use (their clock / 2**{0..=10}), but that may be precisely what you need.

Testing procedure

  • Run the periph_timer test, preferably with an LETIMER board like sltb001a.
  • Before, only one frequency was tested, now all are. Without only the first commit, rather than merely showing an error on exotic frequencies, the system would run into an assert (but didn't because we only tested the preferred frequency anyway).

To test the last commit, we currently have no boards that are configured with single timers, so apply this and run on stk3700:

diff --git a/boards/stk3700/include/periph_conf.h b/boards/stk3700/include/periph_conf.h
index c66959c2dd..68e44469ec 100644
--- a/boards/stk3700/include/periph_conf.h
+++ b/boards/stk3700/include/periph_conf.h
@@ -222,12 +222,21 @@ static const timer_conf_t timer_config[] = {
         },
         .irq = LETIMER0_IRQn,
         .channel_numof = 2
-    }
+    },
+    {
+        .timer = {
+            .dev = TIMER2,
+            .cmu = cmuClock_TIMER2
+        },
+        .irq = TIMER2_IRQn,
+        .channel_numof = 3
+    },
 };
 
 #define TIMER_NUMOF         ARRAY_SIZE(timer_config)
 #define TIMER_0_ISR         isr_timer1
 #define TIMER_1_ISR         isr_letimer0
+#define TIMER_2_ISR         isr_timer2
 /** @} */
 
 /**
diff --git a/tests/periph_timer/main.c b/tests/periph_timer/main.c
index 7a8b1af7c6..1019bf6f7f 100644
--- a/tests/periph_timer/main.c
+++ b/tests/periph_timer/main.c
@@ -125,7 +125,7 @@ int main(void)
         res += test_timer(i, TIMER_SPEED);
     }
 
-    unsigned speeds[] = {32768, 250000, 500000, 1000000};
+    unsigned speeds[] = {32768, 250000, 500000, 1000000, 3000000};
     for (unsigned s = 0; s < ARRAY_SIZE(speeds); ++s) {
         unsigned speed = speeds[s];
         if (speed == TIMER_SPEED) {

and then run the periph_timers test. It will formally fail (because for automatable testing we'd need to give a per-timer known-good frequency, but there are no such boards right now so it's pointless anyway), but you can observe that timer 2 fails the 32kiHz test and the others, but then passes the 3MHz test.

Issues/PRs references

(I previously had a lot of potential follow-ups in here that are now all in this PR).

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework labels May 4, 2022
@chrysn chrysn changed the title cpu/efm32 timers: Report error on invalid configurations timers: Increase test variation, use init fallibility in EFM32 May 4, 2022
@chrysn chrysn changed the title timers: Increase test variation, use init fallibility in EFM32 timers: Increase test variation, increase EFM32 flexibility May 4, 2022
chrysn added 4 commits May 4, 2022 10:39
The configured frequency for the board is the one that is required to
pass; the other common frequencies are tried, and reported for the user
to get an impression of the timer's flexibility.
@chrysn chrysn marked this pull request as ready for review May 4, 2022 08:52
@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: timers Area: timer subsystems CI: run tests If set, CI server will run tests on hardware for the labeled PR labels May 4, 2022
@chrysn chrysn requested review from benemorius and basilfx and removed request for basilfx, aabadie, leandrolanzieri, smlng, MichelRottleuthner and fjmolinas May 4, 2022 08:52
@maribu
Copy link
Member

maribu commented May 4, 2022

Maybe I can interest you in #16349 - but I think currently it is waiting for me to fix some issues.

@chrysn
Copy link
Member Author

chrysn commented May 4, 2022

It's a good addition (esp. for testing), but I think the pressing need for that is more at build time. If it becomes unblocked, I'll be happy to adjust the new tests to that instead.

@fjmolinas
Copy link
Contributor

I ran the test on a set of BOARD, results are here, I did not look into the details of any test, but in master only openmote-b and nucleo-l552ze-q did not pass, while on this PR also: arduino-uno, samr21-xpro, atmega256rfr2-xpro, nucleo-l4r5z, z1, i-nucleo-lrwan1 and arduino-mega2560

@github-actions github-actions bot removed the Area: timers Area: timer subsystems label May 5, 2022
@chrysn
Copy link
Member Author

chrysn commented May 5, 2022

The build failures were due to some unsigned where I should have said uint32_t; fixuped. Oddly, some of the failing ones are not AVR (and thus 16bit), investigating. (And sorry for the force push: I forgot to push earlier and thought all was in sync).

@chrysn chrysn requested review from benpicco, dylad and keestux as code owners May 5, 2022 12:55
@chrysn
Copy link
Member Author

chrysn commented May 5, 2022

I think I found what samr21-xpro tripped over (another assert similar to the removed EFM32 one); could you trigger that test run again?

@chrysn chrysn removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 5, 2022
@chrysn
Copy link
Member Author

chrysn commented May 5, 2022

(Belaying tests until ... WAAH can we please have a version of C in which the number hierarchy and literals is not messed up?)

@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 5, 2022
@kfessel
Copy link
Contributor

kfessel commented May 5, 2022

#17654 might be of interest adding a second periph-timer (LP/LE however one wants to call it) to the ztimer initialization

@chrysn
Copy link
Member Author

chrysn commented May 6, 2022

The only remaining test failure that does not appear to be a fluke is run_test/tests/periph_timer/esp32-wroom-32:gnu, where the timer0 initialization fails -- odd, because I didn't touch the ESP32 code.

And the number of other tests failing "just so" is worrying:

compile/examples/hello-world/esp8266-esp-12x:gnu
run_test/tests/periph_timer/esp32-wroom-32:gnu
run_test/tests/thread_float/samr21-xpro:gnu
run_test/tests/sys_sched_round_robin/esp32-wroom-32:gnu
compile/tests/ztimer_overhead/native:gnu 

@benpicco benpicco requested a review from jue89 May 6, 2022 11:40
@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants