Don't let a stale HEADER_ERR re-arm RX over a live frame

Before: a header that fails on noise, a collision or a foreign frame
latches HEADER_ERR and no RX_DONE follows it. `checkStaleRxFlags()`
counts HEADER_VALID and HEADER_ERR as one boolean, so the error starts
the max-packet window that re-arms RX. A good frame arriving inside that
window raises HEADER_VALID beside the error, which does not restart the
window - a header was already up. One max packet after the error was
first seen, the poll re-arms, and `startReceive()`'s standby aborts the
frame still on the air.

After: the window is timed from the valid header alone. A HEADER_ERR
beside a HEADER_VALID is retired at once and decides nothing, because one
header raises one flag or the other (SX1262 DS rev 2.1 table 13-29), so
the two together are two frames and the error is the older one.

A lone stale HEADER_ERR past its window is now cleared rather than
re-armed on every chip but SX128x. The re-arm exists for the SX1280 wedge
of DS rev 3.3 16.2, which a clear does not unwedge; on SX126x, LR11x0 and
LR20x0 the demodulator is already hunting for a preamble again, and a
clear never aborts a frame that may be in one. `headerErrWedgesRx()` is
the per-chip hook, true only on SX128x. SX126x already clears HEADER_ERR
unconditionally in `resumeRunningReceive()` for the same reason.

The busy decision is unaffected: `receiveDetected()` reads only
HEADER_VALID and PREAMBLE_DETECTED, so the cost of the bug was a dropped
reception, never a talk-over.

How: `staleRxFlagAction()` takes HEADER_VALID and HEADER_ERR separately
plus the per-chip wedge flag, and gains a ClearHeaderErr verdict.
`checkStaleRxFlags()` keeps a second stamp bit for "the window was
started by a valid header", so a valid header restarts a failed one's
window the way develop already has a header restart a bare preamble's.
Five native cases cover it, two of them the regression itself.

Found and analysed by Tom (NomDeTom), who also wrote the commit the
behaviour came from, 45262f825 in #11137.

Claude-Session: https://claude.ai/code/session_01DLRPmdJQTu6MDG7o2QLceR

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

src/mesh/RadioInterface.h

@@ -359,15 +359,21 @@ class RadioInterface
 std::unique_ptr<RadioInterface> 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

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))

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.

src/mesh/SX128xInterface.h

@@ -32,6 +32,9 @@ template <class T> 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

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<int>(StaleRxFlagAction::Keep), static_cast<int>(staleRxFlagAction(false, 0, 99)));
-    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Keep), static_cast<int>(staleRxFlagAction(true, 98, 99)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Keep), static_cast<int>(policy(false, false, 0, 99)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Keep), static_cast<int>(policy(true, false, 98, 99)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Keep), static_cast<int>(policy(false, true, 98, 99)));
 }
 
 static void test_staleRxFlagAction_windowEndsAtExactlyOneMaxPacket()
 {
     // Inclusive at the boundary, matching Throttle::hasElapsed().
-    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::ClearPreamble), static_cast<int>(staleRxFlagAction(false, 99, 99)));
-    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Rearm), static_cast<int>(staleRxFlagAction(true, 99, 99)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::ClearPreamble), static_cast<int>(policy(false, false, 99, 99)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Rearm), static_cast<int>(policy(true, false, 99, 99)));
 }
 
 static void test_staleRxFlagAction_barePreambleIsOnlyCleared()
 {
-    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::ClearPreamble), static_cast<int>(staleRxFlagAction(false, 2200, 2115)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::ClearPreamble), static_cast<int>(policy(false, false, 2200, 2115)));
 }
 
-static void test_staleRxFlagAction_staleHeaderIsRearmed()
+static void test_staleRxFlagAction_staleValidHeaderIsRearmed()
 {
-    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Rearm), static_cast<int>(staleRxFlagAction(true, 60000, 2115)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Rearm), static_cast<int>(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<int>(StaleRxFlagAction::ClearHeaderErr), static_cast<int>(policy(true, true, 10, 2115)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::ClearHeaderErr), static_cast<int>(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<int>(StaleRxFlagAction::ClearHeaderErr),
+                      static_cast<int>(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<int>(StaleRxFlagAction::ClearHeaderErr), static_cast<int>(policy(false, true, 60000, 2115)));
+    TEST_ASSERT_EQUAL(static_cast<int>(StaleRxFlagAction::Rearm),
+                      static_cast<int>(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());
 }