From df34ef1081ff5ff2f7ed23cb8319b6219353196c Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Tue, 1 Sep 2026 16:08:33 +0000 Subject: [PATCH] fix(radio): recover from chip state loss in the RX/TX hot paths too (#11678) * fix(radio): recover from chip state loss in the RX/TX hot paths too * fix(radio): address CodeRabbit findings on the hot-path recovery PR (#11680) * fix(radio): address CodeRabbit findings on the hot-path recovery PR SX128x: startReceive() still called the old asserting setStandby() before the new trySetStandby(). The assert fired first, so the recovery path added below it could never run - the exact chip-state-loss crash this PR exists to fix was still live on SX128x. Remove the stale call. LR11x0: resolvedTcxoVoltage was set once after the primary begin() attempts, but two later paths - firmware recovery and the one-shot firmware update - call begin() again with tcxoVoltage and never updated it. On a TCXO_OPTIONAL board that only came up via one of those paths, reinitChip() would recover with the wrong oscillator setting. Update resolvedTcxoVoltage after each of those begin() calls too. LR20x0: reconfigure() discarded RadioLibInterface::reconfigure()'s result - the band-hop path always returned true regardless, and the same-band path reused the same flag for chip-programming errors, so a base-class failure could both mask itself as success and wrongly trigger a full re-init. Track the base-class result (reconfigureSuccess) separately from the chip result (standbySuccess), and return the former. Also shortens the recovery-rationale comments in RadioLibInterface.h and SX126xInterface.cpp to 1-2 lines per the repo's comment convention, the rationale now covered once in the base class. * fix(radio): finish the recovery ladder and stop recovery from rebooting Follow-up to the CodeRabbit findings, plus two gaps found auditing the branch against its own intent (never reboot on chip state loss; recover in place). RX left off was unrecoverable on an idle node. Every startReceive() call site is event-driven - RX/TX ISR, the CAD-busy branch, startSend()'s failure path, init(), reconfigure() - and a radio with RX off cannot raise an RX interrupt, so nothing re-arms it unless the node happens to transmit or the user changes config. A listen-only or quiet node stayed deaf for good, which is worse than the reboot this replaced. main.cpp's existing 60 s AGC tick now calls periodicRadioMaintenance(), which re-arms RX when rxOffline is set and otherwise does the AGC reset as before. In-place repair now gives up rather than retrying forever. After MAX_CHIP_RECOVERY_FAILURES consecutive failures - a throttle window apart, so minutes of a provably dead chip - schedule rebootAtMsec, the same deliberate reboot Portduino already uses for LoRa_in_error. A reboot re-runs init(), which redoes the power-enable GPIOs, settle delays and TCXO probing that begin() alone skips. Both counters reset in RadioLibInterface::startReceive(), the one point every driver reaches only once the chip accepts the RX start. SX128x: reconfigure()'s recovery reached reinitChip()'s region-mismatch branch, which rewrites config.lora.region, saves, and calls ESP.restart() / NVIC_SystemReset(). A runtime recovery must never reboot - that is the crash this path exists to prevent, and it would fire with a config save pending. Gated to the boot-time call via a fromInit parameter. LR20x0: a rejected setRxBoostedGainMode cleared the success flag and so forced a full fullBegin() chip reset. It is a warn-level cosmetic setting, treated as warn-only in LR11x0's equivalent, and not a lost-state signature. Also logs suppressed recovery attempts at debug level; previously a chip that stayed dead recorded one critical error and then went completely silent. * fix(radio): count RX re-arms, not re-inits, in the recovery ladder LR20x0's recoverChipStateLoss() is fullBegin(), which re-arms RX itself but reports success on begin() alone. A re-init that came back with RX still dead therefore reset chipRecoveryFailures, so a chip that could be re-inited forever while never receiving again held the ladder at zero and never reached the reboot. The other drivers had the same hole from the other side: the caller's retry startReceive() runs after the reset, so a retry that failed again left the count cleared. RadioLibInterface::startReceive() is now the only place the ladder clears, and it only runs once the chip actually accepted RX. The threshold is judged at the top of the next attempt - a throttle window later, after that attempt's retry (the caller's, or fullBegin's own) has had its chance to clear it. That also drops the old false positive where the reboot was armed before the retry that would have succeeded. RF95Interface::startReceive() set isReceiving directly instead of calling the base, so on RF95 nothing ever cleared rxOffline or the ladder: the first failed RX start left periodicRadioMaintenance() re-initing forever, and with the count now advancing it would have rebooted a working radio. --------- Co-authored-by: Ben Meadors --------- Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com> --- src/main.cpp | 4 +- src/mesh/LR11x0Interface.cpp | 54 +++++++++++++++------- src/mesh/LR11x0Interface.h | 3 ++ src/mesh/LR20x0Interface.cpp | 84 +++++++++++++++++++++------------- src/mesh/LR20x0Interface.h | 3 ++ src/mesh/RF95Interface.cpp | 50 ++++++++++++-------- src/mesh/RF95Interface.h | 3 ++ src/mesh/RadioLibInterface.cpp | 47 +++++++++++++++++++ src/mesh/RadioLibInterface.h | 18 ++++++++ src/mesh/SX126xInterface.cpp | 70 +++++++++++++++++----------- src/mesh/SX126xInterface.h | 3 ++ src/mesh/SX128xInterface.cpp | 54 ++++++++++++++-------- src/mesh/SX128xInterface.h | 7 ++- 13 files changed, 285 insertions(+), 115 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 80b8e4f4a9..b5db79b6bb 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1481,11 +1481,11 @@ void loop() RadioLibInterface::instance->pollMissedIrqs(); } - // Periodic AGC reset - warm sleep + recalibrate to prevent stuck AGC gain + // Periodic radio upkeep - re-arms RX if it was left off, else AGC reset (stuck-gain prevention) static uint32_t lastAgcReset; if (!Throttle::isWithinTimespanMs(lastAgcReset, AGC_RESET_INTERVAL_MS)) { lastAgcReset = millis(); - RadioLibInterface::instance->resetAGC(); + RadioLibInterface::instance->periodicRadioMaintenance(); } } diff --git a/src/mesh/LR11x0Interface.cpp b/src/mesh/LR11x0Interface.cpp index 3786dbd678..fdad36fca8 100644 --- a/src/mesh/LR11x0Interface.cpp +++ b/src/mesh/LR11x0Interface.cpp @@ -185,6 +185,8 @@ template bool LR11x0Interface::init() if (lora.updateFirmware(lr11xx_firmware_image, LR11XX_FIRMWARE_IMAGE_SIZE, true) == RADIOLIB_ERR_NONE) { LOG_INFO("LR1110 firmware recovery OK, re-init radio"); res = lora.begin(getFreq(), bw, sf, cr, syncWord, power, preambleLength, tcxoVoltage); + if (res == RADIOLIB_ERR_NONE) + resolvedTcxoVoltage = tcxoVoltage; } #endif if (res != RADIOLIB_ERR_NONE) @@ -222,6 +224,7 @@ template bool LR11x0Interface::init() LOG_ERROR("LR11x0 re-init after firmware update failed %s%d", radioLibErr, res); return false; } + resolvedTcxoVoltage = tcxoVoltage; if (lora.getVersionInfo(&version) == RADIOLIB_ERR_NONE) { transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; @@ -450,16 +453,31 @@ template void LR11x0Interface::startReceive() sleep(); #else - setStandby(); + int16_t err = trySetStandby(); - lora.setPreambleLength(preambleLength); // Solve RX ack fail after direct message sent. Not sure why this is needed. + if (err == RADIOLIB_ERR_NONE) { + lora.setPreambleLength(preambleLength); // Solve RX ack fail after direct message sent. Not sure why this is needed. - // We use a 16 bit preamble so this should save some power by letting radio sit in standby mostly. - int err = - lora.startReceive(RADIOLIB_LR11X0_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS, RADIOLIB_IRQ_RX_DEFAULT_MASK, 0); - if (err) + // We use a 16 bit preamble so this should save some power by letting radio sit in standby mostly. + err = + lora.startReceive(RADIOLIB_LR11X0_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS, RADIOLIB_IRQ_RX_DEFAULT_MASK, 0); + } + + if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("StartReceive error: %d", err); - assert(err == RADIOLIB_ERR_NONE); + if (maybeRecoverChipStateLoss()) { + lora.setPreambleLength(preambleLength); + err = lora.startReceive(RADIOLIB_LR11X0_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS, + RADIOLIB_IRQ_RX_DEFAULT_MASK, 0); + } + } + + if (err != RADIOLIB_ERR_NONE) { + // No assert: leave RX off rather than reboot; periodicRadioMaintenance() re-arms it, throttled + LOG_ERROR("LR11x0 RX offline %s%d", radioLibErr, err); + rxOffline = true; + return; + } RadioLibInterface::startReceive(); @@ -480,16 +498,18 @@ template bool LR11x0Interface::isChannelActive() .timeout = 0, .irqFlags = RADIOLIB_IRQ_CAD_DEFAULT_FLAGS, .irqMask = RADIOLIB_IRQ_CAD_DEFAULT_MASK}}; - int16_t result; + int16_t result = trySetStandby(); + if (result == RADIOLIB_ERR_NONE) { + result = lora.scanChannel(cfg); + if (result == RADIOLIB_LORA_DETECTED) + return true; + if (result != RADIOLIB_ERR_WRONG_MODEM) + return false; + } - setStandby(); - result = lora.scanChannel(cfg); - if (result == RADIOLIB_LORA_DETECTED) - return true; - - assert(result != RADIOLIB_ERR_WRONG_MODEM); - - return false; + // standby failed or the LoRa modem type is gone - the chip lost its runtime state + maybeRecoverChipStateLoss(); + return false; // report the channel free: a recovered chip can TX, a dead one fails startSend safely } /** Could we send right now (i.e. either not actively receiving or transmitting)? */ @@ -537,7 +557,7 @@ template bool LR11x0Interface::sleep() { // \todo Display actual typename of the adapter, not just `LR11x0` LOG_DEBUG("LR11x0 entering sleep mode"); - setStandby(); // Stop any pending operations + (void)trySetStandby(); // Stop any pending operations - the chip is being put to sleep, a failure must not crash // turn off TCXO if it was powered lora.setTCXO(0); diff --git a/src/mesh/LR11x0Interface.h b/src/mesh/LR11x0Interface.h index c1e48ae72c..e3b4f392af 100644 --- a/src/mesh/LR11x0Interface.h +++ b/src/mesh/LR11x0Interface.h @@ -89,6 +89,9 @@ template class LR11x0Interface : public RadioLibInterface /** setStandby()'s body, returning the standby error instead of asserting - for callers that can recover */ int16_t trySetStandby(); + /** Recover a chip that lost its runtime state: hardware-reset via begin() and reprogram */ + bool recoverChipStateLoss() override { return reinitChip() && programModemParams() == RADIOLIB_ERR_NONE; } + /// The TCXO Vref that init() settled on, so reinitChip() can begin() with the same oscillator setup float resolvedTcxoVoltage = 0; }; diff --git a/src/mesh/LR20x0Interface.cpp b/src/mesh/LR20x0Interface.cpp index 587bead397..f6936afe73 100644 --- a/src/mesh/LR20x0Interface.cpp +++ b/src/mesh/LR20x0Interface.cpp @@ -179,7 +179,9 @@ template bool LR20x0Interface::init() template bool LR20x0Interface::reconfigure() { - bool success = RadioLibInterface::reconfigure(); + // Propagated to the return value below, separately from the chip-programming outcome, so a + // base-class failure isn't masked as success. + const bool reconfigureSuccess = RadioLibInterface::reconfigure(); if (config.lora.region == meshtastic_Config_LoRaConfig_RegionCode_LORA_24) { limitPower(LR2021_MAX_POWER_HF); @@ -199,72 +201,73 @@ template bool LR20x0Interface::reconfigure() return false; startReceive(); - return true; + return reconfigureSuccess; } // Same-band reconfigure (previous incremental path) + bool standbySuccess = true; int16_t standbyErr = trySetStandby(); if (standbyErr != RADIOLIB_ERR_NONE) - success = false; + standbySuccess = false; if (standbyErr == RADIOLIB_ERR_NONE) { int err = lora.setFrequency(freq); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setFrequency %.3f MHz %s%d", freq, radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } err = lora.setSpreadingFactor(sf); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setSpreadingFactor(%u) %s%d", sf, radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } err = lora.setBandwidth(bw); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setBandwidth(%.1f) %s%d", bw, radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } err = lora.setCodingRate(cr, cr != 7); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setCodingRate(%u) %s%d", cr, radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } err = lora.setSyncWord(syncWord); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setSyncWord %s%d", radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } err = lora.setPreambleLength(preambleLength); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setPreambleLength(%u) %s%d", preambleLength, radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } err = lora.setOutputPower(power); if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("LR20x0 setOutputPower %d dBm @ %.3f MHz %s%d", power, freq, radioLibErr, err); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); - success = false; + standbySuccess = false; } + // Warn-only, as in LR11x0: a rejected gain mode is cosmetic and not a lost-state signature, so + // it must not drag reconfigure() into a full chip reset. err = lora.setRxBoostedGainMode(config.lora.sx126x_rx_boosted_gain); - if (err != RADIOLIB_ERR_NONE) { + if (err != RADIOLIB_ERR_NONE) LOG_WARN("LR20x0 setRxBoostedGainMode %s%d", radioLibErr, err); - success = false; - } } - if (!success) { + if (!standbySuccess) { // A chip that fails standby or rejects parameter programming (typically WRONG_MODEM, -20) has // lost its runtime configuration to a chip-internal reset or brownout. Recover in place with the // same full begin() the band-hop path uses - it hardware-resets the chip. Crashing here instead @@ -279,7 +282,7 @@ template bool LR20x0Interface::reconfigure() startReceive(); lr20x0LastFreqMHz = freq; - return true; + return reconfigureSuccess; } // The chip-side re-init the band-hop and recovery paths share: front-end switch GPIOs for the target @@ -414,16 +417,31 @@ template void LR20x0Interface::startReceive() sleep(); #else - setStandby(); + int16_t err = trySetStandby(); - lora.setPreambleLength(preambleLength); // Solve RX ack fail after direct message sent. Not sure why this is needed. + if (err == RADIOLIB_ERR_NONE) { + lora.setPreambleLength(preambleLength); // Solve RX ack fail after direct message sent. Not sure why this is needed. - // We use a 16 bit preamble so this should save some power by letting radio sit in standby mostly. - int err = - lora.startReceive(RADIOLIB_LR2021_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS, RADIOLIB_IRQ_RX_DEFAULT_MASK, 0); - if (err) + // We use a 16 bit preamble so this should save some power by letting radio sit in standby mostly. + err = + lora.startReceive(RADIOLIB_LR2021_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS, RADIOLIB_IRQ_RX_DEFAULT_MASK, 0); + } + + if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("StartReceive error: %d", err); - assert(err == RADIOLIB_ERR_NONE); + if (maybeRecoverChipStateLoss()) { + lora.setPreambleLength(preambleLength); + err = lora.startReceive(RADIOLIB_LR2021_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS, + RADIOLIB_IRQ_RX_DEFAULT_MASK, 0); + } + } + + if (err != RADIOLIB_ERR_NONE) { + // No assert: leave RX off rather than reboot; periodicRadioMaintenance() re-arms it, throttled + LOG_ERROR("LR20x0 RX offline %s%d", radioLibErr, err); + rxOffline = true; + return; + } RadioLibInterface::startReceive(); @@ -444,16 +462,18 @@ template bool LR20x0Interface::isChannelActive() .timeout = 0, .irqFlags = RADIOLIB_IRQ_CAD_DEFAULT_FLAGS, .irqMask = RADIOLIB_IRQ_CAD_DEFAULT_MASK}}; - int16_t result; + int16_t result = trySetStandby(); + if (result == RADIOLIB_ERR_NONE) { + result = lora.scanChannel(cfg); + if (result == RADIOLIB_LORA_DETECTED) + return true; + if (result != RADIOLIB_ERR_WRONG_MODEM) + return false; + } - setStandby(); - result = lora.scanChannel(cfg); - if (result == RADIOLIB_LORA_DETECTED) - return true; - - assert(result != RADIOLIB_ERR_WRONG_MODEM); - - return false; + // standby failed or the LoRa modem type is gone - the chip lost its runtime state + maybeRecoverChipStateLoss(); + return false; // report the channel free: a recovered chip can TX, a dead one fails startSend safely } /** Could we send right now (i.e. either not actively receiving or transmitting)? */ @@ -500,7 +520,7 @@ template bool LR20x0Interface::sleep() { // \todo Display actual typename of the adapter, not just `LR20x0` LOG_DEBUG("LR20x0 entering sleep mode"); - setStandby(); // Stop any pending operations + (void)trySetStandby(); // Stop any pending operations - the chip is being put to sleep, a failure must not crash // turn off TCXO if it was powered lora.setTCXO(0); diff --git a/src/mesh/LR20x0Interface.h b/src/mesh/LR20x0Interface.h index df69bf1a2d..5150e9ab8c 100644 --- a/src/mesh/LR20x0Interface.h +++ b/src/mesh/LR20x0Interface.h @@ -80,5 +80,8 @@ template class LR20x0Interface : public RadioLibInterface /** setStandby()'s body, returning the standby error instead of asserting - for callers that can recover */ int16_t trySetStandby(); + + /** Recover a chip that lost its runtime state via the same full begin() the band-hop path uses */ + bool recoverChipStateLoss() override { return fullBegin(getFreq()); } }; #endif diff --git a/src/mesh/RF95Interface.cpp b/src/mesh/RF95Interface.cpp index 0be5feee92..5f2322417f 100644 --- a/src/mesh/RF95Interface.cpp +++ b/src/mesh/RF95Interface.cpp @@ -352,13 +352,24 @@ void RF95Interface::configHardwareForSend() void RF95Interface::startReceive() { setTransmitEnable(false); - setStandby(); - int err = lora->startReceive(); - if (err != RADIOLIB_ERR_NONE) - LOG_ERROR("RF95 startReceive %s%d", radioLibErr, err); - assert(err == RADIOLIB_ERR_NONE); + int16_t err = trySetStandby(); + if (err == RADIOLIB_ERR_NONE) + err = lora->startReceive(); - isReceiving = true; + if (err != RADIOLIB_ERR_NONE) { + LOG_ERROR("RF95 startReceive %s%d", radioLibErr, err); + if (maybeRecoverChipStateLoss()) + err = lora->startReceive(); + } + + if (err != RADIOLIB_ERR_NONE) { + // No assert: leave RX off rather than reboot; periodicRadioMaintenance() re-arms it, throttled + LOG_ERROR("RF95 RX offline %s%d", radioLibErr, err); + rxOffline = true; + return; + } + + RadioLibInterface::startReceive(); // Must be done AFTER, starting receive, because startReceive clears (possibly stale) interrupt pending register bits enableInterrupt(isrRxLevel0); @@ -368,21 +379,24 @@ void RF95Interface::startReceive() bool RF95Interface::isChannelActive() { // check if we can detect a LoRa preamble on the current channel - int16_t result; setTransmitEnable(false); - setStandby(); // needed for smooth transition - result = lora->scanChannel(); + int16_t result = trySetStandby(); // needed for smooth transition + if (result == RADIOLIB_ERR_NONE) { + result = lora->scanChannel(); - if (result == RADIOLIB_PREAMBLE_DETECTED) { - // LOG_DEBUG("Channel is busy"); - return true; + if (result == RADIOLIB_PREAMBLE_DETECTED) { + // LOG_DEBUG("Channel is busy"); + return true; + } + if (result != RADIOLIB_CHANNEL_FREE) + LOG_ERROR("RF95 isChannelActive %s%d", radioLibErr, result); + if (result != RADIOLIB_ERR_WRONG_MODEM) + return false; } - if (result != RADIOLIB_CHANNEL_FREE) - LOG_ERROR("RF95 isChannelActive %s%d", radioLibErr, result); - assert(result != RADIOLIB_ERR_WRONG_MODEM); - // LOG_DEBUG("Channel is free"); - return false; + // standby failed or the LoRa modem type is gone - the chip lost its runtime state + maybeRecoverChipStateLoss(); + return false; // report the channel free: a recovered chip can TX, a dead one fails startSend safely } /** Could we send right now (i.e. either not actively receiving or transmitting)? */ @@ -394,7 +408,7 @@ bool RF95Interface::isActivelyReceiving() bool RF95Interface::sleep() { // put chipset into sleep mode - setStandby(); // First cancel any active receiving/sending + (void)trySetStandby(); // First cancel any active receiving/sending - going to sleep, a failure must not crash lora->sleep(); #ifdef RF95_POWER_EN diff --git a/src/mesh/RF95Interface.h b/src/mesh/RF95Interface.h index b1dd383197..4536dbd502 100644 --- a/src/mesh/RF95Interface.h +++ b/src/mesh/RF95Interface.h @@ -87,5 +87,8 @@ class RF95Interface : public RadioLibInterface /** setStandby()'s body, returning the standby error instead of asserting - for callers that can recover */ int16_t trySetStandby(); + + /** Recover a chip that lost its runtime state: hardware-reset via begin() and reprogram */ + bool recoverChipStateLoss() override { return reinitChip() && programModemParams() == RADIOLIB_ERR_NONE; } }; #endif diff --git a/src/mesh/RadioLibInterface.cpp b/src/mesh/RadioLibInterface.cpp index da0f58d6ec..4a5fb86a25 100644 --- a/src/mesh/RadioLibInterface.cpp +++ b/src/mesh/RadioLibInterface.cpp @@ -707,6 +707,10 @@ void RadioLibInterface::handleReceiveInterrupt() void RadioLibInterface::startReceive() { isReceiving = true; + // Drivers only reach here once the chip actually accepted the RX start, so the radio is alive again. + // This is the sole place the recovery ladder is cleared - nothing short of an armed RX counts as fixed. + rxOffline = false; + chipRecoveryFailures = 0; powerMon->setState(meshtastic_PowerMon_State_Lora_RXOn); } @@ -726,6 +730,49 @@ void RadioLibInterface::resetAGC() // Base implementation: no-op. Override in chip-specific subclasses. } +void RadioLibInterface::periodicRadioMaintenance() +{ + // Every startReceive() call site is event-driven (RX/TX ISR, the CAD-busy branch, reconfigure), and a + // radio left with RX off can no longer raise an RX interrupt - on a node with nothing to transmit + // nothing would ever re-arm it. This periodic tick is that retry; maybeRecoverChipStateLoss() throttles. + if (rxOffline) { + LOG_WARN("Radio RX offline, retrying"); + if (maybeRecoverChipStateLoss()) + startReceive(); + return; // a chip just re-inited (or still dead) has no use for an AGC reset this tick + } + + resetAGC(); +} + +bool RadioLibInterface::maybeRecoverChipStateLoss() +{ + // One attempt per window: the transient resets this recovers from need a single re-init, and a + // chip that stays dead must not stall the TX/RX paths with a begin() attempt on every call + if (lastChipRecoveryMs && Throttle::isWithinTimespanMs(lastChipRecoveryMs, 30 * 1000UL)) { + LOG_DEBUG("Radio recovery suppressed, %us since the last attempt", (millis() - lastChipRecoveryMs) / 1000); + return false; + } + + // The ladder counts re-arms, not re-inits: only RadioLibInterface::startReceive() clears the count, and + // only once the chip really accepted RX. Judging the previous attempt here - a throttle window later, + // after its retry - is what stops a begin() that succeeded while leaving RX dead from crediting itself. + if (chipRecoveryFailures >= MAX_CHIP_RECOVERY_FAILURES && rebootAtMsec == 0) { + // Attempts are a throttle window apart, so this is minutes of a provably deaf chip. begin() alone + // clearly isn't reviving it; reboot to re-run init(), which redoes the power-on sequence it skips. + LOG_ERROR("Radio still deaf after %u re-inits, rebooting", chipRecoveryFailures); + rebootAtMsec = millis() + DEFAULT_REBOOT_SECONDS * 1000; + } + chipRecoveryFailures++; + + lastChipRecoveryMs = millis(); + RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); + LOG_ERROR("Radio chip state lost mid-operation, re-init"); + bool recovered = recoverChipStateLoss(); + LOG_INFO("Radio re-init %s", recovered ? "succeeded" : "failed"); + return recovered; +} + void RadioLibInterface::checkRxDoneIrqFlag() { if (iface->checkIrq(RADIOLIB_IRQ_RX_DONE)) { diff --git a/src/mesh/RadioLibInterface.h b/src/mesh/RadioLibInterface.h index 0142721789..6dcd0876f7 100644 --- a/src/mesh/RadioLibInterface.h +++ b/src/mesh/RadioLibInterface.h @@ -180,6 +180,24 @@ class RadioLibInterface : public RadioInterface, protected concurrency::Notified */ virtual void resetAGC(); + /** Periodic radio upkeep: re-arms RX if a failed startReceive() left it off, otherwise resets AGC. */ + void periodicRadioMaintenance(); + + /** Chip-specific recovery of a chip that lost its state to a reset/brownout. Returns true if reprogrammed. */ + virtual bool recoverChipStateLoss() { return false; } + + /** Throttled recoverChipStateLoss(), so a dead chip can't stall the RX/TX hot paths with repeated begin(). */ + bool maybeRecoverChipStateLoss(); + + uint32_t lastChipRecoveryMs = 0; + + /// Consecutive recovery attempts that never got RX armed again, before rebooting to re-run init() + static constexpr uint8_t MAX_CHIP_RECOVERY_FAILURES = 5; + uint8_t chipRecoveryFailures = 0; + + /// Set by a driver's startReceive() when it gives up and leaves RX off; cleared once RX is armed again. + bool rxOffline = false; + /** * Debugging counts */ diff --git a/src/mesh/SX126xInterface.cpp b/src/mesh/SX126xInterface.cpp index 7bc6bfb66f..aa55335b44 100644 --- a/src/mesh/SX126xInterface.cpp +++ b/src/mesh/SX126xInterface.cpp @@ -284,10 +284,8 @@ template bool SX126xInterface::reconfigure() err = programModemParams(); if (err != RADIOLIB_ERR_NONE) { - // A chip that fails standby or rejects parameter programming (typically WRONG_MODEM, -20) has - // lost its runtime configuration - packet type included - to a chip-internal reset or brownout. - // Recover in place: begin() hardware-resets the chip and restores the LoRa packet type. Crashing - // here instead would reboot before MeshService persists the config change that triggered us. + // Chip likely lost its state (reset/brownout); recover in place rather than crash - see + // RadioLibInterface::recoverChipStateLoss(). RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); LOG_ERROR("SX126x rejected modem params, chip state lost? Full re-init"); if (!reinitChip() || (err = programModemParams()) != RADIOLIB_ERR_NONE) { @@ -424,26 +422,43 @@ template void SX126xInterface::startReceive() #else setTransmitEnable(false); - setStandby(); #ifdef ARCH_PORTDUINO_WASM - // Continuous RX in the browser: duty-cycle sleep parks BUSY high between RX - // windows and stalls the slow WebUSB SPI link. No battery to save here. - int err = lora.startReceive(RADIOLIB_SX126X_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); const char *rxMethod = "startReceive"; #else - // We use a 16 bit preamble so this should save some power by letting radio sit in standby mostly. - int err = lora.startReceiveDutyCycleAuto(preambleLength, 8, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); const char *rxMethod = "startReceiveDutyCycleAuto"; #endif - if (err != RADIOLIB_ERR_NONE) + auto tryStartRx = [&]() -> int16_t { +#ifdef ARCH_PORTDUINO_WASM + // Continuous RX in the browser: duty-cycle sleep parks BUSY high between RX + // windows and stalls the slow WebUSB SPI link. No battery to save here. + return lora.startReceive(RADIOLIB_SX126X_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); +#else + // We use a 16 bit preamble so this should save some power by letting radio sit in standby mostly. + return lora.startReceiveDutyCycleAuto(preambleLength, 8, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); +#endif + }; + + int16_t err = trySetStandby(); + if (err == RADIOLIB_ERR_NONE) + err = tryStartRx(); + + if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("SX126X %s %s%d", rxMethod, radioLibErr, err); + if (maybeRecoverChipStateLoss()) + err = tryStartRx(); + } + + if (err != RADIOLIB_ERR_NONE) { #ifdef ARCH_PORTDUINO - if (err != RADIOLIB_ERR_NONE) portduino_status.LoRa_in_error = true; #else - assert(err == RADIOLIB_ERR_NONE); + // No assert: leave RX off rather than reboot; periodicRadioMaintenance() re-arms it, throttled + LOG_ERROR("SX126X RX offline %s%d", radioLibErr, err); + rxOffline = true; + return; #endif + } RadioLibInterface::startReceive(); @@ -464,22 +479,23 @@ template bool SX126xInterface::isChannelActive() .timeout = 0, .irqFlags = RADIOLIB_IRQ_CAD_DEFAULT_FLAGS, .irqMask = RADIOLIB_IRQ_CAD_DEFAULT_MASK}}; - int16_t result; setTransmitEnable(false); - setStandby(); - result = lora.scanChannel(cfg); - if (result == RADIOLIB_LORA_DETECTED) - return true; - if (result != RADIOLIB_CHANNEL_FREE) - LOG_ERROR("SX126X scanChannel %s%d", radioLibErr, result); + int16_t result = trySetStandby(); + if (result == RADIOLIB_ERR_NONE) { + result = lora.scanChannel(cfg); + if (result == RADIOLIB_LORA_DETECTED) + return true; + if (result != RADIOLIB_CHANNEL_FREE) + LOG_ERROR("SX126X scanChannel %s%d", radioLibErr, result); + if (result != RADIOLIB_ERR_WRONG_MODEM) + return false; + } #ifdef ARCH_PORTDUINO - if (result == RADIOLIB_ERR_WRONG_MODEM) - portduino_status.LoRa_in_error = true; -#else - assert(result != RADIOLIB_ERR_WRONG_MODEM); + portduino_status.LoRa_in_error = true; #endif - - return false; + // standby failed or the LoRa modem type is gone - the chip lost its runtime state + maybeRecoverChipStateLoss(); + return false; // report the channel free: a recovered chip can TX, a dead one fails startSend safely } /** Could we send right now (i.e. either not actively receiving or transmitting)? */ @@ -495,7 +511,7 @@ template bool SX126xInterface::sleep() // Not keeping config is busted - next time nrf52 board boots lora sending fails tcxo related? - see datasheet // \todo Display actual typename of the adapter, not just `SX126x` LOG_DEBUG("SX126x entering sleep mode"); // (FIXME, don't keep config) - setStandby(); // Stop any pending operations + (void)trySetStandby(); // Stop any pending operations - the chip is being put to sleep, a failure must not crash // turn off TCXO if it was powered // FIXME - this isn't correct diff --git a/src/mesh/SX126xInterface.h b/src/mesh/SX126xInterface.h index b683eff136..eb1080d8b9 100644 --- a/src/mesh/SX126xInterface.h +++ b/src/mesh/SX126xInterface.h @@ -99,5 +99,8 @@ template class SX126xInterface : public RadioLibInterface /** setStandby()'s body, returning the standby error instead of asserting - for callers that can recover */ int16_t trySetStandby(); + + /** Recover a chip that lost its runtime state: hardware-reset via begin() and reprogram */ + bool recoverChipStateLoss() override { return reinitChip() && programModemParams() == RADIOLIB_ERR_NONE; } }; #endif \ No newline at end of file diff --git a/src/mesh/SX128xInterface.cpp b/src/mesh/SX128xInterface.cpp index a73f0b44d3..3de65fad0e 100644 --- a/src/mesh/SX128xInterface.cpp +++ b/src/mesh/SX128xInterface.cpp @@ -62,7 +62,7 @@ template bool SX128xInterface::init() RadioLibInterface::init(); - if (!reinitChip()) + if (!reinitChip(/*fromInit=*/true)) return false; startReceive(); // start receiving @@ -72,7 +72,7 @@ template bool SX128xInterface::init() // begin() and the chip-side setup that a reset chip loses. Shared by init() and by reconfigure()'s // recovery of a chip that lost its state. -template bool SX128xInterface::reinitChip() +template bool SX128xInterface::reinitChip(bool fromInit) { // Clamp here, not just in programModemParams(): applyModemConfig() resets `power` to the raw // config value, and the recovery path reaches begin() without passing through the params clamp @@ -87,6 +87,12 @@ template bool SX128xInterface::reinitChip() return false; if ((config.lora.region != meshtastic_Config_LoRaConfig_RegionCode_LORA_24) && (res == RADIOLIB_ERR_INVALID_FREQUENCY)) { + // Boot-time only: rebooting out of a runtime recovery would reintroduce exactly the crash this + // recovery path exists to avoid, and would do it while a config save is still pending. + if (!fromInit) { + LOG_ERROR("SX128x rejected the frequency during recovery; leaving region alone"); + return false; + } LOG_WARN("Radio only supports 2.4GHz LoRa. Adjusting Region and rebooting"); config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_LORA_24; nodeDB->saveToDisk(SEGMENT_CONFIG); @@ -294,8 +300,6 @@ template void SX128xInterface::startReceive() sleep(); #else - setStandby(); - #if ARCH_PORTDUINO if (portduino_config.lora_rxen_pin.pin != RADIOLIB_NC) { digitalWrite(portduino_config.lora_rxen_pin.pin, HIGH); @@ -313,11 +317,22 @@ template void SX128xInterface::startReceive() #endif #endif - int err = lora.startReceive(RADIOLIB_SX128X_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); + int16_t err = trySetStandby(); + if (err == RADIOLIB_ERR_NONE) + err = lora.startReceive(RADIOLIB_SX128X_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); - if (err != RADIOLIB_ERR_NONE) + if (err != RADIOLIB_ERR_NONE) { LOG_ERROR("SX128X startReceive %s%d", radioLibErr, err); - assert(err == RADIOLIB_ERR_NONE); + if (maybeRecoverChipStateLoss()) + err = lora.startReceive(RADIOLIB_SX128X_RX_TIMEOUT_INF, MESHTASTIC_RADIOLIB_IRQ_RX_FLAGS); + } + + if (err != RADIOLIB_ERR_NONE) { + // No assert: leave RX off rather than reboot; periodicRadioMaintenance() re-arms it, throttled + LOG_ERROR("SX128X RX offline %s%d", radioLibErr, err); + rxOffline = true; + return; + } RadioLibInterface::startReceive(); @@ -338,17 +353,20 @@ template bool SX128xInterface::isChannelActive() .timeout = 0, .irqFlags = RADIOLIB_IRQ_CAD_DEFAULT_FLAGS, .irqMask = RADIOLIB_IRQ_CAD_DEFAULT_MASK}}; - int16_t result; + int16_t result = trySetStandby(); + if (result == RADIOLIB_ERR_NONE) { + result = lora.scanChannel(cfg); + if (result == RADIOLIB_LORA_DETECTED) + return true; + if (result != RADIOLIB_CHANNEL_FREE) + LOG_ERROR("SX128X scanChannel %s%d", radioLibErr, result); + if (result != RADIOLIB_ERR_WRONG_MODEM) + return false; + } - setStandby(); - result = lora.scanChannel(cfg); - if (result == RADIOLIB_LORA_DETECTED) - return true; - if (result != RADIOLIB_CHANNEL_FREE) - LOG_ERROR("SX128X scanChannel %s%d", radioLibErr, result); - assert(result != RADIOLIB_ERR_WRONG_MODEM); - - return false; + // standby failed or the LoRa modem type is gone - the chip lost its runtime state + maybeRecoverChipStateLoss(); + return false; // report the channel free: a recovered chip can TX, a dead one fails startSend safely } /** Could we send right now (i.e. either not actively receiving or transmitting)? */ @@ -362,7 +380,7 @@ template bool SX128xInterface::sleep() // Not keeping config is busted - next time nrf52 board boots lora sending fails tcxo related? - see datasheet // \todo Display actual typename of the adapter, not just `SX128x` LOG_DEBUG("SX128x entering sleep mode"); // (FIXME, don't keep config) - setStandby(); // Stop any pending operations + (void)trySetStandby(); // Stop any pending operations - the chip is being put to sleep, a failure must not crash // turn off TCXO if it was powered // FIXME - this isn't correct diff --git a/src/mesh/SX128xInterface.h b/src/mesh/SX128xInterface.h index 1857d4ced5..967142c49a 100644 --- a/src/mesh/SX128xInterface.h +++ b/src/mesh/SX128xInterface.h @@ -80,8 +80,13 @@ template class SX128xInterface : public RadioLibInterface int16_t programModemParams(); /** begin() and chip-side setup, shared by init() and by reconfigure()'s recovery of a chip that lost its state */ - bool reinitChip(); + /** @param fromInit true only for the boot-time call, which may adjust region and reboot on a + * 2.4GHz-only part; a runtime recovery must never reboot the node. */ + bool reinitChip(bool fromInit = false); /** setStandby()'s body, returning the standby error instead of asserting - for callers that can recover */ int16_t trySetStandby(); + + /** Recover a chip that lost its runtime state: hardware-reset via begin() and reprogram */ + bool recoverChipStateLoss() override { return reinitChip() && programModemParams() == RADIOLIB_ERR_NONE; } };