From 01d1873bfc09cb1df6dacc540ff6dbe02285e8bf Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Fri, 24 Jul 2026 04:02:51 -0500 Subject: [PATCH] Deliver locally-generated replies addressed to us to the phone (#11185) * Deliver locally-generated replies addressed to us to the phone 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. * Address review: trim comments, assert the QueueStatus count 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. --- src/mesh/MeshService.cpp | 9 +++- test/test_mesh_module/test_main.cpp | 72 +++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index 689d5847c..0efd23c81 100644 --- a/src/mesh/MeshService.cpp +++ b/src/mesh/MeshService.cpp @@ -324,13 +324,20 @@ void MeshService::sendToMesh(meshtastic_MeshPacket *p, RxSource src, bool ccToPh uint32_t mesh_packet_id = p->id; nodeDB->updateFrom(*p); // update our local DB for this packet (because phone might have sent position packets etc...) + // callModules' loopback gate keeps RX_SRC_LOCAL packets from RoutingModule, the only module + // that forwards to the phone, so deliver our own reply's copy here or the client never sees it. + const bool localDelivery = isToUs(p); + if (src == RX_SRC_LOCAL && localDelivery) + ccToPhone = true; + // Note: We might return !OK if our fifo was full, at that point the only option we have is to drop it ErrorCode res = router->sendLocal(p, src); /* NOTE(pboldin): Prepare and send QueueStatus message to the phone as a * high-priority message. */ meshtastic_QueueStatus qs = router->getQueueStatus(); - ErrorCode r = sendQueueStatusToPhone(qs, res, mesh_packet_id); + // SHOULD_RELEASE means "caller frees", not a send failure, so don't report it as one. + ErrorCode r = sendQueueStatusToPhone(qs, (res == ERRNO_SHOULD_RELEASE && localDelivery) ? ERRNO_OK : res, mesh_packet_id); if (r != ERRNO_OK) { LOG_DEBUG("Can't send status to phone"); } diff --git a/test/test_mesh_module/test_main.cpp b/test/test_mesh_module/test_main.cpp index 6ba038af3..17e4915e8 100644 --- a/test/test_mesh_module/test_main.cpp +++ b/test/test_mesh_module/test_main.cpp @@ -256,6 +256,8 @@ void tearDown(void) while (auto *status = mockService->getQueueStatusForPhone()) mockService->releaseQueueStatusToPool(status); + while (auto *toPhone = mockService->getForPhone()) + mockService->releaseToPool(toPhone); delete mockService; mockService = nullptr; service = nullptr; @@ -471,6 +473,74 @@ static void test_dispatch_realNeighborInfoCannotShadowTelemetryOwner() TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); } +// A reply addressed to ourselves reaches the phone queue, with SHOULD_RELEASE reported as success. +static void test_localReplyToSelf_isDeliveredToPhone() +{ + meshtastic_MeshPacket reply = meshtastic_MeshPacket_init_zero; + reply.from = LOCAL_NODE; + reply.to = LOCAL_NODE; + reply.id = 0xABCD1234; + reply.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + reply.decoded.portnum = meshtastic_PortNum_ADMIN_APP; + reply.decoded.request_id = 0x12345678; + + service->sendToMesh(packetPool.allocCopy(reply)); // default src == RX_SRC_LOCAL, as callModules sends replies + + meshtastic_MeshPacket *toPhone = mockService->getForPhone(); + TEST_ASSERT_NOT_NULL(toPhone); + TEST_ASSERT_EQUAL_UINT32(0xABCD1234, toPhone->id); + TEST_ASSERT_EQUAL_UINT32(0x12345678, toPhone->decoded.request_id); + mockService->releaseToPool(toPhone); + TEST_ASSERT_NULL(mockService->getForPhone()); // exactly one delivery + + meshtastic_QueueStatus *qs = mockService->getQueueStatusForPhone(); + TEST_ASSERT_NOT_NULL(qs); + TEST_ASSERT_EQUAL(ERRNO_OK, qs->res); + mockService->releaseQueueStatusToPool(qs); + + TEST_ASSERT_EQUAL_UINT32(0, mockRouter->sentPackets.size()); // nothing went toward the radio +} + +// Full loop: a phone-originated want_response request (from == 0, RX_SRC_USER) dispatched +// through the real router must produce a module reply that reaches the phone queue. +static void test_phoneRequest_replyReachesPhone() +{ + auto *replyOwner = registerDispatchModule( + new SyntheticReplyModule("private-owner", meshtastic_PortNum_PRIVATE_APP, meshtastic_PortNum_PRIVATE_APP)); + + meshtastic_MeshPacket request = meshtastic_MeshPacket_init_zero; + request.from = 0; // phone-originated, as MeshService::handleToRadio stamps it + request.to = LOCAL_NODE; + request.id = 0x5EED0001; + request.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + request.decoded.portnum = meshtastic_PortNum_PRIVATE_APP; + request.decoded.want_response = true; + + service->sendToMesh(packetPool.allocCopy(request), RX_SRC_USER); + + TEST_ASSERT_EQUAL_UINT32(1, replyOwner->allocReplyCalls); + + meshtastic_MeshPacket *toPhone = mockService->getForPhone(); + TEST_ASSERT_NOT_NULL(toPhone); + TEST_ASSERT_EQUAL(meshtastic_PortNum_PRIVATE_APP, toPhone->decoded.portnum); + TEST_ASSERT_EQUAL_UINT32(0x5EED0001, toPhone->decoded.request_id); + TEST_ASSERT_EQUAL_UINT32(LOCAL_NODE, toPhone->to); + mockService->releaseToPool(toPhone); + TEST_ASSERT_NULL(mockService->getForPhone()); // the request itself must not echo back + + // One QueueStatus for the request, one for the reply, both reporting success + uint32_t statuses = 0; + while (auto *qs = mockService->getQueueStatusForPhone()) { + TEST_ASSERT_EQUAL(ERRNO_OK, qs->res); + mockService->releaseQueueStatusToPool(qs); + statuses++; + } + TEST_ASSERT_EQUAL_UINT32(2, statuses); + + TEST_ASSERT_EQUAL_UINT32(0, mockRouter->sentPackets.size()); + TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); +} + void setup() { initializeTestEnvironment(); @@ -495,6 +565,8 @@ void setup() RUN_TEST(test_dispatch_noResponderSendsNak); RUN_TEST(test_dispatch_ignoreRequestIsClearedPerPacket); RUN_TEST(test_dispatch_realNeighborInfoCannotShadowTelemetryOwner); + RUN_TEST(test_localReplyToSelf_isDeliveredToPhone); + RUN_TEST(test_phoneRequest_replyReachesPhone); exit(UNITY_END()); }