Align telemetry broadcast want_response behavior with traceroute (#9717)

thebentern · web-flow · commit cac45d9cedb1 · 2026-02-24T13:26:47.000-06:00
* Align telemetry broadcast want_response behavior with traceroute

* Fixes

* Reduce side-effects by making the telemetry modules handle the ignorerequest

* Remove unnecessary ignoreRequest flag

* Try inheriting from MeshModule

* Add exclusion for sensor/router roles and add base telem module
diff --git a/src/mesh/MeshModule.h b/src/mesh/MeshModule.h
@@ -106,6 +106,18 @@ class MeshModule
     /* We allow modules to ignore a request without sending an error if they have a specific reason for it. */
     bool ignoreRequest = false;
 
+    /**
+     * Check if the current request is a multi-hop broadcast. Modules should call this in allocReply()
+     * and return NULL to prevent reply storms from broadcast requests that have already been relayed.
+     */
+    bool isMultiHopBroadcastRequest()
+    {
+        if (currentRequest && isBroadcast(currentRequest->to) && currentRequest->hop_limit < currentRequest->hop_start) {
+            return true;
+        }
+        return false;
+    }
+
     /** If a bound channel name is set, we will only accept received packets that come in on that channel.
      * A special exception (FIXME, not sure if this is a good idea) - packets that arrive on the local interface
      * are allowed on any channel (this lets the local user do anything).
diff --git a/src/modules/Telemetry/AirQualityTelemetry.cpp b/src/modules/Telemetry/AirQualityTelemetry.cpp
@@ -321,6 +321,10 @@ bool AirQualityTelemetryModule::getAirQualityTelemetry(meshtastic_Telemetry *m)
 meshtastic_MeshPacket *AirQualityTelemetryModule::allocReply()
 {
     if (currentRequest) {
+        if (isMultiHopBroadcastRequest() && !isSensorOrRouterRole()) {
+            ignoreRequest = true;
+            return NULL;
+        }
         auto req = *currentRequest;
         const auto &p = req.decoded;
         meshtastic_Telemetry scratch;
diff --git a/src/modules/Telemetry/AirQualityTelemetry.h b/src/modules/Telemetry/AirQualityTelemetry.h
@@ -4,6 +4,8 @@
 
 #pragma once
 
+#include "BaseTelemetryModule.h"
+
 #ifndef AIR_QUALITY_TELEMETRY_MODULE_ENABLE
 #define AIR_QUALITY_TELEMETRY_MODULE_ENABLE 0
 #endif
@@ -17,6 +19,7 @@
 
 class AirQualityTelemetryModule : private concurrency::OSThread,
                                   public ScanI2CConsumer,
+                                  public BaseTelemetryModule,
                                   public ProtobufModule<meshtastic_Telemetry>
 {
     CallbackObserver<AirQualityTelemetryModule, const meshtastic::Status *> nodeStatusObserver =
diff --git a/src/modules/Telemetry/BaseTelemetryModule.h b/src/modules/Telemetry/BaseTelemetryModule.h
@@ -0,0 +1,14 @@
+#pragma once
+
+#include "NodeDB.h"
+#include "configuration.h"
+
+class BaseTelemetryModule
+{
+  protected:
+    bool isSensorOrRouterRole() const
+    {
+        return config.device.role == meshtastic_Config_DeviceConfig_Role_SENSOR ||
+               config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER;
+    }
+};
diff --git a/src/modules/Telemetry/DeviceTelemetry.cpp b/src/modules/Telemetry/DeviceTelemetry.cpp
@@ -19,8 +19,7 @@
 int32_t DeviceTelemetryModule::runOnce()
 {
     refreshUptime();
-    bool isImpoliteRole =
-        IS_ONE_OF(config.device.role, meshtastic_Config_DeviceConfig_Role_SENSOR, meshtastic_Config_DeviceConfig_Role_ROUTER);
+    bool isImpoliteRole = isSensorOrRouterRole();
     if (((lastSentToMesh == 0) ||
          ((uptimeLastMs - lastSentToMesh) >=
           Default::getConfiguredOrDefaultMsScaled(moduleConfig.telemetry.device_update_interval,
@@ -60,6 +59,10 @@ bool DeviceTelemetryModule::handleReceivedProtobuf(const meshtastic_MeshPacket &
 meshtastic_MeshPacket *DeviceTelemetryModule::allocReply()
 {
     if (currentRequest) {
+        if (isMultiHopBroadcastRequest() && !isSensorOrRouterRole()) {
+            ignoreRequest = true;
+            return NULL;
+        }
         auto req = *currentRequest;
         const auto &p = req.decoded;
         meshtastic_Telemetry scratch;
diff --git a/src/modules/Telemetry/DeviceTelemetry.h b/src/modules/Telemetry/DeviceTelemetry.h
@@ -1,11 +1,14 @@
 #pragma once
 #include "../mesh/generated/meshtastic/telemetry.pb.h"
+#include "BaseTelemetryModule.h"
 #include "NodeDB.h"
 #include "ProtobufModule.h"
 #include <OLEDDisplay.h>
 #include <OLEDDisplayUi.h>
 
-class DeviceTelemetryModule : private concurrency::OSThread, public ProtobufModule<meshtastic_Telemetry>
+class DeviceTelemetryModule : private concurrency::OSThread,
+                              public BaseTelemetryModule,
+                              public ProtobufModule<meshtastic_Telemetry>
 {
     CallbackObserver<DeviceTelemetryModule, const meshtastic::Status *> nodeStatusObserver =
         CallbackObserver<DeviceTelemetryModule, const meshtastic::Status *>(this, &DeviceTelemetryModule::handleStatusUpdate);
diff --git a/src/modules/Telemetry/EnvironmentTelemetry.cpp b/src/modules/Telemetry/EnvironmentTelemetry.cpp
@@ -577,6 +577,10 @@ bool EnvironmentTelemetryModule::getEnvironmentTelemetry(meshtastic_Telemetry *m
 meshtastic_MeshPacket *EnvironmentTelemetryModule::allocReply()
 {
     if (currentRequest) {
+        if (isMultiHopBroadcastRequest() && !isSensorOrRouterRole()) {
+            ignoreRequest = true;
+            return NULL;
+        }
         auto req = *currentRequest;
         const auto &p = req.decoded;
         meshtastic_Telemetry scratch;
diff --git a/src/modules/Telemetry/EnvironmentTelemetry.h b/src/modules/Telemetry/EnvironmentTelemetry.h
@@ -4,6 +4,8 @@
 
 #pragma once
 
+#include "BaseTelemetryModule.h"
+
 #ifndef ENVIRONMENTAL_TELEMETRY_MODULE_ENABLE
 #define ENVIRONMENTAL_TELEMETRY_MODULE_ENABLE 0
 #endif
@@ -17,6 +19,7 @@
 
 class EnvironmentTelemetryModule : private concurrency::OSThread,
                                    public ScanI2CConsumer,
+                                   public BaseTelemetryModule,
                                    public ProtobufModule<meshtastic_Telemetry>
 {
     CallbackObserver<EnvironmentTelemetryModule, const meshtastic::Status *> nodeStatusObserver =
diff --git a/src/modules/Telemetry/HealthTelemetry.cpp b/src/modules/Telemetry/HealthTelemetry.cpp
@@ -192,6 +192,10 @@ bool HealthTelemetryModule::getHealthTelemetry(meshtastic_Telemetry *m)
 meshtastic_MeshPacket *HealthTelemetryModule::allocReply()
 {
     if (currentRequest) {
+        if (isMultiHopBroadcastRequest() && !isSensorOrRouterRole()) {
+            ignoreRequest = true;
+            return NULL;
+        }
         auto req = *currentRequest;
         const auto &p = req.decoded;
         meshtastic_Telemetry scratch;
diff --git a/src/modules/Telemetry/HealthTelemetry.h b/src/modules/Telemetry/HealthTelemetry.h
@@ -4,12 +4,15 @@
 
 #pragma once
 #include "../mesh/generated/meshtastic/telemetry.pb.h"
+#include "BaseTelemetryModule.h"
 #include "NodeDB.h"
 #include "ProtobufModule.h"
 #include <OLEDDisplay.h>
 #include <OLEDDisplayUi.h>
 
-class HealthTelemetryModule : private concurrency::OSThread, public ProtobufModule<meshtastic_Telemetry>
+class HealthTelemetryModule : private concurrency::OSThread,
+                              public BaseTelemetryModule,
+                              public ProtobufModule<meshtastic_Telemetry>
 {
     CallbackObserver<HealthTelemetryModule, const meshtastic::Status *> nodeStatusObserver =
         CallbackObserver<HealthTelemetryModule, const meshtastic::Status *>(this, &HealthTelemetryModule::handleStatusUpdate);
diff --git a/src/modules/Telemetry/PowerTelemetry.cpp b/src/modules/Telemetry/PowerTelemetry.cpp
@@ -217,6 +217,10 @@ bool PowerTelemetryModule::getPowerTelemetry(meshtastic_Telemetry *m)
 meshtastic_MeshPacket *PowerTelemetryModule::allocReply()
 {
     if (currentRequest) {
+        if (isMultiHopBroadcastRequest() && !isSensorOrRouterRole()) {
+            ignoreRequest = true;
+            return NULL;
+        }
         auto req = *currentRequest;
         const auto &p = req.decoded;
         meshtastic_Telemetry scratch;
diff --git a/src/modules/Telemetry/PowerTelemetry.h b/src/modules/Telemetry/PowerTelemetry.h
@@ -5,12 +5,15 @@
 #if !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR
 
 #include "../mesh/generated/meshtastic/telemetry.pb.h"
+#include "BaseTelemetryModule.h"
 #include "NodeDB.h"
 #include "ProtobufModule.h"
 #include <OLEDDisplay.h>
 #include <OLEDDisplayUi.h>
 
-class PowerTelemetryModule : private concurrency::OSThread, public ProtobufModule<meshtastic_Telemetry>
+class PowerTelemetryModule : private concurrency::OSThread,
+                             public BaseTelemetryModule,
+                             public ProtobufModule<meshtastic_Telemetry>
 {
     CallbackObserver<PowerTelemetryModule, const meshtastic::Status *> nodeStatusObserver =
         CallbackObserver<PowerTelemetryModule, const meshtastic::Status *>(this, &PowerTelemetryModule::handleStatusUpdate);
diff --git a/test/test_mesh_module/test_main.cpp b/test/test_mesh_module/test_main.cpp
@@ -0,0 +1,116 @@
+#include "MeshModule.h"
+#include "MeshTypes.h"
+#include "TestUtil.h"
+#include <unity.h>
+
+// Minimal concrete subclass for testing the base class helper
+class TestModule : public MeshModule
+{
+  public:
+    TestModule() : MeshModule("TestModule") {}
+    virtual bool wantPacket(const meshtastic_MeshPacket *p) override { return true; }
+    using MeshModule::currentRequest;
+    using MeshModule::isMultiHopBroadcastRequest;
+};
+
+static TestModule *testModule;
+static meshtastic_MeshPacket testPacket;
+
+void setUp(void)
+{
+    testModule = new TestModule();
+    memset(&testPacket, 0, sizeof(testPacket));
+    TestModule::currentRequest = &testPacket;
+}
+
+void tearDown(void)
+{
+    TestModule::currentRequest = NULL;
+    delete testModule;
+}
+
+// Zero-hop broadcast (hop_limit == hop_start): should be allowed
+static void test_zeroHopBroadcast_isAllowed()
+{
+    testPacket.to = NODENUM_BROADCAST;
+    testPacket.hop_start = 3;
+    testPacket.hop_limit = 3; // Not yet relayed
+
+    TEST_ASSERT_FALSE(testModule->isMultiHopBroadcastRequest());
+}
+
+// Multi-hop broadcast (hop_limit < hop_start): should be blocked
+static void test_multiHopBroadcast_isBlocked()
+{
+    testPacket.to = NODENUM_BROADCAST;
+    testPacket.hop_start = 7;
+    testPacket.hop_limit = 4; // Already relayed 3 hops
+
+    TEST_ASSERT_TRUE(testModule->isMultiHopBroadcastRequest());
+}
+
+// Direct message (not broadcast): should always be allowed regardless of hops
+static void test_directMessage_isAllowed()
+{
+    testPacket.to = 0x12345678; // Specific node
+    testPacket.hop_start = 7;
+    testPacket.hop_limit = 4;
+
+    TEST_ASSERT_FALSE(testModule->isMultiHopBroadcastRequest());
+}
+
+// Broadcast with hop_limit == 0 (fully relayed): should be blocked
+static void test_fullyRelayedBroadcast_isBlocked()
+{
+    testPacket.to = NODENUM_BROADCAST;
+    testPacket.hop_start = 3;
+    testPacket.hop_limit = 0;
+
+    TEST_ASSERT_TRUE(testModule->isMultiHopBroadcastRequest());
+}
+
+// No current request: should not crash, should return false
+static void test_noCurrentRequest_isAllowed()
+{
+    TestModule::currentRequest = NULL;
+
+    TEST_ASSERT_FALSE(testModule->isMultiHopBroadcastRequest());
+}
+
+// Broadcast with hop_start == 0 (legacy or local): should be allowed
+static void test_legacyPacket_zeroHopStart_isAllowed()
+{
+    testPacket.to = NODENUM_BROADCAST;
+    testPacket.hop_start = 0;
+    testPacket.hop_limit = 0;
+
+    // hop_limit == hop_start, so not multi-hop
+    TEST_ASSERT_FALSE(testModule->isMultiHopBroadcastRequest());
+}
+
+// Single hop relayed broadcast (hop_limit = hop_start - 1): should be blocked
+static void test_singleHopRelayedBroadcast_isBlocked()
+{
+    testPacket.to = NODENUM_BROADCAST;
+    testPacket.hop_start = 3;
+    testPacket.hop_limit = 2;
+
+    TEST_ASSERT_TRUE(testModule->isMultiHopBroadcastRequest());
+}
+
+void setup()
+{
+    initializeTestEnvironment();
+
+    UNITY_BEGIN();
+    RUN_TEST(test_zeroHopBroadcast_isAllowed);
+    RUN_TEST(test_multiHopBroadcast_isBlocked);
+    RUN_TEST(test_directMessage_isAllowed);
+    RUN_TEST(test_fullyRelayedBroadcast_isBlocked);
+    RUN_TEST(test_noCurrentRequest_isAllowed);
+    RUN_TEST(test_legacyPacket_zeroHopStart_isAllowed);
+    RUN_TEST(test_singleHopRelayedBroadcast_isBlocked);
+    exit(UNITY_END());
+}
+
+void loop() {}
