Skip to content

xtimer: Handle overflows in xtimer_spin()#4183

Merged
jnohlgard merged 2 commits intoRIOT-OS:masterfrom
jnohlgard:pr/xtimer-spin-overflow
Mar 22, 2016
Merged

xtimer: Handle overflows in xtimer_spin()#4183
jnohlgard merged 2 commits intoRIOT-OS:masterfrom
jnohlgard:pr/xtimer-spin-overflow

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Oct 28, 2015

(This is based on #4181)

This PR fixes a potential problem with xtimer_spin where xtimer_spin will exit early if we are spinning across an overflow boundary of the underlying hardware timer.

@jnohlgard jnohlgard added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) State: waiting for other PR State: The PR requires another PR to be merged first Area: timers Area: timer subsystems labels Oct 28, 2015
@jnohlgard jnohlgard added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Oct 28, 2015
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Oct 28, 2015

Doesn't modulo 2**32 make this safe for all sane values of offset?

An overflow would give this sequence of comparisons (start = 0xFFFFFFFE): (0xFFFFFFFF - 0xFFFFFFFE) = 1, (0x0 - 0xFFFFFFFE) = 2, (0x1 - 0xFFFFFFFE) = 3

So the substraction always yields the nr of ticks since start.

@jnohlgard jnohlgard mentioned this pull request Oct 28, 2015
Closed
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Oct 28, 2015

hmm, you have a point. I think this change needs further review. I still believe there is an issue with less-than-32-bit-wide timers though.. (uint32_t)(0 - 0xffff) == 0xffff0001

@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Nov 11, 2015

@gebart could you rebase this to master to make the diff a little bit more clear?

@jnohlgard jnohlgard force-pushed the pr/xtimer-spin-overflow branch from 2a08a8f to 1e1e047 Compare November 23, 2015 07:51
@jnohlgard jnohlgard removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 23, 2015
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Nov 23, 2015

@cgundogan, rebased.

cgundogan
cgundogan reviewed Nov 23, 2015
View reviewed changes
sys/include/xtimer.h Outdated
Copy link
Copy Markdown
Member

@cgundogan cgundogan Nov 23, 2015

Choose a reason for hiding this comment

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

Wouldn't > suffice here?

Copy link
Copy Markdown
Member Author

@jnohlgard jnohlgard Nov 23, 2015

Choose a reason for hiding this comment

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

no, because then if the low level timer has not yet incremented since the start tick then we will immediately jump out of the spin.

@OlegHahm OlegHahm modified the milestone: Release 2016.03 Dec 8, 2015
@jnohlgard jnohlgard force-pushed the pr/xtimer-spin-overflow branch from 1e1e047 to 745dcfc Compare January 19, 2016 13:34
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Jan 19, 2016

I rewrote this based on the discussion above. To me it seems like this should work for less than 32 bit wide hardware timers as well. What do you think @cgundogan, @kaspar030?

@jnohlgard jnohlgard force-pushed the pr/xtimer-spin-overflow branch from 745dcfc to 14cc40a Compare February 16, 2016 06:05
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 16, 2016

rebased on latest master

@jnohlgard jnohlgard mentioned this pull request Feb 16, 2016
Closed
@jnohlgard jnohlgard added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Feb 16, 2016
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Feb 27, 2016

@kaspar030, ping!

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 27, 2016
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Mar 14, 2016

@kaspar030 ping

OK to squash?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Mar 14, 2016

Yes, looks good! ACK.

@jnohlgard jnohlgard force-pushed the pr/xtimer-spin-overflow branch from 14cc40a to d95e7a3 Compare March 14, 2016 10:20
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Mar 14, 2016

rebased, squashed

@jnohlgard 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 Mar 14, 2016
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Mar 19, 2016

Getting failures from a Doxygen warning in an unrelated part of the file. I piggybacked one Doxygen fix but I will refactor this file soon-ish so I don't want to spend effort documenting stuff that I haven't touched in this PR.

@jnohlgard jnohlgard 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 Mar 20, 2016
jnohlgard pushed a commit that referenced this pull request Mar 22, 2016
Merge pull request #4183 from gebart/pr/xtimer-spin-overflow
9f6d5c3 
xtimer: Handle overflows in xtimer_spin()
@jnohlgard jnohlgard merged commit 9f6d5c3 into RIOT-OS:master Mar 22, 2016
@jnohlgard jnohlgard deleted the pr/xtimer-spin-overflow branch March 22, 2016 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@kaspar030 kaspar030

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 Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2016.04

Development

Successfully merging this pull request may close these issues.

4 participants

@jnohlgard @kaspar030 @cgundogan @OlegHahm