fix(mesh): return real status from reconfigure() and stop panicking on SPI errors#10430
Open
DatanoiseTV wants to merge 1 commit into
Open
fix(mesh): return real status from reconfigure() and stop panicking on SPI errors#10430DatanoiseTV wants to merge 1 commit into
DatanoiseTV wants to merge 1 commit into
Conversation
…I errors reconfigure() in all five chip drivers (SX126x, SX128x, LR11x0, LR20x0, RF95) was returning true unconditionally, even when individual SPI setters failed. The caller in RadioInterface::cppInit() reboots the device when reconfigure returns false, so the failure-recovery path was silently disabled. Track failures in a local bool and return its accumulated state. Same drivers also asserted on remote SPI calls in reconfigure() and setStandby() — a momentary glitch on the SPI bus would panic-reset the firmware. Replaced with log-and-continue; the new return value lets the caller make the reboot decision instead of crashing inline.
8 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make radio driver reconfigure() report real failure status (so RadioInterface::cppInit() can trigger its reboot-based recovery path) and to reduce panic-resets caused by SPI glitches by replacing some assert() calls with log-and-continue behavior.
Changes:
- Update
reconfigure()across multiple radio drivers to accumulate per-setter failures in a localbool okand return it instead of always returningtrue. - Replace several
assert(err == RADIOLIB_ERR_NONE)checks in radio reconfigure/standby paths with logging and failure tracking.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mesh/SX128xInterface.cpp | Adds ok accumulation in reconfigure() and removes a standby assert; still calls startReceive() unconditionally. |
| src/mesh/SX126xInterface.cpp | Adds ok accumulation in reconfigure() and removes a standby assert; ok does not include RX gain setter, and standby failures now only log at DEBUG. |
| src/mesh/RF95Interface.cpp | Adds ok accumulation in reconfigure(), but RF95 still has assert-based SPI panics in setStandby()/startReceive() that are reachable from reconfigure(). |
| src/mesh/LR20x0Interface.cpp | Adds ok accumulation in reconfigure() and removes standby assert; ok does not include RX gain setter and startReceive() is unconditional. |
| src/mesh/LR11x0Interface.cpp | Adds ok accumulation in reconfigure() and removes standby assert; ok does not include RX gain setter and startReceive() is unconditional. |
Comments suppressed due to low confidence (3)
src/mesh/SX126xInterface.cpp:276
reconfigure()now returnsok, butokis not updated whenlora.setRxBoostedGainMode(...)fails. As a result, a SPI glitch during this setter can still yieldreconfigure()==trueand prevent the caller’s reboot/recovery path. Consider settingok = falseon error (and optionally recording a critical error) to match the comment about tracking every SPI setter.
// Apply RX gain mode — valid in STDBY (datasheet §9.6), matches resetAGC() pattern
err = lora.setRxBoostedGainMode(config.lora.sx126x_rx_boosted_gain);
if (err != RADIOLIB_ERR_NONE)
LOG_WARN("SX126X setRxBoostedGainMode %s%d", radioLibErr, err);
src/mesh/LR11x0Interface.cpp:231
reconfigure()logs onsetRxBoostedGainMode(...)failure but does not setok = false, soreconfigure()can still return success after a SPI glitch in this setter. Consider markingokfalse on error to ensure the caller can trigger the intended recovery path.
// Apply RX gain mode — valid in STDBY, matches resetAGC() pattern
err = lora.setRxBoostedGainMode(config.lora.sx126x_rx_boosted_gain);
if (err != RADIOLIB_ERR_NONE)
LOG_WARN("LR11x0 setRxBoostedGainMode %s%d", radioLibErr, err);
src/mesh/LR20x0Interface.cpp:259
setRxBoostedGainMode(...)failures are only logged, butokis not set false. This can still makereconfigure()return true after a SPI glitch in this setter, bypassing the caller’s recovery logic. Consider settingok = falseon error.
// Apply RX gain mode — valid in STDBY, matches resetAGC() pattern
err = lora.setRxBoostedGainMode(config.lora.sx126x_rx_boosted_gain);
if (err != RADIOLIB_ERR_NONE)
LOG_WARN("LR20x0 setRxBoostedGainMode %s%d", radioLibErr, err);
| @@ -262,7 +277,7 @@ | |||
|
|
|||
| startReceive(); // restart receiving | |||
Comment on lines
292
to
299
| int err = lora.standby(); | ||
|
|
||
| if (err != RADIOLIB_ERR_NONE) | ||
| LOG_DEBUG("SX126x standby %s%d", radioLibErr, err); | ||
| #ifdef ARCH_PORTDUINO | ||
| if (err != RADIOLIB_ERR_NONE) | ||
| portduino_status.LoRa_in_error = true; | ||
| #else | ||
| assert(err == RADIOLIB_ERR_NONE); | ||
| #endif |
| ok = false; | ||
| } | ||
|
|
||
| startReceive(); // restart receiving |
| @@ -212,7 +232,7 @@ template <typename T> bool LR11x0Interface<T>::reconfigure() | |||
|
|
|||
| startReceive(); // restart receiving | |||
| @@ -240,7 +260,7 @@ template <typename T> bool LR20x0Interface<T>::reconfigure() | |||
|
|
|||
| startReceive(); // restart receiving | |||
Comment on lines
269
to
272
| startReceive(); // restart receiving | ||
|
|
||
| return true; | ||
| return ok; | ||
| } |
Member
|
@DatanoiseTV could you address the copilot findings here? |
Contributor
|
Tested on Nordic nRF54L15-DK with EBYTE E22-900M30S (SX1262) on EU_868,
LGTM. Tested-by: cvaldess |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
reconfigure() in all five chip drivers (SX126x, SX128x, LR11x0, LR20x0,
RF95) was returning true unconditionally, even when individual SPI
setters failed. The caller in RadioInterface::cppInit() reboots the
device when reconfigure returns false, so the failure-recovery path was
silently disabled. Track failures in a local bool and return its
accumulated state.
Same drivers also asserted on remote SPI calls in reconfigure() and
setStandby() — a momentary glitch on the SPI bus would panic-reset the
firmware. Replaced with log-and-continue; the new return value lets the
caller make the reboot decision instead of crashing inline.
Split out from #10424 per @thebentern's request — single-concern PR.
Build verification
pio run -e t-deck-tftsucceeds, no new warnings.Attestations
t-deck-tftonly.