diff --git a/src/mesh/RadioLibInterface.cpp b/src/mesh/RadioLibInterface.cpp index 55d5ad235f..7fae324a6a 100644 --- a/src/mesh/RadioLibInterface.cpp +++ b/src/mesh/RadioLibInterface.cpp @@ -248,8 +248,14 @@ bool RadioLibInterface::isSending() bool RadioLibInterface::cancelSending(NodeNum from, PacketId id) { auto p = txQueue.remove(from, id); - if (p) + if (p) { +#if !MESHTASTIC_EXCLUDE_BEACON + // Every path that abandons a queued packet clears its beacon target first; the restore is + // gated on no target being live, so a leftover entry would pin the radio on the beacon config. + MeshBeaconModule::clearTargetRadioSettings(p); +#endif packetPool.release(p); // free the packet we just removed + } bool result = (p != NULL); LOG_DEBUG("cancelSending id=0x%08x, removed=%d", id, result); diff --git a/src/modules/MeshBeaconModule.cpp b/src/modules/MeshBeaconModule.cpp index 9982f8d157..e880c557b4 100644 --- a/src/modules/MeshBeaconModule.cpp +++ b/src/modules/MeshBeaconModule.cpp @@ -46,6 +46,16 @@ static bool getTargetRadioSettings(const meshtastic_MeshPacket *p, meshtastic_Co return false; } +// Is a target entry still live for this packet id? Unlike sendingPacket or the radio's standby +// state, this is our own bookkeeping - it answers "has that beacon finished" without asking the radio. +static bool targetRadioSettingsLive(uint32_t id) +{ + for (const auto &entry : targetRadioSettings) + if (entry.inUse && entry.id == id) + return true; + return false; +} + // --------------------------------------------------------------------------- // MeshBeaconModule base // --------------------------------------------------------------------------- @@ -74,8 +84,13 @@ void MeshBeaconModule::setTargetRadioSettings(const meshtastic_MeshPacket *p, me if (!target && !entry.inUse) target = &entry; } - if (!target) + if (!target) { + // All slots live: another beacon's target is about to be overwritten and that packet will key + // up on whatever config is running instead of its own. + LOG_WARN("Beacon: target table full (%u slots), evicting packet 0x%08x for 0x%08x", + (unsigned)(sizeof(targetRadioSettings) / sizeof(targetRadioSettings[0])), targetRadioSettings[0].id, p->id); target = &targetRadioSettings[0]; + } target->inUse = true; target->id = p->id; target->preset = preset; @@ -151,14 +166,32 @@ meshtastic_ChannelSettings MeshBeaconModule::beaconChannelSettings(const meshtas bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_MeshPacket *p) { - // True while a beacon radio switch is in effect and still needs undoing. We track the switch - // explicitly rather than inferring it from "live config differs from the snapshot", because that - // heuristic both missed cases (a channel name/PSK swap that left preset/slot/region unchanged would - // never be restored) and fired falsely (a legitimate non-beacon channel edit would be reverted on - // the next TX). With the flag the restore fires for ANY field we changed and only when we changed - // it - including on TX-failure paths, which route through this same restore call. + // The four statics below hold the switch state explicitly. Inferring it from "live config differs + // from the snapshot" instead missed name/PSK-only swaps and fired on legitimate channel edits. static bool radioSwitched = false; + // Consecutive switches with no restore between them, so a multi-target run can be read off the log + // and the held home snapshot is attributable to a specific switch. + static uint8_t switchDepth = 0; + + // The packet that armed the outstanding switch. Every caller that abandons or completes a beacon + // clears its target settings first, so a live entry here means that TX has not finished yet. + static uint32_t switchedForId = 0; + + // Both branches end in iface->reconfigure(), whose setStandby() runs completeSending() and calls + // straight back in here. Ignore that re-entry: the outer call owns the config it is applying. + static bool applying = false; + if (applying) { + // Expected once per switch and once per restore. A burst of these means something new re-enters. + LOG_DEBUG("Beacon: ignore re-entrant reconfigure while a radio config is being applied"); + return false; + } + struct ApplyingScope { + bool &flag; + explicit ApplyingScope(bool &f) : flag(f) { flag = true; } + ~ApplyingScope() { flag = false; } + } applyingScope(applying); + meshtastic_ChannelSettings *primaryCh = &channels.getByIndex(channels.getPrimaryIndex()).settings; meshtastic_Config_LoRaConfig_ModemPreset targetPreset; uint16_t targetSlot; @@ -202,18 +235,22 @@ bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_ return false; } - // Snapshot current (non-beacon) settings so we restore to the latest config. Skip while a - // switch is already active, so a second switch before the restore can't capture the beacon - // config as the "home" we later restore to. + // Snapshot the live (non-beacon) config as "home". Skipped while a switch is already active, + // so a second switch before the restore cannot capture the beacon config instead. if (!radioSwitched) { originalModemPreset = config.lora.modem_preset; originalLoraChannel = config.lora.channel_num; originalRegion = config.lora.region; originalPrimaryChannel = *primaryCh; + switchDepth = 0; } + switchDepth++; - LOG_INFO("Beacon: switch radio for packet 0x%08x to preset=%d slot=%u region=%d", p->id, targetPreset, targetSlot, - targetRegion); + LOG_INFO("Beacon: switch #%u radio for packet 0x%08x to preset=%d slot=%u region=%d", switchDepth, p->id, targetPreset, + targetSlot, targetRegion); + if (switchDepth > 1) + LOG_WARN("Beacon: switching again with no restore between; home preset=%d slot=%u region=%d still held", + originalModemPreset, originalLoraChannel, originalRegion); config.lora.modem_preset = targetPreset; config.lora.channel_num = targetSlot; if (targetRegion != config.lora.region) @@ -222,13 +259,22 @@ bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_ channels.fixupChannel(channels.getPrimaryIndex()); p->channel = channels.getHash(channels.getPrimaryIndex()); + radioSwitched = true; // set before reconfigure(), so the flag never lags the radio it describes + switchedForId = p->id; iface->reconfigure(); - radioSwitched = true; return true; } else if ((!p || !getTargetRadioSettings(p, nullptr, nullptr)) && radioSwitched) { - LOG_INFO("Beacon: restore radio config after TX"); + // Only restore once the beacon that armed the switch has finished; a caller arriving here on + // a radio state change would put the home config back under a beacon that has not keyed up. + if (targetRadioSettingsLive(switchedForId)) { + LOG_DEBUG("Beacon: skip restore, packet 0x%08x has not finished sending", switchedForId); + return false; + } + + LOG_INFO("Beacon: restore radio config after TX, undoing %u switch(es) -> preset=%d slot=%u region=%d", switchDepth, + originalModemPreset, originalLoraChannel, originalRegion); config.lora.modem_preset = originalModemPreset; config.lora.channel_num = originalLoraChannel; config.lora.region = originalRegion; @@ -236,8 +282,10 @@ bool MeshBeaconModule::reconfigureForBeaconTX(RadioInterface *iface, meshtastic_ primaryCh->name[sizeof(primaryCh->name) - 1] = '\0'; channels.fixupChannel(channels.getPrimaryIndex()); + radioSwitched = false; // cleared before reconfigure(), so the flag never lags the radio it describes + switchDepth = 0; + switchedForId = 0; iface->reconfigure(); - radioSwitched = false; return true; } return false; diff --git a/test/test_mesh_beacon/test_main.cpp b/test/test_mesh_beacon/test_main.cpp index bb1a0abc2f..70d722c6de 100644 --- a/test/test_mesh_beacon/test_main.cpp +++ b/test/test_mesh_beacon/test_main.cpp @@ -1338,6 +1338,178 @@ static void test_broadcaster_distinctTargets_bothSent(void) TEST_ASSERT_EQUAL_UINT32_MESSAGE(2, mockRouter->sentPackets.size(), "distinct targets must each be sent"); } +// --------------------------------------------------------------------------- +// Radio switch/restore re-entrancy +// --------------------------------------------------------------------------- + +/** + * Stands in for a real driver on the restore path. RadioLibInterface::reconfigure() standbys the + * chip, setStandby() calls completeSending(), and completeSending() calls back into + * reconfigureForBeaconTX(iface, nullptr) - so reconfigure() re-entering is the normal case, not an + * exotic one. Bounded, so a regression fails an assertion instead of overflowing the stack. + */ +class ReentrantRadioInterface : public RadioInterface +{ + public: + static constexpr int kReentryLimit = 16; + int reconfigureCalls = 0; + bool reenterOnReconfigure = false; + + ErrorCode send(meshtastic_MeshPacket *p) override + { + packetPool.release(p); + return ERRNO_OK; + } + + uint32_t getPacketTime(uint32_t totalPacketLen, bool received = false) override + { + (void)totalPacketLen; + (void)received; + return 0; + } + + bool reconfigure() override + { + reconfigureCalls++; + if (reenterOnReconfigure && reconfigureCalls < kReentryLimit) + MeshBeaconModule::reconfigureForBeaconTX(this, nullptr); + return true; + } +}; + +/** + * The restore must clear its guard before reconfiguring, or completeSending() re-enters the restore + * branch and it reconfigures the radio once per level until the stack runs out. Seen in the field as + * a run of "Beacon: restore radio config after TX" with a full applyModemConfig() between each. + */ +static void test_beaconRestore_isNotReenteredByCompleteSending(void) +{ + resetConfig(); + static const uint8_t homePsk[16] = {0xAA, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}; + installTestPrimaryChannel("Home", homePsk, sizeof(homePsk)); + + ReentrantRadioInterface radio; + meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero; + pkt.id = 0x5EED0001; + MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + + // Switch to the beacon config. Not the case under test, so leave re-entry off. + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &pkt), "beacon switch should have applied"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset, + "switch must leave the radio on the beacon preset"); + + // Now restore, with reconfigure() re-entering exactly as completeSending() does. + MeshBeaconModule::clearTargetRadioSettings(&pkt); + radio.reconfigureCalls = 0; + radio.reenterOnReconfigure = true; + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore should have applied"); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, radio.reconfigureCalls, "restore must reconfigure the radio exactly once"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset, + "restore must put the home preset back"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name, + "restore must put the home channel back"); +} + +/** + * A second switch before the restore has run must survive the same re-entry. completeSending() calls + * in with a null packet, which reads as "restore" - so without the guard it would undo the switch that + * is still being applied, leaving the beacon to transmit on the home channel instead of its target. + */ +static void test_beaconSwitch_isNotUndoneByCompleteSending(void) +{ + resetConfig(); + static const uint8_t homePsk[16] = {0xAA, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}; + installTestPrimaryChannel("Home", homePsk, sizeof(homePsk)); + + ReentrantRadioInterface radio; + meshtastic_MeshPacket first = meshtastic_MeshPacket_init_zero; + first.id = 0x5EED0002; + MeshBeaconModule::setTargetRadioSettings(&first, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &first), "first switch should have applied"); + + // Second switch with the restore still outstanding, and reconfigure() re-entering. + meshtastic_MeshPacket second = meshtastic_MeshPacket_init_zero; + second.id = 0x5EED0003; + MeshBeaconModule::setTargetRadioSettings(&second, meshtastic_Config_LoRaConfig_ModemPreset_SHORT_FAST, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + radio.reconfigureCalls = 0; + radio.reenterOnReconfigure = true; + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &second), "second switch should have applied"); + + TEST_ASSERT_EQUAL_INT_MESSAGE(1, radio.reconfigureCalls, "second switch must reconfigure the radio exactly once"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_SHORT_FAST, config.lora.modem_preset, + "second switch must not be undone mid-flight"); + + // The home config must still be recoverable afterwards - the snapshot survives a second switch. + radio.reenterOnReconfigure = false; + MeshBeaconModule::clearTargetRadioSettings(&first); + MeshBeaconModule::clearTargetRadioSettings(&second); + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore should have applied"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset, + "restore must return to the home preset, not the first beacon target"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name, + "restore must return to the home channel"); +} + +/** A restore with nothing switched must do nothing at all - the guard is what makes re-entry safe. */ +static void test_beaconRestore_withoutSwitch_isNoOp(void) +{ + resetConfig(); + ReentrantRadioInterface radio; + + // reconfigureForBeaconTX() keeps its switched/not-switched state in a function-local static, so an + // earlier test that aborted mid-way can leave a switch outstanding. Drain it before asserting. + MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr); + + radio.reconfigureCalls = 0; + radio.reenterOnReconfigure = true; + TEST_ASSERT_FALSE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore without a switch is a no-op"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "no-op restore must not touch the radio"); +} + +/** + * The restore is driven by "our beacon finished", not by "the radio changed state". completeSending() + * clears a packet's target settings before restoring, so a caller that arrives without that - the + * pre-TX channel scan standbys the radio, and setStandby() calls completeSending() - must be refused. + * Otherwise the home config goes back under a beacon that has not keyed up yet, and it transmits on + * the wrong preset with the beacon channel hash already stamped on it. + */ +static void test_beaconRestore_deferredUntilPacketCompletes(void) +{ + resetConfig(); + static const uint8_t homePsk[16] = {0xAA, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}; + installTestPrimaryChannel("Home", homePsk, sizeof(homePsk)); + + ReentrantRadioInterface radio; + meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero; + pkt.id = 0x5EED0004; + MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, &pkt), "beacon switch should have applied"); + + // The packet has not been sent yet, so its target settings are still live. + radio.reconfigureCalls = 0; + TEST_ASSERT_FALSE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), + "restore must be refused while the beacon is still outstanding"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "a refused restore must not touch the radio"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset, + "the beacon preset must still be in place when the packet keys up"); + + // completeSending() clears the target settings first; only then is the restore ours to make. + MeshBeaconModule::clearTargetRadioSettings(&pkt); + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::reconfigureForBeaconTX(&radio, nullptr), "restore should have applied"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset, + "restore must put the home preset back"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name, + "restore must put the home channel back"); +} + } // namespace // =========================================================================== @@ -1462,6 +1634,13 @@ BEACON_TEST_ENTRY void setup() RUN_TEST(test_broadcaster_duplicateTargets_dedupedToOnePacket); RUN_TEST(test_broadcaster_distinctTargets_bothSent); + printf("\n=== Radio switch/restore re-entrancy ===\n"); + + RUN_TEST(test_beaconRestore_isNotReenteredByCompleteSending); + RUN_TEST(test_beaconSwitch_isNotUndoneByCompleteSending); + RUN_TEST(test_beaconRestore_withoutSwitch_isNoOp); + RUN_TEST(test_beaconRestore_deferredUntilPacketCompletes); + exit(UNITY_END()); }