Improper use of signed integers

[edgar-bonet]

The millis() function returns an unsigned long integer. Implicitly casting it to a signed long is a terrible idea, for several reasons:

1. After roughly 24.9 days, the most significant bit of millis() is set, meaning the cast will result in a negative number, and all the if(time < 0) tests will evaluate to true.
2. last_run + interval can overflow, and overflowing a signed number in C++ yields undefined behavior, which is always a bug.
3. In the lucky event that the overflow yields the expected wrap-around, _cached_next_run will be negative and the test time >= _cached_next_run will immediately evaluate to true, and continue to do so as long as time is positive.

All these issues can be avoided by using the proper type for timestamps: unsigned long.

[ivanseidel]

Hellow @edgar-bonet,

You are correct. However, the idea behind if(time < 0) is that you can either specify the time during it's call, or, set it yourself.

The total 24.9 days is actually half of what would be if we set to unsigned long, but then, the paramenter wouldn't accept a negative number (the default one is -1).

Do you have any suggestions? One Idea that I had, is to set the default to 0, but that would result in a call to millis() if the actual time was 0 for the first time. Not a big problem, but not perfect...

[edgar-bonet]

Hi!

The total 24.9 days is actually half of what would be if we set to unsigned long

No. If using unsigned long the program can run indefinitely. Unsigned integers do not really overflow: they always give the “correct” value modulo 2^(number of bits).

I suggest you get rid of _cached_next_run and decide whether it's time to run by testing

time - last_run >= interval

This works well across the millis() rollover thanks to the properties of modular arithmetic. If you really need to avoid the subtraction, it is possible, but the code would be less readable.

Do you have any suggestions?

If you really need both options (specify the time or let it default to “now”), overload the methods. You cannot reserve a special time value as “invalid”, because all possible values of a 32-bit word are valid and meaningful.

[edgar-bonet]

Submited a patch in the form of pull request #4.

Oh, BTW, nothing to do with the current bug but... when compiling the library, I got a warning about the variable found not being used in ThreadController.cpp, line 68.

[ivanseidel]

fixed by #4