Conversation
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
Contributor
📝 WalkthroughWalkthroughAdds configurable LR1110 firmware embedding for supported targets. Initialization records device information, recovers from selected startup errors, updates older firmware once, and verifies reinitialization. ChangesLR11x0 firmware lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LR11x0Interface
participant LR1110
participant EmbeddedFirmware
LR11x0Interface->>LR1110: initialize
LR1110-->>LR11x0Interface: startup status and version
LR11x0Interface->>EmbeddedFirmware: select target image
LR11x0Interface->>LR1110: recover or update firmware
LR1110-->>LR11x0Interface: update result
LR11x0Interface->>LR1110: reinitialize and query version
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
variants/esp32s3/ELECROW-ThinkNode-M7/variant.h (1)
36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce this comment to two lines.
Keep the update reason and removal condition. Move detailed release-history information to PR documentation or an issue.
As per coding guidelines, keep comments minimal—normally one or two lines.
Proposed reduction
-// TEMPORARY: units shipped with LR1110 transceiver FW 0x0303 (the original 2020 release), which cannot -// reliably demodulate 500 kHz LoRa - Turbo presets fail RX with ~41% byte errors while TX and narrower -// bandwidths are fine. 0x0303 also predates GetLoRaRxHeaderInfos, which computePacketTime() calls on every -// received packet. Confirmed fixed by updating to 0x0307; targeting 0x0402 additionally picks up the -// out-of-band emission fix for consecutive LoRa transmissions (0x0401) and three CVE fixes (0x0402). -// Costs ~240 kB of flash. Remove once the affected units are updated. +// Temporarily update LR1110 0x0303 units because Turbo RX and GetLoRaRxHeaderInfos require newer firmware. +// Target 0x0402 includes emission and security fixes. Remove after affected units update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@variants/esp32s3/ELECROW-ThinkNode-M7/variant.h` around lines 36 - 41, Reduce the comment above the LR1110 firmware requirement to two lines, retaining only the update reason and the condition that it should be removed after affected units are updated. Remove detailed firmware release history, failure behavior, fix details, and flash-size information from the comment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mesh/LR11x0Interface.cpp`:
- Around line 153-189: Reset transceiverFw and transceiverDevice before the
initial getVersionInfo call so a failed query cannot reuse stale state. After
the firmware update and re-initialization, require getVersionInfo to succeed,
refresh both fields, and verify the resulting firmware is at least
LR11X0_UPDATE_FIRMWARE_TO before reporting success; otherwise log the failure or
version mismatch and return false.
---
Nitpick comments:
In `@variants/esp32s3/ELECROW-ThinkNode-M7/variant.h`:
- Around line 36-41: Reduce the comment above the LR1110 firmware requirement to
two lines, retaining only the update reason and the condition that it should be
removed after affected units are updated. Remove detailed firmware release
history, failure behavior, fix details, and flash-size information from the
comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d21baf69-3572-4b66-8767-4aa233967dc6
📒 Files selected for processing (3)
src/mesh/LR11x0Interface.cppsrc/mesh/LR11x0Interface.hvariants/esp32s3/ELECROW-ThinkNode-M7/variant.h
Comment on lines
153
to
+189
| LR11x0VersionInfo_t version; | ||
| res = lora.getVersionInfo(&version); | ||
| if (res == RADIOLIB_ERR_NONE) | ||
| if (res == RADIOLIB_ERR_NONE) { | ||
| LOG_DEBUG("LR11x0 Device %d, HW %d, FW %d.%d, WiFi %d.%d, GNSS %d.%d", version.device, version.hardware, version.fwMajor, | ||
| version.fwMinor, version.fwMajorWiFi, version.fwMinorWiFi, version.fwGNSS, version.almanacGNSS); | ||
| transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; | ||
| transceiverDevice = version.device; | ||
| } | ||
|
|
||
| #ifdef LR11X0_UPDATE_FIRMWARE_TO | ||
| // One-shot transceiver firmware update, opt-in per variant. Only runs when the part is an LR1110 running | ||
| // older firmware than the baked-in image, so once it has succeeded it is a no-op on subsequent boots. | ||
| if (transceiverDevice == RADIOLIB_LR11X0_DEVICE_LR1110 && transceiverFw != 0 && transceiverFw < LR11X0_UPDATE_FIRMWARE_TO) { | ||
| LOG_WARN("LR1110 transceiver FW %d.%d is older than %d.%d - updating now. DO NOT POWER OFF: this " | ||
| "erases and rewrites the radio's own flash.", | ||
| transceiverFw >> 8, transceiverFw & 0xFF, LR11X0_UPDATE_FIRMWARE_TO >> 8, LR11X0_UPDATE_FIRMWARE_TO & 0xFF); | ||
|
|
||
| int upd = lora.updateFirmware(lr11xx_firmware_image, LR11XX_FIRMWARE_IMAGE_SIZE, true); | ||
| if (upd != RADIOLIB_ERR_NONE) { | ||
| // The radio is likely sitting in bootloader mode. It is not bricked - the update is retried on | ||
| // the next boot because the version check above will still see old (or unreadable) firmware. | ||
| LOG_ERROR("LR1110 firmware update FAILED %s%d - power-cycle to retry", radioLibErr, upd); | ||
| return false; | ||
| } | ||
|
|
||
| LOG_INFO("LR1110 firmware update complete, re-initializing radio"); | ||
| res = lora.begin(getFreq(), bw, sf, cr, syncWord, power, preambleLength, tcxoVoltage); | ||
| if (res != RADIOLIB_ERR_NONE) { | ||
| LOG_ERROR("LR11x0 re-init after firmware update failed %s%d", radioLibErr, res); | ||
| return false; | ||
| } | ||
|
|
||
| if (lora.getVersionInfo(&version) == RADIOLIB_ERR_NONE) { | ||
| transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; | ||
| transceiverDevice = version.device; | ||
| LOG_INFO("LR1110 now running transceiver FW %d.%d", version.fwMajor, version.fwMinor); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset and verify the transceiver version state.
A failed getVersionInfo() call leaves the previous transceiverFw and transceiverDevice values intact. A later init() can then start an erase-and-reflash operation from stale version data during a communication failure.
After the update, a failed version query also returns success without verifying that the radio runs at least LR11X0_UPDATE_FIRMWARE_TO. Clear both fields before the first query. After the update, require a successful query and validate the resulting version.
Proposed fix
+ transceiverFw = 0;
+ transceiverDevice = 0;
LR11x0VersionInfo_t version;
res = lora.getVersionInfo(&version);
if (res == RADIOLIB_ERR_NONE) {
LOG_DEBUG("LR11x0 Device %d, HW %d, FW %d.%d, WiFi %d.%d, GNSS %d.%d", version.device, version.hardware, version.fwMajor,
version.fwMinor, version.fwMajorWiFi, version.fwMinorWiFi, version.fwGNSS, version.almanacGNSS);
transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor;
transceiverDevice = version.device;
}
...
- if (lora.getVersionInfo(&version) == RADIOLIB_ERR_NONE) {
- transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor;
- transceiverDevice = version.device;
- LOG_INFO("LR1110 now running transceiver FW %d.%d", version.fwMajor, version.fwMinor);
- }
+ res = lora.getVersionInfo(&version);
+ if (res != RADIOLIB_ERR_NONE) {
+ LOG_ERROR("LR1110 firmware update could not be verified %s%d", radioLibErr, res);
+ return false;
+ }
+
+ transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor;
+ transceiverDevice = version.device;
+ if (transceiverFw < LR11X0_UPDATE_FIRMWARE_TO) {
+ LOG_ERROR("LR1110 firmware update did not reach target version");
+ return false;
+ }
+ LOG_INFO("LR1110 now running transceiver FW %d.%d", version.fwMajor, version.fwMinor);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| LR11x0VersionInfo_t version; | |
| res = lora.getVersionInfo(&version); | |
| if (res == RADIOLIB_ERR_NONE) | |
| if (res == RADIOLIB_ERR_NONE) { | |
| LOG_DEBUG("LR11x0 Device %d, HW %d, FW %d.%d, WiFi %d.%d, GNSS %d.%d", version.device, version.hardware, version.fwMajor, | |
| version.fwMinor, version.fwMajorWiFi, version.fwMinorWiFi, version.fwGNSS, version.almanacGNSS); | |
| transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; | |
| transceiverDevice = version.device; | |
| } | |
| #ifdef LR11X0_UPDATE_FIRMWARE_TO | |
| // One-shot transceiver firmware update, opt-in per variant. Only runs when the part is an LR1110 running | |
| // older firmware than the baked-in image, so once it has succeeded it is a no-op on subsequent boots. | |
| if (transceiverDevice == RADIOLIB_LR11X0_DEVICE_LR1110 && transceiverFw != 0 && transceiverFw < LR11X0_UPDATE_FIRMWARE_TO) { | |
| LOG_WARN("LR1110 transceiver FW %d.%d is older than %d.%d - updating now. DO NOT POWER OFF: this " | |
| "erases and rewrites the radio's own flash.", | |
| transceiverFw >> 8, transceiverFw & 0xFF, LR11X0_UPDATE_FIRMWARE_TO >> 8, LR11X0_UPDATE_FIRMWARE_TO & 0xFF); | |
| int upd = lora.updateFirmware(lr11xx_firmware_image, LR11XX_FIRMWARE_IMAGE_SIZE, true); | |
| if (upd != RADIOLIB_ERR_NONE) { | |
| // The radio is likely sitting in bootloader mode. It is not bricked - the update is retried on | |
| // the next boot because the version check above will still see old (or unreadable) firmware. | |
| LOG_ERROR("LR1110 firmware update FAILED %s%d - power-cycle to retry", radioLibErr, upd); | |
| return false; | |
| } | |
| LOG_INFO("LR1110 firmware update complete, re-initializing radio"); | |
| res = lora.begin(getFreq(), bw, sf, cr, syncWord, power, preambleLength, tcxoVoltage); | |
| if (res != RADIOLIB_ERR_NONE) { | |
| LOG_ERROR("LR11x0 re-init after firmware update failed %s%d", radioLibErr, res); | |
| return false; | |
| } | |
| if (lora.getVersionInfo(&version) == RADIOLIB_ERR_NONE) { | |
| transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; | |
| transceiverDevice = version.device; | |
| LOG_INFO("LR1110 now running transceiver FW %d.%d", version.fwMajor, version.fwMinor); | |
| } | |
| transceiverFw = 0; | |
| transceiverDevice = 0; | |
| LR11x0VersionInfo_t version; | |
| res = lora.getVersionInfo(&version); | |
| if (res == RADIOLIB_ERR_NONE) { | |
| LOG_DEBUG("LR11x0 Device %d, HW %d, FW %d.%d, WiFi %d.%d, GNSS %d.%d", version.device, version.hardware, version.fwMajor, | |
| version.fwMinor, version.fwMajorWiFi, version.fwMinorWiFi, version.fwGNSS, version.almanacGNSS); | |
| transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; | |
| transceiverDevice = version.device; | |
| } | |
| `#ifdef` LR11X0_UPDATE_FIRMWARE_TO | |
| // One-shot transceiver firmware update, opt-in per variant. Only runs when the part is an LR1110 running | |
| // older firmware than the baked-in image, so once it has succeeded it is a no-op on subsequent boots. | |
| if (transceiverDevice == RADIOLIB_LR11X0_DEVICE_LR1110 && transceiverFw != 0 && transceiverFw < LR11X0_UPDATE_FIRMWARE_TO) { | |
| LOG_WARN("LR1110 transceiver FW %d.%d is older than %d.%d - updating now. DO NOT POWER OFF: this " | |
| "erases and rewrites the radio's own flash.", | |
| transceiverFw >> 8, transceiverFw & 0xFF, LR11X0_UPDATE_FIRMWARE_TO >> 8, LR11X0_UPDATE_FIRMWARE_TO & 0xFF); | |
| int upd = lora.updateFirmware(lr11xx_firmware_image, LR11XX_FIRMWARE_IMAGE_SIZE, true); | |
| if (upd != RADIOLIB_ERR_NONE) { | |
| // The radio is likely sitting in bootloader mode. It is not bricked - the update is retried on | |
| // the next boot because the version check above will still see old (or unreadable) firmware. | |
| LOG_ERROR("LR1110 firmware update FAILED %s%d - power-cycle to retry", radioLibErr, upd); | |
| return false; | |
| } | |
| LOG_INFO("LR1110 firmware update complete, re-initializing radio"); | |
| res = lora.begin(getFreq(), bw, sf, cr, syncWord, power, preambleLength, tcxoVoltage); | |
| if (res != RADIOLIB_ERR_NONE) { | |
| LOG_ERROR("LR11x0 re-init after firmware update failed %s%d", radioLibErr, res); | |
| return false; | |
| } | |
| res = lora.getVersionInfo(&version); | |
| if (res != RADIOLIB_ERR_NONE) { | |
| LOG_ERROR("LR1110 firmware update could not be verified %s%d", radioLibErr, res); | |
| return false; | |
| } | |
| transceiverFw = ((uint16_t)version.fwMajor << 8) | version.fwMinor; | |
| transceiverDevice = version.device; | |
| if (transceiverFw < LR11X0_UPDATE_FIRMWARE_TO) { | |
| LOG_ERROR("LR1110 firmware update did not reach target version"); | |
| return false; | |
| } | |
| LOG_INFO("LR1110 now running transceiver FW %d.%d", version.fwMajor, version.fwMinor); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mesh/LR11x0Interface.cpp` around lines 153 - 189, Reset transceiverFw and
transceiverDevice before the initial getVersionInfo call so a failed query
cannot reuse stale state. After the firmware update and re-initialization,
require getVersionInfo to succeed, refresh both fields, and verify the resulting
firmware is at least LR11X0_UPDATE_FIRMWARE_TO before reporting success;
otherwise log the failure or version mismatch and return false.
vidplace7
added
the
hardware-support
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
3 participants
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.
Turns out the M7 ships with really old firmware on the Semtech chip itself, and that firmware refuses to RX our Turbo presets. This patch adds the firmware update, but also uses an extra 250k of flash to do so.
Flash: [==========] 97.8% (used 2372003 bytes from 2424832 bytes)This is an 8 meg flash chip, and we could switch to the 8 meg partition, but that would result in a wipe.
Summary by CodeRabbit
New Features
Bug Fixes