fix(admin): persist TAK module config (team color / member role) (#11216)

Setting TAK team/role ACKed and rebooted but stored nothing:
handleSetModuleConfig had no tak case, saveToDisk never set has_tak,
and handleGetModuleConfig had no TAK_CONFIG case. Add all three, plus
a native suite sweeping every ModuleConfig submessage through
set -> save -> load -> get and a TAK value-fidelity suite.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

.github/copilot-instructions.md

@@ -677,6 +677,8 @@ Unit tests in `test/` directory. The canonical suite count is in `test/native-su
 - `test_radio/` - Radio interface
 - `test_rtc/` - RTC / time handling
 - `test_serial/` - Serial communication
+- `test_tak_config/` - TAK (ATAK) team/role value fidelity through set/save/load/get
+- `test_module_config/` - every ModuleConfig submessage survives admin set -> save -> load -> get
 - `test_traffic_management/` - Traffic management (dedup, rate-limit, hop-trim, role exceptions)
 - `test_transmit_history/` - Retransmission tracking
 - `test_type_conversions/` - NodeDB v25 type conversion (bitfield round-trips, NodeInfoLite)

src/mesh/NodeDB.cpp

@@ -3008,6 +3008,7 @@ bool NodeDB::saveToDiskNoRetry(int saveWhat)
         moduleConfig.has_audio = true;
         moduleConfig.has_paxcounter = true;
         moduleConfig.has_statusmessage = true;
+        moduleConfig.has_tak = true;
 #if !MESHTASTIC_EXCLUDE_BEACON
         moduleConfig.has_mesh_beacon = true;
 #endif

src/modules/AdminModule.cpp

@@ -1299,6 +1299,11 @@ bool AdminModule::handleSetModuleConfig(const meshtastic_ModuleConfig &c)
         moduleConfig.has_traffic_management = true;
         moduleConfig.traffic_management = c.payload_variant.traffic_management;
         break;
+    case meshtastic_ModuleConfig_tak_tag:
+        LOG_INFO("Set module config: TAK");
+        moduleConfig.has_tak = true;
+        moduleConfig.tak = c.payload_variant.tak;
+        break;
 #if !MESHTASTIC_EXCLUDE_BEACON
     case meshtastic_ModuleConfig_mesh_beacon_tag: {
         LOG_INFO("Set module config: MeshBeacon");
@@ -1617,6 +1622,11 @@ void AdminModule::handleGetModuleConfig(const meshtastic_MeshPacket &req, const
             res.get_module_config_response.which_payload_variant = meshtastic_ModuleConfig_traffic_management_tag;
             res.get_module_config_response.payload_variant.traffic_management = moduleConfig.traffic_management;
             break;
+        case meshtastic_AdminMessage_ModuleConfigType_TAK_CONFIG:
+            configName = "TAK";
+            res.get_module_config_response.which_payload_variant = meshtastic_ModuleConfig_tak_tag;
+            res.get_module_config_response.payload_variant.tak = moduleConfig.tak;
+            break;
 #if !MESHTASTIC_EXCLUDE_BEACON
         case meshtastic_AdminMessage_ModuleConfigType_MESHBEACON_CONFIG:
             configName = "MeshBeacon";

test/README.md

@@ -404,6 +404,8 @@ A well-structured test suite follows this pattern:
 | `test_position_precision`    | Position precision helpers    |
 | `test_radio`                 | Radio interface               |
 | `test_serial`                | Serial communication          |
+| `test_module_config`         | AdminModule module config     |
+| `test_tak_config`            | TAK (ATAK) team/role values   |
 | `test_traffic_management`    | Traffic management            |
 | `test_transmit_history`      | Retransmission tracking       |
 | `test_type_conversions`      | NodeDB v25 type conversions   |

test/native-suite-count

@@ -1 +1 @@
-38
+40

test/test_module_config/test_main.cpp

@@ -0,0 +1,195 @@
+// AdminModule module-config lifecycle coverage: every submessage must survive set -> save -> load -> get.
+//
+// Motivated by firmware#11182 / Meshtastic-Android#6430 (TAK team/role never persisted): a submessage
+// with no case in handleSetModuleConfig() falls through the switch silently - the setter still ACKs
+// and reboots - and one missing has_* flag in saveToDisk() makes nanopb drop the struct on the flash
+// write. Both defects are invisible to per-module tests until someone hits them in the field, so the
+// sweeps below enumerate the generated constants and put each type through the full lifecycle.
+//
+// The sweeps assert presence and routing, not field semantics - per-module value validation belongs
+// to the per-module suites (test_mesh_beacon, test_admin_session_repro, ...).
+
+#include "MeshTypes.h" // include BEFORE TestUtil.h
+#include "TestUtil.h"
+#include <unity.h>
+
+#include "mesh/NodeDB.h"
+#include "mesh/mesh-pb-constants.h"
+#include "modules/AdminModule.h"
+#include "support/AdminModuleTestShim.h"
+#include "support/MockMeshService.h"
+#include <cstdio>
+#include <cstring>
+
+static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A;
+static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B;
+
+static MockMeshService *mockService = nullptr;
+static AdminModuleTestShim *admin = nullptr;
+
+// Both generated sets are contiguous: ModuleConfigType runs 0.._MAX and the matching ModuleConfig
+// submessage tags run 1.._MAX+1, so type + 1 == tag and no lookup table is needed.
+static const uint32_t moduleConfigTypeCount = _meshtastic_AdminMessage_ModuleConfigType_MAX + 1;
+
+// The submessages whose handlers are compiled out on some builds. Everything else must be present
+// unconditionally.
+static bool moduleConfigIsCompiledOut(pb_size_t tag)
+{
+    (void)tag;
+#if MESHTASTIC_EXCLUDE_MQTT
+    if (tag == meshtastic_ModuleConfig_mqtt_tag)
+        return true;
+#endif
+#if MESHTASTIC_EXCLUDE_BEACON
+    if (tag == meshtastic_ModuleConfig_mesh_beacon_tag)
+        return true;
+#endif
+    return false;
+}
+
+// Unity assertion messages are plain strings, so name the failing iteration by number - nanopb keeps
+// no field names to print. Cross-reference module_config.pb.h / admin.pb.h.
+static const char *describeTag(const char *what, pb_size_t tag)
+{
+    static char buf[96];
+    snprintf(buf, sizeof(buf), "%s: ModuleConfig submessage tag %u", what, (unsigned)tag);
+    return buf;
+}
+
+static meshtastic_MeshPacket makeGetRequest(NodeNum from)
+{
+    meshtastic_MeshPacket req = meshtastic_MeshPacket_init_zero;
+    req.from = from;
+    req.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
+    req.decoded.want_response = true;
+    return req;
+}
+
+// Read back the variant tag of whatever get_module_config response the handler queued.
+static bool decodeResponseTagFromReply(meshtastic_MeshPacket *reply, pb_size_t &out)
+{
+    meshtastic_AdminMessage am = meshtastic_AdminMessage_init_zero;
+    if (!reply || reply->which_payload_variant != meshtastic_MeshPacket_decoded_tag)
+        return false;
+    if (!pb_decode_from_bytes(reply->decoded.payload.bytes, reply->decoded.payload.size, &meshtastic_AdminMessage_msg, &am))
+        return false;
+    if (am.which_payload_variant != meshtastic_AdminMessage_get_module_config_response_tag)
+        return false;
+    out = am.get_module_config_response.which_payload_variant;
+    return true;
+}
+
+// The proto guarantees the two enumerations stay in step; the sweeps below rely on it to pair a
+// ModuleConfigType with its submessage tag without a lookup table.
+void test_tagRangeMatchesTypeRange(void)
+{
+    TEST_ASSERT_EQUAL_MESSAGE(moduleConfigTypeCount, meshtastic_ModuleConfig_mesh_beacon_tag,
+                              "ModuleConfigType and ModuleConfig submessage tags are no longer 1:1");
+}
+
+// The full lifecycle per submessage: admin set -> encode (what saveToDisk writes) -> decode into the
+// live global (a reboot's loadFromDisk) -> admin get answers with that submessage. A missing set case
+// shows up as a 0-byte encode; a missing get case as an empty or mismatched response.
+void test_everyModuleConfig_survivesSetSaveLoadGet(void)
+{
+    for (pb_size_t tag = 1; tag <= moduleConfigTypeCount; tag++) {
+        if (moduleConfigIsCompiledOut(tag))
+            continue;
+        moduleConfig = meshtastic_LocalModuleConfig_init_zero;
+
+        // Set: the only content is this one submessage, so the encode below stands or falls with it.
+        meshtastic_ModuleConfig mc = meshtastic_ModuleConfig_init_zero;
+        mc.which_payload_variant = tag;
+        TEST_ASSERT_TRUE_MESSAGE(admin->handleSetModuleConfig(mc), describeTag("setter rejected", tag));
+
+        // Save: presence flag unset means nanopb drops the struct and the write encodes to nothing.
+        uint8_t saved[meshtastic_LocalModuleConfig_size];
+        size_t savedLen = pb_encode_to_bytes(saved, sizeof(saved), &meshtastic_LocalModuleConfig_msg, &moduleConfig);
+        TEST_ASSERT_GREATER_THAN_size_t_MESSAGE(0, savedLen, describeTag("dropped on save, presence flag not set", tag));
+
+        // Load: overwrite the live global, as a reboot would.
+        moduleConfig = meshtastic_LocalModuleConfig_init_zero;
+        TEST_ASSERT_TRUE_MESSAGE(pb_decode_from_bytes(saved, savedLen, &meshtastic_LocalModuleConfig_msg, &moduleConfig),
+                                 describeTag("failed to decode", tag));
+
+        // Re-encoding what was loaded must reproduce the saved bytes - presence survived the reload.
+        uint8_t reloaded[meshtastic_LocalModuleConfig_size];
+        size_t reloadedLen = pb_encode_to_bytes(reloaded, sizeof(reloaded), &meshtastic_LocalModuleConfig_msg, &moduleConfig);
+        TEST_ASSERT_EQUAL_size_t_MESSAGE(savedLen, reloadedLen, describeTag("changed size across reload", tag));
+        TEST_ASSERT_EQUAL_MEMORY_MESSAGE(saved, reloaded, savedLen, describeTag("changed content across reload", tag));
+
+        // Get: remote and local requests take the same branch and must answer with this submessage.
+        const NodeNum requesters[] = {ADMIN_NODE, 0};
+        for (NodeNum from : requesters) {
+            meshtastic_MeshPacket req = makeGetRequest(from);
+            admin->handleGetModuleConfig(req, tag - 1); // type pairs 1:1 with tag (guard test above)
+
+            pb_size_t responseTag = 0;
+            bool decoded = decodeResponseTagFromReply(admin->reply(), responseTag);
+            admin->drainReply();
+
+            TEST_ASSERT_TRUE_MESSAGE(decoded, describeTag("no get response", tag));
+            TEST_ASSERT_EQUAL_MESSAGE(tag, responseTag, describeTag("get answered with a different submessage", tag));
+        }
+    }
+}
+
+// Each set is a single-submessage edit: writing type N must not erase types 1..N-1. Every additional
+// stored submessage grows the encoded config, so a clobbered earlier write shows up as a non-increase.
+void test_sequentialSets_preserveEarlierWrites(void)
+{
+    size_t prevLen = 0;
+    for (pb_size_t tag = 1; tag <= moduleConfigTypeCount; tag++) {
+        if (moduleConfigIsCompiledOut(tag))
+            continue;
+
+        meshtastic_ModuleConfig mc = meshtastic_ModuleConfig_init_zero;
+        mc.which_payload_variant = tag;
+        admin->handleSetModuleConfig(mc);
+
+        uint8_t buf[meshtastic_LocalModuleConfig_size];
+        size_t len = pb_encode_to_bytes(buf, sizeof(buf), &meshtastic_LocalModuleConfig_msg, &moduleConfig);
+        TEST_ASSERT_GREATER_THAN_size_t_MESSAGE(prevLen, len, describeTag("set erased an earlier submessage", tag));
+        prevLen = len;
+    }
+}
+
+void setUp(void)
+{
+    mockService = new MockMeshService();
+    service = mockService;
+
+    admin = new AdminModuleTestShim();
+    admin->deferSaves(); // no disk write or reboot when a setter is accepted
+
+    moduleConfig = meshtastic_LocalModuleConfig_init_zero;
+    config = meshtastic_LocalConfig_init_zero;
+    myNodeInfo = meshtastic_MyNodeInfo_init_zero;
+    myNodeInfo.my_node_num = LOCAL_NODE;
+}
+
+void tearDown(void)
+{
+    admin->drainReply();
+    delete admin;
+    admin = nullptr;
+
+    service = nullptr;
+    delete mockService;
+    mockService = nullptr;
+}
+
+void setup()
+{
+    delay(10);
+    initializeTestEnvironment();
+    UNITY_BEGIN();
+
+    RUN_TEST(test_tagRangeMatchesTypeRange);
+    RUN_TEST(test_everyModuleConfig_survivesSetSaveLoadGet);
+    RUN_TEST(test_sequentialSets_preserveEarlierWrites);
+
+    exit(UNITY_END());
+}
+
+void loop() {}

test/test_tak_config/test_main.cpp

@@ -0,0 +1,154 @@
+// TAK (ATAK) team/role value fidelity: the fields set by the app must come back verbatim.
+//
+// Companion to test_module_config, which proves every submessage's presence survives the
+// set -> save -> load -> get lifecycle but sweeps with zero-valued payloads and so cannot tell a
+// copied field from an ignored one. These tests pin the values themselves for the submessage that
+// shipped broken (firmware#11182 / Meshtastic-Android#6430: team/role reverted to "unspecified"
+// after every save).
+
+#include "MeshTypes.h" // include BEFORE TestUtil.h
+#include "TestUtil.h"
+#include <unity.h>
+
+#include "mesh/NodeDB.h"
+#include "mesh/mesh-pb-constants.h"
+#include "modules/AdminModule.h"
+#include "support/AdminModuleTestShim.h"
+#include "support/MockMeshService.h"
+#include <cstring>
+
+static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A;
+static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B;
+
+static MockMeshService *mockService = nullptr;
+static AdminModuleTestShim *admin = nullptr;
+
+// A set_module_config payload carrying the tak submessage, as the phone app sends it.
+static meshtastic_ModuleConfig makeTakModuleConfig(meshtastic_Team team, meshtastic_MemberRole role)
+{
+    meshtastic_ModuleConfig mc = meshtastic_ModuleConfig_init_zero;
+    mc.which_payload_variant = meshtastic_ModuleConfig_tak_tag;
+    mc.payload_variant.tak.team = team;
+    mc.payload_variant.tak.role = role;
+    return mc;
+}
+
+static meshtastic_MeshPacket makeGetRequest(NodeNum from)
+{
+    meshtastic_MeshPacket req = meshtastic_MeshPacket_init_zero;
+    req.from = from;
+    req.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
+    req.decoded.want_response = true;
+    return req;
+}
+
+// Pull the TAKConfig out of the get_module_config response a handler queued in myReply.
+static bool decodeTakFromReply(meshtastic_MeshPacket *reply, meshtastic_ModuleConfig_TAKConfig &out)
+{
+    meshtastic_AdminMessage am = meshtastic_AdminMessage_init_zero;
+    if (!reply || reply->which_payload_variant != meshtastic_MeshPacket_decoded_tag)
+        return false;
+    if (!pb_decode_from_bytes(reply->decoded.payload.bytes, reply->decoded.payload.size, &meshtastic_AdminMessage_msg, &am))
+        return false;
+    if (am.which_payload_variant != meshtastic_AdminMessage_get_module_config_response_tag ||
+        am.get_module_config_response.which_payload_variant != meshtastic_ModuleConfig_tak_tag)
+        return false;
+    out = am.get_module_config_response.payload_variant.tak;
+    return true;
+}
+
+void test_setStoresTeamAndRoleVerbatim(void)
+{
+    TEST_ASSERT_TRUE(admin->handleSetModuleConfig(makeTakModuleConfig(meshtastic_Team_Cyan, meshtastic_MemberRole_Medic)));
+
+    TEST_ASSERT_TRUE(moduleConfig.has_tak);
+    TEST_ASSERT_EQUAL(meshtastic_Team_Cyan, moduleConfig.tak.team);
+    TEST_ASSERT_EQUAL(meshtastic_MemberRole_Medic, moduleConfig.tak.role);
+}
+
+// Clearing back to the zero values is a real edit, not an absent field: has_tak must stay set so the
+// cleared state is what gets written, rather than the previous value surviving.
+void test_clearingToUnspecifiedIsStored(void)
+{
+    admin->handleSetModuleConfig(makeTakModuleConfig(meshtastic_Team_Red, meshtastic_MemberRole_TeamLead));
+    admin->handleSetModuleConfig(makeTakModuleConfig(meshtastic_Team_Unspecifed_Color, meshtastic_MemberRole_Unspecifed));
+
+    TEST_ASSERT_TRUE(moduleConfig.has_tak);
+    TEST_ASSERT_EQUAL(meshtastic_Team_Unspecifed_Color, moduleConfig.tak.team);
+    TEST_ASSERT_EQUAL(meshtastic_MemberRole_Unspecifed, moduleConfig.tak.role);
+}
+
+// The reported symptom end to end: values set, saved, reloaded (as a reboot would), and read back.
+void test_valuesSurviveSaveLoad(void)
+{
+    admin->handleSetModuleConfig(makeTakModuleConfig(meshtastic_Team_Magenta, meshtastic_MemberRole_Sniper));
+
+    uint8_t buf[meshtastic_LocalModuleConfig_size];
+    size_t len = pb_encode_to_bytes(buf, sizeof(buf), &meshtastic_LocalModuleConfig_msg, &moduleConfig);
+
+    moduleConfig = meshtastic_LocalModuleConfig_init_zero;
+    TEST_ASSERT_TRUE(pb_decode_from_bytes(buf, len, &meshtastic_LocalModuleConfig_msg, &moduleConfig));
+
+    TEST_ASSERT_TRUE(moduleConfig.has_tak);
+    TEST_ASSERT_EQUAL(meshtastic_Team_Magenta, moduleConfig.tak.team);
+    TEST_ASSERT_EQUAL(meshtastic_MemberRole_Sniper, moduleConfig.tak.role);
+}
+
+// Remote and local admin reads take the same branch; both must carry the stored values unredacted.
+void test_getReturnsStoredValues(void)
+{
+    admin->handleSetModuleConfig(makeTakModuleConfig(meshtastic_Team_Dark_Blue, meshtastic_MemberRole_K9));
+
+    const NodeNum requesters[] = {ADMIN_NODE, 0};
+    for (NodeNum from : requesters) {
+        meshtastic_MeshPacket req = makeGetRequest(from);
+        admin->handleGetModuleConfig(req, meshtastic_AdminMessage_ModuleConfigType_TAK_CONFIG);
+
+        meshtastic_ModuleConfig_TAKConfig tak;
+        TEST_ASSERT_TRUE_MESSAGE(decodeTakFromReply(admin->reply(), tak), "TAK_CONFIG request must produce a tak response");
+        TEST_ASSERT_EQUAL(meshtastic_Team_Dark_Blue, tak.team);
+        TEST_ASSERT_EQUAL(meshtastic_MemberRole_K9, tak.role);
+        admin->drainReply();
+    }
+}
+
+void setUp(void)
+{
+    mockService = new MockMeshService();
+    service = mockService;
+
+    admin = new AdminModuleTestShim();
+    admin->deferSaves(); // no disk write or reboot when a setter is accepted
+
+    moduleConfig = meshtastic_LocalModuleConfig_init_zero;
+    config = meshtastic_LocalConfig_init_zero;
+    myNodeInfo = meshtastic_MyNodeInfo_init_zero;
+    myNodeInfo.my_node_num = LOCAL_NODE;
+}
+
+void tearDown(void)
+{
+    admin->drainReply();
+    delete admin;
+    admin = nullptr;
+
+    service = nullptr;
+    delete mockService;
+    mockService = nullptr;
+}
+
+void setup()
+{
+    delay(10);
+    initializeTestEnvironment();
+    UNITY_BEGIN();
+
+    RUN_TEST(test_setStoresTeamAndRoleVerbatim);
+    RUN_TEST(test_clearingToUnspecifiedIsStored);
+    RUN_TEST(test_valuesSurviveSaveLoad);
+    RUN_TEST(test_getReturnsStoredValues);
+
+    exit(UNITY_END());
+}
+
+void loop() {}