Skip to content

kinetis: lptmr reload instead of spinning#10020

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-lptmr-reload
Sep 11, 2019
Merged

kinetis: lptmr reload instead of spinning#10020
benpicco merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-lptmr-reload

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Sep 24, 2018

Contribution description

Spinning for the correct time has the side effect that it may cause
infinite recursion if the callback function calls timer_set.

timer_set->callback->...->timer_set->callback->...->timer_set-> infinity

In theory, the drawback is that the callback for very short timeouts
(<2 lptmr ticks) may be delayed up to 2 lptmr ticks (61 µs)

In practice however, tests performed using tests/bench_timers shows that
this change only affects the accuracy of the timer target when timer_set
is called with a timeout of 0, which results in a delay of 30 µs.

Testing procedure

Run the test in #10019 on a kinetis board with an LPTMR configuration (e.g. any of the frdm boards, for k64f, see #9930) with this PR applied.

Issues/PRs references

Regression test for this bug #10019
LPTMR configuration for frdm-k64f: #9930

@jnohlgard jnohlgard added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ARM Platform: This PR/issue effects ARM-based platforms 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 labels Sep 24, 2018
@jnohlgard jnohlgard added this to the Release 2018.10 milestone Sep 24, 2018
@jnohlgard jnohlgard requested a review from kYc0o September 24, 2018 07:11
@jnohlgard jnohlgard mentioned this pull request Sep 26, 2018
Open
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Nov 13, 2018

I have noticed that bench_timers shows that the target is missed sometimes when LPTMR timer_set is called with timeout=0. This PR fixes it by reloading whenever timeout == 0 instead of trying to race update the compare register.

@kaspar030 kaspar030 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 20, 2018
kaspar030
kaspar030 approved these changes Nov 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Untested ACK. Don't have the hardware. Could someone give this a quick spin?

kinetis: lptmr reload instead of spinning
fa3b916 
Spinning for the correct time has the side effect that it may cause
infinite recursion if the callback function calls timer_set.

timer_set->callback->...->timer_set->callback->...->timer_set-> infinity

In theory, the drawback is that the callback for very short timeouts
(<2 lptmr ticks) may be delayed up to 2 lptmr ticks (61 µs)

In practice however, tests performed using tests/bench_timers shows that
this change only affects the accuracy of the timer target when timer_set
is called with a timeout of 0, which results in a delay of 30 µs.
@jnohlgard jnohlgard force-pushed the pr/kinetis-lptmr-reload branch from d971602 to fa3b916 Compare December 14, 2018 11:01
@miri64 miri64 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 Dec 18, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 18, 2018

@smlng @aabadie anyone of you care to test?

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Mar 26, 2019

I tested this PR with tests/periph_timer_timeout0 from #10019 on a frdm-k22f. At first it failed due to coordination of tests and reset (an the fact it is on a raspberry pi) but it passes when I make flash -C tests/periph_timer_timeout0 then make test -C tests/periph_timer_timeout0

frdm-k22f 

2019-03-26 13:52:42,295 - INFO # main(): This is RIOT! (Version: 2018.10-RC1-549-g438a1-test10020)
2019-03-26 13:52:42,297 - INFO # 
2019-03-26 13:52:42,305 - INFO # Test for timer_set with timeout=0
2019-03-26 13:52:42,307 - INFO # 
2019-03-26 13:52:42,312 - INFO # Available timers: 3
2019-03-26 13:52:42,319 - INFO # 
2019-03-26 13:52:42,324 - INFO # Testing TIMER_DEV(0):
2019-03-26 13:52:42,331 - INFO # TIMER_DEV(0) running at 1000000 Hz
2019-03-26 13:52:42,357 - INFO # (debug) TIMER_DEV(0) switches: 1
2019-03-26 13:52:42,359 - INFO # 
2019-03-26 13:52:42,363 - INFO # Testing TIMER_DEV(1):
2019-03-26 13:52:42,368 - INFO # TIMER_DEV(1) running at 1000000 Hz
2019-03-26 13:52:42,421 - INFO # (debug) TIMER_DEV(1) switches: 1
2019-03-26 13:52:42,421 - INFO # 
2019-03-26 13:52:42,423 - INFO # Testing TIMER_DEV(2):
2019-03-26 13:52:42,426 - INFO # TIMER_DEV(2) running at 32768 Hz
2019-03-26 13:52:43,042 - INFO # (debug) TIMER_DEV(2) switches: 10000
2019-03-26 13:52:43,043 - INFO # 
2019-03-26 13:52:43,044 - INFO # TEST SUCCEEDED

I just did the test only, I am relying on @kaspar030 review. Maybe he can merge then.

@MrKevinWeiss MrKevinWeiss added Reviewed: 3-testing The PR was tested according to the maintainer guidelines 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 Mar 26, 2019
@MrKevinWeiss MrKevinWeiss self-assigned this May 28, 2019
@MrKevinWeiss MrKevinWeiss removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 1, 2019
@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 1, 2019
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Jul 1, 2019

ping @kaspar030 @miri64

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Sep 10, 2019

Everything is green here already - any reason not to press the button?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 10, 2019

#10020 (comment) was the only thing missing, but @MrKevinWeiss was taking care of this. Let's just give Murdock another spin, since the last build of this PR is quite old.

@miri64 miri64 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 Sep 10, 2019
@benpicco benpicco merged commit b6fe0e2 into RIOT-OS:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

@kYc0o kYc0o Awaiting requested review from kYc0o

Assignees

@MrKevinWeiss MrKevinWeiss

Labels

Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

7 participants

@jnohlgard @miri64 @MrKevinWeiss @benpicco @kaspar030 @jia200x @aabadie