Deliver locally-generated replies addressed to us to the phone - #11185
Merged
Conversation
Config get/set from the phone times out on every device: the client sends an admin request, the node handles it, and the response is silently dropped before it reaches the phone queue. #10967 changed Router::sendLocal's isToUs branch from enqueueReceivedMessage() to handleReceived(p, src), so a local packet keeps its RxSource instead of being relabeled RX_SRC_RADIO by the queue round-trip. That is the right call for the new policy gates, but module replies go out through MeshService::sendToMesh() with the default RX_SRC_LOCAL, and a reply to a phone-originated request is addressed to our own node (setReplyTo resolves from == 0 to ourNodeNum). Those replies now re-enter callModules as RX_SRC_LOCAL, where the loopback gate skips every module whose loopbackOk is false - including RoutingModule, whose promiscuous sniff is the only path that moves a received packet into toPhoneQueue. The reply is released, never sent. Requests still work, because the phone's own packets arrive as RX_SRC_USER and pass the gate, so a set_config is applied and only its acknowledgement is lost. That is why a client can connect and download config but times out on every config screen and every setter. Deliver the phone's copy from sendToMesh() instead: for a local packet addressed to us, the loopback gate is doing its job in keeping the packet away from module re-dispatch, and the phone copy is exactly what is missing. Setting loopbackOk on RoutingModule would instead echo every locally-generated broadcast back to the phone, and relabeling replies RX_SRC_RADIO would undo the origin separation #10967 added. Also stop reporting ERRNO_SHOULD_RELEASE (35) to the phone in the QueueStatus for these packets. It means "caller frees", not a send failure, and the same hunk changed it from the 0 the phone used to see.
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesLocal delivery behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mesh/MeshService.cpp (1)
327-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the added comments to at most two lines.
src/mesh/MeshService.cpp#L327-L331: condense the loopback rationale to two lines.test/test_mesh_module/test_main.cpp#L476-L483: shorten the regression rationale and test description to one or two lines each.As per coding guidelines, “Keep comments minimal—one or two lines maximum.”
🤖 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/MeshService.cpp` around lines 327 - 331, Condense the loopback rationale comment in src/mesh/MeshService.cpp lines 327-331 to no more than two lines while preserving its purpose. Also shorten the regression rationale and test description in test/test_mesh_module/test_main.cpp lines 476-483 to one or two lines each; make comment-only changes and keep the existing test behavior unchanged.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.
Inline comments:
In `@test/test_mesh_module/test_main.cpp`:
- Around line 538-542: Update the QueueStatus verification in the test around
mockService->getQueueStatusForPhone() to count the records consumed and assert
that exactly two statuses were emitted, while retaining the ERRNO_OK checks for
each record and releasing each through releaseQueueStatusToPool.
---
Nitpick comments:
In `@src/mesh/MeshService.cpp`:
- Around line 327-331: Condense the loopback rationale comment in
src/mesh/MeshService.cpp lines 327-331 to no more than two lines while
preserving its purpose. Also shorten the regression rationale and test
description in test/test_mesh_module/test_main.cpp lines 476-483 to one or two
lines each; make comment-only changes and keep the existing test behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23074f58-44da-4634-aa5a-3dcf84db42f9
📒 Files selected for processing (2)
src/mesh/MeshService.cpptest/test_mesh_module/test_main.cpp
Condense the added comments to the one-or-two-line house style; the rationale lives in the commit message and PR. The reply test drained QueueStatus records in a while loop, which would have passed just as happily on an empty queue. Count them and require both the request's and the reply's.
Contributor
Author
|
Both addressed in 6efce75. Comments condensed to the one-or-two-line house style — the rationale belongs in the commit message and PR body, which carry it. The QueueStatus point was a real weakness, thanks: the |
Contributor
⚡ 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 (30)
Build artifacts expire on 2026-08-23. Updated for |
mverch67
approved these changes
Jul 24, 2026
mverch67
left a comment
mverch67
left a comment
Collaborator
There was a problem hiding this comment.
Tested with ThinkNode M9 and the bug from 11094 is fixed.
thebentern
added a commit
that referenced
this pull request
Jul 24, 2026
…FS (#11190) Since #10967 made Router::sendLocal handle self-addressed packets synchronously, the entire phone-API chain for a BLE client runs inline in the Bluefruit characteristic write callback: toRadioWriteCb -> PhoneAPI::handleToRadio -> admin set-config -> radio reconfigure -> NodeDB::saveToDisk. That callback executes on the Bluefruit BLE FreeRTOS task, whose stock stack is 5 KB (CFG_BLE_TASK_STACKSIZE = 256*5 words) - not the Arduino loop task that #10944 already raised to 8 KB. The loop-task fix therefore protects the wrong task for BLE-originated writes. On a Seeed Wio Tracker L1 the 5 KB stack overflows during pairing first-sync, resetting the device mid-LittleFS-write, every single time. Repeated mid-write resets tear the LittleFS metadata, lfs_assert fires on the next boot, and the corruption handler formats the whole filesystem: region, channels, module config, and the node's keypair are all lost (critical fault #13, new node identity on next region set). Reproduced end-to-end tonight on stock develop 6908d27; with this change the same device pairs, serves config screens, and survives back-to-back config.proto saves over BLE. Raise the BLE task to the same 2048 words (8 KB) as LOOP_STACK_SZ, for the same reason. bluefruit.cpp's #ifndef guard makes the -D take effect with no framework patch. Costs 3 KB of RAM on nrf52840 targets only. Credit where due: Ixitxachitl independently established in #11155 testing that the save-path crash persists after #11185 and that re-queueing sendLocal (moving the pipeline back to the Router thread) makes it go away - which corroborates this diagnosis from the other direction. This commit is the minimal capacity-side fix; #11155's relocation of the pipeline off the BLE task remains the right architectural follow-up, and this guard stays correct even after it lands. Likely also explains #10905 (L1 display-thread crash when a client requests full configuration) and the 2.8 field reports of idle nodes losing region and keys after a BLE session.
8 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Config get/set from a client times out on every device running current develop. The client
connects fine and downloads config at connect, but opening a config screen or changing a
setting (e.g. Device Role) times out.
Reported by an alpha tester on a Seeed Xiao nRF52840 Kit against
2.8.0.ef1aedd, working on2.8.0.5ded0ec. It is not board- or app-specific — both Android 2.8.0 and 2.7.14 behave thesame, and the mechanism is platform-independent.
Regression
d6b12ea3f(#10967, packet authenticity policies) changedRouter::sendLocal'sisToUsbranch:
Preserving the
RxSourceis the right call — the queue round-trip used to relabel localpackets
RX_SRC_RADIO, which is exactly the confusion the new policy gates need gone. Butmodule replies are sent through
MeshService::sendToMesh()with the defaultRX_SRC_LOCAL,and a reply to a phone-originated request is addressed to our own node (
setReplyToresolvesfrom == 0toourNodeNum). So those replies now re-entercallModulesasRX_SRC_LOCAL,where the loopback gate skips every module with
loopbackOk == false:https://github.com/meshtastic/firmware/blob/develop/src/mesh/MeshModule.cpp#L121-L124
That includes
RoutingModule, whose promiscuous sniff is the only code that moves a receivedpacket into
toPhoneQueue:https://github.com/meshtastic/firmware/blob/develop/src/modules/RoutingModule.cpp#L35-L38
The reply is released without ever being queued.
Requests still work, because the phone's own packets arrive as
RX_SRC_USERand pass thegate. So a
set_configis applied and only its acknowledgement is lost — which is why aclient can connect and download config but times out on every config screen and every setter.
Testers who saw a timeout may find the setting actually took effect.
The fix
Deliver the phone's copy from
sendToMesh()for a local packet addressed to us. The loopbackgate is doing its job keeping the packet out of module re-dispatch; the phone copy is the part
that went missing.
Two alternatives I ruled out:
loopbackOk = trueonRoutingModule— would also echo every locally-generated broadcast(position, nodeinfo, telemetry) back to the phone through the promiscuous sniff.
RX_SRC_RADIOinsendLocal— re-muddles the exact local/remotedistinction feat(security): enforce packet authenticity policies #10967 introduced.
Also stops reporting
ERRNO_SHOULD_RELEASE(35) to the phone in theQueueStatusfor thesepackets. It means "caller frees", not a send failure, and the same hunk changed it from the 0
the phone used to see.
Tests
Two regression tests in
test/test_mesh_module:test_localReplyToSelf_isDeliveredToPhone— a reply addressed to ourselves reaches thephone queue exactly once, with a success
QueueStatusand no radio traffic.test_phoneRequest_replyReachesPhone— full loop through the real router: afrom == 0want_responserequest produces a module reply that reaches the phone queue with the rightrequest_id, and the request itself does not echo back.Both fail on unpatched develop (empty phone queue) and pass with the fix.
Validation
Native suites with the fix applied: native-macos 577 cases, 573 passed + 4
environment-conditional ignores, 0 failed; Docker 751/751 passed.
End-to-end on the portduino simulator over the TCP PhoneAPI (same client path as BLE), as a
minimal pair at the reporter's exact commit
ef1aedd56, with onlyMeshService.cppdiffering:ef1aedd56ef1aedd56+ this fix--set device.role CLIENT_MUTE--get-ringtone(admin round-trip)--get device.role--get device.rolepassing on both is itself diagnostic — the Python CLI serves it from thewant_config cache rather than an admin round-trip, which is the same reason a client can
connect while every config screen times out.
Summary by CodeRabbit