tests/ztimer_periodic: iterate over clocks #16254

maribu · 2021-03-30T11:15:30Z

Contribution description

Instead of only testing for ZTIMER_MSEC, iterate over a list of
clocks and run the test for each. The reasoning is that this test
does not only test ztimer_periodic, but also that the used clock
backend operates correctly in a typical use case.

Testing procedure

The test should still pass.

Issues/PRs references

Useful for #16172

fjmolinas · 2021-03-30T11:37:03Z

The test should still pass.

I would say this is not a requirement, this yet might expose bugs, but this PR should not necessarily handle fixing them IMO.

tests/ztimer_periodic/Makefile

fjmolinas · 2021-03-30T12:32:14Z

@maribu, the following is also needed to override de the default RTT_FREQUENCY for those that set it low ( I think the only one doing it is the arduino-due ( which I realized is not yet configurable, so I opened #16257)

ifneq (,$(filter stm32 nrf52 sam%,$(CPU)))
  RTT_FREQUENCY ?= RTT_MAX_FREQUENCY
  CFLAGS += -DRTT_FREQUENCY=$(RTT_FREQUENCY)
endif

maribu · 2021-03-30T15:51:04Z

Rebased on top of master to include #16257

maribu · 2021-03-30T15:51:27Z

May I squash to safe one Murdock cycle?

fjmolinas · 2021-03-30T16:46:55Z

May I squash to safe one Murdock cycle?

Sure, but since in #16257 I did not change the default frequency you might still want to include somewhere:

ifneq (,$(filter stm32 nrf52 sam%,$(CPU)))
  RTT_FREQUENCY ?= RTT_MAX_FREQUENCY
  CFLAGS += -DRTT_FREQUENCY=$(RTT_FREQUENCY)
endif

fjmolinas · 2021-04-07T08:40:40Z

I ran the tes on a lsit of BOARDS the following failed:

arduino-uno/failuresummary

Failures during test:

tests/ztimer_periodic

z1/failuresummary

Failures during test:

tests/ztimer_periodic

atmega256rfr2-xpro/failuresummary

Failures during test:

tests/ztimer_periodic

All these succeeded

nucleo-g071rb/failuresummary

dwm1001/failuresummary

pba-d-01-kw2x/failuresummary

nucleo-f207zg/failuresummary

nucleo-f334r8/failuresummary

nucleo-g474re/failuresummary

nrf52dk/failuresummary

frdm-k64f/failuresummary

nucleo-l152re/failuresummary

nucleo-f767zi/failuresummary

samr21-xpro/failuresummary

p-nucleo-wb55/failuresummary

nucleo-l496zg/failuresummary

nucleo-l452re/failuresummary

nucleo-l433rc/failuresummary

ek-lm4f120xl/failuresummary

i-nucleo-lrwan1/failuresummary

openmote-b/failuresummary

nucleo-l4r5zi/failuresummary

nucleo-f091rc/failuresummary

arduino-zero/failuresummary

nucleo-f303k8/failuresummary

nucleo-f103rb/failuresummary

b-l072z-lrwan1/failuresummary

nucleo-f303re/failuresummary

slstk3402a/failuresummary

nucleo-f446re/failuresummary

nucleo-f030r8/failuresummary

nucleo-l073rz/failuresummary

tests/ztimer_periodic/main.c

maribu · 2021-04-07T09:00:41Z

Looks to me like on the z1 the first period is missed by a to high offset for the usec clock. I guess this might just be the MSP430 being to slow in terms of CPU power to match the short period. I added a fixup to increase the period for the usec clock by factor 10. Maybe this makes the z1 pass?

The kernel panic with the AVR seems to be a bug exposed by the test. I have the feeling that the AVR port is generally having some stability issues that are in need of fixing :-(

fjmolinas · 2021-04-07T09:10:49Z

Looks to me like on the z1 the first period is missed by a to high offset for the usec clock. I guess this might just be the MSP430 being to slow in terms of CPU power to match the short period. I added a fixup to increase the period for the usec clock by factor 10. Maybe this makes the z1 pass?

The kernel panic with the AVR seems to be a bug exposed by the test. I have the feeling that the AVR port is generally having some stability issues that are in need of fixing :-(

In any case failing tests are not a blocker here.

maribu · 2021-04-07T09:14:53Z

In any case failing tests are not a blocker here.

Maybe for the z1 it indeed is the tests fault: If it puts the poor CPU under more load than it can handle, we test for something the hardware just cannot deliver. But if it is indeed an unrelated bug, I totally agree.

It is a pity that I haven't found the time yet to port RIOT to my MSP-EXP430FR5994 boards, so that I could test myself.

fjmolinas · 2021-04-13T08:25:49Z

In any case failing tests are not a blocker here.

Maybe for the z1 it indeed is the tests fault: If it puts the poor CPU under more load than it can handle, we test for something the hardware just cannot deliver. But if it is indeed an unrelated bug, I totally agree.

It is a pity that I haven't found the time yet to port RIOT to my MSP-EXP430FR5994 boards, so that I could test myself.

Actually I think the issue is that calling ztimer_periodic_start takes some time and that should be factored out, I think the first callback triggered should be ignored. With that the test will pass for z1. What do you think @maribu? With that IMO this one is ready to go in.

fjmolinas · 2021-04-13T08:28:08Z

In any case failing tests are not a blocker here.

Maybe for the z1 it indeed is the tests fault: If it puts the poor CPU under more load than it can handle, we test for something the hardware just cannot deliver. But if it is indeed an unrelated bug, I totally agree.
It is a pity that I haven't found the time yet to port RIOT to my MSP-EXP430FR5994 boards, so that I could test myself.

Actually I think the issue is that calling ztimer_periodic_start takes some time and that should be factored out, I think the first callback triggered should be ignored. With that the test will pass for z1. What do you think @maribu? With that IMO this one is ready to go in.

It also would make it pass on atmega256rfr2-xpro I think

fjmolinas · 2021-06-01T08:26:38Z

@maribu ping :)

maribu · 2021-06-01T10:27:02Z

In any case failing tests are not a blocker here.

Maybe for the z1 it indeed is the tests fault: If it puts the poor CPU under more load than it can handle, we test for something the hardware just cannot deliver. But if it is indeed an unrelated bug, I totally agree.
It is a pity that I haven't found the time yet to port RIOT to my MSP-EXP430FR5994 boards, so that I could test myself.

Actually I think the issue is that calling ztimer_periodic_start takes some time and that should be factored out, I think the first callback triggered should be ignored. With that the test will pass for z1. What do you think @maribu? With that IMO this one is ready to go in.

I changed the behavior that a too high offset for the first timeout results in a warning, rather than a test failure. To compensate, I increased the number of tests.

Generally, I would like not to lower the bar of the tests, but rather try to get the implementation passing it. But missing only the first timeout and none afterwards clearly a less severe test failure than missing random timeouts, so downgrading a miss of the first deadline could be considered a compromise to better match the severity of the different failure modes.

fjmolinas · 2021-06-01T10:52:01Z

tests/ztimer_periodic/main.c

+        printf("Testing clock: %s\n", _names[j]);
+        ztimer_clock_t *clock = clocks[j];
+        ztimer_periodic_init(clock, &t, callback, clock, _intervals[j]);
+        uint32_t last = ztimer_now(clock);


The other option is to use the internal value of ztimer_periodic_t:

Suggested change

uint32_t last = ztimer_now(clock);

uint32_t last = t.last;

Re-running the tests

The situation improved a bit (offset is smaller) but the same BOARD's are still failing... :(

arduino-uno/failuresummary

Failures during test:

tests/ztimer_periodic

z1/failuresummary

Failures during test:

tests/ztimer_periodic

atmega256rfr2-xpro/failuresummary

Failures during test:

tests/ztimer_periodic

On the arduino-uno, the ZTIMER_USEC times are consistently bad, while on atmega256rfr2-xpro it works fine for all but the first timeout. I think both are running at 16 MHz, are using the same peripheral time and should have (almost) the same IPC, so I wonder what is causing the difference.

So how do we move forward?

I'd say the test is fine, but the implementation in AVR is flanky. I would like to debug this, but I currently don't have the time :-/

fjmolinas · 2021-06-15T14:47:46Z

@maribu tunning ADJUST values fixes the failing test so lets simply gets this in since it flags cases where adjustments are needed (see #16554), please squash!

fjmolinas · 2021-06-15T14:48:02Z

@maribu tunning ADJUST values fixes the failing test so lets simply gets this in since it flags cases where adjustments are needed (see #16554), please squash!

And trigger CI afterwards

fjmolinas · 2021-06-16T07:29:00Z

@maribu can you rebase now that the fixing PR's are in?

maribu · 2021-06-16T11:05:34Z

Three failures:
- compilation for nucleo-l011k4 failed due to RAM overflow --> Makefile.ci
- test for congure fails, but does also with master --> ignore
- compilation with KConfig results in different binaries, as the logic in Makefile.board.dep is not mirrored in KConfig

I cannot find any application with an Makefile.board.dep and a corresponding KConfig, so I'm not sure how to properly do this.

fjmolinas · 2021-06-16T11:08:19Z

* Three failures:
  
  * compilation for nucleo-l011k4 failed due to RAM overflow --> Makefile.ci
  * test for congure fails, but does also with master --> ignore
  * compilation with KConfig results in different binaries, as the logic in `Makefile.board.dep` is not mirrored in KConfig

I cannot find any application with an Makefile.board.dep and a corresponding KConfig, so I'm not sure how to properly do this.

You can drop Makefile.board.dep, that logic is now in master with https://github.com/RIOT-OS/RIOT/pull/16553/files, so I think a reabase would do

fjmolinas · 2021-06-16T11:08:32Z

tests/ztimer_periodic/Makefile

+# ztimer_msec needs an RTT frequency of at least 1 kHz for a resolution of
+# roughly 1 ms, but some boards set the RTT to 1 Hz by default. This should
+# work around this at least for STM32, nRF52, and SAM*
+ifneq (,$(filter stm32 nrf52 sam%,$(CPU)))
+  RTT_FREQUENCY ?= RTT_MAX_FREQUENCY
+  CFLAGS += -DRTT_FREQUENCY=$(RTT_FREQUENCY)
+endif
+


Can be dropped with #16553

fjmolinas · 2021-06-16T11:08:40Z

tests/ztimer_periodic/Makefile.board.dep

+ifneq (,$(filter stm32 nrf52 sam%,$(CPU)))
+  FEATURES_OPTIONAL += periph_rtt
+  ifneq (,$(filter periph_rtt,$(FEATURES_USED)))
+    USEMODULE += ztimer_periph_rtt
+  endif
+endif


This file can be dropped with https://github.com/RIOT-OS/RIOT/pull/16553/files

Instead of only testing for `ZTIMER_MSEC`, iterate over a list of clocks and run the test for each. The reasoning is that this test does not only test `ztimer_periodic`, but also that the used clock backend operates correctly in a typical use case.

fjmolinas · 2021-06-16T11:11:33Z

Tested that with the proposed rebase and changes Kconfig build is the same

OARD=samr21-xpro make -C tests/ztimer_periodic/ -j3
Warning! EXTERNAL_MODULE_DIRS is a search folder since 2021.07-branch, see https://doc.riot-os.org/creating-modules.html#modules-outside-of-riotbase
Building application "tests_ztimer_periodic" for "samr21-xpro" with MCU "samd21".

"make" -C /home/francisco/workspace/RIOT/boards/samr21-xpro
"make" -C /home/francisco/workspace/RIOT/core
"make" -C /home/francisco/workspace/RIOT/cpu/samd21
"make" -C /home/francisco/workspace/RIOT/cpu/cortexm_common
"make" -C /home/francisco/workspace/RIOT/cpu/cortexm_common/periph
"make" -C /home/francisco/workspace/RIOT/drivers
"make" -C /home/francisco/workspace/RIOT/drivers/periph_common
"make" -C /home/francisco/workspace/RIOT/cpu/sam0_common
"make" -C /home/francisco/workspace/RIOT/cpu/sam0_common/periph
"make" -C /home/francisco/workspace/RIOT/cpu/samd21/periph
"make" -C /home/francisco/workspace/RIOT/cpu/samd21/vectors
"make" -C /home/francisco/workspace/RIOT/sys
"make" -C /home/francisco/workspace/RIOT/sys/auto_init
"make" -C /home/francisco/workspace/RIOT/sys/fmt
"make" -C /home/francisco/workspace/RIOT/sys/frac
"make" -C /home/francisco/workspace/RIOT/sys/isrpipe
"make" -C /home/francisco/workspace/RIOT/sys/malloc_thread_safe
"make" -C /home/francisco/workspace/RIOT/sys/newlib_syscalls_default
"make" -C /home/francisco/workspace/RIOT/sys/pm_layered
"make" -C /home/francisco/workspace/RIOT/sys/stdio_uart
"make" -C /home/francisco/workspace/RIOT/sys/test_utils/interactive_sync
"make" -C /home/francisco/workspace/RIOT/sys/tsrb
"make" -C /home/francisco/workspace/RIOT/sys/ztimer
   text	   data	    bss	    dec	    hex	filename
  12460	    180	   2532	  15172	   3b44	/home/francisco/workspace/RIOT/tests/ztimer_periodic/bin/samr21-xpro/tests_ztimer_periodic.elf
  ~/workspace/RIOT  |pr-16254 {218} U:2 ✗|
→ TEST_KCONFIG=1 BOARD=samr21-xpro make -C tests/ztimer_periodic/ -j3
Warning! EXTERNAL_MODULE_DIRS is a search folder since 2021.07-branch, see https://doc.riot-os.org/creating-modules.html#modules-outside-of-riotbase
=== [ATTENTION] Testing Kconfig dependency modelling ===
Warning! EXTERNAL_MODULE_DIRS is a search folder since 2021.07-branch, see https://doc.riot-os.org/creating-modules.html#modules-outside-of-riotbase
=== [ATTENTION] Testing Kconfig dependency modelling ===
Building application "tests_ztimer_periodic" for "samr21-xpro" with MCU "samd21".

"make" -C /home/francisco/workspace/RIOT/boards/samr21-xpro
"make" -C /home/francisco/workspace/RIOT/core
"make" -C /home/francisco/workspace/RIOT/cpu/samd21
"make" -C /home/francisco/workspace/RIOT/cpu/cortexm_common
"make" -C /home/francisco/workspace/RIOT/cpu/sam0_common
"make" -C /home/francisco/workspace/RIOT/cpu/sam0_common/periph
"make" -C /home/francisco/workspace/RIOT/cpu/cortexm_common/periph
"make" -C /home/francisco/workspace/RIOT/cpu/samd21/periph
"make" -C /home/francisco/workspace/RIOT/cpu/samd21/vectors
"make" -C /home/francisco/workspace/RIOT/drivers
"make" -C /home/francisco/workspace/RIOT/drivers/periph_common
"make" -C /home/francisco/workspace/RIOT/sys
"make" -C /home/francisco/workspace/RIOT/sys/auto_init
"make" -C /home/francisco/workspace/RIOT/sys/fmt
"make" -C /home/francisco/workspace/RIOT/sys/frac
"make" -C /home/francisco/workspace/RIOT/sys/isrpipe
"make" -C /home/francisco/workspace/RIOT/sys/malloc_thread_safe
"make" -C /home/francisco/workspace/RIOT/sys/newlib_syscalls_default
"make" -C /home/francisco/workspace/RIOT/sys/pm_layered
"make" -C /home/francisco/workspace/RIOT/sys/stdio_uart
"make" -C /home/francisco/workspace/RIOT/sys/test_utils/interactive_sync
"make" -C /home/francisco/workspace/RIOT/sys/tsrb
"make" -C /home/francisco/workspace/RIOT/sys/ztimer
   text	   data	    bss	    dec	    hex	filename
  12460	    180	   2532	  15172	   3b44	/home/francisco/workspace/RIOT/tests/ztimer_periodic/bin/samr21-xpro/tests_ztimer_periodic.elf

fjmolinas

All my BOARDS are now passing, ACK!

maribu requested review from aabadie, fjmolinas, leandrolanzieri, MichelRottleuthner and smlng as code owners March 30, 2021 11:15

maribu added Area: tests Area: tests and testing framework Area: timers Area: timer subsystems Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Mar 30, 2021

fjmolinas reviewed Mar 30, 2021

View reviewed changes

tests/ztimer_periodic/Makefile Outdated Show resolved Hide resolved

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 30, 2021

fjmolinas mentioned this pull request Mar 30, 2021

boards/arduino-due: allow changing frequency #16257

Merged

maribu force-pushed the tests/ztimer_periodic branch from 0d0dce9 to 524f634 Compare March 30, 2021 15:50

maribu force-pushed the tests/ztimer_periodic branch from 524f634 to cc0d4e4 Compare March 30, 2021 18:23

maribu commented Apr 7, 2021

View reviewed changes

tests/ztimer_periodic/main.c Outdated Show resolved Hide resolved

maribu force-pushed the tests/ztimer_periodic branch from cc0d4e4 to e37cbd3 Compare April 7, 2021 08:56

maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 7, 2021

github-actions bot removed the Area: timers Area: timer subsystems label Jun 1, 2021

fjmolinas reviewed Jun 1, 2021

View reviewed changes

fjmolinas mentioned this pull request Jun 15, 2021

boards/atmega: add USEC_ADJUST values #16554

Merged

fjmolinas mentioned this pull request Jun 15, 2021

boards/z1: add CONFIG_ZTIMER_USEC_ADJUST_% values #16555

Merged

maribu force-pushed the tests/ztimer_periodic branch from 66f6bd5 to b3281d0 Compare June 16, 2021 07:37

maribu added 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 labels Jun 16, 2021

fjmolinas reviewed Jun 16, 2021

View reviewed changes

maribu force-pushed the tests/ztimer_periodic branch from b3281d0 to 258d384 Compare June 16, 2021 11:12

fjmolinas approved these changes Jun 16, 2021

View reviewed changes

fjmolinas merged commit c09753f into RIOT-OS:master Jun 17, 2021

MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021

maribu deleted the tests/ztimer_periodic branch January 23, 2022 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/ztimer_periodic: iterate over clocks #16254

tests/ztimer_periodic: iterate over clocks #16254

maribu commented Mar 30, 2021

fjmolinas commented Mar 30, 2021

fjmolinas commented Mar 30, 2021

maribu commented Mar 30, 2021

maribu commented Mar 30, 2021

fjmolinas commented Mar 30, 2021

fjmolinas commented Apr 7, 2021

maribu commented Apr 7, 2021

fjmolinas commented Apr 7, 2021

maribu commented Apr 7, 2021

fjmolinas commented Apr 13, 2021

fjmolinas commented Apr 13, 2021

fjmolinas commented Jun 1, 2021

maribu commented Jun 1, 2021

fjmolinas Jun 1, 2021

maribu Jun 1, 2021

fjmolinas Jun 1, 2021

fjmolinas Jun 1, 2021

maribu Jun 1, 2021

fjmolinas Jun 8, 2021

maribu Jun 8, 2021

fjmolinas commented Jun 15, 2021

fjmolinas commented Jun 15, 2021

fjmolinas commented Jun 16, 2021

maribu commented Jun 16, 2021

fjmolinas commented Jun 16, 2021

fjmolinas Jun 16, 2021

fjmolinas Jun 16, 2021

fjmolinas commented Jun 16, 2021

fjmolinas left a comment

tests/ztimer_periodic: iterate over clocks #16254

tests/ztimer_periodic: iterate over clocks #16254

Conversation

maribu commented Mar 30, 2021

Contribution description

Testing procedure

Issues/PRs references

fjmolinas commented Mar 30, 2021

fjmolinas commented Mar 30, 2021

maribu commented Mar 30, 2021

maribu commented Mar 30, 2021

fjmolinas commented Mar 30, 2021

fjmolinas commented Apr 7, 2021

arduino-uno/failuresummary

z1/failuresummary

atmega256rfr2-xpro/failuresummary

nucleo-g071rb/failuresummary

dwm1001/failuresummary

pba-d-01-kw2x/failuresummary

nucleo-f207zg/failuresummary

nucleo-f334r8/failuresummary

nucleo-g474re/failuresummary

nrf52dk/failuresummary

frdm-k64f/failuresummary

nucleo-l152re/failuresummary

nucleo-f767zi/failuresummary

samr21-xpro/failuresummary

p-nucleo-wb55/failuresummary

nucleo-l496zg/failuresummary

nucleo-l452re/failuresummary

nucleo-l433rc/failuresummary

ek-lm4f120xl/failuresummary

i-nucleo-lrwan1/failuresummary

openmote-b/failuresummary

nucleo-l4r5zi/failuresummary

nucleo-f091rc/failuresummary

arduino-zero/failuresummary

nucleo-f303k8/failuresummary

nucleo-f103rb/failuresummary

b-l072z-lrwan1/failuresummary

nucleo-f303re/failuresummary

slstk3402a/failuresummary

nucleo-f446re/failuresummary

nucleo-f030r8/failuresummary

nucleo-l073rz/failuresummary

maribu commented Apr 7, 2021

fjmolinas commented Apr 7, 2021

maribu commented Apr 7, 2021

fjmolinas commented Apr 13, 2021

fjmolinas commented Apr 13, 2021

fjmolinas commented Jun 1, 2021

maribu commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arduino-uno/failuresummary

z1/failuresummary

atmega256rfr2-xpro/failuresummary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Jun 15, 2021

fjmolinas commented Jun 15, 2021

fjmolinas commented Jun 16, 2021

maribu commented Jun 16, 2021

fjmolinas commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Jun 16, 2021

fjmolinas left a comment

Choose a reason for hiding this comment