From c29bd009719ddf10672deac5d7f86af7a33462f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Fri, 18 Sep 2026 10:34:53 +0000 Subject: [PATCH] Rewrite the MQTT region root topic only on the default broker (#11899) * fix(mqtt): rewrite the region root topic only on the default broker A region change rewrote any root starting with "msh", on any broker. Custom roots such as msh/home were clobbered, and private brokers had their topics moved even though a regional broker is regional already. An empty root, which MQTT treats as the default, was never updated. Region changes now go through MQTT::applyRegionRootTopic(), which rewrites the root only on the default broker and only when the root is empty, the default, or a msh/ the firmware wrote itself. * fix(mqtt): parse the broker address before the default-server check Persist module config only when the root actually changed. Replace a stray NUL byte in the test with the \0 escape. * fix(mqtt): count the regional roots as the default root topic --- src/graphics/draw/MenuHandler.cpp | 9 ++-- .../InkHUD/Applets/System/Menu/MenuApplet.cpp | 9 ++-- src/modules/AdminModule.cpp | 20 +++---- src/mqtt/MQTT.cpp | 28 +++++++++- src/mqtt/MQTT.h | 3 ++ test/test_mqtt/MQTT.cpp | 54 +++++++++++++++++++ 6 files changed, 106 insertions(+), 17 deletions(-) diff --git a/src/graphics/draw/MenuHandler.cpp b/src/graphics/draw/MenuHandler.cpp index bc0da947c3..45a3e9760f 100644 --- a/src/graphics/draw/MenuHandler.cpp +++ b/src/graphics/draw/MenuHandler.cpp @@ -28,6 +28,9 @@ #include "mesh/RadioLibInterface.h" #include "modules/AdminModule.h" #include "modules/CannedMessageModule.h" +#if !MESHTASTIC_EXCLUDE_MQTT +#include "mqtt/MQTT.h" +#endif #include "modules/ExternalNotificationModule.h" #include "modules/GeofenceModule.h" #include "modules/KeyVerificationModule.h" @@ -289,10 +292,10 @@ static void applyLoraRegion(meshtastic_Config_LoRaConfig_RegionCode region, bool if (getEffectiveDutyCycle() < 100) { config.lora.ignore_mqtt = true; } - if (strncmp(moduleConfig.mqtt.root, default_mqtt_root, strlen(default_mqtt_root)) == 0) { - snprintf(moduleConfig.mqtt.root, sizeof(moduleConfig.mqtt.root), "%s/%s", default_mqtt_root, myRegion->name); +#if !MESHTASTIC_EXCLUDE_MQTT + if (MQTT::applyRegionRootTopic(myRegion->name)) changes |= SEGMENT_MODULECONFIG; - } +#endif #if !MESHTASTIC_EXCLUDE_GPS // Enable gps if it was previously disabled due to region not being set if (gps != nullptr && !gps->isEnabled() && config.position.gps_mode == meshtastic_Config_PositionConfig_GpsMode_ENABLED) diff --git a/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp b/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp index 6de9c48a5c..3393bb4d65 100644 --- a/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp +++ b/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp @@ -12,6 +12,9 @@ #include "UptimeClock.h" #include "airtime.h" #include "gps/RTC.h" +#if !MESHTASTIC_EXCLUDE_MQTT +#include "mqtt/MQTT.h" +#endif #include "graphics/niche/InkHUD/Applets/Bases/Map/MapApplet.h" #include "graphics/niche/InkHUD/Applets/User/Waypoints/WaypointListApplet.h" #include "graphics/niche/Utils/FlashData.h" @@ -342,10 +345,10 @@ static void applyLoRaRegion(meshtastic_Config_LoRaConfig_RegionCode region) config.lora.ignore_mqtt = true; } - if (strncmp(moduleConfig.mqtt.root, default_mqtt_root, strlen(default_mqtt_root)) == 0) { - snprintf(moduleConfig.mqtt.root, sizeof(moduleConfig.mqtt.root), "%s/%s", default_mqtt_root, myRegion->name); +#if !MESHTASTIC_EXCLUDE_MQTT + if (MQTT::applyRegionRootTopic(myRegion->name)) changes |= SEGMENT_MODULECONFIG; - } +#endif // Notify UI that changes are being applied InkHUD::InkHUD::getInstance()->notifyApplyingChanges(); service->reloadConfig(changes); diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 4fae5b1432..4a6503066a 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1068,11 +1068,11 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) if (getEffectiveDutyCycle() < 100) { validatedLora.ignore_mqtt = true; // Ignore MQTT by default if region has a duty cycle limit } - if (strncmp(moduleConfig.mqtt.root, default_mqtt_root, strlen(default_mqtt_root)) == 0) { - // Default root is in use, so subscribe to the appropriate MQTT topic for this region - snprintf(moduleConfig.mqtt.root, sizeof(moduleConfig.mqtt.root), "%s/%s", default_mqtt_root, myRegion->name); - } - changes |= SEGMENT_CONFIG | SEGMENT_MODULECONFIG; +#if !MESHTASTIC_EXCLUDE_MQTT + if (MQTT::applyRegionRootTopic(myRegion->name)) + changes |= SEGMENT_MODULECONFIG; +#endif + changes |= SEGMENT_CONFIG; } else { // Region validation has failed, so just copy all of the old config over the new config validatedLora = oldLoraConfig; @@ -1108,11 +1108,11 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) if (getEffectiveDutyCycle() < 100) { validatedLora.ignore_mqtt = true; // Ignore MQTT by default if region has a duty cycle limit } - if (strncmp(moduleConfig.mqtt.root, default_mqtt_root, strlen(default_mqtt_root)) == 0) { - // Default root is in use, so subscribe to the appropriate MQTT topic for this region - snprintf(moduleConfig.mqtt.root, sizeof(moduleConfig.mqtt.root), "%s/%s", default_mqtt_root, myRegion->name); - } - changes |= SEGMENT_CONFIG | SEGMENT_MODULECONFIG; +#if !MESHTASTIC_EXCLUDE_MQTT + if (MQTT::applyRegionRootTopic(myRegion->name)) + changes |= SEGMENT_MODULECONFIG; +#endif + changes |= SEGMENT_CONFIG; } // use_preset and bandwidth are coerced into valid values by the check. } diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index e8b9010f6a..8e7d344d91 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -247,9 +247,22 @@ bool isDefaultServer(const String &host) return host.length() == 0 || host == default_mqtt_address; } +// "msh/" is what the default broker's convention produces; any other suffix is the user's own. +bool isRegionRootTopic(const char *root) +{ + const size_t prefixLen = strlen(default_mqtt_root) + 1; + if (strncmp(root, default_mqtt_root "/", prefixLen) != 0) + return false; + for (const RegionInfo *r = regions; r->code != meshtastic_Config_LoRaConfig_RegionCode_UNSET; r++) + if (strcmp(r->name, root + prefixLen) == 0) + return true; + return false; +} + +// The regional roots count as default: they are what a region change writes on the default broker. bool isDefaultRootTopic(const String &root) { - return root.length() == 0 || root == default_mqtt_root; + return root.length() == 0 || root == default_mqtt_root || isRegionRootTopic(root.c_str()); } struct PubSubConfig { @@ -366,6 +379,19 @@ void MQTT::onReceive(char *topic, byte *payload, size_t length) onReceiveProto(topic, payload, length); } +bool MQTT::applyRegionRootTopic(const char *regionName) +{ + // The region suffix is a convention of the default broker; a regional broker is regional already. + auto [host, parsedPort] = parseHostAndPort(moduleConfig.mqtt.address); + (void)parsedPort; + if (!isDefaultServer(host)) + return false; + if (!isDefaultRootTopic(moduleConfig.mqtt.root)) + return false; // the user picked their own root + snprintf(moduleConfig.mqtt.root, sizeof(moduleConfig.mqtt.root), "%s/%s", default_mqtt_root, regionName); + return true; +} + void mqttInit() { if (!moduleConfig.mqtt.enabled) diff --git a/src/mqtt/MQTT.h b/src/mqtt/MQTT.h index 5a905433c8..eb1b117554 100644 --- a/src/mqtt/MQTT.h +++ b/src/mqtt/MQTT.h @@ -58,6 +58,9 @@ class MQTT : private concurrency::OSThread bool isUsingDefaultServer() { return isConfiguredForDefaultServer; } bool isUsingDefaultRootTopic() { return isConfiguredForDefaultRootTopic; } + /** Point the root topic at the current region on the default broker. Returns true if it rewrote it. */ + static bool applyRegionRootTopic(const char *regionName); + /// Validate the meshtastic_ModuleConfig_MQTTConfig. static bool isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config) { return isValidConfig(config, nullptr); } diff --git a/test/test_mqtt/MQTT.cpp b/test/test_mqtt/MQTT.cpp index 0229c0fe59..afe46ff2cb 100644 --- a/test/test_mqtt/MQTT.cpp +++ b/test/test_mqtt/MQTT.cpp @@ -1389,6 +1389,58 @@ void test_rootChange_rebuildsTopics(void) TEST_ASSERT_EQUAL_STRING("msh/EU_868/2/e/test/!12345678", topic.c_str()); } +// The "/" suffix (msh/US, msh/EU_868) is a convention of the default Meshtastic broker, so a +// region change only rewrites the root there, and only when the root is still the default one. Guards against +// clobbering a root the user chose (including one that merely starts with "msh"), and against moving a private +// broker's topics, which are regional already if they need to be. +void test_applyRegionRootTopic_rewritesDefaultBrokerRootsOnly(void) +{ + strcpy(moduleConfig.mqtt.root, "msh/US"); + TEST_ASSERT_TRUE(MQTT::applyRegionRootTopic("EU_868")); + TEST_ASSERT_EQUAL_STRING("msh/EU_868", moduleConfig.mqtt.root); + + strcpy(moduleConfig.mqtt.root, default_mqtt_root); + TEST_ASSERT_TRUE(MQTT::applyRegionRootTopic("EU_868")); + TEST_ASSERT_EQUAL_STRING("msh/EU_868", moduleConfig.mqtt.root); + + // An empty root is the default too: MQTT falls back to "msh" when building its topics. + moduleConfig.mqtt.root[0] = '\0'; + strcpy(moduleConfig.mqtt.address, default_mqtt_address); + TEST_ASSERT_TRUE(MQTT::applyRegionRootTopic("EU_868")); + TEST_ASSERT_EQUAL_STRING("msh/EU_868", moduleConfig.mqtt.root); + + // The default broker with an explicit port is still the default broker. + strcpy(moduleConfig.mqtt.address, default_mqtt_address ":1883"); + strcpy(moduleConfig.mqtt.root, "msh/US"); + TEST_ASSERT_TRUE(MQTT::applyRegionRootTopic("EU_868")); + TEST_ASSERT_EQUAL_STRING("msh/EU_868", moduleConfig.mqtt.root); + + // A root of the user's own is left alone, even when it starts with "msh". + strcpy(moduleConfig.mqtt.root, "msh/home"); + TEST_ASSERT_FALSE(MQTT::applyRegionRootTopic("EU_868")); + TEST_ASSERT_EQUAL_STRING("msh/home", moduleConfig.mqtt.root); + + // A private broker keeps its topics across a region change. + strcpy(moduleConfig.mqtt.address, "mqtt.example.org"); + strcpy(moduleConfig.mqtt.root, "msh/US"); + TEST_ASSERT_FALSE(MQTT::applyRegionRootTopic("EU_868")); + TEST_ASSERT_EQUAL_STRING("msh/US", moduleConfig.mqtt.root); +} + +// USERPREFS_EVENT_MODE gates the public broker on isUsingDefaultRootTopic() (Channels::anyMqttEnabled()). +// A "msh/" root is what a region change writes on the default broker, so it has to keep counting as +// the default root; otherwise an event build silently loses that guard the first time the region changes. +void test_regionRootTopic_countsAsTheDefaultRoot(void) +{ + strcpy(moduleConfig.mqtt.root, "msh/EU_868"); + MQTTUnitTest::restart(); + TEST_ASSERT_TRUE(mqtt->isUsingDefaultRootTopic()); + + strcpy(moduleConfig.mqtt.root, "msh/home"); + MQTTUnitTest::restart(); + TEST_ASSERT_FALSE(mqtt->isUsingDefaultRootTopic()); +} + // Empty configuration is valid. void test_configEmptyIsValid(void) { @@ -1585,6 +1637,8 @@ void setup() RUN_TEST(test_mqttInitSkipsAllocationWhenDisabled); RUN_TEST(test_customMqttRoot); RUN_TEST(test_rootChange_rebuildsTopics); + RUN_TEST(test_applyRegionRootTopic_rewritesDefaultBrokerRootsOnly); + RUN_TEST(test_regionRootTopic_countsAsTheDefaultRoot); RUN_TEST(test_configEmptyIsValid); RUN_TEST(test_configEnabledEmptyIsValid); RUN_TEST(test_configWithDefaultServer);