Fix/call modules - #11155
Conversation
Merge from upstream Develop
Merge from develop
Add native Windows build of meshtasticd (meshtastic#11031)
…tracking isn't lost.
⚡ 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-21. Updated for |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
caveman99
left a comment
caveman99
left a comment
There was a problem hiding this comment.
Overall agreement, please see comments inline.
| // Note: we avoid calling shouldFilterReceived if we are supposed to ignore certain nodes - because some overrides might | ||
| // cache/learn of the existence of nodes (i.e. FloodRouter) that they should not | ||
| handleReceived(p); | ||
| handleReceived(p, src); |
There was a problem hiding this comment.
locally-addressed packets now traverse perhapsHandleReceived() which means they are dedup ed and checked against the ignore-list. They are also added to the PacketHistory. Confirm a module emitting rapid to-phone-only packets can't get deduped.
There was a problem hiding this comment.
You're right, locally-generated replies were falling through the ignore-list/PacketHistory/MQTT/pre-hop checks meant for radio ingress. handleReceived() already special-cases RX_SRC_LOCAL internally (e.g. it only applies the routing-auth cache when src == RX_SRC_RADIO), so I've made perhapsHandleReceived() skip straight to handleReceived() when src == RX_SRC_LOCAL, bypassing the filter pipeline entirely for local packets, same as the pre-PR behavior, just routed through the queue instead of called inline. Pushed in a2c03e5.
There was a problem hiding this comment.
Follow-up: that guard was too narrow. RX_SRC_USER (a phone/serial packet addressed to the local node, e.g. a config request) hits the same perhapsHandleReceived() path and wasn't exempted, so it still traversed the ignore-list/dedup/pre-hop/routing-auth filters meant only for radio ingress — the same risk you flagged, just on the request side instead of the reply side. In the pre-PR code RX_SRC_USER never went through perhapsHandleReceived() at all (synchronous handleReceived() call), so this was a regression from queuing it. Widened the check from src == RX_SRC_LOCAL to src != RX_SRC_RADIO to cover both trusted non-radio sources. Pushed.
| // within itself. The queue carries src through so the packet is still replayed with its | ||
| // true origin instead of defaulting to RX_SRC_RADIO. | ||
| enqueueReceivedMessage(p, src); | ||
| return ERRNO_OK; |
There was a problem hiding this comment.
return code changed from ERRNO_SHOULD_RELEASE to ERRNO_OK (the queue now owns/frees the packet). Every other sendLocal caller must be verified to no longer release on ERRNO_OK or else we have another double-free/leak.
There was a problem hiding this comment.
Checked all three sendLocal() callers:
- MQTT.cpp:127 — if (router->sendLocal(pAck) == ERRNO_SHOULD_RELEASE) packetPool.release(pAck);
- RoutingModule.cpp:60 — same pattern
- MeshService.cpp:348 — same pattern
All three only release when the result is ERRNO_SHOULD_RELEASE, never on ERRNO_OK. Since the local/queued path now returns ERRNO_OK, none of them double-release, the queue owns the packet and it's freed once in perhapsHandleReceived() after processing. No double-free/leak.
… fix/callModules
… in perhapsHandleReceived, and fix tests broken by the queue-defer change
|
This also fixes the same config-get/set phone-timeout regression that #11185 targets (both trace back to #10967 making sendLocal()'s isToUs branch call handleReceived() synchronously) — MeshModule.cpp's existing service->sendToMesh(currentReply, RX_SRC_LOCAL, isToUs(currentReply)) already forces phone delivery at the call site that matters, so no equivalent to #11185's MeshService.cpp change is needed here. Unlike #11185, this approach doesn't reintroduce the synchronous re-entry into callModules(), which I confirmed overflows the stack on nRF52840 (crash on save, e.g. Wio Tracker L1) — #11185 alone still has that crash since it keeps the synchronous handleReceived() call. Rebasing onto current develop will need two test updates that aren't new regressions, just stale coupling to the old synchronous contract: test_packet_signing's test_C8 asserts the old ERRNO_SHOULD_RELEASE return, and test_mqtt's MockRouter::enqueueReceivedMessage override needs the widened RxSource parameter to compile. Happy to push both. |
|
i opted for approving #11185 since it's the most surgical approach of fixing the regression. Regarding the stack overflow, that'as actually easier to fix now #11185 decouples phone delivery from handleReceived entirely. Add a depth counter to the router. if it is not zero, it's calld from within callModules and you enqueue a copy of the packet with its RxSource into a small local queue, then return ERRNO_SHOULD_RELEASE. Depth 0 has no change to today. |
…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.
…eshtastic#11191) Prevents an nRF52 task-stack overflow on config save. Replaces draft meshtastic#11155.
…eshtastic#11191) Prevents an nRF52 task-stack overflow on config save. Replaces draft meshtastic#11155.
2 participants
Summary
Router::sendLocal()was callinghandleReceived()synchronously for locally-addressed packets. When a module (e.g. an admin/module-config handler) generates a reply from insideMeshModule::callModules()and sends it viaservice->sendToMesh(), that synchronous call re-enteredcallModules()from within itself, which caused reply packets to phone requests to be dropped instead of delivered.This PR restores queuing of locally-addressed packets through
fromRadioQueue(avoiding the re-entrancy), while fixing the underlying reason queuing had been avoided before: queuing used to erase the packet'sRxSource, making a local phone/module-generated packet indistinguishable from already-decoded remote ingress once it was dequeued.Changes
Router::fromRadioQueueis now aTypedQueue<QueuedFromRadio>(packet +RxSourcepair) instead of aPointerQueue<meshtastic_MeshPacket>, so origin is preserved across the queue.Router::enqueueReceivedMessage()andRouter::perhapsHandleReceived()now take/forward anRxSource(defaulting toRX_SRC_RADIOfor existing radio/MQTT/UDP callers).Router::sendLocal()queues locally-addressed packets viaenqueueReceivedMessage(p, src)instead of callinghandleReceived()in-line, avoiding re-entrantcallModules()calls while still replaying the packet with its true origin.MeshModule::callModules()now explicitly passesRX_SRC_LOCAL(andisToUs(currentReply)) when sending a reply packet, so it reaches the phone instead of being silently dropped by the loopback guard.Fixes reply packets (e.g. admin/module-config responses to the phone) being silently dropped.
🤝 Attestations
Tested on a Seeed WIO Tracker L1. Have not been able to test on the other listed hardware — help testing on those devices would be appreciated.