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()); }