Skip to content

Use unsigned long for representing times. - #4

Merged
ivanseidel merged 1 commit into
ivanseidel:masterfrom
edgar-bonet:master
Jun 11, 2015
Merged

ivanseidel merged 1 commit into
ivanseidel:masterfrom
edgar-bonet:master

Conversation

@edgar-bonet

Copy link
Copy Markdown
Contributor

All the time calculations made on signed longs were wrong after 2^31 ms
~ 24.9 days. This patch fixes this by using unsigned longs. As unsigned
numbers follow the rules of modular arithmetic, the computations are
correct event after the millis() rollover in ~ 49.7 days.

Fixes issue #3: Improper use of signed integers.

All the time calculations made on signed longs were wrong after 2^31 ms
~ 24.9 days. This patch fixes this by using unsigned longs. As unsigned
numbers follow the rules of modular arithmetic, the computations are
correct event after the millis() rollover in ~ 49.7 days.

Fixes issue #3: Improper use of signed integers.
@edgar-bonet

Copy link
Copy Markdown
Contributor Author

This patch is untested. Here is what it does:

  • All longs are turned into unsigned longs.
  • Instead of using -1 to mean “now”, runned() and shouldRun() are overloaded with inline versions that take no parameters:
void runned() { runned(millis()); }
bool shouldRun() { return shouldRun(millis()); }

this generates smaller and faster code than the current version, because there is no need to test for the parameter being −1.

  • The test for knowing whether it's time to run is a little bit more complex. Timestamps cannot be directly compared, because of the possible rollover. Durations (i.e. time differences), however, can be meaningfully compared. The test could be written
bool time_to_run = (signed long)(time - _cached_next_run) >= 0;

and it would work as expected if compiled with gcc. But since this is implementation-defined behavior, it should be avoided in portable code. Instead of casting to a signed number, the same result can be achieved by testing what would be the sign bit:

// If the "sign" bit is set the signed difference would be negative
bool time_remaining = (time - _cached_next_run) & 0x80000000;

which is as fast as a comparison. Then, shouldRun() returns !time_remaining && enabled.

I am not sure I will be able to test all this today. If you want to test yourself, on an AVR-based Arduino you can time-travel to just before the millis() rollover as follows:

extern unsigned long timer0_millis;

void setup() {
    timer0_millis = -1000;  // rollover in one second
    ...
}

On the Dues this does not work, because the equivalent variable is declared as static. You may make it work if you modify the Arduino core by removing the static qualifier from _dwTickCount in hardware/arduino/sam/system/libsam/source/timetick.c.

@edgar-bonet

Copy link
Copy Markdown
Contributor Author

Update: I tested the patched library to see how it deals with the millis() rollover. It runs as expected, keeping a regular pace across the rollover. Here is my test program:

#include <Thread.h>
#include <ThreadController.h>

const int ledPin = 13;
extern unsigned long timer0_millis;

// First thread: toggle the LED every 200 ms.
void blink_led() {
    static bool ledState;
    ledState = !ledState;
    digitalWrite(ledPin, ledState);
}
Thread blink_thread(blink_led, 200);

// Second thread: print the current time every second.
void print_time() {
    Serial.println(millis());
}
Thread print_thread(print_time, 1000);

// Controller for both threads.
ThreadController scheduler;

void setup() {
    pinMode(ledPin, OUTPUT);
    Serial.begin(9600);
    Serial.println("\rStarting");
    scheduler.add(&blink_thread);
    scheduler.add(&print_thread);

    // Test across the millis() rollover.
    timer0_millis = -4300;  // time-travel to rollover minus 4.3 s
    blink_thread.run();     // update _cached_next_run
    print_thread.run();     // ditto
}

void loop() {
    scheduler.run();
}

@ivanseidel

Copy link
Copy Markdown
Owner

Awesome!
That is just perfect. And with 100% compatibility...

Will merge.

Thanks for the effort

ivanseidel added a commit that referenced this pull request Jun 11, 2015
Use unsigned long for representing times.
@ivanseidel
ivanseidel merged commit f29966e into ivanseidel:master Jun 11, 2015
@ivanseidel

Copy link
Copy Markdown
Owner

fixes #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants