xtimer: xtimer_set

[malosek]

Hello,

still having some hypothetical issues with xtimer_set:
have single "blink" thread with endless loop which toggles the LED and calls
xtimer_set_wakeup(&xtimer, 500000, me);
thread_sleep();
which I believe is valid code.

in my hypothetical use case im calling
xtimer_set_wakeup with offset >= XTIMER_BACKOFF
this boils down to xtimer_set and hits the
uint32_t target = xtimer_now() + offset;
_xtimer_set_absolute(timer, target);

lets assume that xtimer_now is 0xFFFFFFFF
just after the target is loaded with 0xFFFFFFFF + offset, lets say 40,
now the timer IRQ comes and increases _long_cnt in _next_period, maybe scheduling some other thread...

after "blink" thread is resumed when timers are lets say 0x00000001 0x000000FF
now calling _xtimer_set_absolute with target 40
in xtimer set we are going with target < now condition which results in wrong timer->long_cnt value.

is it too hypothetical or it can really happen?

anyway there are two xtimer spins on the way, in xtimer_set and _xtimer_set_absolute - is it intentional?

or one more hypothetical case: xtimer_spin called when timers are 0x1 0xFFFFFFFF and offset lets say 39?

wbr
malo

[kaspar030]

lets assume that xtimer_now is 0xFFFFFFFF
just after the target is loaded with 0xFFFFFFFF + offset, lets say 40,
now the timer IRQ comes and increases _long_cnt in _next_period, maybe scheduling some other thread...

Yeah, that needs fixing. Probably all of the timer setup code needs to be protected from interrupts... :(

[kaspar030]

lets assume that xtimer_now is 0xFFFFFFFF
just after the target is loaded with 0xFFFFFFFF + offset, lets say 40,
now the timer IRQ comes and increases _long_cnt in _next_period, maybe scheduling some other thread...

I think moving up the disableIRQ() in _timer_set_absolute() to before the current long_cnt is determined will solve this problem. @malosek, do you agree?

[kaspar030]

@malosek Do you think #4903 fixes the first problem you're describing?

[malosek]

Hello Kaspar,

for me the change makes no difference in this case...
since the overflow can still happen between
uint32_t target = xtimer_now() + offset;
in xtimer_set
and
unsigned state = disableIRQ();
in _xtimer_set_absolute

or?

Trying to show you what you can not reliably set _xtimer_set_absolute with 32bits target, where upper 32bits are determined latter on with IRQ/counting enabled in between, while having 64bits clock counter.

What about dropping this interface and use only 64 bits versions?

wbr
malo

[kaspar030]

since the overflow can still happen between
uint32_t target = xtimer_now() + offset;
in xtimer_set
and
unsigned state = disableIRQ();
in _xtimer_set_absolute

or?

But now xtimer determines in which period the timer should trigger inside of the critical section.
This PR prevents the overflow happening between determining _long_cnt and _this_high_period().

[kaspar030]

What about dropping this interface and use only 64 bits versions?

That would mean a lot more codesize... We do have a table somewhere that shows the cyclecount needed for comparing, adding, subtracting 32 and 64bit values on 8bit, 16bit and 32bit machines.

[jnohlgard]

How about changing the underlying lltimer to use timer_set for relative offsets and timer_set_absolute only for absolute timeouts?
This might complicate the timer callback though..

[malosek]

Hello,

Im playing with the idea of

    else {
        _xtimer_load (timer, offset);
        _xtimer_arm (timer);
    }

in xtimer_set
with

static void _xtimer_load (xtimer_t *timer, uint32_t offset)
{
    unsigned state = disableIRQ();
    uint32_t target = xtimer_now() + offset;
    timer->target = target;
    timer->long_target = _long_cnt;
    restoreIRQ(state);
    if (target < offset)
    {
        timer->long_target++;
    }
}

and _xtimer_arm (timer); would modified _xtimer_set_absolute wo

   timer->target = target;
    timer->long_target = _long_cnt;
    if (target < now) {
        timer->long_target++;
    }

will continue in this direction at the mo...
or some other ideas?

wbr
malo

[malosek]

Hello,

introducing _xtimer_load and _xtimer_arm showed to be useless complication...
there is cleaner solution just to call _xtimer_set_absolute with offset instead of target and add long_offset arg as well.

in this way you could drop _xtimer_set64

if you are interested check my branch xtimer-tweaking
note that is not finished and probably wont compile, since I do not have compiler at the mo...

wbr
malo

[haukepetersen]

Hej, is this issue critical for the release and/or is there a chance for it to be solved (very) soon? Otherwise I would like to remark it for 2016.07

[malosek]

Hello,
believe that issue is still there and we should call it critical.
lets say that there is no 0 chance that I'll look at it next week - wo promising anything (have all 3 kids sick at the mo)
wbr
malo

[kYc0o]

So, it will be postponed since the PR solving this issue is WIP.

[OlegHahm]

I think we need a consistent rule how to handle open issues that contain bug reports. When I was release manager, I preferred to keep them tagged for the current release milestone until after the release in order to easily find them for the release notes. Other people may prefer other solutions, so it should be up the release manager to decide: @haukepetersen, what do you prefer?

(@authmillenon and me had a similar discussion yesterday in another PR.)

[kYc0o]

I'm maybe forced to postpone this issue for the next release, only if at the end of the week one of the xtimer PRs solve this...

[kYc0o]

@OlegHahm I'd like to reconsider your proposition but it cannot be done by filtering all the bug issues which are or not marked for a release? They still are bugs anyways...

[OlegHahm]

What's the state here?

[malosek]

closing...

wbr
malo