millis() timeouts not wrap-safe

[PaulZC]

As reported by Clive:

} while (isBlocking && (millis() < (start + waitForSemaphoreTimeout_millis)));

needs to be of the form

} while (isBlocking && ((millis() - start) < waitForSemaphoreTimeout_millis));

---

Tricky to test using millis()... The wrap occurs at 49.71 days (2^32 seconds).

Testing on ESP32 (Arduino esp32 v3.0.7):

unsigned long sfeMillis = 0xFFFFFFF8;
unsigned long sfeStart;
const unsigned long sfeTimeout = 0x10;

void setup() {
  delay(1000);
  Serial.begin(115200);
  sfeStart = sfeMillis;
}

void loop() {
  Serial.printf("%lu %lu %lu %d\r\n",
    sfeStart,
    sfeTimeout,
    sfeMillis,
    ((sfeMillis - sfeStart) < sfeTimeout));

  sfeMillis = sfeMillis + 1;
  
  delay(1000);
}

produces:

09:42:46.864 -> 4294967288 16 4294967289 1
09:42:47.822 -> 4294967288 16 4294967290 1
09:42:48.831 -> 4294967288 16 4294967291 1
09:42:49.838 -> 4294967288 16 4294967292 1
09:42:50.845 -> 4294967288 16 4294967293 1
09:42:51.855 -> 4294967288 16 4294967294 1
09:42:52.862 -> 4294967288 16 4294967295 1
09:42:53.821 -> 4294967288 16 0 1
09:42:54.829 -> 4294967288 16 1 1
09:42:55.834 -> 4294967288 16 2 1
09:42:56.839 -> 4294967288 16 3 1
09:42:57.844 -> 4294967288 16 4 1
09:42:58.848 -> 4294967288 16 5 1
09:42:59.854 -> 4294967288 16 6 1
09:43:00.857 -> 4294967288 16 7 1
09:43:01.863 -> 4294967288 16 8 0
09:43:02.821 -> 4294967288 16 9 0
09:43:03.829 -> 4294967288 16 10 0
09:43:04.835 -> 4294967288 16 11 0
09:43:05.836 -> 4294967288 16 12 0
09:43:06.837 -> 4294967288 16 13 0
09:43:07.840 -> 4294967288 16 14 0
09:43:08.844 -> 4294967288 16 15 0

[cturvey]

Tricky to test using...

I know, and a bit of an obscure/corner case, but best to identify these things. These things in a real-world implementation frequently get left running for months or years on end. Or effectively indefinitely, as restarting can be very disruptive.

I just glanced at the code, and it looked wrong, not sure how many other usages there might be. I didn't go fishing

I’d probably suggest migrating to a bool millis_timeout_check(unsigned long basis, unsigned long timeout) type approach where the code is contained in a single routine rather than dispersed in 100’s of instances.

On the Keil MDK 6.x compilers they’ve moved to a 64-bit “unsigned long”, but still best practice to dereference the starting basis, and let the MCU's math do the work and compare the delta-time.

Anyway, not meant as a poke, really just an alert, especially as some of this kit is left plugged in charging, or whatever, indefinitely. Plus power-cycling, when there's an auxiliary battery, gets to be more of a challenge.

Best Regards, -Clive

[PaulZC]

Oh, this is fun! We also need this version:

unsigned long sfeMillis = 0xFFFFFFF8;
const unsigned long sfeEventDuration = 0x10;
unsigned long sfeNextEvent;

void setup() {
  delay(1000);
  Serial.begin(115200);
  
  sfeNextEvent = sfeMillis + sfeEventDuration; // Wrap-around
}

void loop() {
  // https://stackoverflow.com/a/3097744
  bool sfeNextEventReached = !((long)(sfeNextEvent - sfeMillis) > 0);
  
  Serial.printf("%lu %lu %lu %d\r\n",
    sfeNextEvent,
    sfeEventDuration,
    sfeMillis,
    sfeNextEventReached);

  sfeMillis = sfeMillis + 1;
  
  delay(1000);
}

produces:

10:49:02.152 -> 8 16 4294967288 0
10:49:03.148 -> 8 16 4294967289 0
10:49:04.154 -> 8 16 4294967290 0
10:49:05.159 -> 8 16 4294967291 0
10:49:06.164 -> 8 16 4294967292 0
10:49:07.172 -> 8 16 4294967293 0
10:49:08.136 -> 8 16 4294967294 0
10:49:09.147 -> 8 16 4294967295 0
10:49:10.157 -> 8 16 0 0
10:49:11.168 -> 8 16 1 0
10:49:12.179 -> 8 16 2 0
10:49:13.139 -> 8 16 3 0
10:49:14.144 -> 8 16 4 0
10:49:15.150 -> 8 16 5 0
10:49:16.157 -> 8 16 6 0
10:49:17.169 -> 8 16 7 0
10:49:18.180 -> 8 16 8 1
10:49:19.139 -> 8 16 9 1
10:49:20.141 -> 8 16 10 1
10:49:21.150 -> 8 16 11 1
10:49:22.164 -> 8 16 12 1
10:49:23.177 -> 8 16 13 1
10:49:24.135 -> 8 16 14 1
10:49:25.148 -> 8 16 15 1

[PaulZC]

Updates to cure this are in my pcUpdates branch:

ebe96f9