Skip to content

cpu/native: fix bug in periph_timer#20009

Merged
bors[bot] merged 2 commits intoRIOT-OS:masterfrom
maribu:native_timer_bug_work_around
Nov 3, 2023
Merged

cpu/native: fix bug in periph_timer#20009
bors[bot] merged 2 commits intoRIOT-OS:masterfrom
maribu:native_timer_bug_work_around

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Oct 23, 2023 •
edited
Loading

Contribution description

While debugging #18977 (comment) it became obvious that the periph_timer in native is broken and issues early IRQs. This replaces the use of setitimer that cannot use a monotonic clock source with timer_settime().

Testing procedure

I have some non-publishable code that tests if the time an ISR fires in terms of timer_read() is no earlier than the time expected. This occasionally triggered with master, but I didn't see any of these issues anymore with this PR. I guess I should revive my PR to spice up the periph timer tests and add a polished version of this and let this run for an hour or two.

The tests ins tests/periph/timer* should still succeed on native. (They do for me in a container running riot/riotbuild).

Issues/PRs references

Found while debugging #18977 (comment)

@maribu maribu requested a review from benpicco October 23, 2023 11:19
@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: cpu Area: CPU/MCU ports labels Oct 23, 2023
@Wer-Wolf
Copy link
Copy Markdown
Contributor

Wer-Wolf commented Oct 24, 2023

Could this be because we use ITIMER_REAL as clock source, which is affected by adjtime(2) and settimeofday(2)?
Maybe this problem could be solved by using timer_create() with CLOCK_MONOTONIC.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Oct 24, 2023

Should this be backported? Probably not a bad idea.

@maribu maribu force-pushed the native_timer_bug_work_around branch from fd90d58 to 96eef23 Compare October 26, 2023 09:08
@maribu maribu changed the title cpu/native: work around bug in periph_timer cpu/native: fix bug in periph_timer Oct 26, 2023
@maribu maribu force-pushed the native_timer_bug_work_around branch 2 times, most recently from eabef72 to fd1fd2c Compare October 26, 2023 09:11
kaspar030
kaspar030 reviewed Oct 26, 2023
View reviewed changes
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 26, 2023

Could this be because we use ITIMER_REAL as clock source, which is affected by adjtime(2) and settimeofday(2)? Maybe this problem could be solved by using timer_create() with CLOCK_MONOTONIC.

Yes, indeed! This was actually also my first intuition. But rather than reading the code I did a quick grep for CLOCK_[A-Z]*, which only showed matches for CLOCK_MONOTONIC and none CLOCK_REALTIME, so I assumed this wasn't the issue. Sometime actually reading the code containing the bug does pay off 😅

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 26, 2023

Hm I still get

>>> timeout happened 9739 µs early <<<
>>> timeout happened 9623 µs early <<<
>>> timeout happened 2156 µs early <<<
>>> timeout happened 3265 µs early <<<
>>> timeout happened 9689 µs early <<<
>>> timeout happened 9795 µs early <<<
>>> timeout happened 2262 µs early <<<
>>> timeout happened 9846 µs early <<<
>>> timeout happened 9796 µs early <<<
>>> timeout happened 9647 µs early <<<
>>> timeout happened 4028 µs early <<<
>>> timeout happened 9670 µs early <<<
>>> timeout happened 4650 µs early <<<
>>> timeout happened 9687 µs early <<<
>>> timeout happened 9769 µs early <<<

with this (and the patches from #18977) applied

@Wer-Wolf
Copy link
Copy Markdown
Contributor

Wer-Wolf commented Oct 26, 2023

Maybe the fact that timer_start()/timer_stop() do not stop the value returned by timer_read() from incrementing could also be the reason for this.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 26, 2023

@benpicco: That's expected. Most of the time that bug triggered it was not due to the periph_timer bug. But it should be a tad less frequent now, with one of the causes identified and fixed.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 27, 2023
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Oct 27, 2023 •
edited
Loading

Urgh so are you suggesting there is (at least) a third bug (besides this one and #18977) at play here that makes timeouts unreliable 😩

Please squash btw

benpicco
benpicco approved these changes Oct 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I'm trusting our timer unittests

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 27, 2023

Urgh so are you suggesting there is (at least) a third bug

Yes :) But on the other hand 2/3 of the issue is hopefully solved. (Well, the third bug is the one with the most fallout, though).

@maribu maribu force-pushed the native_timer_bug_work_around branch from 5b2d9b2 to f9f0c27 Compare October 27, 2023 12:35
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 27, 2023 •
edited
Loading

Murdock results

✔️ PASSED

50b841e cpu/native: drop unused real_setitimer

Success Failures Total Runtime
7953 0 7953 17m:12s

Artifacts

@MrKevinWeiss MrKevinWeiss added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Nov 1, 2023
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Nov 2, 2023

Do we think this can get in the next little while or should we make it a known issue for this release?

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 2, 2023

Murdock found quite the bug. I really should give more priority my rewrite of the periph_timer test app higher on my todo, given that the current one was unable to detect the completely broken timer_stop() implementation 😱

maribu added 2 commits November 2, 2023 14:12
@maribu
cpu/native: fix bug in periph_timer
cea7fce 
Also use `CLOCK_MONOTONIC` for the timeouts, not just for
`timer_read()`. This fixes mismatches between when a timeout
occurs and what is expected in the context of the values returned by
`timer_read()`.
@maribu
cpu/native: drop unused real_setitimer
50b841e
@maribu maribu force-pushed the native_timer_bug_work_around branch from f9f0c27 to 50b841e Compare November 2, 2023 13:13
@maribu maribu added the CI: ready for merge train 🚃 PR is ready to be merged and awaiting the next merge train label Nov 2, 2023
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Nov 3, 2023

bors merge

@bors
Copy link
Copy Markdown
Contributor

bors bot commented Nov 3, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4250c15 into RIOT-OS:master Nov 3, 2023
@MrKevinWeiss MrKevinWeiss mentioned this pull request Nov 6, 2023
Merged
@maribu maribu deleted the native_timer_bug_work_around branch December 5, 2023 08:21
@chrysn
Copy link
Copy Markdown
Member

chrysn commented Aug 17, 2024

A brief note from a workshop, this broke building on Debian 11.

maribu added a commit to maribu/RIOT that referenced this pull request Aug 17, 2024
@maribu
cpu/native/periph_timer: add missing -lrt to linker flags
29a00be 
This fixes RIOT-OS#20009 (comment)
@maribu maribu mentioned this pull request Aug 17, 2024
Merged
maribu added a commit to maribu/RIOT that referenced this pull request Aug 18, 2024
@maribu
cpu/native/periph_timer: add missing -lrt to linker flags
11ac1e5 
This fixes RIOT-OS#20009 (comment)

(cherry picked from commit 29a00be)
@maribu maribu mentioned this pull request Aug 18, 2024
Merged
ant9000 pushed a commit to ant9000/RIOT that referenced this pull request Aug 23, 2024
@maribu @ant9000
cpu/native/periph_timer: add missing -lrt to linker flags
d78c5e2 
This fixes RIOT-OS#20009 (comment)
dprigoshij pushed a commit to dprigoshij/RIOT that referenced this pull request Mar 24, 2025
@maribu
cpu/native/periph_timer: add missing -lrt to linker flags
68aef26 
This fixes RIOT-OS#20009 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@benpicco benpicco benpicco approved these changes

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: ready for merge train 🚃 PR is ready to be merged and awaiting the next merge train Platform: native Platform: This PR/issue effects the native platform Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch

Projects

None yet

Milestone

Release 2023.10

Development

Successfully merging this pull request may close these issues.

7 participants

@maribu @Wer-Wolf @MrKevinWeiss @benpicco @riot-ci @chrysn @kaspar030