Skip to content

Fix rollover-unsafe millis() timeout in scannerGraphic.ino#1073

Merged
TMRh20 merged 2 commits into
masterfrom
copilot/patch-millis-timeout-unsafe
Jun 6, 2026
Merged

Fix rollover-unsafe millis() timeout in scannerGraphic.ino#1073
TMRh20 merged 2 commits into
masterfrom
copilot/patch-millis-timeout-unsafe

Conversation

Copilot AI commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

millis() + N produces an absolute deadline that wraps incorrectly near the 32-bit rollover (~49.7 days), causing the comparison millis() < deadline to fail immediately and skip the intended wait.

Change

examples/scannerGraphic/scannerGraphic.ino — replace absolute-deadline pattern with unsigned elapsed-time subtraction:

// Before (unsafe — wraps near millis() rollover)
unsigned long inputTimeout = millis() + 7000;
while (!Serial.available() && millis() < inputTimeout) { ... }

// After (safe — unsigned subtraction is correct across rollover)
unsigned long inputTimeout = millis();
while (!Serial.available() && millis() - inputTimeout < 7000) { ... }

Unsigned subtraction wraps symmetrically, so millis() - start always yields the correct elapsed time regardless of rollover.

Original prompt

Patch rollover-unsafe millis() timeout patterns in the nRF24/RF24 repository by replacing absolute-deadline logic like uint32_t timeout = millis() + N; while (millis() < timeout) {} or if (millis() > timeout) with rollover-safe elapsed-time comparisons using unsigned subtraction, such as const uint32_t start = millis(); while (millis() - start < N) {}.

Confirmed unsafe site already identified from repository code/search results:

  • examples/scannerGraphic/scannerGraphic.ino: input timeout using unsigned long inputTimeout = millis() + 7000; with while (!Serial.available() && millis() < inputTimeout).

Requirements:

  • Create a pull request in nRF24/RF24.
  • Keep the fix minimal and behavior-preserving except for rollover correctness.
  • Prefer introducing local start-time variables and elapsed-time comparisons rather than changing APIs.
  • Preserve formatting/style as much as possible.
  • If additional rollover-unsafe millis() deadline comparisons are discovered during implementation in this repo and they are clearly the same class of bug, patch them too.
  • Do not make unrelated cleanup changes.
  • In the PR description, summarize the unsafe pattern and why subtraction-based elapsed-time checks are rollover-safe.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: can you find all rollover unsafe millis() calls in RF24, RF24Network, RF24Mesh, RF24Ethernet & RF24Gateway?

Assistant: Yes — I found the clearly rollover-unsafe millis() usage patterns in those repos.

Unsafe pattern I looked for: storing an absolute deadline like timeout = millis() + N and later comparing with millis() < timeout or similar. That breaks across millis() wraparound.
Safe pattern: if (millis() - start >= interval) using unsigned subtraction.

Confirmed rollover-unsafe calls

RF24Mesh

  1. RF24Mesh.cpprequestAddress()
uint32_t timeout = millis() + 55;
#define MESH_MAXPOLLS 4
uint16_t contactNode[MESH_MAXPOLLS];
uint8_t pollCount = 0;

while (millis() < timeout && pollCount < MESH_MAXPOLLS) {

This is rollover-unsafe because it compares current time to an absolute future timestamp. If wrap happens before/around the loop, the condition can fail early or behave incorrectly.

RF24Ethernet

  1. RF24Client.hConnectState in-class initialization
volatile uint32_t connectTimestamp = millis();
volatile uint32_t sConnectionTimeout = serverConnectionTimeout;
volatile uint32_t serverTimer = millis();
volatile uint32_t cConnectionTimeout = clientConnectionTimeout;
volatile uint32_t clientTimer = millis();
volatile uint32_t closeTimer = millis();

These initializations are not inherently unsafe by themselves, but they are suspect state seeds because they store absolute times. Whether they’re unsafe depends on how they’re compared later.

  1. RF24Client.cpp — reopen-window timing
if (u->windowOpened == true && u->state & UIP_CLIENT_RESTART && millis() - u->restartTime > u->restartInterval)
{
    u->restartTime = millis();

This one is safe. Included here only because it came up in search and uses the correct subtraction form.

  1. RF24Client.cppwaitAvailable()
uint32_t start = millis();
while (available() < 1)
{
    if (millis() - start > timeout)
    {
        return 0;
    }

Also safe.

So for RF24Ethernet, from the results I could verify, I found no confirmed unsafe compare pattern, only timestamp storage in ConnectState that should be audited further if you want exhaustive proof.

RF24Gateway

  1. RF24Gateway.cpp — ARP timeout wait
arp_timeout = millis();
while (millis() - arp_timeout < 15) {
    uint8_t returnVal = network.update();

This is safe.

The gateway examples also use:

if (millis() - mesh_timer > 30000)
if (millis() - failTimer > 1000)

Those are safe too. Example: examples/RF24GatewayNode.cpp.
So for RF24Gateway, I found no confirmed rollover-unsafe millis() comparisons in the surfaced results.

Repos with no confirmed unsafe hits from what I could verify

  • RF24: no confirmed unsafe millis() comparison surfaced in the returned results.
  • RF24Network: no confirmed unsafe millis() comparison surfaced in the returned results.
  • **R...

This pull request was created from Copilot chat.

Replace `unsigned long inputTimeout = millis() + 7000` / `millis() < inputTimeout`
with a subtraction-based elapsed-time check:
`unsigned long inputTimeout = millis()` / `millis() - inputTimeout < 7000`.

With unsigned arithmetic the subtraction wraps correctly across the ~49.7-day
millis() rollover boundary, so the 7-second wait is always correct regardless
of when the sketch starts.
Copilot AI changed the title [WIP] Fix rollover-unsafe millis timeout patterns in nRF24/RF24 Fix rollover-unsafe millis() timeout in scannerGraphic.ino Jun 4, 2026
Copilot AI requested a review from TMRh20 June 4, 2026 08:30
Copilot finished work on behalf of TMRh20 June 4, 2026 08:30
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Memory usage change @ eee89fd

Board Flash % RAM for global variables %
arduino:avr:nano 0 - 0 0 - 0 0 - 0 0 - 0
arduino:samd:mkrzero 0 - 0 0 - 0 0 - 0 0 - 0
Click for full report per board

arduino:avr:nano

Sketch Flash % RAM for global variables %
examples/GettingStarted 0 0 0 0
examples/AcknowledgementPayloads 0 0 0 0
examples/ManualAcknowledgements 0 0 0 0
examples/StreamingData 0 0 0 0
examples/MulticeiverDemo 0 0 0 0
examples/InterruptConfigure 0 0 0 0
examples/scanner 0 0 0 0
examples/encodeRadioDetails 0 0 0 0

arduino:samd:mkrzero

Sketch Flash % RAM for global variables %
examples/GettingStarted 0 0 0 0
examples/AcknowledgementPayloads 0 0 0 0
examples/ManualAcknowledgements 0 0 0 0
examples/StreamingData 0 0 0 0
examples/MulticeiverDemo 0 0 0 0
examples/InterruptConfigure 0 0 0 0
examples/scanner 0 0 0 0
examples/encodeRadioDetails 0 0 0 0

@TMRh20 TMRh20 marked this pull request as ready for review June 4, 2026 09:05
@TMRh20 TMRh20 requested a review from 2bndy5 June 4, 2026 09:05
@2bndy5

2bndy5 commented Jun 4, 2026

Copy link
Copy Markdown
Member

The size deltas is 0 because the changed example is not part of the calculation. And this is only effective when debugging the example.

@TMRh20 TMRh20 merged commit 45af279 into master Jun 6, 2026
123 checks passed
@TMRh20 TMRh20 deleted the copilot/patch-millis-timeout-unsafe branch June 6, 2026 11:08
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.

3 participants