sys/ztimer: add auto_adjust module #17633

fjmolinas · 2022-02-09T13:54:00Z

Contribution description

ztimer_sleep and ztimer_set have an overhead themselves and since #14936 both can be compensated for. But in practice very few BOARD's use this value.

This PR adds work from @kaspar030 to add a module to auto-configure this values on startup unless there is an existing configuration value, e.g.: CONFIG_ZTIMER_USEC_ADJUST_SLEEP and CONFIG_ZTIMER_USEC_ADJUST_SET. These values have also been documented with this PR.

The module is enabled by default but that could be discussed since it increases startup time. I would prefer doing that and having people who care about performance configuring CONFIG_ZTIMER_USEC_ADJUST% for their hardware.

Testing procedure

This should lead prerry small offsets:

USEMODULE="ztimer_xtimer_compat" BOARD=dwm1001 make -C tests/xtimer_usleep flash test -j

main(): This is RIOT! (Version: 2022.04-devel-300-g3a3362-pr_ztimer_auto_adjust)
Running test 5 times with 7 distinct sleep times
Slept for 10002 us (expected: 10000 us) Offset: 2 us
Slept for 50001 us (expected: 50000 us) Offset: 1 us
Slept for 10235 us (expected: 10234 us) Offset: 1 us
Slept for 56781 us (expected: 56780 us) Offset: 1 us
Slept for 12123 us (expected: 12122 us) Offset: 1 us
Slept for 98766 us (expected: 98765 us) Offset: 1 us
Slept for 75001 us (expected: 75000 us) Offset: 1 us
Slept for 10001 us (expected: 10000 us) Offset: 1 us
Slept for 50001 us (expected: 50000 us) Offset: 1 us
Slept for 10235 us (expected: 10234 us) Offset: 1 us
Slept for 56781 us (expected: 56780 us) Offset: 1 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98766 us (expected: 98765 us) Offset: 1 us
Slept for 75001 us (expected: 75000 us) Offset: 1 us
Slept for 10001 us (expected: 10000 us) Offset: 1 us
Slept for 50001 us (expected: 50000 us) Offset: 1 us
Slept for 10235 us (expected: 10234 us) Offset: 1 us
Slept for 56781 us (expected: 56780 us) Offset: 1 us
Slept for 12123 us (expected: 12122 us) Offset: 1 us
Slept for 98765 us (expected: 98765 us) Offset: 0 us
Slept for 75001 us (expected: 75000 us) Offset: 1 us
Slept for 10001 us (expected: 10000 us) Offset: 1 us
Slept for 50001 us (expected: 50000 us) Offset: 1 us
Slept for 10235 us (expected: 10234 us) Offset: 1 us
Slept for 56781 us (expected: 56780 us) Offset: 1 us
Slept for 12123 us (expected: 12122 us) Offset: 1 us
Slept for 98766 us (expected: 98765 us) Offset: 1 us
Slept for 75001 us (expected: 75000 us) Offset: 1 us
Slept for 10001 us (expected: 10000 us) Offset: 1 us
Slept for 50001 us (expected: 50000 us) Offset: 1 us
Slept for 10235 us (expected: 10234 us) Offset: 1 us
Slept for 56781 us (expected: 56780 us) Offset: 1 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98766 us (expected: 98765 us) Offset: 1 us
Slept for 75001 us (expected: 75000 us) Offset: 1 us
Test ran for 1727139 us

tests/ztimer_overhead yields the configuration values:

BOARD=dwm1001 make -C tests/ztimer_overhead/ flash test -j

main(): This is RIOT! (Version: 2022.04-devel-300-g3a3362-pr_ztimer_auto_adjust)
unset ZTIMER_USEC auto_adjust parameters:
    ZTIMER_USEC->adjust_set = 7
    ZTIMER_USEC->adjust_sleep = 19
zitmer_overhead_set...
min=7 max=7 avg_diff=7
zitmer_overhead_sleep...
min=20 max=20 avg_diff=20
ZTIMER_USEC adjust params for dwm1001:
    CONFIG_ZTIMER_USEC_ADJUST_SET    7
    CONFIG_ZTIMER_USEC_ADJUST_SLEEP  20

Issues/PRs references

Depends on #17632, but could be split out
Part of #17111

kaspar030 · 2022-02-09T13:57:52Z

Nice!

I think we'd also need a test that checks that if any CONFIG_ZTIMER_USEC_ADJUST_* is configured for a board, those values match what the auto_adjust would spit out.

edit maybe tests/ztimer_overhead can fill this role, just comparing the before / after values that are now printed. Or, we bake that logic into the test script?

fjmolinas · 2022-02-09T14:01:34Z

edit maybe tests/ztimer_overhead can fill this role, just comparing the before / after values that are now printed. Or, we bake that logic into the test script?

I'm not sure, the test can be more picky run more loops or whatever, while the auto_adjust module would not, I think that it's ok if they are not the same as long as they are around +-1 (and the auto_adjust one should never be higher!)

kaspar030 · 2022-02-09T14:05:03Z

I think that it's ok if they are not the same as long as they are around +-1 (and the auto_adjust one should never be higher!)

that sounds reasonable! Also, the values can vary by +-1 due to whatever factors.

fjmolinas · 2022-02-09T20:45:13Z

I think that it's ok if they are not the same as long as they are around +-1 (and the auto_adjust one should never be higher!)

that sounds reasonable! Also, the values can vary by +-1 due to whatever factors.

And what are your thoughts regarding defaults, default on?

leandrolanzieri · 2022-02-10T07:08:30Z

sys/ztimer/init.c

+    if (IS_ACTIVE(CONFIG_ZTIMER_USEC_ADJUST_SLEEP)) {
+        ZTIMER_USEC->adjust_sleep = CONFIG_ZTIMER_USEC_ADJUST_SLEEP;
+    }
+    else if (IS_USED(MODULE_ZTIMER_AUTO_ADJUST)) {}


These braces don't look like something you intended

kaspar030 · 2022-02-10T08:42:02Z

And what are your thoughts regarding defaults, default on?

hmm. off means, ztimers are late. on means, ztimers are on time, but there's quite some boot delay, and extra code size. Can we quantify those?

chrysn · 2022-02-10T12:15:00Z

And what are your thoughts regarding defaults, default on?

How about auto-on with DEVELHELP (or in some prominent examples like default), and printing a warning to stdout if there are no values (or the values don't match what is measured)?

fjmolinas · 2022-02-10T12:33:51Z

How about auto-on with DEVELHELP (or in some prominent examples like default

I don't like the idea of enabling this with DEVELHELP as it would change the behavior much too silently for my taste. Once the dependency is in I'll get some numbers.

and printing a warning to stdout if there are no values (or the values don't match what is measured)?

I don't quite understand this suggestion, you mean in the test?

fjmolinas · 2022-02-10T12:34:13Z

I don't like the idea of enabling this with DEVELHELP as it would change the behavior much too silently for my taste. Once the dependency is in I'll get some numbers.

There is probable some possible optimization in the init code as well.

chrysn · 2022-02-10T13:32:37Z

and printing a warning to stdout if there are no values (or the values don't match what is measured)?

I don't quite understand this suggestion, you mean in the test?

I mean that whenever it is affordable to do this on startup (may not be with develhelp, might be in some example), the measurement should be taken immaterial of whether an adjust value is set or not. Thus, not only missing values will be acted on, but also wrong values. (Messages might be "Warning: No CONFIG_ZTIMER_USEC_ADJUST_SLEEP set. A value of 27 was measured and should be added to the ... configuration" vs. "Warning: The CONFIG_ZTIMER_USEC_ADJUST_SLEEP value of 30 that was configured disagrees with startup measurements that produced a value of 27.")

fjmolinas · 2022-02-10T13:43:12Z

I mean that whenever it is affordable to do this on startup (may not be with develhelp, might be in some example), the measurement should be taken immaterial of whether an adjust value is set or not. Thus, not only missing values will be acted on, but also wrong values. (Messages might be "Warning: No CONFIG_ZTIMER_USEC_ADJUST_SLEEP set. A value of 27 was measured and should be added to the ... configuration" vs. "Warning: The CONFIG_ZTIMER_USEC_ADJUST_SLEEP value of 30 that was configured disagrees with startup measurements that produced a value of 27.")

Humm but here you are saying you trust the measured at runtime value more than the configured value, and I would rather think the other way around. But I guess this is something to evaluate as well.

chrysn · 2022-02-10T13:48:32Z

Yeah, I deliberately left that open; stability of the values across settings may play a part in that evaluation.

fjmolinas · 2022-02-15T21:04:35Z

Let me suggest getting this in without being default, postponing that decision.

Removed as default

fjmolinas · 2022-02-15T21:05:16Z

What I'd really like to see is the test that verifies any configured values match the calculated ones. It won't have much value if only two boards are regularly tested, but let's hope that improves.

Added, I'll later run tests on the BOARDs I have, but currently busy with another set of tests.

fjmolinas · 2022-02-16T09:49:14Z

makefiles/pseudomodules.inc.mk

+## @defgroup pseudomodule_ztimer_auto_adjust ztimer_auto_adjust
+## @brief A module to set on init ztimer->adjust_sleep/adjust_set values
+##
+## When this module is active (which it is by default), then on init


This needs to be changed since its not on by default

fjmolinas · 2022-02-17T06:57:54Z

May I squash?

kaspar030 · 2022-02-17T08:11:46Z

This needs to be changed since its not on by default

this is still the case, no? yeah, you can squash!

kaspar030

ACK.

fjmolinas · 2022-02-17T08:41:57Z

Hmm the test failed on some avr:

arduino-uno

main(): This is RIOT! (Version: riot/pr/17633)
ZTIMER_USEC auto_adjust params:
    ZTIMER_USEC->adjust_set = 125
    ZTIMER_USEC->adjust_sleep = 117
ZTIMER_USEC auto_adjust params cleared
zitmer_overhead_set...
min=128 max=128 avg_diff=128
zitmer_overhead_sleep...
min=116 max=120 avg_diff=119
ZTIMER_USEC adjust params for arduino-uno:
    CONFIG_ZTIMER_USEC_ADJUST_SET    128
    CONFIG_ZTIMER_USEC_ADJUST_SLEEP  116

arduino-mega2560

main(): This is RIOT! (Version: riot/pr/17633)
ZTIMER_USEC auto_adjust params:
    ZTIMER_USEC->adjust_set = 129
    ZTIMER_USEC->adjust_sleep = 125
ZTIMER_USEC auto_adjust params cleared
zitmer_overhead_set...
min=132 max=136 avg_diff=133
zitmer_overhead_sleep...
min=120 max=124 avg_diff=123
ZTIMER_USEC adjust params for arduino-mega2560:
    CONFIG_ZTIMER_USEC_ADJUST_SET    132
    CONFIG_ZTIMER_USEC_ADJUST_SLEEP  120

atmega256rfr2-xpro

main(): This is RIOT! (Version: riot/pr/17633)
ZTIMER_USEC auto_adjust params:
    ZTIMER_USEC->adjust_set = 129
    ZTIMER_USEC->adjust_sleep = 121
ZTIMER_USEC auto_adjust params cleared
zitmer_overhead_set...
min=132 max=136 avg_diff=133
zitmer_overhead_sleep...
min=120 max=124 avg_diff=123
ZTIMER_USEC adjust params for atmega256rfr2-xpro:
    CONFIG_ZTIMER_USEC_ADJUST_SET    132
    CONFIG_ZTIMER_USEC_ADJUST_SLEEP  120

kfessel · 2022-02-17T11:17:52Z

@fjmolinas
there might be a potential difference in the time a pointer needs to be processed from avr < 128 to avr >= 128 since its addressspace increases

might be possible to get the value related/adjusted by using the lenght (sizeof?) of a function containing a JMP/CALL (som avr need to use eijmp (with some reg initialised and some extra long returns)
there are nice 1/2/3 cpu-cycles does this take in the avr-datasheets
https://ww1.microchip.com/downloads/en/DeviceDoc/AVR-InstructionSet-Manual-DS40002198.pdf Table 5-3

needs another look

fjmolinas · 2022-02-18T08:53:19Z

@kaspar030 care to take another look?

fjmolinas · 2022-02-18T08:56:26Z

@kfessel I think this is still a bit premature optimization, configuring sensible values is still the best option IMO, this would just allow it to work in (mostly) all cases but with this extra boot-up time.

kaspar030

re-ACK.

kfessel

avoid reducing to much by default Arduino / AVR common

boards/common/arduino-atmega/include/board_common.h

fjmolinas · 2022-02-18T12:08:03Z

Squashed

fjmolinas · 2022-02-21T07:28:39Z

@kaspar030 @kfessel all green!

fjmolinas · 2022-02-23T08:08:45Z

Anything holding this one back?

fjmolinas · 2022-02-23T09:04:27Z

Thanks!

fjmolinas requested a review from kaspar030 February 9, 2022 13:54

fjmolinas requested review from aabadie, bergzand, cgundogan, jia200x, leandrolanzieri, MichelRottleuthner, miri64 and smlng as code owners February 9, 2022 13:54

fjmolinas requested a review from kfessel February 9, 2022 13:54

github-actions bot added Area: build system Area: Build system Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Feb 9, 2022

fjmolinas changed the title ~~Pr ztimer auto adjust~~ sys/ztimer: add auto_adjust module Feb 9, 2022

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 9, 2022

leandrolanzieri reviewed Feb 10, 2022

View reviewed changes

fjmolinas added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Feb 15, 2022

fjmolinas commented Feb 16, 2022

View reviewed changes

sys/include/ztimer: document CONFIG_ZTIMER_USEC_ADJUST_%

2005ba5

fjmolinas force-pushed the pr_ztimer_auto_adjust branch from b9f962a to c0d4047 Compare February 17, 2022 08:30

kaspar030 previously approved these changes Feb 17, 2022

View reviewed changes

github-actions bot added the Area: tools Area: Supplementary tools label Feb 17, 2022

kaspar030 approved these changes Feb 18, 2022

View reviewed changes

kfessel requested changes Feb 18, 2022

View reviewed changes

boards/common/arduino-atmega/include/board_common.h Outdated Show resolved Hide resolved

kfessel approved these changes Feb 18, 2022

View reviewed changes

kaspar030 and others added 3 commits February 18, 2022 13:07

sys/ztimer: add auto_adjust module

d758cb9

tests/ztimer_overhead: add ztimer_sleep overhead

a8006a7

boards: update CONFIG_ZTIMER_USEC_ADJUST% values

59d069c

fjmolinas force-pushed the pr_ztimer_auto_adjust branch from 9d5b998 to 59d069c Compare February 18, 2022 12:07

kaspar030 merged commit c615122 into RIOT-OS:master Feb 23, 2022

fjmolinas deleted the pr_ztimer_auto_adjust branch February 23, 2022 09:04

OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022

mguetschow mentioned this pull request Nov 18, 2024

sys/ztimer: mention ztimer_auto_adjust in relevant places #21003

Merged

sys/ztimer: add auto_adjust module #17633

sys/ztimer: add auto_adjust module #17633

Uh oh!

Conversation

fjmolinas commented Feb 9, 2022

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kaspar030 commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Feb 9, 2022

Uh oh!

kaspar030 commented Feb 9, 2022

Uh oh!

fjmolinas commented Feb 9, 2022

Uh oh!

leandrolanzieri Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Feb 10, 2022

Uh oh!

chrysn commented Feb 10, 2022

Uh oh!

fjmolinas commented Feb 10, 2022

Uh oh!

fjmolinas commented Feb 10, 2022

Uh oh!

chrysn commented Feb 10, 2022

Uh oh!

fjmolinas commented Feb 10, 2022

Uh oh!

chrysn commented Feb 10, 2022

Uh oh!

fjmolinas commented Feb 15, 2022

Uh oh!

fjmolinas commented Feb 15, 2022

Uh oh!

fjmolinas Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 17, 2022

Uh oh!

kaspar030 commented Feb 17, 2022

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 17, 2022

Uh oh!

kfessel commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Feb 18, 2022

Uh oh!

fjmolinas commented Feb 18, 2022

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas commented Feb 18, 2022

Uh oh!

fjmolinas commented Feb 21, 2022

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

Uh oh!

kaspar030 commented Feb 9, 2022 •

edited

Loading

kfessel commented Feb 17, 2022 •

edited

Loading