mcci-catena / arduino-lmic

LoraWAN-MAC-in-C library, adapted to run under the Arduino environment
https://forum.mcci.io/c/device-software/arduino-lmic/
MIT License
650 stars 212 forks source link

What if ostime_t is overrun by scheduled time? #225

Closed hb9eue closed 5 years ago

hb9eue commented 5 years ago

Hi

Sorry, maybe this is again a newbie question.

From the example I took over in my code, which stops working occasionally, I wonder if this could be the cause:

ostime_t is a 32bit signed int and wraps around, ok so far.

Scheduling a job happens with:

os_setTimedCallback(&sendjob, os_getTime() + sec2osticks(TX_INTERVAL), do_send);

What happens, if the value resulting of os_getTime() + sec2osticks(TX_INTERVAL) is too big to fit in that 32bit signed int?

-Benoît-

VlKaspar commented 5 years ago

The general question is, if these arithmetic operations are correct, for example, what happens if "mintime" overflows ?

ostime_t mintime = now + /*8h*/sec2osticks(28800);

u1_t band = 0;

for (u1_t bi = 0; bi<4; bi++) {
    if ((bmap & (1 << bi)) && mintime - LMIC.bands[bi].avail > 0)
        mintime = LMIC.bands[band = bi].avail;

And of course, os_setTimedCallback is used for planning tasks for lorawan stack itself...

terrillmoore commented 5 years ago

It wraps around and nothing bad happens, unless there's a bug in the coding at the particular point.

For signed or unsigned time stamps, the pattern is (newtime - oldtime) < delta -- even if newtime wraps, delta is still correct. (Wraps are possible in both cases.)

In the case of signed time, (time1 - time2) > delta also works, provided that the difference is not huge. But any time scheme based on integers will make that assumption. The overflows just don't matter. This is akin to what happens in DSP CIC filters, where (because you're making differences) the overflows don't matter because the result is correct.

It is never correct to compare two times (e.g. time2 > time1).

Therefore there is no issue with adding a delta to a time; overflows are no problem.

There may be bugs, because "even Homer nodded" -- you have to follow the pattern and the compiler won't tell you if you goof. But there is no design problem.

Closing this as there is no effort required.

frazar commented 5 years ago

As @terrillmoore mentioned in his answer

(time1 - time2) > delta also works, provided that the difference is not huge.

However I was wondering, how huge does the difference have to be so that the above code stops working?

I've found a very interesting and comprehensive write-up on StackOverflow which discusses the limitations of using unsigend 32bit integers with millisecond-resolution when evaluating if an instants precedes another one. In particular, it states that comparing two instants that are more than (approximately) 24.85 days apart yields incorrect results.

In LMIC, job deadlines are measured with signed 32bit integers at the resolution determined by OSTICKS_PER_SEC, which is 16 μs by default. This means that the hal_checkTimer() function will only return the correct result if the supplied time argument is within 1<<31 ticks (approximately 9.5 hours) of the current time (returned by hal_ticks()). If that is not true, the task will "look" as if it was scheduled in the past, and will be executed earlier!

I think this limitation should be explicitly stated in the documentaiton for os_setTimedCallback(). Maybe a run-time warning should also be emitted?

I attach an Arduino sketch that verifyies the behaviour using the same functions used in LMIC.