fix(firmware): repair nRF USB firmware update and post-update reconnect#6018
Merged
Conversation
The USB (UF2) firmware-update path for nRF52 nodes was effectively broken
end-to-end. Three distinct issues, all fixed and verified on a RAK4631:
1. Update never started. performUsbUpdate sets state to AwaitingFileSave
(the deliberate pause for the SAF file picker that saveDfuFile resumes),
but FirmwareUpdateViewModel.startUpdate()/startUpdateFromFile() only
handled Success/Error — AwaitingFileSave fell into the else
"defense-in-depth" branch and was overwritten with Error. Tapping
"Update via USB File Transfer" always failed. Add an explicit
AwaitingFileSave branch to both when-blocks (+ regression tests).
2. Unstable device identity. USB serial devices were keyed by
UsbDevice.deviceName (the /dev/bus/usb/BBB/DDD path), which Android
reassigns on every re-enumeration. After the post-update reboot the node
returns on a new path, so the saved address no longer resolved and
auto-reconnect could not recognise it. Key by a stable
vendorId-productId identity (UsbDevice.usbSerialStableKey()) in
UsbRepository, and build the saved SERIAL address from that stable key
in AndroidGetDiscoveredDevicesUseCase (it previously used the path).
3. Post-update reconnect race. verifyUpdateResult forced a one-shot
setDeviceAddress ~5s after reboot. For USB the node re-enumerates
through the bootloader over ~20s, so the forced reconnect fired into the
enumeration gap, failed ("Serial device not found"), and landed in
Disconnected — preempting the dedicated USB auto-recovery
(observeUsbRecoveryTriggers), which only arms from DeviceSleep. Skip the
forced reconnect for serial and let USB auto-recovery own it; BLE/TCP
still need the explicit re-address (no hot-plug presence; address is set
to "n" during the update).
Also register the 0x239A/0x4405 USB CDC id in ProbeTableProvider so Elecrow
ThinkNode M3/M4/M6 and LilyGo T-Echo are recognised as USB serial devices
at all (previously only RAK4631 0x8029 was). This piece is unverified on
hardware — see PR notes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a stable ChangesUSB device identity and firmware update flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@core/network/src/androidMain/kotlin/org/meshtastic/core/network/repository/UsbRepository.kt`:
- Around line 62-66: The USB discovery map in UsbRepository currently replaces
the old path-based SERIAL key with only usbSerialStableKey(), which breaks saved
deviceName aliases and causes identical VID/PID boards to collide. Update the
device lookup/discovery flow to preserve backward compatibility by keeping an
alias or fallback for the old USB path key while also publishing the stable key,
and make sure duplicate devices are retained instead of silently overwriting
each other in the map. Apply the same fix in the related discovery block
referenced by the comment so existing installs still resolve previously saved
addresses.
In
`@feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.kt`:
- Around line 388-391: The post-update logging in FirmwareUpdateViewModel
currently prints fullAddr directly, which can expose device identifiers. Update
the Logger.i messages in the reconnect path to pass fullAddr through the
existing anonymize helper first, and ensure the anonymize import is present in
FirmwareUpdateViewModel. Keep the actual reconnect behavior via
radioController.setDeviceAddress unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 009e96ba-f7cc-4d70-958b-328b11ca4a8e
📒 Files selected for processing (5)
core/network/src/androidMain/kotlin/org/meshtastic/core/network/repository/ProbeTableProvider.ktcore/network/src/androidMain/kotlin/org/meshtastic/core/network/repository/UsbRepository.ktfeature/connections/src/androidMain/kotlin/org/meshtastic/feature/connections/domain/usecase/AndroidGetDiscoveredDevicesUseCase.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.ktfeature/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt
Addresses CodeRabbit review on #6018: fullAddr can contain a persisted BLE MAC / TCP endpoint / serial id, so route it through the existing core.model.util.anonymize helper before logging. Reconnect behaviour (setDeviceAddress) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Why
A community report ("can we do USB DFU for nRF nodes? I'm only seeing OTA") turned out not to be a missing feature — the USB (UF2) firmware-update path for nRF52 already exists, but it was broken end-to-end. Tapping Update via USB File Transfer always failed, and even when it flashed, the app couldn't reconnect afterward. This fixes the whole flow. Verified on a RAK4631 over USB.
🐛 Bug fixes
USB update never started.
performUsbUpdatesets state toAwaitingFileSave— the deliberate pause where the UI launches the SAF file picker andsaveDfuFile()resumes the flow. ButFirmwareUpdateViewModel.startUpdate()/startUpdateFromFile()only handledSuccess/Error, soAwaitingFileSavefell into theelse"defense-in-depth" branch and was overwritten withError. Symptom in logcat:Firmware update returned without terminal state: AwaitingFileSave(...). Added an explicitAwaitingFileSavebranch to bothwhenblocks.Post-update reconnect race.
verifyUpdateResultforced a one-shotsetDeviceAddress~5s after reboot. A USB node re-enumerates through the bootloader over ~20s, so the forced reconnect fired into the enumeration gap, failed (Serial device not found), and landed the transport inDisconnected— which preempts the dedicated USB auto-recovery (observeUsbRecoveryTriggers, which only arms fromDeviceSleep). Now we skip the forced reconnect for serial and let auto-recovery own it. BLE/TCP still force it (they have no hot-plug presence and the address is set to"n"during the update, so the explicit re-address is load-bearing there).🛠️ Improvements
Stable USB serial identity. Devices were keyed by
UsbDevice.deviceName(the/dev/bus/usb/BBB/DDDpath), which Android reassigns on every re-enumeration — so after the post-update reboot the saved address no longer matched and auto-reconnect couldn't recognise the node. Now keyed by a stablevendorId-productId(UsbDevice.usbSerialStableKey()) inUsbRepository, and the savedSERIALaddress is built from that key inAndroidGetDiscoveredDevicesUseCase(it previously baked in the path). Deliberately not the USB serial number —getSerialNumber()needs a permission grant that's missing right after re-enumeration, which would defeat the recovery. (Trade-off: two identical boards collide; documented in-code with the upgrade path.)Recognise more nRF boards over USB. Registered⚠️ Unverified on hardware — I don't have an M3/M4/M6/T-Echo to test the connect path; the vid:pid is confirmed against the firmware board JSONs (
0x239A/0x4405inProbeTableProviderso Elecrow ThinkNode M3/M4/M6 and LilyGo T-Echo are detected as USB CDC serial devices at all (previously only RAK46310x8029was).build.hwids[0]). Happy to split this into its own PR if reviewers prefer to keep this one all-verified.Testing performed
:feature:firmware:jvmTest— green (19 tests, incl. 2 new regression tests assertingstartUpdate/startUpdateFromFilekeepAwaitingFileSaveinstead of clobbering it toError).spotlessCheck+detekt— clean.DeviceSleeppreserved →Post-update: leaving USB reconnect to USB auto-recovery→Serial device connected in 23ms→Connected. The device's stable key (9114-32809) held across the path churn (/002 → … → /011).Reviewer notes
verifyUpdateResultis intentional and asymmetric: USB skips the forced reconnect (auto-recovery handles it), BLE/WiFi keep it (no presence signal; address is"n"post-update). See the in-code comment for the rationale.UsbDeviceinstance and re-prompts on re-enumeration unless the user checked "always open this app" via thedevice_filter.xmlattach dialog.Summary by CodeRabbit
New Features
Bug Fixes
Tests