Skip to content

Conversation

jnohlgard
Copy link
Member

On a fast CPU with a slow timer (e.g. XTIMER_SHIFT > 0) it is possible
that now == _xtimer_now() when spinning for the overflow. In the extreme
case When this happens _next_period() will be called more than once
until the timer overflows for real.

Fault observed in real life when running on a 32.768 kHz timer on a
~96 MHz clocked mulle (Kinetis K60, Cortex-M4). _next_period() was
called 9 times during the same ISR call before the 32 kHz timer
overflowed.

On a fast CPU with a slow timer (e.g. XTIMER_SHIFT > 0) it is possible
that now == _xtimer_now() when spinning for the overflow. In the extreme
case When this happens _next_period() will be called more than once
until the timer overflows for real.

Fault observed in real life when running on a 32.768 kHz timer on a
~96 MHz clocked mulle (Kinetis K60, Cortex-M4). _next_period() was
called 9 times during the same ISR call before the 32 kHz timer
overflowed.
@jnohlgard jnohlgard added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 30, 2015
@jnohlgard
Copy link
Member Author

The severity of this bug varies greatly depending on how well the XTIMER_BACKOFF value is tuned to the actual required backoff. A greater XTIMER_BACKOFF value will make the effects more severe (more invocations of _next_period).

@kaspar030
Copy link
Contributor

ACK. We're ironing out more and more potential time-travels, great!

@miri64
Copy link
Member

miri64 commented Sep 30, 2015

There needs to be a PR to the release branch too for this, right?

@jnohlgard
Copy link
Member Author

@authmillenon this is kind of a corner case in the current implementation which won't happen on the current platforms until we start using slower timers, e.g. via #3990
It was during my work on a new PR which depends on #3990 for the Kinetis low power timer that I found this problem, it won't occur on the configurations in the release branch.

@jnohlgard
Copy link
Member Author

@authmillenon I opened #4000 for the same fix for the release

miri64 added a commit that referenced this pull request Sep 30, 2015
sys/xtimer: Avoid race incrementing multiple periods in _timer_callback
@miri64 miri64 merged commit 001fdc5 into RIOT-OS:master Sep 30, 2015
@jnohlgard jnohlgard deleted the pr/xtimer-long-term-fast-cpu-bug branch September 30, 2015 10:42
@jnohlgard
Copy link
Member Author

Don't you love when a whole-evening bug hunting session results in a patch which changes a single character in the source code?

@miri64
Copy link
Member

miri64 commented Sep 30, 2015

Off-by-one errors are the best :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants