From a661fd8cd4e8b54b72e10dd8de1dd8795239847c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Fri, 14 Aug 2026 09:12:31 +0000 Subject: [PATCH] fixes #11466 (#11487) * fixes #11466 * Keep locally-addressed routing feedback out of the phone echo filter allocForSending stamps ACK/NAK packets with from == our nodenum and sendLocal defaults to RX_SRC_RADIO, so the loopback gate never applies. Filtering on isFromUs alone dropped implicit rebroadcast ACKs, duty-cycle and NO_INTERFACE NAKs, and PhoneAPI rate-limit errors on their way to the client. Add coverage through the real RoutingModule, which the mocked one used by the rest of the suite cannot exercise, and correct the test seam comment. * Clean up the temporary RoutingModule in tearDown() A failed Unity assertion longjmps out of the test, so the in-test delete never ran and the module stayed registered in MeshModule::modules for every later test. Track it at file scope, as realNeighborInfoModule already is. --- src/mesh/MeshService.cpp | 8 ++ src/mesh/MeshService.h | 3 + test/test_mesh_module/test_main.cpp | 120 ++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index 591245db9..0667e0b13 100644 --- a/src/mesh/MeshService.cpp +++ b/src/mesh/MeshService.cpp @@ -114,6 +114,14 @@ int MeshService::handleFromRadio(const meshtastic_MeshPacket *mp) } } + // Our own packet heard back off the mesh, which the duplicate cache only suppresses best-effort. + // Clients can't tell an echo from genuine ingress, so it surfaces as an incoming message. Packets + // addressed to us are locally-generated feedback (implicit ACK, NAK, routing error), not an echo. + if (isFromUs(mp) && !isToUs(mp)) { + LOG_DEBUG("Skip phone echo of our own packet 0x%08x", mp->id); + return 0; + } + printPacket("Forwarding to phone", mp); if (auto *toPhone = packetPool.allocCopy(*mp)) sendToPhone(toPhone); diff --git a/src/mesh/MeshService.h b/src/mesh/MeshService.h index 8ddc6e434..7adcdb7c6 100644 --- a/src/mesh/MeshService.h +++ b/src/mesh/MeshService.h @@ -222,6 +222,9 @@ class MeshService /// needs to keep the packet around it makes a copy int handleFromRadio(const meshtastic_MeshPacket *p); friend class RoutingModule; +#ifdef PIO_UNIT_TESTING + friend class MeshServicePhoneDeliveryTest; +#endif }; extern MeshService *service; diff --git a/test/test_mesh_module/test_main.cpp b/test/test_mesh_module/test_main.cpp index a39988064..9cc0d1811 100644 --- a/test/test_mesh_module/test_main.cpp +++ b/test/test_mesh_module/test_main.cpp @@ -265,6 +265,7 @@ static MockMeshService *mockService; static MockRouter *mockRouter; static MockRoutingModule *mockRoutingModule; static NeighborInfoModule *realNeighborInfoModule; +static RoutingModule *realRoutingModule; static std::vector dispatchModules; template static T *registerDispatchModule(T *module) @@ -273,6 +274,14 @@ template static T *registerDispatchModule(T *module) return module; } +// Swap the mocked RoutingModule for a real one. tearDown() owns the cleanup because a failed +// assertion longjmps out of the test, which would otherwise leave it registered in MeshModule::modules. +static void installRealRoutingModule() +{ + realRoutingModule = new RoutingModule(); + routingModule = realRoutingModule; +} + static meshtastic_MeshPacket makeRequest(meshtastic_PortNum port) { meshtastic_MeshPacket packet = meshtastic_MeshPacket_init_zero; @@ -335,6 +344,7 @@ void setUp(void) mockRoutingModule = new MockRoutingModule(); routingModule = mockRoutingModule; + realRoutingModule = nullptr; testModule = new TestModule(); memset(&testPacket, 0, sizeof(testPacket)); @@ -355,6 +365,9 @@ void tearDown(void) delete testModule; testModule = nullptr; + delete realRoutingModule; + realRoutingModule = nullptr; + delete mockRoutingModule; mockRoutingModule = nullptr; routingModule = nullptr; @@ -606,6 +619,108 @@ static void test_localReplyToSelf_isDeliveredToPhone() TEST_ASSERT_EQUAL_UINT32(0, mockRouter->sentPackets.size()); // nothing went toward the radio } +// handleFromRadio() is private to MeshService, which befriends RoutingModule and, under +// PIO_UNIT_TESTING, this seam. +class MeshServicePhoneDeliveryTest +{ + public: + static void deliver(const meshtastic_MeshPacket &p) { service->handleFromRadio(&p); } +}; + +static void test_handleFromRadio_remotePacketReachesPhone() +{ + meshtastic_MeshPacket rx = meshtastic_MeshPacket_init_zero; + rx.from = REMOTE_NODE; + rx.to = NODENUM_BROADCAST; + rx.id = 0x0BADF00D; + rx.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + rx.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + + MeshServicePhoneDeliveryTest::deliver(rx); + + meshtastic_MeshPacket *toPhone = mockService->getForPhone(); + TEST_ASSERT_NOT_NULL(toPhone); + TEST_ASSERT_EQUAL_UINT32(0x0BADF00D, toPhone->id); + mockService->releaseToPool(toPhone); + TEST_ASSERT_NULL(mockService->getForPhone()); +} + +// A packet we originated, coming back around, must not be echoed to the client that sent it. +static void test_handleFromRadio_ownPacketIsNotEchoedToPhone() +{ + meshtastic_MeshPacket ours = meshtastic_MeshPacket_init_zero; + ours.from = LOCAL_NODE; + ours.to = NODENUM_BROADCAST; + ours.id = 0x5E1F0001; + ours.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + ours.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + + MeshServicePhoneDeliveryTest::deliver(ours); + TEST_ASSERT_NULL(mockService->getForPhone()); + + // Same for the from==0 spelling handleToRadio stamps on phone-originated packets. + ours.from = 0; + ours.id = 0x5E1F0002; + MeshServicePhoneDeliveryTest::deliver(ours); + TEST_ASSERT_NULL(mockService->getForPhone()); +} + +// A packet from us *addressed to us* is locally-generated feedback, not an echo, and must still be +// delivered - suppressing it would silently drop every ACK/NAK the client relies on. +static void test_handleFromRadio_ownPacketAddressedToUsReachesPhone() +{ + meshtastic_MeshPacket ack = meshtastic_MeshPacket_init_zero; + ack.from = LOCAL_NODE; + ack.to = LOCAL_NODE; + ack.id = 0x5E1F0003; + ack.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + ack.decoded.portnum = meshtastic_PortNum_ROUTING_APP; + ack.decoded.request_id = 0x0C0FFEE0; + + MeshServicePhoneDeliveryTest::deliver(ack); + + meshtastic_MeshPacket *toPhone = mockService->getForPhone(); + TEST_ASSERT_NOT_NULL(toPhone); + TEST_ASSERT_EQUAL_UINT32(0x0C0FFEE0, toPhone->decoded.request_id); + mockService->releaseToPool(toPhone); + TEST_ASSERT_NULL(mockService->getForPhone()); +} + +// sendAckNak stamps from == our nodenum and to == us, and sendLocal defaults to RX_SRC_RADIO, so the +// loopback gate never applies and only handleFromRadio's filter gates the implicit ACK / NAK path. +static void test_localAckNak_reachesPhoneViaRealRoutingModule() +{ + installRealRoutingModule(); + + realRoutingModule->sendAckNak(meshtastic_Routing_Error_NONE, LOCAL_NODE, 0xFEEDBEEF, 0); + + meshtastic_MeshPacket *toPhone = mockService->getForPhone(); + TEST_ASSERT_NOT_NULL(toPhone); + TEST_ASSERT_EQUAL(meshtastic_PortNum_ROUTING_APP, toPhone->decoded.portnum); + TEST_ASSERT_EQUAL_UINT32(0xFEEDBEEF, toPhone->decoded.request_id); + TEST_ASSERT_EQUAL_UINT32(LOCAL_NODE, toPhone->to); + TEST_ASSERT_EQUAL_UINT32(LOCAL_NODE, toPhone->from); + mockService->releaseToPool(toPhone); +} + +// The mirror of the above: a broadcast we originated, heard back off the mesh, must not reach the +// phone even though it travels the same RoutingModule path. +static void test_ownBroadcastEcho_isDroppedByRealRoutingModule() +{ + installRealRoutingModule(); + + meshtastic_MeshPacket echo = meshtastic_MeshPacket_init_zero; + echo.from = LOCAL_NODE; + echo.to = NODENUM_BROADCAST; + echo.id = 0x5E1F0004; + echo.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + echo.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + + MeshModule::callModules(echo, RX_SRC_RADIO); + + TEST_ASSERT_NULL(mockService->getForPhone()); +} + // 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() @@ -736,6 +851,11 @@ void setup() RUN_TEST(test_dispatch_ignoreRequestIsClearedPerPacket); RUN_TEST(test_dispatch_realNeighborInfoCannotShadowTelemetryOwner); RUN_TEST(test_localReplyToSelf_isDeliveredToPhone); + RUN_TEST(test_handleFromRadio_remotePacketReachesPhone); + RUN_TEST(test_handleFromRadio_ownPacketIsNotEchoedToPhone); + RUN_TEST(test_handleFromRadio_ownPacketAddressedToUsReachesPhone); + RUN_TEST(test_localAckNak_reachesPhoneViaRealRoutingModule); + RUN_TEST(test_ownBroadcastEcho_isDroppedByRealRoutingModule); RUN_TEST(test_phoneRequest_replyReachesPhone); RUN_TEST(test_nestedLocalSend_isDeferred_notReentrant); RUN_TEST(test_deferredChain_drainsBreadthFirst);