From 7e11bde8c8d14bda624ea9e57eeffd6978fa7822 Mon Sep 17 00:00:00 2001 From: Tom <116762865+NomDeTom@users.noreply.github.com> Date: Tue, 25 Aug 2026 19:29:34 +0000 Subject: [PATCH] fix(beacon): repair the MeshBeacon radio switch/restore regression from #11573 (#11596) * fix(radio): put the beacon restore back inside completeSending's if (p) Reverts the RadioLibInterface and RadioInterface changes from #11573 (ac330e6a6). Hoisting MeshBeaconModule::reconfigureForBeaconTX() out of the if (p) block changed its meaning from "a send completed" to "the radio went to standby, for any reason" - and every driver's setStandby() calls completeSending() unconditionally: on the pre-TX LBT scan, on startReceive(), and inside reconfigure(). Two shipping faults followed, both confirmed on hardware the next day. Every beacon transmitted on the wrong preset. isChannelActive() standbys the radio immediately before each transmit, so the restore ran between the switch and the key-up. The packet went out carrying the beacon channel hash with home modem settings - inaudible to listeners on the target preset, an unknown hash to listeners on the home one. Inert in both directions. And unbounded recursion: the restore calls iface->reconfigure(), which standbys, which calls completeSending(), which restores again, each level running a full applyModemConfig(). It terminated in a HardFault and a silent reboot (Reset reason 0x4 on nRF52, no panic output). The crash masked the misdirection - the node died before Started Tx, so the wrong preset was invisible until the recursion was fixed. completeSending() clears sendingPacket at the top, so any nested call sees p == NULL. The if (p) block was an accidental re-entrancy guard, and nothing named it as such; removing it created both faults at once. Name it now. This also reverts the beginSending() failure return that motivated the move, and the startSend() scaffolding built to reach the restore on that path. The payload bounds check it replaced is reinstated in the next commit, at a point where refusing a packet is already a supported outcome. * fix(radio): bound the payload at the radio queue, not mid-transmit #11573 replaced beginSending()'s assert with a runtime check that logged, released the packet and returned 0. beginSending() had never returned 0 before, so startSend() gained a failure path it had to unwind - and the release moved ownership of the packet out of the caller that held it. That new return value is what made hoisting the beacon restore look necessary. The check itself is worth keeping. MeshPacket.encrypted has a nanopb maximum of 256 bytes against a 240-byte radio buffer, and beginSending() is on the path for relayed frames and phone-sourced packets, neither under our control. Asserts are commonly compiled out in release builds, so what shipped was an unchecked 256-into-240 memcpy driven by remote input. Move it to Router::send(), immediately before iface->send(p) - the single funnel for every over-the-air transmit. Refusing a packet there is already a supported outcome: it returns TOO_LARGE, which is what perhapsEncode() already returns for the same condition on the decoded path, and releases or NAKs exactly as the duty-cycle limit above it does. Nothing radio-side has happened at that point, so there is no half-started transmit to tear back down. perhapsEncode()'s existing check does not cover this case: relayed and phone-sourced frames arrive already encrypted and never reach it. beginSending() keeps a last line of defence, but clamps rather than failing, so it stays a call that always succeeds. Adds MAX_RADIO_PAYLOAD_LEN so both sites name the same number instead of recomputing it. Nothing about a beacon can trigger any of this - broadcast_message is admin-truncated to 100 bytes, the whole MeshBeacon protobuf tops out at 180, and observed beacons run to 106 - which is why this is separated from the beacon changes rather than carried with them. Tests: Router::send() refuses an oversized payload and still sends one that exactly fills the buffer; beginSending() clamps instead of rejecting, and leaves ordinary traffic whole. * fix(beacon): guard the radio switch/restore against re-entry and early restore Two checks in reconfigureForBeaconTX(), both independent of radio state, so the switch/restore state machine no longer rests on sendingPacket's lifetime - which is exactly the implicit coupling that let #11573 through. A re-entrancy guard. Both branches end in iface->reconfigure(), whose setStandby() runs completeSending(), which calls straight back in here. While one call is applying a config, a nested call returns false and leaves it alone. This covers the switch branch too, which had the same exposure with a quieter symptom: a second switch before the restore would take the re-entrant call as a restore and undo the switch still being applied, sending the beacon on the home channel instead of its target. A restore gate. The restore now waits for the packet that armed the switch to actually finish, tracked by id against our own target table rather than by asking the radio. Every caller that completes or abandons a beacon clears that packet's target settings first, so a live entry means the TX has not happened yet. cancelSending() now clears too, which is what keeps a cancelled beacon from pinning the radio on the beacon config. Together these make explicit the invariant completeSending()'s if (p) block was carrying by accident: a future hoist of that call gets a logged no-op instead of a crash and a misdirected beacon. Also sets radioSwitched before reconfigure() rather than after, in both branches, so the flag never describes a radio state that is not yet true. Diagnostics, because every step of this dance was previously silent about its own state. Count consecutive switches with no restore between them and log the depth on both sides, so a change-change-change-restore run reads off the log; switch #2 onwards prints the held home snapshot, which is the value that has to survive a second switch. The restore names the config it is restoring to, so a stale snapshot is visible directly. The re-entrancy guard logs when it fires - expected exactly twice per beacon, so a burst means something new is re-entering rather than a silent reboot. And setTargetRadioSettings() now warns on the slot eviction that previously left a packet to key up on whatever config was running - no crash, no log, wrong channel. Reachable only with beacon broadcast enabled (the default flags are LISTEN_ENABLED | LEGACY_SPLIT, so broadcast is off) and a target differing from the running config; an identical target takes the early return and never switches. Tests: three re-entrancy cases against a RadioInterface whose reconfigure() re-enters exactly as completeSending() does - bounded, so a regression fails an assertion instead of overflowing the stack and taking the runner with it - plus a restore that must defer until the beacon it switched for completes. * fix(beacon,radio): address review findings on #11596 Payload ceiling was one byte too generous. RadioBuffer::payload is 240 bytes because the buffer reserves MAX_LORA_PAYLOAD_LEN + 1, but the PHY caps a whole frame at 255 and beginSending() adds a 16-byte header - so a 240-byte payload produced a 256-byte frame. Define the ceiling as MAX_LORA_PAYLOAD_LEN - sizeof(PacketHeader), matching what perhapsEncode() already enforces, with a static_assert that it still fits the buffer. Target-table eviction could unblock the restore gate. With every slot live, setTargetRadioSettings() overwrote slot 0 - and if that slot held the packet the outstanding switch is gated on, the restore came unblocked and put the home config back under a beacon that had not keyed up. Skip that entry when choosing a victim, and refuse the target outright if every slot is in flight. Needs radioSwitched/switchedForId at file scope so the setter can see them. Restore on every abandon path, not just the clear. cancelSending() dropped a queued packet's target without restoring, so a beacon pre-switched by onNotify() and then cancelled left the radio receiving on the beacon config; removePendingTXPacket() did neither. Both now route through abandonBeaconTarget(), as does startSend()'s tx-disabled branch. The restore gate makes it a no-op when the abandoned packet is not the one we switched for. No NAK on the oversize drop. p->channel is a wire hash by that point, not an index, and Channels::getIndexByHash() is declared but never defined. Only already-encrypted ingress can reach the gate anyway - perhapsEncode() bounds everything it encodes - and those carry no index to answer on. Release and log. Tests clear sendingPacket before releasing their packet, and assert against the payload ceiling rather than the buffer size. * fix(beacon): route the invalid-target drop through abandonBeaconTarget onNotify()'s invalid-config drop was the one packet-abandonment path still clearing the target directly instead of going through abandonBeaconTarget(), so a packet that armed the radio switch and then failed validation would be released with the radio left on the beacon config and nothing to restore it. The helper's restore gate (targetRadioSettingsLive(switchedForId)) makes the call a no-op for any packet that did not arm the switch, so this closes the gap without risking a premature restore. Also trims the switch-state comment to the two-line limit. * fix(radio): take the abandoned packet as a pointer to const cppcheck's constParameterPointer failed the check matrix on every board: abandonBeaconTarget() only forwards the packet to clearTargetRadioSettings(), which already takes a const pointer, so the parameter should be const too. * refactor(radio): drive the beacon radio switch through TX hooks RadioLibInterface named MeshBeaconModule at six call sites behind MESHTASTIC_EXCLUDE_BEACON guards, so the driver carried per-packet beacon state: when to switch preset, when a target config was invalid mid-transmit, and when not to listen on a busy channel. Review on #11596 asked for the module dependency to come out. RadioTxHook is what the driver knows instead - beforeTransmit() returning send/defer/drop, holdsRadio(), packetReleased() - on a self-registering intrusive list, so nothing is allocated and a build without the beacon module registers nothing and every call is a no-op. The four abandon paths (cancel, remove-pending, TX disabled, completeSending) collapse onto one packetReleased(), and the tri-state means the driver no longer has to know why a packet wanted a re-delay or a drop. MeshBeaconTxHook wraps the existing statics; the switch/restore logic, its re-entrancy guard and its restore gate are untouched. It is created in Modules.cpp inside the existing exclusion guard, so MESHTASTIC_EXCLUDE_BEACON now works by nothing registering rather than by #ifdefs in the driver. Behaviour is unchanged. The invalid-config LOG_DEBUG moves into the module and the driver logs a generic refusal. Four tests cover the send/defer/drop mapping and that an empty hook list is a no-op; native:test_mesh_beacon is 59/59. Also notes in sendBeaconPacket that beacons uplink to MQTT on the primary slot's uplink_enabled, and that the topic follows the beacon channel under the crypto-override swap - both intentional. * fix(beacon): restore the home config for a packet that jumps the queue The restore gate added in 9cb7b96c9 refused to put the home config back while the beacon that armed the switch was still live. That is right for a release - completeSending() runs on every setStandby(), and restoring there would undo the switch before the beacon had keyed up - but it also caught the case where the driver is asking about a different packet it is about to transmit. MeshPacketQueue::enqueue() inserts by priority (std::upper_bound over CompareMeshPacketFunc), so an ACK or routing packet queued during the beacon's deferred transmit delay lands ahead of it. beforeTransmit() then saw an untagged packet, found the beacon still queued, skipped the restore and returned PRETX_SEND - and the packet transmitted on the beacon's preset, slot and region. It was encrypted and hashed for the home channel, so no receiver on either preset could use it. Apply the gate only to a null p. A non-null untagged packet is the driver about to key up, which always restores; the restore returns PRETX_DEFER, so the driver re-runs the delay and the channel scan on the config it will actually transmit on. beforeTransmit() is the only caller that passes a non-null untagged packet, so nothing else changes. Found by CodeRabbit on #11596. native:test_mesh_beacon 60/60, including a regression test for the queue transition; the four re-entrancy tests still cover the null-p gate. --------- Co-authored-by: Ben Meadors --- src/mesh/RadioInterface.cpp | 18 +- src/mesh/RadioInterface.h | 5 + src/mesh/RadioLibInterface.cpp | 74 ++--- src/mesh/RadioTxHook.cpp | 43 +++ src/mesh/RadioTxHook.h | 50 ++++ src/mesh/Router.cpp | 9 + src/modules/MeshBeaconModule.cpp | 122 +++++++-- src/modules/MeshBeaconModule.h | 19 +- src/modules/Modules.cpp | 1 + test/test_mesh_beacon/test_main.cpp | 343 ++++++++++++++++++++++++ test/test_nexthop_routing/test_main.cpp | 20 ++ test/test_radio/test_main.cpp | 41 ++- 12 files changed, 656 insertions(+), 89 deletions(-) create mode 100644 src/mesh/RadioTxHook.cpp create mode 100644 src/mesh/RadioTxHook.h diff --git a/src/mesh/RadioInterface.cpp b/src/mesh/RadioInterface.cpp index c0212f4ded..7da9325d46 100644 --- a/src/mesh/RadioInterface.cpp +++ b/src/mesh/RadioInterface.cpp @@ -1518,15 +1518,17 @@ size_t RadioInterface::beginSending(meshtastic_MeshPacket *p) // if the sender nodenum is zero, that means uninitialized assert(radioBuffer.header.from); - // Runtime packet payload size bounds check against radioBuffer to prevent overflow in memcpy() - if (static_cast(p->encrypted.size) > sizeof(radioBuffer.payload)) { - LOG_ERROR("Packet payload size %u exceeds radioBuffer capacity %u", static_cast(p->encrypted.size), - static_cast(sizeof(radioBuffer.payload))); - packetPool.release(p); - return 0; + + // Oversize is rejected at the radio queue in Router::send(); clamp rather than fail here so this + // stays a call that always succeeds, with no failure return for startSend() to unwind. + size_t payloadLen = p->encrypted.size; + if (payloadLen > MAX_RADIO_PAYLOAD_LEN) { + LOG_ERROR("Payload %u exceeds radioBuffer capacity %u, truncate", (unsigned)payloadLen, (unsigned)MAX_RADIO_PAYLOAD_LEN); + payloadLen = MAX_RADIO_PAYLOAD_LEN; } - memcpy(radioBuffer.payload, p->encrypted.bytes, p->encrypted.size); + + memcpy(radioBuffer.payload, p->encrypted.bytes, payloadLen); sendingPacket = p; - return p->encrypted.size + sizeof(PacketHeader); + return payloadLen + sizeof(PacketHeader); } diff --git a/src/mesh/RadioInterface.h b/src/mesh/RadioInterface.h index eb9315ac12..e76b977bf4 100644 --- a/src/mesh/RadioInterface.h +++ b/src/mesh/RadioInterface.h @@ -67,6 +67,11 @@ typedef struct { } RadioBuffer; +/// On-air ceiling for MeshPacket.encrypted. RadioBuffer holds one byte more, but the PHY caps a whole +/// frame at MAX_LORA_PAYLOAD_LEN, so the header comes out of the same budget (matches perhapsEncode). +constexpr size_t MAX_RADIO_PAYLOAD_LEN = MAX_LORA_PAYLOAD_LEN - sizeof(PacketHeader); +static_assert(MAX_RADIO_PAYLOAD_LEN < sizeof(RadioBuffer::payload), "payload ceiling must fit the buffer"); + /** * Basic operations all radio chipsets must implement. * diff --git a/src/mesh/RadioLibInterface.cpp b/src/mesh/RadioLibInterface.cpp index 3018a34dd3..da0f58d6ec 100644 --- a/src/mesh/RadioLibInterface.cpp +++ b/src/mesh/RadioLibInterface.cpp @@ -2,6 +2,7 @@ #include "MeshTypes.h" #include "NodeDB.h" #include "PowerMon.h" +#include "RadioTxHook.h" #include "SPILock.h" #include "Throttle.h" #include "UptimeClock.h" @@ -9,9 +10,6 @@ #include "error.h" #include "main.h" #include "mesh-pb-constants.h" -#if !MESHTASTIC_EXCLUDE_BEACON -#include "modules/MeshBeaconModule.h" -#endif #include #include @@ -248,8 +246,10 @@ bool RadioLibInterface::isSending() bool RadioLibInterface::cancelSending(NodeNum from, PacketId id) { auto p = txQueue.remove(from, id); - if (p) + if (p) { + RadioTxHooks::packetReleased(this, p); packetPool.release(p); // free the packet we just removed + } bool result = (p != NULL); LOG_DEBUG("cancelSending id=0x%08x, removed=%d", id, result); @@ -408,14 +408,10 @@ void RadioLibInterface::onNotify(uint32_t notification) switch (notification) { case ISR_TX: handleTransmitInterrupt(); // completeSending() already restored the radio to the home config -#if !MESHTASTIC_EXCLUDE_BEACON - // Pre-switch the radio to the NEXT queued packet's beacon config (no-op for normal traffic). - // Not required for correctness - TRANSMIT_DELAY_COMPLETED would switch before CAD anyway - but - // doing it here lets the next beacon skip the switch-only delay cycle and, more importantly, - // keeps the post-TX listen window (and the CAD/LBT that follows) on the channel we're about to - // transmit on. Only engages when the next packet is itself a beacon - exactly when we want it. - MeshBeaconModule::reconfigureForBeaconTX(this, txQueue.getFront()); -#endif + // Let the hooks pre-stage the radio for the NEXT queued packet. Not required for correctness - + // TRANSMIT_DELAY_COMPLETED asks again before the scan, which is where the answer is acted on - + // but it keeps the post-TX listen window on the channel we are about to transmit on. + (void)RadioTxHooks::beforeTransmit(this, txQueue.getFront()); startReceive(); setTransmitDelay(); break; @@ -443,25 +439,20 @@ void RadioLibInterface::onNotify(uint32_t notification) if (txp->tx_after && !Throttle::deadlinePassedAt(now, txp->tx_after)) { // There's still some delay pending on this packet, so resume waiting for it to elapse notifyLater(txp->tx_after - now, TRANSMIT_DELAY_COMPLETED, txTimerOverwrite); -#if !MESHTASTIC_EXCLUDE_BEACON - } else if (MeshBeaconModule::beaconTxConfigInvalid(txp)) { - // The beacon's target radio config is invalid (bad preset/region, or an - // unlicensed node keying up on a ham-only region). Drop the packet - never - // transmit it on the current (home) config - and move on to the next queued packet. - LOG_DEBUG("Beacon: invalid TX radio config, drop packet 0x%08x", txp->id); + } else if (const RadioTxHook::PreTxAction action = RadioTxHooks::beforeTransmit(this, txp); + action == RadioTxHook::PRETX_DROP) { + // A module refuses this packet on the radio config we are holding: drop it rather + // than transmit it, and move on to the next queued packet. meshtastic_MeshPacket *bad = txQueue.dequeue(); - MeshBeaconModule::clearTargetRadioSettings(bad); + LOG_DEBUG("Drop Tx packet 0x%08x, refused before transmit", bad->id); + RadioTxHooks::packetReleased(this, bad); packetPool.release(bad); setTransmitDelay(); - } else if (MeshBeaconModule::reconfigureForBeaconTX(this, txp)) { - setTransmitDelay(); -#endif + } else if (action == RadioTxHook::PRETX_DEFER) { + setTransmitDelay(); // the radio config moved, so re-run the delay and scan on it } else { if (isChannelActive()) { // check if there is currently a LoRa packet on the channel -#if !MESHTASTIC_EXCLUDE_BEACON - if (!MeshBeaconModule::hasTargetRadioSettings(txp)) -#endif - { + if (!RadioTxHooks::holdsRadio(txp)) { startReceive(); // try receiving this packet, afterwards we'll be trying to transmit again } setTransmitDelay(); @@ -561,6 +552,7 @@ bool RadioLibInterface::removePendingTXPacket(NodeNum from, PacketId id, uint32_ meshtastic_MeshPacket *p = txQueue.remove(from, id, true, true, hop_limit_lt); if (p) { LOG_DEBUG("Drop pending-TX packet 0x%08x, hop limit %d", p->id, p->hop_limit); + RadioTxHooks::packetReleased(this, p); packetPool.release(p); return true; } @@ -595,15 +587,13 @@ void RadioLibInterface::completeSending() if (!isFromUs(p)) txRelay++; printPacket("Completed sending", p); -#if !MESHTASTIC_EXCLUDE_BEACON - MeshBeaconModule::clearTargetRadioSettings(p); -#endif + // Keep this inside `if (p)`: completeSending() also runs on every setStandby(), where a hook + // undoing its own pre-TX switch would recurse back through reconfigure(). + RadioTxHooks::packetReleased(this, p); + // We are done sending that packet, release it packetPool.release(p); } -#if !MESHTASTIC_EXCLUDE_BEACON - MeshBeaconModule::reconfigureForBeaconTX(this, nullptr); -#endif } void RadioLibInterface::handleReceiveInterrupt() @@ -771,30 +761,14 @@ bool RadioLibInterface::startSend(meshtastic_MeshPacket *txp) channel scan and actual transmit as low as possible to avoid collisions. */ if (disabled || !config.lora.tx_enabled) { LOG_WARN("Drop Tx packet: LoRa Tx disabled"); -#if !MESHTASTIC_EXCLUDE_BEACON - // This packet may have already triggered a beacon radio switch in TRANSMIT_DELAY_COMPLETED; - // since it never reaches completeSending() here, restore the radio so it isn't left on the - // beacon config (which would also break RX on the home channel). - MeshBeaconModule::clearTargetRadioSettings(txp); - MeshBeaconModule::reconfigureForBeaconTX(this, nullptr); -#endif + // Never reaches completeSending(), so any per-packet radio state has to be released here. + RadioTxHooks::packetReleased(this, txp); packetPool.release(txp); return false; } else { configHardwareForSend(); // must be after setStandby -#if !MESHTASTIC_EXCLUDE_BEACON - MeshBeaconModule::clearTargetRadioSettings(txp); -#endif size_t numbytes = beginSending(txp); - if (numbytes == 0) { - if (!sendingPacket) { - completeSending(); - powerMon->clearState(meshtastic_PowerMon_State_Lora_TXOn); - startReceive(); - } - return false; - } int res = iface->startTransmit((uint8_t *)&radioBuffer, numbytes); if (res != RADIOLIB_ERR_NONE) { diff --git a/src/mesh/RadioTxHook.cpp b/src/mesh/RadioTxHook.cpp new file mode 100644 index 0000000000..f1fc9e8aca --- /dev/null +++ b/src/mesh/RadioTxHook.cpp @@ -0,0 +1,43 @@ +#include "RadioTxHook.h" + +RadioTxHook *RadioTxHook::hookList = nullptr; + +RadioTxHook::RadioTxHook() +{ + nextHook = hookList; + hookList = this; +} + +RadioTxHook::~RadioTxHook() +{ + for (RadioTxHook **slot = &hookList; *slot; slot = &(*slot)->nextHook) { + if (*slot == this) { + *slot = nextHook; + break; + } + } +} + +RadioTxHook::PreTxAction RadioTxHooks::beforeTransmit(RadioInterface *iface, meshtastic_MeshPacket *p) +{ + for (RadioTxHook *h = RadioTxHook::hookList; h; h = h->nextHook) { + const RadioTxHook::PreTxAction action = h->beforeTransmit(iface, p); + if (action != RadioTxHook::PRETX_SEND) + return action; + } + return RadioTxHook::PRETX_SEND; +} + +bool RadioTxHooks::holdsRadio(const meshtastic_MeshPacket *p) +{ + for (RadioTxHook *h = RadioTxHook::hookList; h; h = h->nextHook) + if (h->holdsRadio(p)) + return true; + return false; +} + +void RadioTxHooks::packetReleased(RadioInterface *iface, const meshtastic_MeshPacket *p) +{ + for (RadioTxHook *h = RadioTxHook::hookList; h; h = h->nextHook) + h->packetReleased(iface, p); +} diff --git a/src/mesh/RadioTxHook.h b/src/mesh/RadioTxHook.h new file mode 100644 index 0000000000..3681d59d61 --- /dev/null +++ b/src/mesh/RadioTxHook.h @@ -0,0 +1,50 @@ +#pragma once + +#include "MeshTypes.h" + +class RadioInterface; + +/** + * A module's hook into the radio driver's per-packet TX lifecycle. + * + * The driver knows this interface and nothing about who implements it: a module needing per-packet + * radio state (MeshBeacon's preset switch) subclasses this, and one instance registers itself for + * the life of the program. + */ +class RadioTxHook +{ + friend class RadioTxHooks; + + RadioTxHook *nextHook = nullptr; + static RadioTxHook *hookList; + + public: + /// What the driver should do with the packet at the head of the TX queue. + enum PreTxAction { + PRETX_SEND, ///< nothing pending, transmit as usual + PRETX_DEFER, ///< the radio config changed, re-run the transmit delay before sending + PRETX_DROP ///< this packet must not go out on the current radio config + }; + + RadioTxHook(); + virtual ~RadioTxHook(); + + /// The driver is about to transmit p (NULL when the queue is empty): set up any state it needs. + virtual PreTxAction beforeTransmit(RadioInterface *iface, meshtastic_MeshPacket *p) { return PRETX_SEND; } + + /// True while p needs the radio left on its own config, so the driver must not listen instead. + virtual bool holdsRadio(const meshtastic_MeshPacket *p) { return false; } + + /// The driver is done with p - sent, cancelled or dropped. Release anything held for it. + virtual void packetReleased(RadioInterface *iface, const meshtastic_MeshPacket *p) {} +}; + +/// Driver-side fan-out over the registered hooks; every call is a no-op when none are registered. +class RadioTxHooks +{ + public: + /// The first hook not returning PRETX_SEND decides, and the rest are not consulted. + static RadioTxHook::PreTxAction beforeTransmit(RadioInterface *iface, meshtastic_MeshPacket *p); + static bool holdsRadio(const meshtastic_MeshPacket *p); + static void packetReleased(RadioInterface *iface, const meshtastic_MeshPacket *p); +}; diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 34f477438c..fbb0f9bc91 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -602,6 +602,15 @@ ErrorCode Router::send(meshtastic_MeshPacket *p) } #endif + // Only already-encrypted frames (relayed, phone-sourced) reach here oversized; perhapsEncode() + // bounds everything it encodes. No NAK: p->channel is a wire hash by now, not an index. + if (p->encrypted.size > MAX_RADIO_PAYLOAD_LEN) { + LOG_WARN("Drop 0x%08x: payload %u exceeds radio capacity %u", p->id, (unsigned)p->encrypted.size, + (unsigned)MAX_RADIO_PAYLOAD_LEN); + packetPool.release(p); + return meshtastic_Routing_Error_TOO_LARGE; + } + assert(iface); // This should have been detected already in sendLocal (or we just received a packet from outside) return iface->send(p); } diff --git a/src/modules/MeshBeaconModule.cpp b/src/modules/MeshBeaconModule.cpp index 9982f8d157..1976f054d7 100644 --- a/src/modules/MeshBeaconModule.cpp +++ b/src/modules/MeshBeaconModule.cpp @@ -19,6 +19,11 @@ meshtastic_ChannelSettings MeshBeaconModule::originalPrimaryChannel; static MeshBeaconModule_TargetRadioSettings targetRadioSettings[8]; +// Explicit switch state, not inferred: "live config differs from the snapshot" missed name/PSK-only +// swaps and fired on legitimate channel edits. +static bool radioSwitched = false; +static uint32_t switchedForId = 0; + static bool getTargetRadioSettings(const meshtastic_MeshPacket *p, meshtastic_Config_LoRaConfig_ModemPreset *preset, uint16_t *slot, bool *legacyHopOverride = nullptr, meshtastic_Config_LoRaConfig_RegionCode *region = nullptr, bool *has_channel = nullptr, @@ -46,6 +51,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 +89,22 @@ void MeshBeaconModule::setTargetRadioSettings(const meshtastic_MeshPacket *p, me if (!target && !entry.inUse) target = &entry; } - if (!target) - target = &targetRadioSettings[0]; + if (!target) { + // Table full. Never evict the entry the outstanding switch is gated on: dropping it would + // unblock the restore and put the home config back under a beacon that has not keyed up. + for (auto &entry : targetRadioSettings) { + if (!radioSwitched || entry.id != switchedForId) { + target = &entry; + break; + } + } + if (!target) { + LOG_WARN("Beacon: target table full and every slot is in flight, drop target for 0x%08x", p->id); + return; + } + LOG_WARN("Beacon: target table full (%u slots), evicting packet 0x%08x for 0x%08x", + (unsigned)(sizeof(targetRadioSettings) / sizeof(targetRadioSettings[0])), target->id, p->id); + } target->inUse = true; target->id = p->id; target->preset = preset; @@ -151,13 +180,23 @@ 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. - 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; + + // 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; @@ -202,18 +241,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 +265,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"); + // Null p is "release if nothing holds it": hold off until the arming beacon has finished. A + // non-null untagged p is the driver about to transmit it, so that always restores. + if (!p && 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,13 +288,47 @@ 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; } +// --------------------------------------------------------------------------- +// MeshBeaconTxHook +// --------------------------------------------------------------------------- + +MeshBeaconTxHook *meshBeaconTxHook; + +RadioTxHook::PreTxAction MeshBeaconTxHook::beforeTransmit(RadioInterface *iface, meshtastic_MeshPacket *p) +{ + // Invalid target config (bad preset/region, or an unlicensed node keying up on a ham-only + // region): the packet must never fall through onto the current (home) config, so drop it. + if (MeshBeaconModule::beaconTxConfigInvalid(p)) { + LOG_DEBUG("Beacon: invalid TX radio config, drop packet 0x%08x", p->id); + return PRETX_DROP; + } + // A switch leaves the radio on a channel we have not scanned yet, so the driver owes us a + // fresh transmit delay before it keys up. + return MeshBeaconModule::reconfigureForBeaconTX(iface, p) ? PRETX_DEFER : PRETX_SEND; +} + +bool MeshBeaconTxHook::holdsRadio(const meshtastic_MeshPacket *p) +{ + return MeshBeaconModule::hasTargetRadioSettings(p); +} + +void MeshBeaconTxHook::packetReleased(RadioInterface *iface, const meshtastic_MeshPacket *p) +{ + // Clear first: the restore is gated on the switching packet still being live, so dropping our + // claim before asking is what lets the home config come back. + MeshBeaconModule::clearTargetRadioSettings(p); + MeshBeaconModule::reconfigureForBeaconTX(iface, nullptr); +} + // --------------------------------------------------------------------------- // MeshBeaconBroadcastModule // --------------------------------------------------------------------------- @@ -283,6 +369,8 @@ void MeshBeaconBroadcastModule::rebuildCache() void MeshBeaconBroadcastModule::sendBeaconPacket(meshtastic_MeshPacket *p, meshtastic_Config_LoRaConfig_ModemPreset targetPreset, bool has_channel, const meshtastic_ChannelSettings *overrideChannel) { + // Beacons uplink to MQTT like any other primary-slot packet - Router::send() publishes on slot 0's + // uplink_enabled, and under the swap below the topic is the beacon channel's. Both intentional. const bool cryptoOverride = has_channel && overrideChannel && (overrideChannel->name[0] != '\0' || overrideChannel->psk.size > 0); if (!cryptoOverride) { diff --git a/src/modules/MeshBeaconModule.h b/src/modules/MeshBeaconModule.h index e9faeea4cf..bed9e882c6 100644 --- a/src/modules/MeshBeaconModule.h +++ b/src/modules/MeshBeaconModule.h @@ -3,6 +3,7 @@ #include "Observer.h" #include "ProtobufModule.h" #include "RadioInterface.h" +#include "RadioTxHook.h" #include "concurrency/OSThread.h" #include "mesh/generated/meshtastic/mesh_beacon.pb.h" #include "mesh/generated/meshtastic/module_config.pb.h" @@ -55,13 +56,13 @@ class MeshBeaconModule /** * Returns true if the sidecar table contains an entry for this packet's ID. - * Used by RadioLibInterface to gate the channel-active check. + * Used via MeshBeaconTxHook to keep the driver from listening while a beacon is queued. */ static bool hasTargetRadioSettings(const meshtastic_MeshPacket *p); /** * Remove the sidecar entry for this packet after it has been sent. - * Called from RadioLibInterface::completeSending(). + * Called via MeshBeaconTxHook once the driver is done with the packet. */ static void clearTargetRadioSettings(const meshtastic_MeshPacket *p); @@ -90,6 +91,20 @@ class MeshBeaconModule static meshtastic_ChannelSettings originalPrimaryChannel; }; +/** + * Carries the beacon's radio switching into the radio driver's TX lifecycle, so the driver holds no + * beacon-specific code. One instance is created with the beacon modules and registers itself. + */ +class MeshBeaconTxHook : public RadioTxHook +{ + public: + PreTxAction beforeTransmit(RadioInterface *iface, meshtastic_MeshPacket *p) override; + bool holdsRadio(const meshtastic_MeshPacket *p) override; + void packetReleased(RadioInterface *iface, const meshtastic_MeshPacket *p) override; +}; + +extern MeshBeaconTxHook *meshBeaconTxHook; + /** * Broadcaster: periodically sends MeshBeacon packets on the configured preset/channel. * Active only when the FLAG_BROADCAST_ENABLED bit is set in moduleConfig.mesh_beacon.flags. diff --git a/src/modules/Modules.cpp b/src/modules/Modules.cpp index a2de555e90..34700ed07f 100644 --- a/src/modules/Modules.cpp +++ b/src/modules/Modules.cpp @@ -152,6 +152,7 @@ void setupModules() #if !MESHTASTIC_EXCLUDE_BEACON meshBeaconBroadcastModule = new MeshBeaconBroadcastModule(); meshBeaconListenerModule = new MeshBeaconListenerModule(); + meshBeaconTxHook = new MeshBeaconTxHook(); // registers itself with the radio driver's TX hooks #endif #if !MESHTASTIC_EXCLUDE_GPS positionModule = new PositionModule(); diff --git a/test/test_mesh_beacon/test_main.cpp b/test/test_mesh_beacon/test_main.cpp index bb1a0abc2f..4c2ddad954 100644 --- a/test/test_mesh_beacon/test_main.cpp +++ b/test/test_mesh_beacon/test_main.cpp @@ -1338,6 +1338,334 @@ 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"); +} + +// --------------------------------------------------------------------------- +// MeshBeaconTxHook (the radio driver's view of the beacon) +// --------------------------------------------------------------------------- + +/** + * Normal traffic must pass straight through the hook: no radio switch, no drop, and nothing claimed + * that would stop the driver listening on a busy channel. + */ +static void test_txHook_normalPacket_isSend(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)); + + MeshBeaconTxHook hook; + ReentrantRadioInterface radio; + meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero; + pkt.id = 0x7A000001; + + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_SEND, RadioTxHooks::beforeTransmit(&radio, &pkt), + "a packet with no beacon target must transmit as usual"); + TEST_ASSERT_FALSE_MESSAGE(RadioTxHooks::holdsRadio(&pkt), "normal traffic must not claim the radio"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "normal traffic must not reconfigure the radio"); +} + +/** + * A beacon asks the driver for a fresh transmit delay, because the switch leaves the radio on a + * channel the last channel scan never covered. + */ +static void test_txHook_beaconPacket_isDefer(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)); + + MeshBeaconTxHook hook; + ReentrantRadioInterface radio; + meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero; + pkt.id = 0x7A000002; + MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_DEFER, RadioTxHooks::beforeTransmit(&radio, &pkt), + "a beacon switch must defer the transmit"); + TEST_ASSERT_TRUE_MESSAGE(RadioTxHooks::holdsRadio(&pkt), "a queued beacon must claim the radio"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset, + "the hook must leave the radio on the beacon preset"); + + // packetReleased() is what the driver calls once the packet is sent, cancelled or dropped. + RadioTxHooks::packetReleased(&radio, &pkt); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset, + "releasing the packet must put the home preset back"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name, + "releasing the packet must put the home channel back"); +} + +/** + * SHORT_TURBO is not legal on EU_868, so the beacon has nowhere valid to transmit. The driver must be + * told to drop it rather than let it fall through onto the home config. + */ +static void test_txHook_invalidTarget_isDrop(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)); + + MeshBeaconTxHook hook; + ReentrantRadioInterface radio; + meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero; + pkt.id = 0x7A000003; + MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_SHORT_TURBO, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_DROP, RadioTxHooks::beforeTransmit(&radio, &pkt), + "an invalid target config must be dropped, not transmitted"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "a dropped beacon must not reconfigure the radio"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset, + "a dropped beacon must leave the home preset alone"); + + RadioTxHooks::packetReleased(&radio, &pkt); // the driver's drop path, so the sidecar entry is freed + TEST_ASSERT_FALSE_MESSAGE(MeshBeaconModule::hasTargetRadioSettings(&pkt), "the dropped packet's target must be released"); +} + +/** + * The hook list is what keeps the driver free of module includes: with nothing registered every call + * is a no-op, so a build without the beacon module behaves exactly as one with beacons idle. + */ +static void test_txHook_unregistered_isNoOp(void) +{ + resetConfig(); + ReentrantRadioInterface radio; + meshtastic_MeshPacket pkt = meshtastic_MeshPacket_init_zero; + pkt.id = 0x7A000004; + MeshBeaconModule::setTargetRadioSettings(&pkt, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_SEND, RadioTxHooks::beforeTransmit(&radio, &pkt), + "with no hook registered even a beacon is ordinary traffic to the driver"); + TEST_ASSERT_FALSE_MESSAGE(RadioTxHooks::holdsRadio(&pkt), "with no hook registered nothing claims the radio"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, radio.reconfigureCalls, "with no hook registered the radio is never reconfigured"); + + MeshBeaconModule::clearTargetRadioSettings(&pkt); +} + +/** + * A higher-priority packet can enqueue ahead of a still-queued beacon, so the driver asks the hook about + * an untagged packet while the beacon that armed the switch is live. The restore gate is right to hold + * off a release then, and wrong to hold off this: the untagged packet is about to key up, and without + * the restore it transmits on the beacon's preset, slot and region. + */ +static void test_txHook_untaggedPacketAheadOfQueuedBeacon_restoresHome(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)); + + MeshBeaconTxHook hook; + ReentrantRadioInterface radio; + + // The beacon reaches the head of the queue and the hook switches the radio for it. + meshtastic_MeshPacket beacon = meshtastic_MeshPacket_init_zero; + beacon.id = 0x7A000005; + MeshBeaconModule::setTargetRadioSettings(&beacon, meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, 0, false, + meshtastic_Config_LoRaConfig_RegionCode_UNSET, false, nullptr); + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_DEFER, RadioTxHooks::beforeTransmit(&radio, &beacon), + "the beacon switch should have applied"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset, + "the radio must be on the beacon preset before the queue jump"); + + // An ordinary packet now jumps the queue. The beacon is still queued, so its target is still live. + TEST_ASSERT_TRUE_MESSAGE(MeshBeaconModule::hasTargetRadioSettings(&beacon), + "the queued beacon must still hold its target, or this is not the case under test"); + meshtastic_MeshPacket ordinary = meshtastic_MeshPacket_init_zero; + ordinary.id = 0x7A000006; + + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_DEFER, RadioTxHooks::beforeTransmit(&radio, &ordinary), + "restoring the radio owes the driver a fresh delay and scan"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST, config.lora.modem_preset, + "an untagged packet must never transmit on the beacon preset"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Home", channels.getByIndex(channels.getPrimaryIndex()).settings.name, + "an untagged packet must never transmit on the beacon channel"); + + // The beacon is not lost by the restore: it switches the radio back when it next reaches the head. + TEST_ASSERT_EQUAL_INT_MESSAGE(RadioTxHook::PRETX_DEFER, RadioTxHooks::beforeTransmit(&radio, &beacon), + "the beacon must switch back when it reaches the head again"); + TEST_ASSERT_EQUAL_INT_MESSAGE(meshtastic_Config_LoRaConfig_ModemPreset_LONG_SLOW, config.lora.modem_preset, + "the beacon must be back on its target preset"); + + MeshBeaconModule::clearTargetRadioSettings(&beacon); + RadioTxHooks::packetReleased(&radio, &beacon); +} + } // namespace // =========================================================================== @@ -1462,6 +1790,21 @@ 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); + + printf("\n=== MeshBeaconTxHook ===\n"); + + RUN_TEST(test_txHook_normalPacket_isSend); + RUN_TEST(test_txHook_beaconPacket_isDefer); + RUN_TEST(test_txHook_invalidTarget_isDrop); + RUN_TEST(test_txHook_unregistered_isNoOp); + RUN_TEST(test_txHook_untaggedPacketAheadOfQueuedBeacon_restoresHome); + exit(UNITY_END()); } diff --git a/test/test_nexthop_routing/test_main.cpp b/test/test_nexthop_routing/test_main.cpp index c4891056cd..891504c71f 100644 --- a/test/test_nexthop_routing/test_main.cpp +++ b/test/test_nexthop_routing/test_main.cpp @@ -987,6 +987,25 @@ void test_rebroadcast_declined_send_releases_packet(void) TEST_ASSERT_EQUAL_MESSAGE(1, mockIface->sendCount, "the copy must have reached the mock radio"); } +// An already-encrypted packet never reaches perhapsEncode's TOO_LARGE check, so Router::send() is the +// last gate before the radio queue: MeshPacket.encrypted holds 256 bytes, the radio buffer 240. +void test_send_rejects_payload_larger_than_radio_buffer(void) +{ + MockRadioInterface *mockIface = installMockIface(); + meshtastic_MeshPacket p = makeRebroadcastCandidate(NODENUM_BROADCAST); + p.id = 0x51000010; + p.encrypted.size = MAX_RADIO_PAYLOAD_LEN + 1; + + TEST_ASSERT_EQUAL_MESSAGE(meshtastic_Routing_Error_TOO_LARGE, shim->send(packetPool.allocCopy(p)), + "a payload larger than the radio buffer must be refused"); + TEST_ASSERT_EQUAL_MESSAGE(0, mockIface->sendCount, "the oversized packet must never reach the radio"); + + p.id = 0x51000011; + p.encrypted.size = MAX_RADIO_PAYLOAD_LEN; + TEST_ASSERT_EQUAL_MESSAGE(ERRNO_OK, shim->send(packetPool.allocCopy(p)), "a payload at the radio ceiling must still be sent"); + TEST_ASSERT_EQUAL_MESSAGE(1, mockIface->sendCount, "the fitting packet must reach the radio"); +} + #if USERPREFS_EVENT_MODE void test_event_mode_hop_behavior(void) { @@ -1114,6 +1133,7 @@ void setup() RUN_TEST(test_rebroadcast_normal_broadcast_is_relayed); RUN_TEST(test_rebroadcast_no_lora_broadcast_is_not_relayed); RUN_TEST(test_rebroadcast_declined_send_releases_packet); + RUN_TEST(test_send_rejects_payload_larger_than_radio_buffer); #if USERPREFS_EVENT_MODE RUN_TEST(test_event_mode_hop_behavior); #endif diff --git a/test/test_radio/test_main.cpp b/test/test_radio/test_main.cpp index d9e81c8715..27e74237e8 100644 --- a/test/test_radio/test_main.cpp +++ b/test/test_radio/test_main.cpp @@ -64,7 +64,7 @@ class TestableRadioInterface : public RadioInterface size_t beginSendingPublic(meshtastic_MeshPacket *p) { return beginSending(p); } meshtastic_MeshPacket *getSendingPacket() const { return sendingPacket; } - size_t getRadioBufferPayloadCapacity() const { return sizeof(radioBuffer.payload); } + void clearSendingPacketForTest() { sendingPacket = nullptr; } // Override reconfigure to call the base which invokes applyModemConfig() bool reconfigure() override { return RadioInterface::reconfigure(); } @@ -430,7 +430,9 @@ static int32_t packetPoolLiveBytes() return 0; } -static void test_beginSending_oversizedPayloadAbortsSafely() +// Oversize is refused at the radio queue in Router::send(). If one ever gets this far the memcpy is +// clamped instead of failing, and the packet stays the caller's to release. +static void test_beginSending_oversizedPayloadIsClamped() { const int32_t liveBefore = packetPoolLiveBytes(); @@ -443,20 +445,34 @@ static void test_beginSending_oversizedPayloadAbortsSafely() p->to = 0x87654321; p->id = 0x10203040; p->which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + p->encrypted.size = MAX_RADIO_PAYLOAD_LEN + 10; - // Set encrypted size larger than sizeof(radioBuffer.payload) (which is 256 - sizeof(PacketHeader)) - p->encrypted.size = testRadio->getRadioBufferPayloadCapacity() + 10; + TEST_ASSERT_EQUAL_UINT_MESSAGE(MAX_LORA_PAYLOAD_LEN, testRadio->beginSendingPublic(p), + "an oversized payload must be clamped to the PHY limit, not rejected"); + TEST_ASSERT_EQUAL_PTR_MESSAGE(p, testRadio->getSendingPacket(), "beginSending must still take the packet"); - size_t result = testRadio->beginSendingPublic(p); - - TEST_ASSERT_EQUAL_UINT(0, result); - TEST_ASSERT_NULL(testRadio->getSendingPacket()); - - // The rejected packet went back to the pool. Not pointer identity: the native pool is - // malloc-backed and ASan quarantines the freed block, so the next alloc moves. + // beginSending has no failure path that releases, so the packet is ours to free. + testRadio->clearSendingPacketForTest(); + packetPool.release(p); TEST_ASSERT_EQUAL_INT32(liveBefore, packetPoolLiveBytes()); } +// The clamp must not shorten ordinary traffic, and a maximum-size frame must still fit the PHY. +static void test_beginSending_fittingPayloadIsSentWhole() +{ + meshtastic_MeshPacket *p = packetPool.allocZeroed(); + TEST_ASSERT_NOT_NULL(p); + p->from = 0x12345678; + p->to = 0x87654321; + p->id = 0x10203041; + p->which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + p->encrypted.size = MAX_RADIO_PAYLOAD_LEN; + + TEST_ASSERT_EQUAL_UINT_MESSAGE(MAX_LORA_PAYLOAD_LEN, testRadio->beginSendingPublic(p), + "the largest allowed payload must produce a frame at the PHY limit"); + testRadio->clearSendingPacketForTest(); + packetPool.release(p); +} void setUp(void) { mockMeshService = new MockMeshService(); @@ -507,7 +523,8 @@ void setup() RUN_TEST(test_regionPresetMap_coversAllRegionsWithinBounds); RUN_TEST(test_regionPresetMap_matchesRegionTable); RUN_TEST(test_regionPresetMap_unsetCarriesUserprefsIntent); - RUN_TEST(test_beginSending_oversizedPayloadAbortsSafely); + RUN_TEST(test_beginSending_oversizedPayloadIsClamped); + RUN_TEST(test_beginSending_fittingPayloadIsSentWhole); exit(UNITY_END()); }