Router: defer nested local sends to avoid handleReceived re-entrancy - #11191
Conversation
Prevents an nRF52 task-stack overflow on config save. Replaces draft #11155.
📝 WalkthroughWalkthroughRouter local loopback now routes through depth-aware delivery. Nested packets are copied into a bounded deferred queue, drained by the outermost receive handler, and dispatched without recursive re-entry. Tests cover fanout, chained replies, queue overflow, and phone delivery. ChangesDeferred local delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MeshModule
participant Router
participant DeferredQueue
participant PhoneQueue
MeshModule->>Router: sendLocal(packet, source)
Router->>Router: deliverLocal(packet, source)
Router->>DeferredQueue: enqueue nested packet
Router->>Router: outer handleReceived drains queue
Router->>Router: dispatchReceived(packet, source)
Router->>PhoneQueue: deliver self-addressed reply
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
A different approach to #11155 (credits to @Ixitxachitl for the initial analysis) and complementing the #11190 stack fix, this prevents the original recursive callModules polluting the stack in the first place. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/test_mesh_module/test_main.cpp (1)
690-712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded capacity+2 requires manual sync with
Router.h's private constant.
burst = 6isdeferredLocalCapacity (4) + 2, kept in sync only by a comment. If the capacity inRouter.hchanges, this test silently stops testing the actual overflow boundary (or the drop-count assertion at Line 701 breaks) unless someone remembers the comment. Consider exposingdeferredLocalCapacityunder the existing#ifdef PIO_UNIT_TESTING public:block inRouter.h(same pattern asdeferredLocalPending()) so the test can reference it directly.♻️ Proposed fix
- const uint32_t burst = 6; // deferredLocalCapacity (4) + 2 - keep in sync with Router.h + const uint32_t burst = mockRouter->deferredLocalCapacityForTest() + 2;// src/mesh/Router.h, inside the existing `#ifdef` PIO_UNIT_TESTING public: block uint8_t deferredLocalCapacityForTest() const { return deferredLocalCapacity; }🤖 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 `@test/test_mesh_module/test_main.cpp` around lines 690 - 712, Expose Router’s private deferredLocalCapacity through a deferredLocalCapacityForTest() accessor in the existing PIO_UNIT_TESTING public block, following the deferredLocalPending() pattern. Update test_deferredQueueOverflow_dropsGracefully() to derive burst from that accessor plus the intended overflow amount, removing the hardcoded capacity and synchronization comment while preserving the drop-count assertions.src/mesh/Router.cpp (1)
1256-1292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comments for the new deferred-delivery mechanism exceed the repo's 1-2 line comment cap in both files. Same root cause: the stack-overflow-avoidance rationale was written as multi-paragraph blocks instead of terse comments.
src/mesh/Router.cpp#L1256-L1292: trim the 6-line drain-rationale comment (Lines 1266-1271) and the 6-line ownership comment indispatchReceived()(Lines 1287-1292) to 1-2 lines each.src/mesh/Router.h#L163-L205: trim thedispatchReceived()doc (Lines 163-167, 4 lines),deliverLocal()doc (Lines 170-177, 7 lines), and ring-buffer doc (Lines 192-195, 4 lines) to 1-2 lines each.As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious; do not restate straightforward code or add multi-paragraph explanatory blocks."
🤖 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/Router.cpp` around lines 1256 - 1292, Trim the deferred-delivery comments to the repository’s one- or two-line limit: in src/mesh/Router.cpp lines 1256-1292, shorten the handleReceived() drain rationale and dispatchReceived() ownership rationale; in src/mesh/Router.h lines 163-205, shorten the dispatchReceived() documentation, deliverLocal() documentation, and deferred ring-buffer documentation. Preserve only the essential non-obvious rationale and remove redundant implementation detail.Source: Coding guidelines
🤖 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/Router.cpp`:
- Around line 1256-1292: Trim the deferred-delivery comments to the repository’s
one- or two-line limit: in src/mesh/Router.cpp lines 1256-1292, shorten the
handleReceived() drain rationale and dispatchReceived() ownership rationale; in
src/mesh/Router.h lines 163-205, shorten the dispatchReceived() documentation,
deliverLocal() documentation, and deferred ring-buffer documentation. Preserve
only the essential non-obvious rationale and remove redundant implementation
detail.
In `@test/test_mesh_module/test_main.cpp`:
- Around line 690-712: Expose Router’s private deferredLocalCapacity through a
deferredLocalCapacityForTest() accessor in the existing PIO_UNIT_TESTING public
block, following the deferredLocalPending() pattern. Update
test_deferredQueueOverflow_dropsGracefully() to derive burst from that accessor
plus the intended overflow amount, removing the hardcoded capacity and
synchronization comment while preserving the drop-count assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 10e02cba-7e08-45bf-9529-8e0c570d93b1
📒 Files selected for processing (3)
src/mesh/Router.cppsrc/mesh/Router.htest/test_mesh_module/test_main.cpp
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (31)
Build artifacts expire on 2026-08-23. Updated for |
…eshtastic#11191) Prevents an nRF52 task-stack overflow on config save. Replaces draft meshtastic#11155.
2 participants
Guards Router::handleReceived with a depth counter so a module sending from inside callModules defers its loopback instead of re-entering and overflowing the nRF52 task stack on a config save. Replaces draft #11155.
Summary by CodeRabbit
Bug Fixes
Tests