Skip to content

sys/ztimer: make use of periph_timer_query_freqs#20582

Merged
maribu merged 3 commits intoRIOT-OS:masterfrom
maribu:ztimer_periph_timer_query_freqs
Apr 16, 2025
Merged

sys/ztimer: make use of periph_timer_query_freqs#20582
maribu merged 3 commits intoRIOT-OS:masterfrom
maribu:ztimer_periph_timer_query_freqs

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Apr 15, 2024 •
edited
Loading

Contribution description

This makes use of the periph_timer_query_freqs feature:

  1. It does choose the closest frequency supported before calling timer_init() in the ztimer_periph_timer backend.
  2. It does make use of the actually chosen frequency when using ztimer_convert_frac.
  3. It does assert() the frequency is within 5% of the specified when no frequency conversion is performed or ztimer_convert_shift_up is used.

Motivation

We have a number of MCUs that can run from internal oscillators (without using external crystal for accuracy):

  • ATmega MCUs
  • STM32 MCUs
  • MSP430 MCUs
  • RP2040
  • FE310 (but it doesn't have a proper periph_timer, so not relevant here)
  • ...

If those MCUs are used without external crystal, the actual CPU frequency may be off from what the the board config aims for significantly due to large production variance and temperature and supply voltage dependence. As a result, all periph_timer instances driven by the same clock will also be off significantly, which can hinder accuracy in time keeping significantly.

This adds the support to ztimer to react on the actual frequency information provided by periph_timer_query_freqs and adjust the time keeping accordingly.

Testing procedure

As of now, only the MSP430 (and the FE310) will measure the actual CPU frequency at runtime, and only the MSP430 timer will compensate frequencies exposed via timer_query_freqs(). So only on MSP430 this can really be tested now, e.g. with tests/sys/ztimer_msg.

In master this test sadly fails because of timer_set() not yet working correctly on small timeouts (and waiting one period longer than needed).

With this PR the test passes and the messages at second 30 is indeed about thirty seconds after the start message received. (It would be off by 40 % without compensation.)

Issues/PRs references

Fixes #8251

@maribu maribu added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 15, 2024
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: timers Area: timer subsystems Area: boards Area: Board ports Area: sys Area: System labels Apr 15, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Apr 15, 2024 •
edited
Loading

Murdock results

✔️ PASSED

c05d546 examples,tests: Update Makefile.ci

Success Failures Total Runtime
150647 0 150647 01h:53m:38s

Artifacts

@maribu maribu force-pushed the ztimer_periph_timer_query_freqs branch 3 times, most recently from 6d3abf6 to 4933b9b Compare April 16, 2024 21:34
@maribu maribu mentioned this pull request Apr 30, 2024
Closed
@MrKevinWeiss MrKevinWeiss added State: waiting for end of feature-freeze Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. labels Jan 21, 2025
@MrKevinWeiss MrKevinWeiss removed the Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. label Jan 21, 2025
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Jan 21, 2025

Freeze is over so if anyone wants to pick this up...

crasbe
crasbe reviewed Apr 1, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@crasbe crasbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't have an MSP430 board, so I tested it with an P-Nucleo-WB55 and nRF52840DK. The tests/sys/ztimer_msg tests ran fine on both, so I assume nothing broke?

sys/ztimer/init.c Outdated
#if MODULE_ZTIMER_USEC
# if ZTIMER_TIMER_FREQ != FREQ_1MHZ
# if ZTIMER_TIMER_FREQ == FREQ_250KHZ
/* 250 kHz ± 5% */
Copy link
Copy Markdown
Contributor

@crasbe crasbe Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so that I understand: the +-5% is after searching for a better frequency, right?

Some microcontrollers (the nRF52) report only 10 different frequencies in the tests/periph/timer test:

main(): This is RIOT! (Version: 2024.07-devel-15-g4933b-ztimer_periph_timer_query_freqs)

Test for peripheral TIMERs

Available timers: 4

TIMER 0
=======

  - supported frequencies:
    0: 16000000
    1: 8000000
    2: 4000000
    ....
    9: 31250
Testing timer_get_closest_freq()...
[OK]
Testing timer_query_freqs() for out of bound index...
[OK]
...

Of course it doesn't make any sense to run an nRF52840 without crystal, but my point is that some microcontrollers might exist without fine enough granularity and therefore these asserts might fail.
I don't understand the intricacies of ztimer enough, so I leave it to you to judge this, so just consider this food for thought :)

@mguetschow mguetschow mentioned this pull request Apr 2, 2025
Merged
@mguetschow
Copy link
Copy Markdown
Contributor

mguetschow commented Apr 2, 2025

So only on MSP430 this can really be tested now, e.g. with tests/sys/ztimer_msg.

In master this test sadly fails because of timer_set() not yet working correctly on small timeouts (and waiting one period longer than needed).

Just tried that test on master with BOARD=z1 and the timer is already accurate AFAICT. What do you mean with "fails"? I don't see any assert or such in the test code.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Apr 2, 2025

The python script checks the time between the messages. It is relatively lax (as measuring the time between two stdio messages is not super precise), but it does fail when the clock is off by too much. At least that was the case back then.

The Z1 does indeed use the DCO, but it is using an MSP430F2xx MCU, which has the better clock system. I think the clock calibration on start-up may actually be "good enough" on that board.

On the MSP430F1xx, the DCO often maxes out at about 5 MHz, unless an external resistor is supplied. If the nominal CPU clock is 8 MHz and the actual one is at about 5 MHz, the offset is large enough for the test to fail.

@mguetschow
Copy link
Copy Markdown
Contributor

mguetschow commented Apr 4, 2025 •
edited
Loading

I've rebased this branch on master and tested with telosb:

$ VERBOSE_ASSERT=1 BUILD_IN_DOCKER=1 make -C tests/sys/ztimer_msg BOARD=telosb flash test
...
sys/ztimer/init.c:359 => FAILED ASSERTION.

(on master, the python test succeeds)

@crasbe
Copy link
Copy Markdown
Contributor

crasbe commented Apr 12, 2025

This has to rebased after #20581 was merged.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Apr 12, 2025

$ VERBOSE_ASSERT=1 BUILD_IN_DOCKER=1 make -C tests/sys/ztimer_msg BOARD=telosb flash test
...
sys/ztimer/init.c:359 => FAILED ASSERTION.

That is both good and bad:

  • Good:
    • because it shows that timing is indeed off in master
  • Bad:
    • because it shows the test is not good enough to detect more than 5% offset from nominal frequency
    • because it was probably my brightest moment to go for an assert() here. A board that would - depending on supply current and temperature - barely pass the 5% test or fail would be a pain in the ass to use

I'll improve the test and convert the assert() to a LOG_WARNING() when DEVELHELP is set.

@maribu maribu force-pushed the ztimer_periph_timer_query_freqs branch from 31f783a to 482538c Compare April 15, 2025 13:05
@maribu maribu requested a review from miri64 as a code owner April 15, 2025 13:05
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: examples Area: Example Applications labels Apr 15, 2025
maribu added 2 commits April 15, 2025 15:10
@maribu
sys/ztimer: make use of periph_timer_query_freqs
25b8d65 
This makes use of the `periph_timer_query_freqs` feature:

1. It does choose the closest frequency supported before calling
   timer_init() in the ztimer_periph_timer backend.
2. It does make use of the actually chosen frequency when using
   `ztimer_convert_frac`.
3. It does `assert()` the frequency is within 5% of the specified when
   no frequency conversion is performed or `ztimer_convert_shift_up`
   is used.
@maribu
boards/{olimex-msp430-h1611,telosb}: add ztimer config
53fa78a 
Adding a ztimer configuration that forces the use of frequency conversion
will greatly improve timing accuracy, as the frequency detected at
runtime will just be too much off from the nominal 1 MHz to run without
frequency conversion.
@maribu maribu force-pushed the ztimer_periph_timer_query_freqs branch from 482538c to 28466c0 Compare April 15, 2025 13:10
@mguetschow mguetschow added this pull request to the merge queue Apr 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2025
@maribu maribu force-pushed the ztimer_periph_timer_query_freqs branch from 28466c0 to 3189003 Compare April 16, 2025 07:09
@maribu maribu enabled auto-merge April 16, 2025 07:26
@maribu maribu added this pull request to the merge queue Apr 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2025
@mguetschow mguetschow added CI: no fast fail don't abort PR build after first error 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 Apr 16, 2025
@crasbe crasbe added CI: full build disable CI build filter 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 Apr 16, 2025
@maribu maribu force-pushed the ztimer_periph_timer_query_freqs branch 2 times, most recently from 89f20e8 to b42d7bf Compare April 16, 2025 10:55
@maribu
examples,tests: Update Makefile.ci
c05d546 
This updates the `Makefile.ci` entries for all MSP430 boards plus a few
low end Cortex-M boards.
@maribu maribu force-pushed the ztimer_periph_timer_query_freqs branch from b42d7bf to c05d546 Compare April 16, 2025 13:25
@maribu maribu added this pull request to the merge queue Apr 16, 2025
Merged via the queue into RIOT-OS:master with commit c362a09 Apr 16, 2025
25 checks passed
@maribu maribu deleted the ztimer_periph_timer_query_freqs branch April 16, 2025 19:22
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Apr 16, 2025

Thx a lot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco left review comments

@crasbe crasbe crasbe left review comments

@mguetschow mguetschow mguetschow approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

Assignees

No one assigned

Labels

Area: boards Area: Board ports Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems CI: full build disable CI build filter CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MSP430: periph_timer clock config wrong

6 participants

@maribu @riot-ci @MrKevinWeiss @mguetschow @crasbe @benpicco