fix: improve acknowledged unicast retry reliability - #11320
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change separates initial and remaining retransmission counts, adds non-tracking sends for existing retries, and defines distinct reliable broadcast and unicast attempt budgets. Tests cover cancellation, next-hop preservation, final flooding, and total attempts. ChangesRetransmission tracking and attempt limits
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to The PR adjusts localized unicast retry behavior and pending-packet cancellation, with reported tests and embedded builds passing; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant ReliableRouter
participant NextHopRouter
participant RadioInterface
ReliableRouter->>NextHopRouter: Select broadcast or unicast attempt budget
NextHopRouter->>RadioInterface: Send packet with retransmission tracking
NextHopRouter->>NextHopRouter: Schedule pending retry
NextHopRouter->>RadioInterface: Resend without creating another pending record
NextHopRouter->>RadioInterface: Flood on the final retry when no relay occurs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/NextHopRouter.h (1)
115-120: 🩺 Stability & Availability | 🔵 TrivialVerify airtime/duty-cycle impact of the increased retry budgets.
NUM_INTERMEDIATE_RETXgoes from 2 to 3, andNUM_RELIABLE_UNICAST_ATTEMPTSadds a fifth attempt for originating unicast sends. Each additional attempt increases airtime and duty-cycle consumption on directed routes. This effect is stronger in duty-cycle-limited regions.The PR description states that hardware testing did not cover a genuinely RF-isolated three-node intermediate-hop topology. Confirm mesh-wide airtime and duty-cycle behavior under this new budget with a real multi-hop RF test before this change reaches production, since simulator and soak-test results alone may not surface duty-cycle interactions specific to that topology.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NextHopRouter.h` around lines 115 - 120, Validate the increased retry budgets represented by NUM_INTERMEDIATE_RETX and NUM_RELIABLE_UNICAST_ATTEMPTS with a real RF test using an isolated three-node intermediate-hop topology, measuring mesh-wide airtime and duty-cycle behavior, especially in duty-cycle-limited regions. Do not rely solely on simulator or soak-test results; confirm the observed impact before production release.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/NextHopRouter.h`:
- Around line 115-120: Validate the increased retry budgets represented by
NUM_INTERMEDIATE_RETX and NUM_RELIABLE_UNICAST_ATTEMPTS with a real RF test
using an isolated three-node intermediate-hop topology, measuring mesh-wide
airtime and duty-cycle behavior, especially in duty-cycle-limited regions. Do
not rely solely on simulator or soak-test results; confirm the observed impact
before production release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c4abfe8-31f6-45b4-b506-3e846ec1a8d2
📒 Files selected for processing (5)
src/mesh/NextHopRouter.cppsrc/mesh/NextHopRouter.hsrc/mesh/ReliableRouter.cpptest/test_nexthop_routing/test_main.cpptest/test_packet_signing/test_main.cpp
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
CI is blocked only by the required PR-label gate. I do not have permission to apply labels on |
caveman99
added
bugfix
Claude
|
Will need to validate how this interacts with NEXTHOP_EARLY_FLOOD_ON_UNVERIFIED Recovery via flooding takes twice as long: ~15 s → ~30 s on idle LongFast, ~29 s → ~58 s saturated. If we flood earlier instead of later, this could save a lot of time per message. |
|
@h3lix1 I also have an idea to make hops through next hop not cost anything so there would be a bigger benefit to next hopping |
|
Validated the interaction with
The focused |
5 participants
Summary
Why
This is a scoped follow-up to #9862 and GUVWAF's retry-policy discussion.
Increasing the intermediate-hop budget exposed a latent lifetime bug: a scheduled directed retry re-entered
NextHopRouter::send(), which replaced the samependingmap entry being iterated. That invalidated the active record and could crash when the retry scheduler subsequently updated it. The new helper still refreshes the selected next hop but skips nested retransmission registration for an already-tracked retry.The change is local routing behavior only. It does not modify protobufs, configuration, packet format, or client behavior.
Validation
PATH="$HOME/.platformio/penv/bin:/opt/homebrew/opt/findutils/libexec/gnubin:$PATH" ./bin/run-tests.sh -e native-macos --quiet: GREEN, 43/43 suitestrunk fmt: clean across all five modified filespio run -e rak4631: passedpio run -e heltec-v3: passedpio run -e tbeam-s3-core: passedMeshtasticator evidence
An out-of-tree paired campaign ran 10,000 unique scenarios with 38,932 identical offered unicasts under each policy:
The campaign used a local adaptation of Meshtasticator's discrete-event model to represent both source and intermediate retry budgets while keeping offered workload independent of retry timing. Its model tests were 30 passed with one hard-coded sequence-count golden mismatch (180 expected versus 200) caused by the intentional asynchronous workload generation.
Hardware limitation
The available radios were co-located, so the bench could not suppress direct reception and force a genuine RF-isolated three-node route. Originating unicast behavior has physical hardware coverage; intermediate-hop behavior has native regression and simulator coverage but still needs a separated or attenuated three-node hardware topology for full RF validation.
🤝 Attestations
Summary by CodeRabbit
New Features
Bug Fixes
Tests