fix(firmware): Confirm ESP32 OTA mode before disconnecting#6080
fix(firmware): Confirm ESP32 OTA mode before disconnecting#6080jeremiah-k wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds OTA client-notification filtering, a notification-gated OTA preflight flow in ChangesOTA Preflight Confirmation and Notification Handling
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Esp32OtaUpdateHandler
participant MeshTransport
participant Firmware
Esp32OtaUpdateHandler->>Firmware: trigger reboot OTA (BLE/WiFi mode)
Esp32OtaUpdateHandler->>Esp32OtaUpdateHandler: runOtaPreflight(otaPreflightTimeoutMs)
Esp32OtaUpdateHandler->>Firmware: awaitOtaConfirmation via clientNotification
Firmware-->>Esp32OtaUpdateHandler: ClientNotification message
alt Confirmed
Esp32OtaUpdateHandler->>MeshTransport: disconnectMeshService() + delay
else Rejected
Esp32OtaUpdateHandler->>Esp32OtaUpdateHandler: emit FirmwareUpdateState.Error(rejection reason)
else Timeout
Esp32OtaUpdateHandler->>Esp32OtaUpdateHandler: emit FirmwareUpdateState.Error(timeout)
end
Related issues: None found. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImpl.kt (1)
179-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOTA classification duplicated (and narrower) than
Esp32OtaUpdateHandler's matcher.
isOtaStatusNotification()only suppresses the generic alert when the message starts with"Rebooting to"or one of three hardcoded rejection prefixes.Esp32OtaUpdateHandler.awaitOtaConfirmation()(feature/firmware) classifies any non-baseline message containing"OTA"as preflight-relevant (Confirmed or Rejected). The two files also independently redefine identicalOTA_KEYWORD/OTA_CONFIRM_PREFIXconstants.Today the enumerated prefixes here happen to match every rejection message documented in
Esp32OtaUpdateHandler's comments, so there's no active bug — but if firmware ever adds a new OTA-related rejection string not in this list,Esp32OtaUpdateHandlerwill correctly classify it asRejectedand show its own error, while this function will fail to suppress the generic alert, reintroducing exactly the duplicate-popup bug this PR sets out to fix. Consider loosening this check to mirror the broadercontains("OTA")criterion (or extracting a single shared classifier both files call), so the two consumers can't drift.♻️ Proposed simplification to avoid drift
-private const val OTA_KEYWORD = "OTA" -private const val OTA_CONFIRM_PREFIX = "Rebooting to" -private val OTA_REJECTION_PREFIXES = listOf("Cannot start OTA", "OTA Loader", "Unable to switch to the OTA partition") - internal fun ClientNotification.isOtaStatusNotification(): Boolean { val message = message.trim() - if (message.isBlank() || !message.contains(OTA_KEYWORD)) return false - - return message.startsWith(OTA_CONFIRM_PREFIX) || OTA_REJECTION_PREFIXES.any { message.startsWith(it) } + return message.contains("OTA") }🤖 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 `@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImpl.kt` around lines 179 - 189, `isOtaStatusNotification()` is narrower than `Esp32OtaUpdateHandler.awaitOtaConfirmation()` and duplicates OTA constants, which can let new OTA rejection messages slip through and trigger duplicate alerts. Update the notification classifier in `FromRadioPacketHandlerImpl` to use the same broader OTA-matching rule as the handler, or extract a shared OTA status classifier/constant set that both `isOtaStatusNotification()` and `awaitOtaConfirmation()` call so their behavior cannot drift.
🤖 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
`@feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.kt`:
- Around line 250-256: The downloading message fallback in
Esp32OtaUpdateHandler’s update flow is too broad because safeCatchingAll
swallows every Throwable and can hide real production resource-loading failures
by returning an empty string. Narrow this handling to the specific Skiko
headless test case around
getStringSuspend(Res.string.firmware_update_downloading_percent, 0), or add
logging before falling back so failures are visible while still preserving the
blank-message test behavior. Keep the fix localized to the downloadingMsg
computation and the surrounding OTA status emission path.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImpl.kt`:
- Around line 179-189: `isOtaStatusNotification()` is narrower than
`Esp32OtaUpdateHandler.awaitOtaConfirmation()` and duplicates OTA constants,
which can let new OTA rejection messages slip through and trigger duplicate
alerts. Update the notification classifier in `FromRadioPacketHandlerImpl` to
use the same broader OTA-matching rule as the handler, or extract a shared OTA
status classifier/constant set that both `isOtaStatusNotification()` and
`awaitOtaConfirmation()` call so their behavior cannot drift.
🪄 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: 893c0951-f53b-4ec1-83da-5c5fb6ea3776
📒 Files selected for processing (8)
.skills/compose-ui/strings-index.txtcore/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImplTest.ktcore/resources/src/commonMain/composeResources/values/strings.xmlfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransport.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/DefaultFirmwareUpdateManagerTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt
…ation Replace the fire-and-forget triggerRebootOta + fixed delay + mesh disconnect flow with a preflight gate that waits for the firmware's ClientNotification response before tearing down the mesh transport. The firmware emits a single meshtastic.ClientNotification before its scheduled reboot covering all cases: - Success: 'Rebooting to BLE/WiFi OTA' - Rejection: 'OTA Loader does not support BLE/WiFi', partition missing, switch failed, etc. Previously, the app sent the OTA reboot command, waited a fixed 2s for packet delivery, then unconditionally disconnected the mesh service and entered the OTA transport retry loop — even when the device's OTA loader rejected the request. This caused repeated failed connection attempts to a service that was never listening. Now: after triggerRebootOta, the app races the ClientNotification StateFlow against a 5s timeout. On confirmed reboot → disconnect mesh and proceed with OTA transport. On rejection → fail immediately with actionable error text, mesh transport preserved. On timeout → fail with 'device did not enter OTA mode', mesh transport preserved. The baseline notification message is captured BEFORE the OTA trigger is sent so a stale notification already held in the StateFlow cannot be mistaken for the firmware response. The confirmation match requires both the canonical 'Rebooting to' prefix and the requested mode name (BLE/WiFi), guarding against a wrong-mode ack. New strings: - firmware_update_ota_unsupported - firmware_update_ota_timeout Tests cover the three preflight outcomes (Confirmed, Rejected, Timeout) verifying mesh transport is released only on confirmation.
Keep stale ClientNotification clearing before the OTA trigger and route preflight timeout handling through an overridable timeout value so timeout coverage does not pay the production wait. Replace raw reboot-mode values with named BLE and WiFi reboot mode constants and use a single helper to map reboot modes to firmware notification mode names. Surface firmware rejection messages through a localized error resource so unsupported-loader and partition failures can show the firmware-provided reason instead of only the generic unsupported update message.
jeremiah-k
force-pushed
the
bugfix/esp32-ota-preflight
branch
from
5741015 to
867d8e5
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push. The preflight gating is a good idea, but a few things block merge as-is — most importantly the PR's own headline claim isn't delivered by this branch alone.
The "suppress duplicate OTA status alerts" commit only gates the wrong path. It suppresses notificationManager.dispatch in FromRadioPacketHandlerImpl, but setClientNotification(cn) still fires and UIViewModel's alert collector — untouched on this branch — still pops the generic dialog for every OTA notification. The UIViewModel gate exists only in #6081's extra commits, so #6080 merged on its own still shows the duplicate popup it says it removes. Either fold the UIViewModel gate into this PR or reorder the stack.
Other blockers and notes inline. The older-firmware concern (item on line 193) needs a firmware-history check before this can ship — if any released firmware handles ota_request but predates the confirmation notification, this permanently breaks OTA for it.
| serviceStateWriter.setClientNotification(cn) | ||
|
|
||
| scope.handledLaunch { | ||
| if (cn.isOtaStatusNotification()) { |
There was a problem hiding this comment.
This suppresses only the OS-notification dispatch. setClientNotification(cn) above still runs, and UIViewModel (not modified on this branch) still calls showAlert for every non-null clientNotification — so the generic "Rebooting to BLE OTA" popup this PR claims to remove still fires when #6080 is merged standalone. The gate belongs on the UIViewModel alert path too (it's in #6081 — pull it forward or restack).
| // meshtastic.ClientNotification warning ("OTA Loader does not support <mode>", etc.) emitted before | ||
| // its scheduled reboot, so we race that signal against a bounded timeout. On rejection or timeout we | ||
| // fail fast and leave the mesh connection intact — no disconnect, no OTA-transport retry loop. | ||
| if (runOtaPreflight(rebootMode, baselineMessage, updateState) !is OtaPreflightResult.Confirmed) { |
There was a problem hiding this comment.
OTA is hard-gated on a Confirmed notification within 5s with no firmware-version fallback. If any released firmware handles the ota_request admin message but predates the "Rebooting to OTA" emission, it reboots into OTA yet never sends the notification → permanent Timeout, and OTA that worked before this PR is now broken for it. Please confirm the emission shipped no later than ota_request handling; otherwise gate the preflight on firmware version (fall back to the old fixed-delay path below the threshold).
| when (val preflight = awaitOtaConfirmation(otaPreflightTimeoutMs, rebootMode, baselineMessage)) { | ||
| is OtaPreflightResult.Confirmed -> { | ||
| Logger.i { "ESP32 OTA: Preflight confirmed; releasing mesh transport for OTA" } | ||
| disconnectMeshService() |
There was a problem hiding this comment.
New: on the Confirmed path you disconnectMeshService() but never clear the confirmation notification, so "Rebooting to BLE OTA" stays resident in clientNotification. Combined with the ungated UIViewModel alert, that means a spurious popup on the success path this PR was meant to make silent. Clear it after a confirmed match.
| withTimeoutOrNull(timeoutMs) { | ||
| serviceRepository.clientNotification.first { cn -> | ||
| val msg = cn?.message | ||
| msg != null && msg != baselineMessage && msg.contains(OTA_KEYWORD) |
There was a problem hiding this comment.
The timeout message (firmware_update_ota_timeout, "Device did not enter OTA mode…") is inverted: triggerRebootOta was already sent, so on a lost/late confirmation the device may well be in OTA mode. Also this predicate is an untrimmed contains("OTA") catch-all while the isOtaStatusNotification suppression uses a trimmed prefix allowlist — the two classifiers disagree on whitespace and on any OTA-containing message outside the four prefixes. Worth extracting one shared classifier.
| private val firmwareFileHandler: FirmwareFileHandler, | ||
| private val radioController: RadioController, | ||
| private val nodeRepository: NodeRepository, | ||
| private val serviceRepository: ServiceRepository, |
There was a problem hiding this comment.
Redundant DI: RadioController (already injected) exposes clientNotification and clearClientNotification delegating to this same ServiceRepository. Dropping the extra param would also undo the serviceRepository-mock churn the tests had to add.
| // compose-resources can't load native libs. Production resolves the localized string normally; | ||
| // tests fall back to empty and the Downloading state still emits with a blank message. | ||
| val downloadingMsg = | ||
| safeCatchingAll { getStringSuspend(Res.string.firmware_update_downloading_percent, 0) } |
There was a problem hiding this comment.
This safeCatchingAll is a production behavior change (silent blank label on any resource failure) that the comment admits exists only to dodge a headless-test Skiko init crash — and it's the sole guarded getStringSuspend in the file, so it's inconsistent anyway. Better to stub string resolution in the test than swallow it in prod.
2 participants
Overview
This PR makes ESP32 OTA startup wait for firmware confirmation before disconnecting the normal mesh connection.
Previously, the app could treat delivery of the reboot request as enough to continue into OTA mode. If firmware rejected the requested OTA mode because the installed OTA loader did not support BLE or Wi-Fi, the app could still disconnect and enter a doomed retry loop.
This change waits for a fresh firmware-side OTA response before switching away from the normal connection.
Key Changes
Testing
git diff --checkpasses.Migration Notes
Summary by CodeRabbit