xtimer: Refactor xtimer_usleep_until and rename to xtimer_periodic_wakeup by jnohlgard · Pull Request #5612 · RIOT-OS/RIOT

jnohlgard · 2016-07-07T14:29:33Z

This is a clean up of the function xtimer_usleep_until, which has been renamed xtimer_periodic_wakeup to better reflect the actual purpose of the function.
There were some unexplained uses of magic numbers and the function was unnecessarily difficult to understand. The magic numbers have been removed and the conditionals have been refactored to make them easier to read.

jnohlgard · 2016-07-07T14:31:37Z

sys/xtimer/xtimer.c

     */
    uint32_t offset = target - now;
-
-    if (offset > (XTIMER_BACKOFF * 2)) {


The * 2 was never explained in the original implementation. This may need some tweaking of the board configurations.

Well, the numbers might have seemed arbitrary, but the logic did serve a purpose: use a relative offset for very small offsets. Seems to me you've dropped that safety net?

The comment regarding relative offsets never underflowing is not necessarily true, since all timer set calls inside xtimer_core use timer_set_absolute to set the target time.

Ah, you're right. In my tests it was a lot safer, though.

I hope to fix this completely with #5428.

I'm a little reluctant to just drop the extra safety, because I did do quite some experimenting to get this stable (non-underflowing). But I'm ok with the other changes.

Would you mind splitting this out of this PR?

Yes, I'll prepare a PR which keeps the three decisions but introduces a configurable value instead of hard-coding 512 and BACKOFF*2.

jnohlgard · 2016-07-07T14:38:39Z

The biggest functional change here is the removal of the "set relative target time" outcome for offsets in the interval XTIMER_BACKOFF*2 < offset < 512 [µs]. It was not obvious why 512 µs was chosen as the cut off for absolute target times.

kYc0o · 2016-07-07T14:40:04Z

Code looks OK, I'll test on a pba-d-01-kw2x and give feedback, but I'd like @kaspar030 to look into this big changes.

kYc0o · 2016-07-07T16:01:04Z

in tests/saul:

Building application "saul" for "pba-d-01-kw2x" with MCU "kw2x".

/Users/facosta/git/RIOT-OS/RIOT/tests/saul/main.c: In function 'main':
/Users/facosta/git/RIOT-OS/RIOT/tests/saul/main.c:53:33: error: 'last_wakeup' undeclared (first use in this function)
         xtimer_periodic_wakeup(&last_wakeup, INTERVAL);
                                 ^
/Users/facosta/git/RIOT-OS/RIOT/tests/saul/main.c:53:33: note: each undeclared identifier is reported only once for each function it appears in
/Users/facosta/git/RIOT-OS/RIOT/tests/saul/main.c:35:14: error: unused variable 'last' [-Werror=unused-variable]
     uint32_t last = xtimer_now();

I tested most of the changes so far and this is the only problem I have.

jnohlgard · 2016-07-07T16:09:56Z

oops, copy pasta!
Fixed typo, immediately squashed.

kYc0o · 2016-07-08T08:06:22Z

Great! Let's Murdock take a look ;)

kaspar030 · 2016-07-08T09:12:28Z

sys/xtimer/xtimer.c

-        if (!((target < *last_wakeup) && (target > now))) {
+    if (now < (*last_wakeup)) {
+        /* base timer overflowed between last_wakeup and now */
+        if (!((now < target) && (target < (*last_wakeup)))) {


(now < target) is not equivalent to (target > now).

I don't understand? In C, a < b is equivalent to b > a AFAIK

Sorry, of course. I'll get coffee.

The reason I reversed the order of the comparisons was to make it easier to read.

Mathematically, the inner part of the expression is: now < target < last_wakeup

jnohlgard · 2016-07-08T10:31:37Z

As for the risk of underflows, I propose the following:
Reintroduce the three different outcomes (spin, relative, absolute), but introduce two new macros for the cutoffs.
How about XTIMER_PERIODIC_SPIN, XTIMER_PERIODIC_RELATIVE for the names?

XTIMER_PERIODIC_SPIN would default to XTIMER_BACKOFF since that's what's used in the timer scheduling code for deciding between spinning or set a new timer target.

XTIMER_PERIODIC_RELATIVE default needs to be experimented with, measuring the xtimer_now value at the point of the call to _xtimer_set_absolute and comparing to now should give an indication of how long the code in xtimer_periodic_wakeup takes to execute.

kaspar030 · 2016-07-08T10:41:43Z

Reintroduce the three different outcomes (spin, relative, absolute), but introduce two new macros for the cutoffs.

ok!

How about |XTIMER_PERIODIC_SPIN|, |XTIMER_PERIODIC_RELATIVE| for the names?

ok!

|XTIMER_PERIODIC_SPIN| would default to |XTIMER_BACKOFF| since that's what's used in the timer scheduling code for deciding between spinning or set a new timer target.

Let's not change the semantics in this PR but keep it as cleanup / API change. Changing the actual values is easy. And really, these magic values increased stability a lot. ;)

Well, XTIMER_PERIODIC_RELATIVE needs to be higher, as the code is run with interrupts enabled.

jnohlgard · 2016-07-08T11:47:25Z

Actually, what happens if an interrupt fires in the middle of xtimer_periodic_wakeup and the active thread is context switched out?
That will potentially cause underflows if the target time is small, unless we make the limit for relative targets very large.

One solution would of course be to only use 64 bit times, but I'm not a fan of using 64 bit numbers throughout on these constrained platforms..

jnohlgard · 2016-07-08T11:51:45Z

Introduced the discussed changes

kaspar030 · 2016-07-08T11:52:26Z

One solution would of course be to only use 64 bit times, but I'm not a fan of using 64 bit numbers throughout on these constrained platforms..

In #5428 I went that route. It doesn't look too bad [1].

[1] https://github.com/RIOT-OS/RIOT/pull/5428/files#diff-7412db2749b270423cfd5430e69744b9R61

jnohlgard · 2016-07-08T12:07:50Z

@kaspar030 you're right, it doesn't look too bad.

jnohlgard · 2016-07-08T12:23:16Z

Fixed the mistake in time has already passed check < vs <=

jnohlgard · 2016-07-12T08:28:05Z

OK to squash?

jnohlgard · 2016-07-12T08:28:38Z

(should be able to merge before deadline for release 2016.07)

kYc0o · 2016-07-16T12:56:05Z

@kaspar030 can you give a last advice before squashing?

kYc0o · 2016-07-25T13:13:17Z

So will we postpone this for the next release?

kYc0o · 2016-07-27T15:57:33Z

@gebart do you think this PR should be in this release?

jnohlgard · 2016-07-27T17:00:36Z

I'd like it in as soon as possible, but I think it's more of an enhancement (and clean up) than a bugfix. The only thing that has been missing from this PR is a final review and an ACK.

kaspar030 · 2016-07-27T19:55:09Z

ACK for the change, if the existing code still works, I think we can merge.

-----Original Message-----
From: "Joakim Nohlgård" notifications@github.com
To: RIOT-OS/RIOT RIOT@noreply.github.com
Cc: Kaspar Schleiser kaspar@schleiser.de, Mention mention@noreply.github.com
Sent: Mi., 27 Juli 2016 19:23
Subject: Re: [RIOT-OS/RIOT] xtimer: Refactor xtimer_usleep_until and rename to xtimer_periodic_wakeup (#5612)

I'd like it in as soon as possible, but I think it's more of an enhancement than a bugfix. The only thing that has been missing from this PR is a review and an ACK.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5612 (comment)

kYc0o · 2016-07-28T10:22:48Z

@gebart can you squash please?

jnohlgard · 2016-07-28T12:10:42Z

squashed

jnohlgard · 2016-07-28T12:11:09Z

should I squash more?

kYc0o · 2016-07-28T12:28:24Z

I think is enough. Let's wait for murdock and merge afterwards.

kYc0o · 2016-07-28T12:37:02Z

Murdock failed on the doc for xtimer.h. Can you verify?

jnohlgard · 2016-07-29T07:39:47Z

Fixed all Doxygen warnings in xtimer.h except:

sys/include/xtimer.h:413: warning: Member XTIMER_MASK_SHIFTED (macro definition) of group sys_
xtimer is not documented.

I think that macro might be going away in a refactoring in next release and it's only used internally in xtimer, I don't think we need to fix that documentation right now.

jnohlgard · 2016-07-29T07:40:57Z

OK to squash?

kYc0o · 2016-07-29T08:19:52Z

OK agree on my side and OK to squash.

…keup Rewrote conditions for improved readability, and removed magic number 512

jnohlgard · 2016-07-29T11:05:27Z

Squashed doxygen fixes. Ready

kYc0o · 2016-07-29T11:44:14Z

Excellent! Murdock green and already ACKED so go!

jnohlgard added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation TimerTaskForce Area: timers Area: timer subsystems labels Jul 7, 2016

jnohlgard added this to the Release 2016.07 milestone Jul 7, 2016

jnohlgard assigned kaspar030 and kYc0o Jul 7, 2016

jnohlgard reviewed Jul 7, 2016
View reviewed changes

jnohlgard added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Jul 7, 2016

jnohlgard mentioned this pull request Jul 7, 2016

xtimer: Allow arbitrary timer frequency, second attempt #5608

Merged

jnohlgard force-pushed the pr/xtimer-periodic-wakeup branch from 2bd1d0c to ae6e35a Compare July 7, 2016 16:09

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 8, 2016

kaspar030 added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Jul 8, 2016

kaspar030 reviewed Jul 8, 2016
View reviewed changes

jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jul 8, 2016

jnohlgard mentioned this pull request Jul 12, 2016

sys: make xtimer isr-safe #5428

Closed

jnohlgard force-pushed the pr/xtimer-periodic-wakeup branch from 40fa8e2 to e777d2f Compare July 28, 2016 12:10

jnohlgard removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jul 28, 2016

jnohlgard force-pushed the pr/xtimer-periodic-wakeup branch from 5d96001 to c0ad83c Compare July 29, 2016 11:05

Joakim Nohlgård added 3 commits July 29, 2016 13:05

xtimer: Refactor xtimer_usleep_until and rename to xtimer_periodic_wa…

4449ba4

…keup Rewrote conditions for improved readability, and removed magic number 512

tests/xtimer_periodic_wakeup: Copy and adapt from xtimer_usleep_until

71501de

tests: Update xtimer_usleep_until usage to xtimer_periodic_wakeup

c0ad83c

kYc0o merged commit 1b3012b into RIOT-OS:master Jul 29, 2016

jnohlgard deleted the pr/xtimer-periodic-wakeup branch July 29, 2016 11:47

kYc0o mentioned this pull request Jan 16, 2017

sys: xtimer: WIP: xtimer refactoring #5355

Closed

Conversation

jnohlgard commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 7, 2016

Uh oh!

kYc0o commented Jul 7, 2016

Uh oh!

kYc0o commented Jul 7, 2016

Uh oh!

jnohlgard commented Jul 7, 2016

Uh oh!

kYc0o commented Jul 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jul 8, 2016

Uh oh!

kaspar030 commented Jul 8, 2016 via email

Uh oh!

jnohlgard commented Jul 8, 2016

Uh oh!

jnohlgard commented Jul 8, 2016

Uh oh!

kaspar030 commented Jul 8, 2016

Uh oh!

jnohlgard commented Jul 8, 2016

Uh oh!

jnohlgard commented Jul 8, 2016

Uh oh!

jnohlgard commented Jul 12, 2016

Uh oh!

jnohlgard commented Jul 12, 2016

Uh oh!

kYc0o commented Jul 16, 2016

Uh oh!

kYc0o commented Jul 25, 2016

Uh oh!

kYc0o commented Jul 27, 2016

Uh oh!

jnohlgard commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Jul 27, 2016

Uh oh!

kYc0o commented Jul 28, 2016

Uh oh!

jnohlgard commented Jul 28, 2016

Uh oh!

jnohlgard commented Jul 28, 2016

Uh oh!

kYc0o commented Jul 28, 2016

Uh oh!

kYc0o commented Jul 28, 2016

Uh oh!

jnohlgard commented Jul 29, 2016

Uh oh!

jnohlgard commented Jul 29, 2016

Uh oh!

kYc0o commented Jul 29, 2016

Uh oh!

jnohlgard commented Jul 29, 2016

Uh oh!

jnohlgard commented Jul 27, 2016 •

edited

Loading