diff --git a/src/mesh/RadioInterface.h b/src/mesh/RadioInterface.h index b428f70e9a..7a61d67c0a 100644 --- a/src/mesh/RadioInterface.h +++ b/src/mesh/RadioInterface.h @@ -359,15 +359,21 @@ class RadioInterface std::unique_ptr initLoRa(); // What the plain-RX poll does with PREAMBLE/HEADER flags that no RX_DONE has consumed. -enum class StaleRxFlagAction : uint8_t { Keep, ClearPreamble, Rearm }; +enum class StaleRxFlagAction : uint8_t { Keep, ClearPreamble, ClearHeaderErr, Rearm }; // RX_DONE clears every flag, so one still latched a max packet after first sight has no frame behind it. -// A header may mean a wedged demodulator (SX1280 DS 16.2), so re-arm; a bare preamble is only cleared. -constexpr StaleRxFlagAction staleRxFlagAction(bool headerSeen, uint32_t sinceFirstSeenMsec, uint32_t maxPacketTimeMsec) +// One header raises HEADER_VALID or HEADER_ERR and never both (SX1262 DS rev 2.1 table 13-29), so an error +// beside a valid header is an earlier frame's and is retired without touching the valid header's window. +// Only where a header error wedges RX (SX1280 DS rev 3.3 16.2) is a stale one worth the standby in a re-arm. +constexpr StaleRxFlagAction staleRxFlagAction(bool validSeen, bool errSeen, bool errWedgesRx, uint32_t sinceFirstSeenMsec, + uint32_t maxPacketTimeMsec) { - return sinceFirstSeenMsec < maxPacketTimeMsec ? StaleRxFlagAction::Keep - : headerSeen ? StaleRxFlagAction::Rearm - : StaleRxFlagAction::ClearPreamble; + return validSeen && errSeen ? StaleRxFlagAction::ClearHeaderErr + : sinceFirstSeenMsec < maxPacketTimeMsec ? StaleRxFlagAction::Keep + : validSeen ? StaleRxFlagAction::Rearm + : !errSeen ? StaleRxFlagAction::ClearPreamble + : errWedgesRx ? StaleRxFlagAction::Rearm + : StaleRxFlagAction::ClearHeaderErr; } /// Debug printing for packets diff --git a/src/mesh/RadioLibInterface.cpp b/src/mesh/RadioLibInterface.cpp index 7ca6590ebd..66822e6a84 100644 --- a/src/mesh/RadioLibInterface.cpp +++ b/src/mesh/RadioLibInterface.cpp @@ -943,6 +943,7 @@ void RadioLibInterface::startReceive() chipRecoveryFailures = 0; rxFlagsSeenMs = 0; rxFlagsSeenHeader = false; + rxFlagsSeenValidHeader = false; powerMon->setState(meshtastic_PowerMon_State_Lora_RXOn); } @@ -1031,35 +1032,50 @@ void RadioLibInterface::checkStaleRxFlags() const uint32_t irq = iface->getIrqFlags(); if (irq & iface->getIrqMapped(1UL << RADIOLIB_IRQ_RX_DONE)) return; - // HEADER_ERR counts: on SX1280 it leaves RX wedged with no RX_DONE or TIMEOUT to follow. - const bool headerSeen = irq & iface->getIrqMapped((1UL << RADIOLIB_IRQ_HEADER_VALID) | (1UL << RADIOLIB_IRQ_HEADER_ERR)); + const bool validSeen = irq & iface->getIrqMapped(1UL << RADIOLIB_IRQ_HEADER_VALID); + // HEADER_ERR counts on its own: on SX1280 it leaves RX wedged with no RX_DONE or TIMEOUT to follow. + const bool errSeen = irq & iface->getIrqMapped(1UL << RADIOLIB_IRQ_HEADER_ERR); + const bool headerSeen = validSeen || errSeen; const bool preambleSeen = irq & iface->getIrqMapped(1UL << RADIOLIB_IRQ_PREAMBLE_DETECTED); if (!headerSeen && !preambleSeen) { rxFlagsSeenMs = 0; rxFlagsSeenHeader = false; + rxFlagsSeenValidHeader = false; return; } - // A header's window starts when the header shows, not when an earlier bare preamble did - if (!rxFlagsSeenMs || (headerSeen && !rxFlagsSeenHeader)) { + // A header's window starts when the header shows, not when an earlier bare preamble did - and a valid + // header's starts over a failed header's, whose flag stays latched beside it and is not this frame's. + if (!rxFlagsSeenMs || (headerSeen && !rxFlagsSeenHeader) || (validSeen && !rxFlagsSeenValidHeader)) { rxFlagsSeenMs = Time::skipZero(Time::getMillis()); rxFlagsSeenHeader = headerSeen; + rxFlagsSeenValidHeader = validSeen; return; } const uint32_t maxPacketTimeMsec = getPacketTime(meshtastic_Constants_DATA_PAYLOAD_LEN + sizeof(PacketHeader)); - switch (staleRxFlagAction(headerSeen, Time::getMillis() - rxFlagsSeenMs, maxPacketTimeMsec)) { + switch (staleRxFlagAction(validSeen, errSeen, headerErrWedgesRx(), Time::getMillis() - rxFlagsSeenMs, maxPacketTimeMsec)) { case StaleRxFlagAction::Keep: break; case StaleRxFlagAction::Rearm: LOG_DEBUG("RX header stale, re-arm"); startReceive(); // clears rxFlagsSeenMs break; - case StaleRxFlagAction::ClearPreamble: + case StaleRxFlagAction::ClearHeaderErr: // A clear never aborts a reception, unlike the standby inside startReceive(). + LOG_DEBUG("RX header err stale, cleared"); + iface->clearIrq(1UL << RADIOLIB_IRQ_HEADER_ERR); + if (!validSeen) { // a valid header beside it keeps the window this error did not start + rxFlagsSeenMs = 0; + rxFlagsSeenHeader = false; + rxFlagsSeenValidHeader = false; + } + break; + case StaleRxFlagAction::ClearPreamble: LOG_DEBUG("RX preamble stale, cleared"); iface->clearIrq(1UL << RADIOLIB_IRQ_PREAMBLE_DETECTED); rxFlagsSeenMs = 0; rxFlagsSeenHeader = false; + rxFlagsSeenValidHeader = false; break; } } @@ -1267,7 +1283,7 @@ void RadioLibInterface::readOutFromTask() // A handoff's RX also routes CRC_ERR and HEADER_ERR to DIO1, and a damaged header raises one of those with no // RX_DONE behind it. Left latched, the line stays asserted and the next frame's RX_DONE raises no edge: // checkStaleRxFlags() steps aside for a handoff, so nothing would notice until checkCadHandoffTimeout(). - // Only inside the handoff window: plain RX leaves HEADER_ERR for checkStaleRxFlags() to judge a wedged SX1280 on. + // Only inside the handoff window: plain RX leaves HEADER_ERR to checkStaleRxFlags(), which clears or re-arms on it. if (cadHandoffRxStart) { const uint32_t errIrqs = (1UL << RADIOLIB_IRQ_CRC_ERR) | (1UL << RADIOLIB_IRQ_HEADER_ERR); if (iface->getIrqFlags() & iface->getIrqMapped(errIrqs)) diff --git a/src/mesh/RadioLibInterface.h b/src/mesh/RadioLibInterface.h index c534f8fa32..a798ab9e81 100644 --- a/src/mesh/RadioLibInterface.h +++ b/src/mesh/RadioLibInterface.h @@ -249,10 +249,15 @@ class RadioLibInterface : public RadioInterface, protected concurrency::Notified uint32_t rxFlagsSeenMs = 0; // rxFlagsSeenMs was stamped by a header, not by a bare preamble before it bool rxFlagsSeenHeader = false; + // ...and that header was a valid one, not a failed header before it + bool rxFlagsSeenValidHeader = false; /** Plain-RX twin of checkCadHandoffTimeout(): retire flags no RX_DONE consumed within a max packet. */ virtual void checkStaleRxFlags(); + /** Whether a header error leaves this chip's RX wedged, so only a re-arm recovers it (SX1280 DS 16.2). */ + virtual bool headerErrWedgesRx() const { return false; } + /** * Reset AGC by power-cycling the analog frontend. * Subclasses override with chip-specific calibration sequences. diff --git a/src/mesh/SX128xInterface.h b/src/mesh/SX128xInterface.h index 5344da8f18..cd1f7c2cca 100644 --- a/src/mesh/SX128xInterface.h +++ b/src/mesh/SX128xInterface.h @@ -32,6 +32,9 @@ template class SX128xInterface : public RadioLibInterface bool isIRQPending() override { return lora.getIrqFlags() != 0; } + /// DS rev 3.3 16.2: a header error leaves RX wedged with no RX_DONE or timeout, so only a re-arm clears it + bool headerErrWedgesRx() const override { return true; } + protected: /** * Specific module instance diff --git a/test/test_radio/test_main.cpp b/test/test_radio/test_main.cpp index 1b134c92e8..391280ff7e 100644 --- a/test/test_radio/test_main.cpp +++ b/test/test_radio/test_main.cpp @@ -539,7 +539,17 @@ class FakeIrqRadio : public PhysicalLayer irqMap[i] = 1UL << i; } + // What clearIrq() retired, and the register it retired it from: PhysicalLayer::clearIrq() maps the + // generic flags and hands them to clearIrqFlags(), which is the chip-specific half the fake stands in for. + uint32_t clearedIrqFlags = 0; + uint32_t getIrqFlags() override { return irqFlags; } + int16_t clearIrqFlags(uint32_t irq) override + { + clearedIrqFlags |= irq; + irqFlags &= ~irq; + return RADIOLIB_ERR_NONE; + } Module *getMod() override { return nullptr; } }; @@ -715,32 +725,66 @@ void tearDown(void) // latched one max packet after it was first seen has no frame behind it. // // Pinned: nothing happens inside that window (a frame may still be arriving); after it, a bare preamble is -// only cleared, because a clear never aborts a reception; a header re-arms, because SX1280 DS rev 3.3 16.2 -// leaves RX wedged after a header error with no RX_DONE or timeout to follow, and a clear does not unwedge it. +// only cleared, because a clear never aborts a reception; a valid header re-arms, because nothing short of a +// restart retires HEADER_VALID. A header error re-arms only where it wedges RX - SX1280 DS rev 3.3 16.2 leaves +// RX stuck after one with no RX_DONE or timeout to follow, and a clear does not unwedge it - and is otherwise +// cleared, because on every other chip the demodulator is already back hunting for a preamble. +// +// One header raises HEADER_VALID or HEADER_ERR and never both (SX1262 DS rev 2.1 table 13-29), so the two +// together are two frames: the error is the older one and must not decide anything about the valid header. // Regressions guarded: re-arming on a preamble (startReceive() goes through standby, which aborts a frame -// that is in fact arriving), acting before the window closes, or never acting at all. +// that is in fact arriving), re-arming on a stale error while a good frame is on air, acting before the +// window closes, or never acting at all. + +// errWedgesRx false unless a case is about the SX1280 wedge: every other chip is the common path. +static StaleRxFlagAction policy(bool validSeen, bool errSeen, uint32_t sinceMsec, uint32_t maxPacketMsec) +{ + return staleRxFlagAction(validSeen, errSeen, false, sinceMsec, maxPacketMsec); +} static void test_staleRxFlagAction_keepsFlagsInsideTheWindow() { - TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Keep), static_cast(staleRxFlagAction(false, 0, 99))); - TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Keep), static_cast(staleRxFlagAction(true, 98, 99))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Keep), static_cast(policy(false, false, 0, 99))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Keep), static_cast(policy(true, false, 98, 99))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Keep), static_cast(policy(false, true, 98, 99))); } static void test_staleRxFlagAction_windowEndsAtExactlyOneMaxPacket() { // Inclusive at the boundary, matching Throttle::hasElapsed(). - TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearPreamble), static_cast(staleRxFlagAction(false, 99, 99))); - TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Rearm), static_cast(staleRxFlagAction(true, 99, 99))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearPreamble), static_cast(policy(false, false, 99, 99))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Rearm), static_cast(policy(true, false, 99, 99))); } static void test_staleRxFlagAction_barePreambleIsOnlyCleared() { - TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearPreamble), static_cast(staleRxFlagAction(false, 2200, 2115))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearPreamble), static_cast(policy(false, false, 2200, 2115))); } -static void test_staleRxFlagAction_staleHeaderIsRearmed() +static void test_staleRxFlagAction_staleValidHeaderIsRearmed() { - TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Rearm), static_cast(staleRxFlagAction(true, 60000, 2115))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Rearm), static_cast(policy(true, false, 60000, 2115))); +} + +// The regression. A header error latches, no RX_DONE follows, and a good frame's HEADER_VALID lands beside it. +// Deciding on "a header is up" re-arms one max packet after the ERROR was first seen - a standby in the middle +// of the good frame. The error is the stale one, so it is retired and the valid header's own window decides. +static void test_staleRxFlagAction_staleHeaderErrBesideValidHeaderIsRetired() +{ + // Inside the valid header's window and past it: either way the error goes first and nothing re-arms on it. + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearHeaderErr), static_cast(policy(true, true, 10, 2115))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearHeaderErr), static_cast(policy(true, true, 60000, 2115))); + // And on an SX1280, where a lone error does wedge RX, a valid header beside it still means two frames + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearHeaderErr), + static_cast(staleRxFlagAction(true, true, true, 60000, 2115))); +} + +// A lone stale header error: only the chips it wedges pay a re-arm's standby for it. +static void test_staleRxFlagAction_loneStaleHeaderErrRearmsOnlyWhereItWedgesRx() +{ + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::ClearHeaderErr), static_cast(policy(false, true, 60000, 2115))); + TEST_ASSERT_EQUAL(static_cast(StaleRxFlagAction::Rearm), + static_cast(staleRxFlagAction(false, true, true, 60000, 2115))); } // checkStaleRxFlags() itself, which the cases above cannot see: a header that shows after a bare preamble gets its own @@ -775,6 +819,42 @@ static void test_checkStaleRxFlags_headerAfterPreambleRestartsTheWindow() delete radioIf; } +// The regression, end to end through the poll. A header fails on noise or a collision, HEADER_ERR latches and +// nothing clears it. A good frame then arrives and raises HEADER_VALID beside it. Timing a header from the +// error's stamp re-arms RX one max packet later, and startReceive()'s standby aborts the frame being received. +// Guarded here: the valid header gets its own window, and the error is retired instead of deciding anything. +static void test_checkStaleRxFlags_validHeaderAfterHeaderErrRestartsTheWindow() +{ + FakeIrqRadio phy; + auto *radioIf = new TestableRadioLibInterface(&phy); + radioIf->packetTimeMs = 100; + + // A failed header, seen by the poll: this is where develop starts the clock that goes on to abort the frame + Time::setTestMillis(1000); + phy.irqFlags = phy.getIrqMapped(1UL << RADIOLIB_IRQ_HEADER_ERR); + radioIf->checkStaleRxFlags(); + TEST_ASSERT_EQUAL_UINT32(1000, radioIf->rxFlagsSeenMs); + TEST_ASSERT_TRUE(radioIf->rxFlagsSeenHeader); + TEST_ASSERT_FALSE(radioIf->rxFlagsSeenValidHeader); + + // A good frame's header lands beside the stale error: its own window starts here + Time::setTestMillis(1090); + phy.irqFlags |= phy.getIrqMapped(1UL << RADIOLIB_IRQ_HEADER_VALID); + radioIf->checkStaleRxFlags(); + TEST_ASSERT_EQUAL_UINT32_MESSAGE(1090, radioIf->rxFlagsSeenMs, "a valid header must restart a failed header's window"); + TEST_ASSERT_TRUE(radioIf->rxFlagsSeenValidHeader); + + // Past one max packet from the error, inside the valid header's window: the error goes, the window stays + Time::setTestMillis(1150); + radioIf->checkStaleRxFlags(); + TEST_ASSERT_EQUAL_UINT32(phy.getIrqMapped(1UL << RADIOLIB_IRQ_HEADER_ERR), phy.clearedIrqFlags); + TEST_ASSERT_EQUAL_UINT32_MESSAGE(1090, radioIf->rxFlagsSeenMs, "retiring the error must not restart the window"); + TEST_ASSERT_TRUE(radioIf->rxFlagsSeenValidHeader); + + Time::useRealClock(); + delete radioIf; +} + void setup() { delay(10); @@ -817,8 +897,11 @@ void setup() RUN_TEST(test_staleRxFlagAction_keepsFlagsInsideTheWindow); RUN_TEST(test_staleRxFlagAction_windowEndsAtExactlyOneMaxPacket); RUN_TEST(test_staleRxFlagAction_barePreambleIsOnlyCleared); - RUN_TEST(test_staleRxFlagAction_staleHeaderIsRearmed); + RUN_TEST(test_staleRxFlagAction_staleValidHeaderIsRearmed); + RUN_TEST(test_staleRxFlagAction_staleHeaderErrBesideValidHeaderIsRetired); + RUN_TEST(test_staleRxFlagAction_loneStaleHeaderErrRearmsOnlyWhereItWedgesRx); RUN_TEST(test_checkStaleRxFlags_headerAfterPreambleRestartsTheWindow); + RUN_TEST(test_checkStaleRxFlags_validHeaderAfterHeaderErrRestartsTheWindow); exit(UNITY_END()); }