Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xtimer: timer & target overflow, hang resolved. #9211

Merged
merged 1 commit into from
Nov 2, 2018
Merged

xtimer: timer & target overflow, hang resolved. #9211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions sys/include/xtimer/implementation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,21 @@ static inline uint32_t _xtimer_lltimer_mask(uint32_t val)
* @brief xtimer internal stuff
* @internal
*/

uint64_t _xtimer_now64(void);

/**
* @brief Sets the timer to the appropriate timer_list or list_head.
*
* @note The target to set the timer to has to be at least bigger then the
* ticks needed to jump into the function and calculate '_xtimer_now()'.
* So that 'now' did not pass the target.
Copy link
Contributor

Choose a reason for hiding this comment

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

That is hard to guarantee I think. The problem is that the calling task may be de-scheduled for a few ticks and then you miss it anyway.

As it is a private function, is it not possible to update the API to pass it the now and offset value to handle it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't get this requirement. How can it be that for correct operation one needs to know the time it gets to get to a certain line? IMO this is very fragile.

Copy link
Contributor

Choose a reason for hiding this comment

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

To not pass now and offset, another solution could be to just assume (and ensure) that offset < UINT32_MAX - MAX_MARGIN where MAX_MARGIN is the maximum time we allow to enter the function and read now again.

This way, you are sure that a timer loop can be detected if you took less than MAX_MARGIN to go to now.

A large enough solution could be to say it fits in an int32_t so uses 31bits.
It is an easy thing to ensure over the code base.
With 1Mhz it would still allow working if you take 35 minutes between the first call and being in the function.

Copy link
Contributor Author

@Josar Josar Oct 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is no problem to guarantee. XTIMER_BACKOFF has to be set to as much ticks as it needs to reach the backing-off case.

I allready started an test which helps to extract the required values.
See: #8990

This requirement is based on the fact that the time is ticking while you try to set a timer. If you set your deadline too short you will miss it. Thats it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will look at #8990 as a base.

When you start calling _xtimer_set_absolute you are still in your context, and you could be de-scheduled, for some time (maybe more than XTIMER_BACKOFF), so you do not know that the target is not elapsed, or not too close from your calling function.
Of course if you call the function with something that ends up being in the past, it should _shoot it directly, but you cannot do better as it was asked too late.

All the calling functions of _xtimer_set_absolute already have a now value (2 functions + 1 test).
By getting now and offset you remove the issues with "calling offset was too short" issue and handle this in the function directly.

However I see another issue also with the current implementation, it is that when now is read in the function, and offset is evaluated it is not with masked interrupts. So everything could also go wrong there too, be de-scheduled and get stuck with xtimer_spin_until.

The only hard limitation we cannot do anything about, is that between the first now and the now in the function, there should not be more than UINT32_MAX / Frequency ticks.

I wanted to say 2min23 for 32Mhz timers, but it looks like there are only sub MHz timers.
Even if there is this comment about cc2538 using a 16Mhz timer and code to handle them.

RIOT/sys/include/xtimer/tick_conversion.h

Line 41 in 77dc923

/* e.g. cc2538 uses a 16 MHz timer */ 

git grep XTIMER_HZ  . ':!sys'
boards/common/arduino-atmega/include/board_common.h:#define XTIMER_HZ                   (250000UL)
boards/frdm-kw41z/include/board.h:#define XTIMER_HZ                   (32768ul)
boards/hifive1/include/periph_conf.h:#define XTIMER_HZ                   (32768ul)
boards/ikea-tradfri/include/board.h:#define XTIMER_HZ           (250000UL)
boards/jiminy-mega256rfr2/include/board.h:#define XTIMER_HZ           (125000UL)
boards/mega-xplained/include/board.h:#define XTIMER_HZ                   (125000UL)
boards/mulle/include/board.h:#define XTIMER_HZ                   (32768ul)
boards/slstk3401a/include/board.h:#define XTIMER_HZ           (250000UL)
boards/slstk3402a/include/board.h:#define XTIMER_HZ           (250000UL)
boards/sltb001a/include/board.h:#define XTIMER_HZ           (250000UL)
boards/slwstk6000b/include/board.h:#define XTIMER_HZ           (250000UL)
boards/stk3600/include/board.h:#define XTIMER_HZ           (250000UL)
boards/stk3700/include/board.h:#define XTIMER_HZ           (250000UL)
boards/waspmote-pro/include/board.h:#define XTIMER_HZ                   (62500UL)
cpu/esp32/periph/timer.c:    CHECK_PARAM_RET (freq == XTIMER_HZ_BASE, -1);
cpu/esp32/periph/timer.c:    CHECK_PARAM_RET (freq == XTIMER_HZ_BASE, -1);
cpu/esp8266/periph/timer.c:    CHECK_PARAM_RET (freq == XTIMER_HZ_BASE, -1);
cpu/esp8266/periph/timer.c:    CHECK_PARAM_RET (freq == XTIMER_HZ_BASE, -1);
tests/bench_timers/Makefile:test-xtimer: CFLAGS+=-DTEST_XTIMER -DTIM_TEST_FREQ=XTIMER_HZ -DTIM_TEST_DEV=XTIMER_DEV

Looking at all this really needs a lot of concentration D:

Copy link
Contributor

Choose a reason for hiding this comment

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

I could propose something with my ideas and see if it would help.

Copy link
Member

Choose a reason for hiding this comment

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

Did you see #7630 regarding passing a reference time as argument to _xtimer_set_absolute?
It needs a rebase but it is related to this discussion I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not, I just jumped in the xtimer issue to provide a test, but it means that I had a similar reasoning. I will provide a minimal test to show an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gebart I think your fix still misses something, from what I get, the function should be calling xtimer_now from within a masked interrupt context, so that the time between this now and the periph_timer_set can be guaranteed to be fixed as you cannot be de-scheduled.

If not the comparison with XTIMER_BACKOFF does not guarantee anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to show these with tests.

* This is crucial when using low CPU frequencies and/or when the
* '_xtimer_now()' call needs multiple xtimer ticks to evaluate.
*
* @param[in] timer pointer to xtimer_t which is added to the list.
* @param[in] target Absolute target value in ticks.
*/
int _xtimer_set_absolute(xtimer_t *timer, uint32_t target);
void _xtimer_set(xtimer_t *timer, uint32_t offset);
void _xtimer_set64(xtimer_t *timer, uint32_t offset, uint32_t long_offset);
Expand Down
48 changes: 40 additions & 8 deletions sys/xtimer/xtimer_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* Copyright (C) 2015 Kaspar Schleiser <[email protected]>
* Copyright (C) 2016 Eistec AB
* 2016 Eistec AB
* 2018 Josua Arndt
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
Expand All @@ -15,6 +16,7 @@
* @brief xtimer core functionality
* @author Kaspar Schleiser <[email protected]>
* @author Joakim Nohlgård <[email protected]>
* @author Josua Arndt <[email protected]>
* @}
*/

Expand Down Expand Up @@ -178,12 +180,27 @@ int _xtimer_set_absolute(xtimer_t *timer, uint32_t target)
uint32_t now = _xtimer_now();
jcarrano marked this conversation as resolved.
Show resolved Hide resolved
int res = 0;

DEBUG("timer_set_absolute(): now=%" PRIu32 " target=%" PRIu32 "\n", now, target);

timer->next = NULL;
if ((target >= now) && ((target - XTIMER_BACKOFF) < now)) {

/* Ensure that offset is bigger than 'XTIMER_BACKOFF',
* 'target - now' will allways be the offset no matter if target < or > now.
*
* This expects that target was not set too close to now and overrun now, so
* from setting target up until the call of '_xtimer_now()' above now has not
* become equal or bigger than target.
* This is crucial when using low CPU frequencies so reaching the '_xtimer_now()'
* call needs multiple xtimer ticks.
*
* '_xtimer_set()' and `_xtimer_periodic_wakeup()` ensure this by already
* backing off for small values. */
uint32_t offset = (target - now);

DEBUG("timer_set_absolute(): now=%" PRIu32 " target=%" PRIu32 " offset=%" PRIu32 "\n",
now, target, offset);

if (offset <= XTIMER_BACKOFF) {
/* backoff */
xtimer_spin_until(target + XTIMER_BACKOFF);
xtimer_spin_until(target);
_shoot(timer);
return 0;
}
Expand All @@ -195,6 +212,17 @@ int _xtimer_set_absolute(xtimer_t *timer, uint32_t target)

timer->target = target;
timer->long_target = _long_cnt;

/* Ensure timer is fired in right timer period.
* Backoff condition above ensures that 'target - XTIMER_OVERHEAD` is later
* than 'now', also for values when now will overflow and the value of target
* is smaller then now.
* If `target < XTIMER_OVERHEAD` the new target will be at the end of this
* 32bit period, as `target - XTIMER_OVERHEAD` is a big number instead of a
* small at the beginning of the next period. */
target = target - XTIMER_OVERHEAD;
Copy link
Member

Choose a reason for hiding this comment

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

What happens when target is small (target < XTIMER_OVERHEAD)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the target underflows, it will be bigger than now and no timer overflow will happen, no long target increase needed.
Unless now+overhead > target, which should be seldome. But, this leads me to a condition which schould have been adressed above when checking for backoff.
something like this:

    /* first comparison prevents overflowed target to trigger backoff, target in next period.
     * second comparison backoff condition.
     * Third condition check for overflowed target
     * Fourth condition backoff condition. */
    if ( (target >= now) && ((target - XTIMER_BACKOFF) < now) ||
         (target < now) && (target <= (now + XTIMER_BACKOFF))) {

...
    /* Ensure timer is fired in right hardware timer period.
     * Backoff condition check above ensures that taget - XTIMER_OVERHEAD > now
     * also for values when now will overflow soon and target is overflowed */
    target = target - XTIMER_OVERHEAD;

I will have a deeper look into this tomorrow and also make some tests.

Copy link
Contributor Author

@Josar Josar Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Testing for taget - XTIMER_OVERHEAD > now is allready made in _xtimer_set() or _xtimer_periodic_wakeup by testing that offset < XTIMER_BACKOFF.

If the target for _xtimer_set_absolute(...) is set to close to now and now overleaps target this leads to a whole 32bit cicle delay. But this has to be ensured by the calling functions, which it is.

I added some coments to the code.


/* 32 bit target overflow, target is in next 32bit period */
if (target < now) {
timer->long_target++;
}
Expand All @@ -214,7 +242,7 @@ int _xtimer_set_absolute(xtimer_t *timer, uint32_t target)

if (timer_list_head == timer) {
DEBUG("timer_set_absolute(): timer is new list head. updating lltimer.\n");
_lltimer_set(target - XTIMER_OVERHEAD);
_lltimer_set(target);
}
}
}
Expand Down Expand Up @@ -493,10 +521,14 @@ static void _timer_callback(void)
* time to overflow. In that case we advance to
* next timer period and check again for expired
* timers.*/
if (reference > _xtimer_lltimer_now()) {
/* check if the end of this period is very soon */
uint32_t now = _xtimer_lltimer_now() + XTIMER_ISR_BACKOFF;
if (now < reference) {
DEBUG("_timer_callback: overflowed while executing callbacks. %i\n",
timer_list_head != NULL);
_next_period();
/* wait till overflow */
while( reference < _xtimer_lltimer_now()){}
reference = 0;
goto overflow;
}
Expand All @@ -506,7 +538,7 @@ static void _timer_callback(void)
next_target = timer_list_head->target - XTIMER_OVERHEAD;

/* make sure we're not setting a time in the past */
if (next_target < (_xtimer_lltimer_now() + XTIMER_ISR_BACKOFF)) {
if (next_target < (_xtimer_now() + XTIMER_ISR_BACKOFF)) {
goto overflow;
}
}
Expand Down