fix(firmware): Retry ESP32 OTA service connections cleanly#6081
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR suppresses duplicate OTA-status alerts by adding an OTA notification detector and a FirmwareUpdateStatusRepository consumed in the data and UI layers, and reworks ESP32 OTA firmware updates with a preflight confirmation gate, WiFi UDP discovery, transport timeout handling, connection retries, related strings, and tests. ChangesOTA Status Notification Suppression
Estimated code review effort: 3 (Moderate) | ~25 minutes BLE/WiFi OTA Transport Reliability and Preflight Gating
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Esp32OtaUpdateHandler
participant ServiceRepository
participant WifiOtaDiscovery
participant BleOtaTransport
participant WifiOtaTransport
participant FirmwareUpdateManager
Esp32OtaUpdateHandler->>ServiceRepository: wait for clientNotification confirmation
Esp32OtaUpdateHandler->>ServiceRepository: disconnect mesh after confirmed reboot request
Esp32OtaUpdateHandler->>WifiOtaDiscovery: discoverOtaDevice()
Esp32OtaUpdateHandler->>BleOtaTransport: connect()
Esp32OtaUpdateHandler->>WifiOtaTransport: connect()
Esp32OtaUpdateHandler->>FirmwareUpdateManager: stream OTA chunks
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
♻️ Duplicate comments (1)
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt (1)
341-350: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate of
isOtaStatusNotification()inFromRadioPacketHandlerImpl.kt.Same concern as flagged in
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImpl.kt(lines 179-189): this is a copy of the same constants and matching logic. Recommend consolidating into one shared function used by both the data-layer packet handler and this view model to avoid the two detection rules diverging over time.🤖 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/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt` around lines 341 - 350, This OTA notification matcher is duplicated in the view model and the data-layer handler, so consolidate the shared constants and matching logic into one common function instead of keeping two copies. Move the logic from isOtaStatusNotificationMessage in UIViewModel and the equivalent isOtaStatusNotification() in FromRadioPacketHandlerImpl into a shared utility/location both can call, and keep the OTA_STATUS_KEYWORD, OTA_CONFIRM_PREFIX, and OTA_REJECTION_PREFIXES definitions in that single implementation.
🧹 Nitpick comments (2)
feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt (2)
156-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing BLE analog of the WiFi retry-exhaustion test.
The WiFi path has a test asserting the terminal
Errorstate after exhaustingWIFI_CONNECT_ATTEMPTS(Lines 322-355), but there's no equivalent BLE test verifying the final-error path when every BLE connection attempt fails. Given this PR's stated goal of "surfaces a clearer final error if OTA mode was confirmed but the service never became reachable," this scenario deserves coverage for both transports since they use distincttransportFactory/attempt-count wiring.Consider adding a
Confirmed BLE preflight closes failed transports and surfaces connection error after retriestest mirroring lines 322-355, usingbleTransportFactoryOverridewith an always-failingconnectResult.Also applies to: 322-355
🤖 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 `@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt` around lines 156 - 186, Add a BLE retry-exhaustion test alongside the existing WiFi terminal-error coverage by extending Esp32OtaUpdateHandlerTest with a case that uses bleTransportFactoryOverride and an always-failing FakeOtaTransport sequence. The new test should verify that each failed transport is closed, a fresh transport is created for each retry, and that runConfirmedPreflightTest ultimately records the final FirmwareUpdateState.Error once BLE connection attempts are exhausted. Mirror the existing WiFi retry-exhaustion pattern so the BLE-specific transportFactory and attempt-count behavior are covered by name.
359-407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated fixture setup instead of reusing
runConfirmedPreflightTest.The Rejected and Timeout tests manually recreate
serviceRepository/radioController/nodeRepository/handler/states/emitterboilerplate that's already encapsulated inrunConfirmedPreflightTest(lines 109-137). Extracting a shared fixture builder (or generalizing the existing helper to also cover non-confirmed outcomes) would reduce duplication and keep future changes to fixture setup (e.g., new constructor params) in one place.🤖 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 `@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt` around lines 359 - 407, The Rejected and Timeout preflight tests duplicate the same fixture setup already centralized in runConfirmedPreflightTest. Refactor these tests to reuse that helper or extract a small shared builder for serviceRepository, radioController, nodeRepository, handler, states, and emitter setup so the preflight cases share one source of truth. Update the test helper around runConfirmedPreflightTest and startUpdate usage so future constructor or fixture changes only need to be made in one place.
🤖 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/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImpl.kt`:
- Around line 179-189: The OTA status detection logic is duplicated between the
`isOtaStatusNotification()` extension here and the matching OTA check in
`UIViewModel.kt`, so move the shared constants and matching rules into a single
reusable helper. Extract the `OTA_KEYWORD`/`OTA_STATUS_KEYWORD`,
`OTA_CONFIRM_PREFIX`, `OTA_REJECTION_PREFIXES`, and the `ClientNotification`
message-matching extension into a common module that both the data layer and
`UIViewModel` can call, then update both call sites to use that shared helper so
the suppression rules stay in sync.
In
`@feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.kt`:
- Around line 345-382: The WiFi readiness delay is being skipped whenever
discoverOtaDevice() returns null, even on bind/receive failures, because
resolvePostConfirmTransportFactory() always marks readinessAlreadyWaited as true
for every null result. Update resolvePostConfirmTransportFactory() so it only
sets readinessAlreadyWaited to true when WifiOtaDiscovery.discoverOtaDevice()
actually returns a discovered IP and the factory switches to the UDP-discovered
device; for all null outcomes, keep readinessAlreadyWaited false so
waitForPostConfirmReadiness() still applies the OTA readiness margin.
In
`@feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscovery.kt`:
- Around line 72-73: The senderHost helper is using InetSocketAddress.hostname,
which can trigger reverse-DNS during discovery receive. Update senderHost to
return the sender’s numeric IP directly from the InetSocketAddress instead of
resolving the hostname, and keep the null/blank handling in place so
WifiOtaDiscovery stays non-blocking on the discovery path.
---
Duplicate comments:
In
`@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt`:
- Around line 341-350: This OTA notification matcher is duplicated in the view
model and the data-layer handler, so consolidate the shared constants and
matching logic into one common function instead of keeping two copies. Move the
logic from isOtaStatusNotificationMessage in UIViewModel and the equivalent
isOtaStatusNotification() in FromRadioPacketHandlerImpl into a shared
utility/location both can call, and keep the OTA_STATUS_KEYWORD,
OTA_CONFIRM_PREFIX, and OTA_REJECTION_PREFIXES definitions in that single
implementation.
---
Nitpick comments:
In
`@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt`:
- Around line 156-186: Add a BLE retry-exhaustion test alongside the existing
WiFi terminal-error coverage by extending Esp32OtaUpdateHandlerTest with a case
that uses bleTransportFactoryOverride and an always-failing FakeOtaTransport
sequence. The new test should verify that each failed transport is closed, a
fresh transport is created for each retry, and that runConfirmedPreflightTest
ultimately records the final FirmwareUpdateState.Error once BLE connection
attempts are exhausted. Mirror the existing WiFi retry-exhaustion pattern so the
BLE-specific transportFactory and attempt-count behavior are covered by name.
- Around line 359-407: The Rejected and Timeout preflight tests duplicate the
same fixture setup already centralized in runConfirmedPreflightTest. Refactor
these tests to reuse that helper or extract a small shared builder for
serviceRepository, radioController, nodeRepository, handler, states, and emitter
setup so the preflight cases share one source of truth. Update the test helper
around runConfirmedPreflightTest and startUpdate usage so future constructor or
fixture changes only need to be made in one place.
🪄 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: 6cfdf307-3e41-45ca-a52c-28b5a80d509a
📒 Files selected for processing (14)
.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.xmlcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransport.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscovery.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaTransport.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/DefaultFirmwareUpdateManagerTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransportTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscoveryTest.kt
c1c9ecc to
9a1b483
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuppression logic correctly reuses the shared detector.
Good use of the shared
isOtaStatusNotification()instead of duplicating the matching rules — resolves the previously-flagged duplication between this file and the data layer. Minor nit: the helper lives inorg.meshtastic.core.data.manager, an implementation-oriented package; consider hosting sharedClientNotificationpredicates in a more neutral shared/domain module so UI code isn't importing from a "manager" package.Also applies to: 260-264
🤖 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/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt` at line 41, Move the shared ClientNotification predicate used by UIViewModel away from org.meshtastic.core.data.manager into a more neutral shared/domain location, then update the UIViewModel import to reference that new home for isOtaStatusNotification(). Keep the existing suppression logic unchanged; only relocate the helper and its import so UI code no longer depends on a manager-oriented package.
🤖 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.
Nitpick comments:
In
`@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt`:
- Line 41: Move the shared ClientNotification predicate used by UIViewModel away
from org.meshtastic.core.data.manager into a more neutral shared/domain
location, then update the UIViewModel import to reference that new home for
isOtaStatusNotification(). Keep the existing suppression logic unchanged; only
relocate the helper and its import so UI code no longer depends on a
manager-oriented package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cebd35bf-15dd-4494-9810-6325ea13d141
📒 Files selected for processing (14)
.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.xmlcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransport.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscovery.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaTransport.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/DefaultFirmwareUpdateManagerTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransportTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscoveryTest.kt
✅ Files skipped from review due to trivial changes (2)
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscoveryTest.kt
- core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImplTest.kt
🚧 Files skipped from review as they are similar to previous changes (10)
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/DefaultFirmwareUpdateManagerTest.kt
- core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.kt
- core/resources/src/commonMain/composeResources/values/strings.xml
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransportTest.kt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaTransport.kt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscovery.kt
- .skills/compose-ui/strings-index.txt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransport.kt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.kt
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt
9a1b483 to
eb16a52
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/model/src/commonMain/kotlin/org/meshtastic/core/model/util/Extensions.kt`:
- Around line 143-151: The current ClientNotification.isOtaStatusNotification
matcher in Extensions.kt is too broad because it uses a plain contains("OTA")
substring check, which can misclassify unrelated messages like ROTATE or QUOTA.
Tighten this logic in isOtaStatusNotification to use a more specific OTA check,
such as a case-insensitive word-boundary match or a known OTA phrase/prefix
list, so only real OTA status notifications are suppressed. Keep the shared
helper as the single source of truth for FromRadioPacketHandlerImpl and
UIViewModel.
🪄 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: 0f21488a-c450-46ba-8179-aae9a7e0c09b
📒 Files selected for processing (14)
.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/model/src/commonMain/kotlin/org/meshtastic/core/model/util/Extensions.ktcore/resources/src/commonMain/composeResources/values/strings.xmlcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransport.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscovery.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaTransport.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransportTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscoveryTest.kt
✅ Files skipped from review due to trivial changes (1)
- .skills/compose-ui/strings-index.txt
🚧 Files skipped from review as they are similar to previous changes (11)
- core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaTransport.kt
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscoveryTest.kt
- core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImplTest.kt
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransportTest.kt
- core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.kt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/WifiOtaDiscovery.kt
- core/resources/src/commonMain/composeResources/values/strings.xml
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/BleOtaTransport.kt
- feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandlerTest.kt
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/Esp32OtaUpdateHandler.kt
eb16a52 to
9019e02
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push. Two good fixes landed:
- ✅
senderHostnow usesresolveAddress()IPv4 bytes instead of Ktor's reverse-DNShostname. - ✅
readinessAlreadyWaited = discoveredIp != null, so an instant discovery failure still honors the 8s wait.
But the UDP discovery path has a correctness hole that breaks the feature on real networks (inline), so I'm requesting changes. Remaining items and one new classifier bug are also inline.
| val socket = aSocket(selector).udp().bind(InetSocketAddress("0.0.0.0", port)) | ||
| try { | ||
| Logger.i { "WiFi OTA: Listening for OTA device discovery broadcast on port $port" } | ||
| val datagram = socket.receive() |
There was a problem hiding this comment.
Discovery accepts the first datagram from any host on port 3232 (the ArduinoOTA/espota default) with no payload validation, and the discovered IP then replaces the configured IP for every TCP retry with no fallback (see line 368). Any other device chattering on 3232 — a second ESP in OTA mode, an espota client, IoT gear — hijacks the whole update to the wrong host and it can't recover. Validate the payload (expect a known OTA beacon signature) and fall back to the configured IP if the datagram doesn't match.
|
|
||
| private const val IPV4_ADDRESS_BYTES = 4 | ||
| private const val BYTE_MASK = 0xFF | ||
| private const val DEFAULT_TIMEOUT_MS = 15_000L |
There was a problem hiding this comment.
DEFAULT_TIMEOUT_MS = 15_000L and discovery runs once before the retry loop, so a loader that serves TCP on the known IP but never broadcasts costs a fixed 15s stall before the first connect attempt, and discovery is never re-run across the 10 retries (a late broadcast can't correct a wrong IP). Consider racing discovery against a direct connect to the configured IP, or shortening the timeout.
| 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.
New classifier bug: this gate is a substring contains("OTA"), but the suppression path uses a word-boundary \bOTA\b. A message like "QUOTA…"/"ROTATE…" arriving in the 5s window is consumed here as the OTA response (then classified Rejected since it doesn't start with "Rebooting to"), aborting a legitimately-succeeding update. Use the same matcher on both sides.
| private val dispatchers: CoroutineDispatchers, | ||
| ) : FirmwareUpdateHandler { | ||
|
|
||
| internal var otaPreflightTimeoutMs: Long = OTA_PREFLIGHT_TIMEOUT_MS |
There was a problem hiding this comment.
These 8 mutable internal var seams live on a Koin @Single, and production control flow branches on them (line 355: wifiTransportFactoryOverride != null && wifiOtaDiscoveryOverride == null decides whether real UDP discovery runs). A stray write, or a test mutating the shared singleton, changes real update behavior. Prefer constructor params with defaults.
| .onEach { notification -> | ||
| // OTA status notifications (e.g. "Rebooting to WiFi OTA") are consumed by the firmware update | ||
| // preflight gate — don't show a popup dialog for them. | ||
| if (notification.isOtaStatusNotification()) { |
There was a problem hiding this comment.
This gate (and the FromRadioPacketHandlerImpl one) suppress OTA notifications with no is-update-active check, and this path never calls clearClientNotification. Two consequences: an OTA triggered by another client (CLI over TCP) or a late confirmation after preflight timeout is swallowed with zero user feedback; and the suppressed value stays resident in the conflated StateFlow, so a later notification with an identical message string is never re-emitted. Gate on "this device has an active update flow", and clear after suppressing.
cd3fe8c to
3284b7b
Compare
Gate the mesh BLE disconnect behind a firmware OTA-entry confirmation so the radio only tears down its transport after the bootloader acknowledges the mode switch. On rejection, leave the mesh connection intact and surface the device's diagnostic. After confirmation, connect to the OTA bootloader with bounded retries using a fresh transport per attempt. Close failed transports before retrying to avoid leaking Kable peripherals. Route timing, delays, and transport factory creation through an injectable Esp32OtaUpdateEnvironment so tests can drive the retry paths without real BLE timing. Add Wi-Fi OTA loader discovery via UDP beacon, validating the payload before using the discovered address. Add FirmwareUpdateStatusRepository to track active OTA state across the handler and notification path.
Add a shared word-boundary OTA status classifier (isOtaStatusNotification) so notification suppression matches whole words rather than broad substring contains. Route suppression through FirmwareUpdateStatusRepository so alerts are only hidden during an active firmware update, not whenever any OTA-related string appears.
Add comprehensive tests for the OTA preflight gate (confirmed, rejected, legacy fallback), post-confirm connect retry with fresh transports, Wi-Fi UDP discovery hit/miss, and OTA status suppression in the packet handler and UI ViewModel.
3284b7b to
4a27919
Compare
|
I pushed the refreshed post-confirm branch. This now keeps OTA status detection shared and word-boundary based, gates duplicate OTA-status suppression to an active update flow, validates Wi-Fi OTA discovery beacons before using a discovered address, preserves the readiness wait on discovery misses, and uses the environment seam instead of mutable singleton test hooks. I also validated the full ESP32 OTA stack on hardware through the combined branch. The BLE GATT cache refresh/reconnect layer is split into a follow-up branch; this PR is the post-confirm retry/preflight layer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/FirmwareUpdateStatusRepository.kt (1)
37-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
.update {}instead of read-modify-write to avoid a lost update.
_status.value = _status.value.copy(...)is a non-atomic read-then-write. IffinishOtaPreflight()races withbeginOtaUpdate()/endOtaUpdate()from another coroutine/dispatcher (plausible given BLE/WiFi retry paths run on different dispatchers), one update can silently overwrite the other.♻️ Proposed fix
fun finishOtaPreflight() { - _status.value = _status.value.copy(isAwaitingOtaStatus = false) + _status.update { it.copy(isAwaitingOtaStatus = false) } }🤖 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/repository/src/commonMain/kotlin/org/meshtastic/core/repository/FirmwareUpdateStatusRepository.kt` around lines 37 - 39, The `finishOtaPreflight()` implementation in `FirmwareUpdateStatusRepository` uses a non-atomic read-modify-write on `_status`, which can lose concurrent updates from `beginOtaUpdate()` or `endOtaUpdate()`. Change this method to update `_status` via `update {}` so the `copy(isAwaitingOtaStatus = false)` is applied atomically alongside other status changes.
🤖 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.
Nitpick comments:
In
`@core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/FirmwareUpdateStatusRepository.kt`:
- Around line 37-39: The `finishOtaPreflight()` implementation in
`FirmwareUpdateStatusRepository` uses a non-atomic read-modify-write on
`_status`, which can lose concurrent updates from `beginOtaUpdate()` or
`endOtaUpdate()`. Change this method to update `_status` via `update {}` so the
`copy(isAwaitingOtaStatus = false)` is applied atomically alongside other status
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51a0c43b-2584-446a-bc19-fd1bccbb2b74
📒 Files selected for processing (10)
.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/model/src/commonMain/kotlin/org/meshtastic/core/model/util/Extensions.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/FirmwareUpdateStatusRepository.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/di/CoreRepositoryModule.ktcore/resources/src/commonMain/composeResources/values/strings.xmlcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- core/model/src/commonMain/kotlin/org/meshtastic/core/model/util/Extensions.kt
- core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModelImportSummaryTest.kt
- core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.kt
- core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/FromRadioPacketHandlerImplTest.kt
Replace non-atomic read-modify-write on _status.value with
_state.update {} to avoid lost updates if finishOtaPreflight races
with beginOtaUpdate/endOtaUpdate on different dispatchers.
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Reviewed the post-confirmation OTA retry machinery closely — it's sound. Verified: fresh transport per attempt with each failed transport closed exactly once under NonCancellable (idempotent on the WiFi double-close path), both retry loops finite (BLE 5 / WiFi 10), preflight and UDP discovery both time-bounded, CancellationException rethrown on every path, and the UDP socket+selector closed on every exit. Blast-radius check came back clean too: the notification suppression in FromRadioPacketHandlerImpl/UIViewModel is double-gated on isOtaUpdateActive and isOtaStatusNotification(), and isOtaUpdateActive is set only in the ESP32 performUpdate and reset in finally, so key-verification and non-OTA alerts are untouched. LGTM.
Two LOW, non-blocking notes:
-
UDP discovery trusts any format-matching beacon (
WifiOtaDiscovery.kt) — the redirected TCP target is the sender IP of the first datagram matchingMeshtastic_XXXX <ver>, with no subnet check against the configured device IP. A LAN host spoofing that payload could win the race and redirect the firmware upload to itself (LAN-local DoS of the update + it receives the public firmware image — no key/PII exposure). Inherent to the esp32-unified-ota broadcast design, but a cheap guard would be requiring the discovered IP in the same subnet as the configured device IP. -
Success-path
transport.close()isn'tNonCancellable— every failed close got theNonCancellabletreatment, but the successful-transport close in the callerfinallyis left plain, so a cancel mid-upload can throw out ofclose()before the socket/GATT is released (leak-until-GC). Minor, and it's the one close path that didn't get the PR's own clean-teardown treatment.
(Also INFO: a fast UDP discovery hit skips the 8s Wi-Fi-readiness delay, so the first couple connects may hit a not-yet-listening port — self-corrects via the retry loop, only costs budget.)
2 participants
Overview
This PR hardens ESP32 OTA after the device has confirmed that it is rebooting into OTA mode.
Even after confirmation, the temporary OTA service may not be immediately reachable. BLE can connect before the expected OTA GATT service is available, and Wi-Fi can fail while the loader is still rebooting, reconnecting, starting its server, or using a different DHCP address than the normal firmware connection.
This change retries post-confirmation OTA connections with fresh transports, closes failed attempts before retrying, and uses the ESP32 OTA loader’s UDP discovery broadcast to find the post-reboot Wi-Fi address when available.
Key Changes
Testing
git diff --checkpasses.Migration Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests