Skip to content

Patch rollover-unsafe millis() deadline checks in RF24Network.cpp - #270

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

Patch rollover-unsafe millis() deadline checks in RF24Network.cpp#270
TMRh20 merged 2 commits into
masterfrom
copilot/patch-rollover-unsafe-timeouts

Conversation

Copilot AI commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

This PR replaces rollover-unsafe timeout logic in RF24Network that used absolute deadlines (millis() + N) with elapsed-time comparisons based on unsigned subtraction. This preserves current timing behavior while making the checks correct across millis() wraparound.

  • update() overrun timeout

    • Replaced timeout = millis() + 100 / if (millis() > timeout) with a start-time + elapsed check.
  • Fragmentation retry delay

    • Replaced timer = millis() + 2 / while (millis() < timer) with an elapsed-time wait loop.
  • Pattern consistency

    • Kept changes local and API-neutral; only the timeout comparison form changed.
const uint32_t start = millis();
while (millis() - start < 2) {
}
Original prompt

Patch rollover-unsafe millis() timeout patterns in the nRF24/RF24Network 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 sites already identified from repository code/search results:

  • RF24Network.cpp: uint32_t timeout = millis() + 100; with if (millis() > timeout) in update().
  • RF24Network.cpp: fragmentation retry delay using uint32_t timer = millis() + 2; while (millis() < timer) {}.

Requirements:

  • Create a pull request in nRF24/RF24Network.
  • Keep the fixes 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 `mil...

This pull request was created from Copilot chat.

Copilot AI changed the title [WIP] Fix rollover-unsafe millis() timeout patterns Patch rollover-unsafe millis() deadline checks in RF24Network.cpp Jun 4, 2026
Copilot finished work on behalf of TMRh20 June 4, 2026 08:35
Copilot AI requested a review from TMRh20 June 4, 2026 08:35
@TMRh20
TMRh20 marked this pull request as ready for review June 4, 2026 08:56
@TMRh20
TMRh20 requested a review from 2bndy5 June 4, 2026 08:56
@TMRh20
TMRh20 merged commit f9b4131 into master Jun 6, 2026
22 checks passed
@TMRh20
TMRh20 deleted the copilot/patch-rollover-unsafe-timeouts branch June 6, 2026 11:09
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