Fix ESP32 BLE OTA terminal OK handling#2032
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:
📝 WalkthroughWalkthroughThe ESP32 BLE OTA flow now uses shared protocol helpers to interpret chunk and terminal responses, with upload completion gated on a final verification response. Tests and firmware docs were updated to match the new behavior. ChangesESP32 OTA chunk/terminal protocol decisions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ESP32BLEOTAViewModel
participant ESP32OTAProtocol
participant Device
ESP32BLEOTAViewModel->>Device: send firmware chunk
Device-->>ESP32BLEOTAViewModel: chunk response
ESP32BLEOTAViewModel->>ESP32OTAProtocol: chunkDecision(response, nextOffset, fileSize)
alt advance
ESP32OTAProtocol-->>ESP32BLEOTAViewModel: .advance
ESP32BLEOTAViewModel->>ESP32BLEOTAViewModel: update offset and progress
else complete
ESP32OTAProtocol-->>ESP32BLEOTAViewModel: .complete
ESP32BLEOTAViewModel->>ESP32BLEOTAViewModel: mark upload completed
else error
ESP32OTAProtocol-->>ESP32BLEOTAViewModel: throw BLEOTAFailure
end
opt terminal verification needed
ESP32BLEOTAViewModel->>Device: read terminal response
Device-->>ESP32BLEOTAViewModel: terminal response
ESP32BLEOTAViewModel->>ESP32OTAProtocol: validateTerminalResponse(response)
ESP32OTAProtocol-->>ESP32BLEOTAViewModel: success / throw
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
355bdc8 to
0ddb79b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
Meshtastic/Views/Settings/Firmware/ESP32 OTA/BLE/ESP32BLEOTASheet.swift (1)
183-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
defer-based cleanup correctly guarantees the flag is reset on every exit path. This is a robust pattern for the disconnect/reboot window.Note: since this touches user-visible OTA flow behavior, confirm whether
docs/user/firmware.mdneeds an update.As per path instructions: "Update
docs/user/firmware.mdwhen code inMeshtastic/Views/Settings/Firmware/changes in a user-visible way."🤖 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 `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/BLE/ESP32BLEOTASheet.swift around lines 183 - 190, The OTA cleanup logic in ESP32BLEOTASheet is fine, but this user-visible firmware flow change also requires a docs update. Review the behavior change around otaInProgress reset and post-OTA discovery in ESP32BLEOTASheet and update docs/user/firmware.md to reflect the new OTA/reconnect behavior so the user guidance stays accurate.Source: Path instructions
Meshtastic/Views/Settings/Settings.swift (1)
14-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePolicy extraction looks correct and matches the test matrix. Selection outcomes for the OTA-disconnect / normal-disconnect cases align with
FirmwareOTASelectionPolicyTests.Since this alters connected-node selection behavior on the Settings screen, confirm whether
docs/user/settings.mdneeds an update.As per path instructions: "Update
docs/user/settings.mdfor general settings changes."🤖 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 `@Meshtastic/Views/Settings/Settings.swift` around lines 14 - 51, The selection policy in SettingsConnectedNodeSelectionPolicy looks correct, but the connected-node behavior change should be reflected in the user-facing settings documentation. Update the Settings documentation to describe the current selection behavior after active device changes and preferred peripheral changes, especially the OTA-disconnect and normal-disconnect cases handled by selectionAfterActiveDeviceChange and selectionAfterPreferredPeripheralChange.Source: Path instructions
Meshtastic/Views/Settings/Firmware/NRF DFU/DFUModel.swift (1)
148-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
dfuErrordrops raw failure diagnostics. The reworked handler no longer logs anything, and theerror: DFUErrorcode is discarded entirely. For a fragile OTA path, losing the technical error/message from the logs makes field debugging harder—especially when classification returns the passthroughmessage. Consider logging the raw error before overwriting state.As per coding guidelines, use
OSLog/Loggerfor all logging.♻️ Suggested logging
func dfuError(_ error: DFUError, didOccurWithMessage message: String) { let classifiedMessage = NRFDFUFailureClassifier.userFacingMessage( for: message, logMessages: dfuLogMessages, progress: lastProgressPercent ) + Logger.services.error("NRF DFU error \(error.rawValue): \(message)") self.state = .error(classifiedMessage) self.statusMessage = "Error: \(classifiedMessage)" UIApplication.shared.isIdleTimerDisabled = false }Also note this changes user-facing DFU failure messaging; update
docs/user/firmware.mdaccordingly. As per path instructions, updatedocs/user/firmware.mdwhen code inMeshtastic/Views/Settings/Firmware/changes in a user-visible way.🤖 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 `@Meshtastic/Views/Settings/Firmware/NRF` DFU/DFUModel.swift around lines 148 - 157, The dfuError handler in DFUModel is dropping the raw DFUError and original failure details, so add OSLog/Logger logging in dfuError(_:didOccurWithMessage:) before state is overwritten, including both the error and message for diagnostics. Keep the user-facing classification via NRFDFUFailureClassifier.userFacingMessage, but ensure the raw technical failure is emitted to logs while still updating state/statusMessage. Since this changes user-visible DFU failure messaging in Meshtastic/Views/Settings/Firmware/DFUModel.swift, also update docs/user/firmware.md to reflect the new behavior.Sources: Coding guidelines, Path instructions
Meshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swift (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing file header comment.
New file has no
// MARK: FileNameor copyright header. As per coding guidelines, "Add// MARK: FileNameor a file-level copyright comment at the top of files."🤖 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 `@Meshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swift` around lines 1 - 3, Add a file-level header at the top of FirmwareUpdateNotificationPolicy.swift to satisfy the new-file guideline; this file currently starts directly with the import and enum declaration. Update the top of the file to include either a `// MARK: FirmwareUpdateNotificationPolicy` marker or the required copyright/header comment before the `FirmwareUpdateNotificationPolicy` enum so the file is clearly identified.Source: Coding guidelines
Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift (3)
125-131: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFull-table fetch for a membership check.
hasKnownHardwarefetches everyDeviceHardwareEntitythen filters in memory, on every successful connect. A predicate-basedFetchDescriptor(withfetchLimit = 1) would avoid loading/deserializing the whole catalog just to check membership.⚡ Proposed predicate-based fetch
private static func hasKnownHardware(platformioTarget: String, context: ModelContext) -> Bool { - let descriptor = FetchDescriptor<DeviceHardwareEntity>() - return (try? context.fetch(descriptor))?.contains { - $0.platformioTarget == platformioTarget - } == true + var descriptor = FetchDescriptor<DeviceHardwareEntity>( + predicate: `#Predicate` { $0.platformioTarget == platformioTarget } + ) + descriptor.fetchLimit = 1 + return (try? context.fetchCount(descriptor)) ?? 0 > 0 }🤖 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 `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift` around lines 125 - 131, The hasKnownHardware helper currently fetches every DeviceHardwareEntity and filters in memory, which is wasteful on each successful connect. Update FirmwareUpdateNotifier.hasKnownHardware(platformioTarget:context:) to use a predicate-based FetchDescriptor<DeviceHardwareEntity> that matches platformioTarget directly and set fetchLimit to 1, then return whether any row was found instead of loading the full catalog.
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing file header comment.
Same as the policy file: no
// MARK: FileNameor copyright header. As per coding guidelines, "Add// MARK: FileNameor a file-level copyright comment at the top of files."🤖 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 `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift` around lines 1 - 4, Add a file-level header comment at the top of FirmwareUpdateNotifier.swift, since the file currently starts directly with imports. Follow the project guideline by adding either a `// MARK: FileName` style header or an approved copyright/header comment before the import statements, keeping the existing FirmwareUpdateNotifier file contents otherwise unchanged.Source: Coding guidelines
133-149: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueLoads and sorts the entire release catalog in memory.
latestStableFirmwareVersionfetches allFirmwareReleaseEntityrows, filters to stable releases, then sorts by decoded version components in Swift, on every connect. As the release catalog grows this becomes an increasingly wasteful full scan+sort for what is effectively a "top 1" query.🤖 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 `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift` around lines 133 - 149, The latestStableFirmwareVersion method is doing a full fetch, in-memory stable filter, and Swift sort before taking the first result. Update this query to return only the top stable firmware release directly from ModelContext/FetchDescriptor by pushing the releaseType filter and version ordering into the fetch instead of sorting all FirmwareReleaseEntity rows in memory. Keep the logic localized to latestStableFirmwareVersion and preserve the versionMajor/versionMinor/versionPatch precedence when selecting the newest versionId.Meshtastic/Views/Settings/Firmware/ESP32 OTA/WiFi/ESP32WifiOTAViewModel.swift (1)
127-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
docs/user/firmware.mdfor the new OTA progress/status messages.These changes introduce new user-visible states ("Erasing partition...", "Verifying...") and progress-bar hardening (forcing 100% before verification) during WiFi OTA.
As per coding guidelines, "Update docs/user/firmware.md when code in Meshtastic/Views/Settings/Firmware/ changes in a user-visible way."
Also applies to: 166-177, 193-195
🤖 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 `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/WiFi/ESP32WifiOTAViewModel.swift around lines 127 - 129, Update docs/user/firmware.md to document the new user-visible WiFi OTA states introduced in ESP32WifiOTAViewModel, including “Erasing partition...” and “Verifying...”, and note that the progress bar is forced to 100% before verification. Keep the wording aligned with the behavior in the OTA progress/status handling paths so users understand the full sequence shown during firmware updates.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.
Nitpick comments:
In `@Meshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swift`:
- Around line 1-3: Add a file-level header at the top of
FirmwareUpdateNotificationPolicy.swift to satisfy the new-file guideline; this
file currently starts directly with the import and enum declaration. Update the
top of the file to include either a `// MARK: FirmwareUpdateNotificationPolicy`
marker or the required copyright/header comment before the
`FirmwareUpdateNotificationPolicy` enum so the file is clearly identified.
In `@Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift`:
- Around line 125-131: The hasKnownHardware helper currently fetches every
DeviceHardwareEntity and filters in memory, which is wasteful on each successful
connect. Update
FirmwareUpdateNotifier.hasKnownHardware(platformioTarget:context:) to use a
predicate-based FetchDescriptor<DeviceHardwareEntity> that matches
platformioTarget directly and set fetchLimit to 1, then return whether any row
was found instead of loading the full catalog.
- Around line 1-4: Add a file-level header comment at the top of
FirmwareUpdateNotifier.swift, since the file currently starts directly with
imports. Follow the project guideline by adding either a `// MARK: FileName`
style header or an approved copyright/header comment before the import
statements, keeping the existing FirmwareUpdateNotifier file contents otherwise
unchanged.
- Around line 133-149: The latestStableFirmwareVersion method is doing a full
fetch, in-memory stable filter, and Swift sort before taking the first result.
Update this query to return only the top stable firmware release directly from
ModelContext/FetchDescriptor by pushing the releaseType filter and version
ordering into the fetch instead of sorting all FirmwareReleaseEntity rows in
memory. Keep the logic localized to latestStableFirmwareVersion and preserve the
versionMajor/versionMinor/versionPatch precedence when selecting the newest
versionId.
In `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/BLE/ESP32BLEOTASheet.swift:
- Around line 183-190: The OTA cleanup logic in ESP32BLEOTASheet is fine, but
this user-visible firmware flow change also requires a docs update. Review the
behavior change around otaInProgress reset and post-OTA discovery in
ESP32BLEOTASheet and update docs/user/firmware.md to reflect the new
OTA/reconnect behavior so the user guidance stays accurate.
In `@Meshtastic/Views/Settings/Firmware/ESP32`
OTA/WiFi/ESP32WifiOTAViewModel.swift:
- Around line 127-129: Update docs/user/firmware.md to document the new
user-visible WiFi OTA states introduced in ESP32WifiOTAViewModel, including
“Erasing partition...” and “Verifying...”, and note that the progress bar is
forced to 100% before verification. Keep the wording aligned with the behavior
in the OTA progress/status handling paths so users understand the full sequence
shown during firmware updates.
In `@Meshtastic/Views/Settings/Firmware/NRF` DFU/DFUModel.swift:
- Around line 148-157: The dfuError handler in DFUModel is dropping the raw
DFUError and original failure details, so add OSLog/Logger logging in
dfuError(_:didOccurWithMessage:) before state is overwritten, including both the
error and message for diagnostics. Keep the user-facing classification via
NRFDFUFailureClassifier.userFacingMessage, but ensure the raw technical failure
is emitted to logs while still updating state/statusMessage. Since this changes
user-visible DFU failure messaging in
Meshtastic/Views/Settings/Firmware/DFUModel.swift, also update
docs/user/firmware.md to reflect the new behavior.
In `@Meshtastic/Views/Settings/Settings.swift`:
- Around line 14-51: The selection policy in
SettingsConnectedNodeSelectionPolicy looks correct, but the connected-node
behavior change should be reflected in the user-facing settings documentation.
Update the Settings documentation to describe the current selection behavior
after active device changes and preferred peripheral changes, especially the
OTA-disconnect and normal-disconnect cases handled by
selectionAfterActiveDeviceChange and selectionAfterPreferredPeripheralChange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc7b55b1-5573-497d-a877-823bb0a96d9c
📒 Files selected for processing (17)
Meshtastic.xcodeproj/project.pbxprojMeshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swiftMeshtastic/Accessory/Accessory Manager/AccessoryManager.swiftMeshtastic/Extensions/UserDefaults.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotifier.swiftMeshtastic/Views/Settings/Firmware/ESP32 OTA/BLE/ESP32BLEOTASheet.swiftMeshtastic/Views/Settings/Firmware/ESP32 OTA/BLE/ESP32BLEOTAViewModel.swiftMeshtastic/Views/Settings/Firmware/ESP32 OTA/Helpers/OTAEnums.swiftMeshtastic/Views/Settings/Firmware/ESP32 OTA/WiFi/ESP32WifiOTASheet.swiftMeshtastic/Views/Settings/Firmware/ESP32 OTA/WiFi/ESP32WifiOTAViewModel.swiftMeshtastic/Views/Settings/Firmware/NRF DFU/DFUModel.swiftMeshtastic/Views/Settings/Firmware/NRF DFU/NRFDFUSheet.swiftMeshtastic/Views/Settings/Settings.swiftMeshtasticTests/FirmwareAndAPITests.swiftMeshtasticTests/FirmwareUpdateNotificationPolicyTests.swiftMeshtasticTests/FirmwareUpdateNotifierTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/user/firmware.md (1)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new firmware-update notification.
This section covers in-flight OTA UX but omits the new post-connect notification (per node/PlatformIO target/stable-release, deep-linking to Settings → Firmware Updates) introduced in this cohort. Users encountering this new notification will have no doc reference explaining when/why it fires or how to disable it.
🤖 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 `@docs/user/firmware.md` around lines 33 - 42, Add documentation for the new post-connect firmware-update notification in this OTA section, since it currently only covers in-flight update states. Update the firmware guide to mention that the app can show a notification after connect for node/PlatformIO target/stable-release updates, that it deep-links to Settings → Firmware Updates, and explain when it appears and how users can disable it there. Keep the new note near the existing OTA behavior descriptions so readers can find it alongside the update flow details.docs/user/settings.md (1)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor: doc omits the non-OTA disconnected preferred-peripheral case.
Per
SettingsConnectedNodeSelectionPolicy.selectionAfterPreferredPeripheralChange(Meshtastic/Views/Settings/Settings.swift), when the preferred peripheral changes while disconnected and no OTA is in progress, selection resets to 0 — this case isn't mentioned here, only the "connected" and "OTA disconnect" cases are.🤖 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 `@docs/user/settings.md` around lines 27 - 28, Update the Settings documentation to cover the missing non-OTA disconnected case for preferred peripheral changes. In the behavior described alongside SettingsConnectedNodeSelectionPolicy.selectionAfterPreferredPeripheralChange, add that when the preferred peripheral changes while disconnected and no OTA is in progress, selection resets to 0; keep the existing connected and OTA-disconnect cases, but expand the wording so all three outcomes are documented.
🤖 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 `@docs/user/firmware.md`:
- Around line 33-42: Add documentation for the new post-connect firmware-update
notification in this OTA section, since it currently only covers in-flight
update states. Update the firmware guide to mention that the app can show a
notification after connect for node/PlatformIO target/stable-release updates,
that it deep-links to Settings → Firmware Updates, and explain when it appears
and how users can disable it there. Keep the new note near the existing OTA
behavior descriptions so readers can find it alongside the update flow details.
In `@docs/user/settings.md`:
- Around line 27-28: Update the Settings documentation to cover the missing
non-OTA disconnected case for preferred peripheral changes. In the behavior
described alongside
SettingsConnectedNodeSelectionPolicy.selectionAfterPreferredPeripheralChange,
add that when the preferred peripheral changes while disconnected and no OTA is
in progress, selection resets to 0; keep the existing connected and
OTA-disconnect cases, but expand the wording so all three outcomes are
documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e36bdc04-9080-491e-a8ab-46b3778890b0
📒 Files selected for processing (10)
Meshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swiftMeshtastic/Model/Firmware/FirmwareUpdateNotifier.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/firmware.mdMeshtastic/Resources/docs/markdown/user/settings.mdMeshtastic/Resources/docs/user/firmware.htmlMeshtastic/Resources/docs/user/settings.htmlMeshtastic/Views/Settings/Firmware/NRF DFU/DFUModel.swiftdocs/user/firmware.mddocs/user/settings.md
✅ Files skipped from review due to trivial changes (3)
- Meshtastic/Resources/docs/user/firmware.html
- Meshtastic/Resources/docs/user/settings.html
- Meshtastic/Resources/docs/markdown/user/firmware.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Meshtastic/Model/Firmware/FirmwareUpdateNotificationPolicy.swift
- Meshtastic/Model/Firmware/FirmwareUpdateNotifier.swift
- Meshtastic/Views/Settings/Firmware/NRF DFU/DFUModel.swift
garthvh
left a comment
garthvh
left a comment
There was a problem hiding this comment.
Requesting changes.
To set expectations up front: we're not looking to take "hardening" PRs into this app. We don't want broad reliability sweeps over the OTA flows — every change to a flashing path needs to be tied to a specific, reproducible failure we can point at, not a general "make it more robust" pass. A PR framed as hardening is very hard to review and inevitably mixes a real fix with speculative churn, which is what's happening here. Please drop the hardening framing entirely.
Concretely, this needs to be reworked before it can move forward:
Scope — split it up. This is three unrelated changes under one label:
- The
FirmwareUpdateNotifier/FirmwareUpdateNotificationPolicy/ Settings work is a new user-facing feature (a "your firmware is behind" prompt), not OTA reliability, and it's the majority of the diff. That's a product decision that has to be discussed and evaluated on its own — pull it into a separate PR. - The ESP32 BLE OTA changes and the nRF DFU changes are different flows and should be separate, independently justified PRs.
Description — it over-claims. Rewrite the summary to state exactly what changed and why, each item tied to a concrete repro:
- The nRF summary says this moves to a "nonzero packet receipt notification interval," but the interval was already 8 — the actual change is 8 → 4. Either justify why 4 beats the value that already works, with the specific failure it fixes, or drop it. Lowering a working PRN changes transfer behavior and shouldn't move on a hunch.
- Hardware-bench results we can't reproduce or see aren't something anyone here can review against. Keep the description to changes that ship with a repro a reviewer can actually follow.
Code concern:
NRFDFUFailureClassifiermatches on raw substrings of the DFU library's log text ("operation failed", "100306", "receive firmware image"). That silently breaks the moment the library rewords a message. If a friendlier error is worth having, key it off a stable error code/type, not log strings — otherwise drop it.
The one piece that looks genuinely worthwhile is the ESP32 BLE terminal-response handling (waiting for the final OK after the last ACK) and moving the ACK/OK/ERR decision into a testable helper. Split that out on its own, tied to the specific transfer failure it fixes, and it's reviewable.
Thanks.
fc6a1ad to
4ce1afb
Compare
RCGV1
changed the title
4ce1afb to
5561f29
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
Meshtastic/Views/Settings/Firmware/ESP32 OTA/BLE/ESP32BLEOTAViewModel.swift (3)
181-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double-trim before calling
chunkDecision.
respStris trimmed locally intotrimmedand thenchunkDecisiontrims it again internally (OTAEnums.swiftline 32). Pass the rawrespStrand let the protocol helper own trimming, per its own encapsulation.♻️ Proposed simplification
- let trimmed = respStr.trimmingCharacters(in: .whitespacesAndNewlines) - - switch try ESP32OTAProtocol.chunkDecision(response: trimmed, nextOffset: nextOffset, fileSize: fileSize) { + switch try ESP32OTAProtocol.chunkDecision(response: respStr, nextOffset: nextOffset, fileSize: fileSize) {🤖 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 `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/BLE/ESP32BLEOTAViewModel.swift around lines 181 - 186, The BLE OTA response handling in ESP32BLEOTAViewModel is doing a redundant trim before calling ESP32OTAProtocol.chunkDecision, which already normalizes the response internally. Update the code around the chunk processing switch to pass respStr directly into chunkDecision and remove the local trimmed variable so trimming stays encapsulated in the protocol helper.
193-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate completion-state block in both branches.
The
.completecase (lines 193-199) and the post-loop terminal-response success path (lines 214-218) both settransferProgress,otaStatus,statusMessage, and log a success message identically except for the log string. Extract a small helper to avoid drift between the two paths.♻️ Proposed refactor
case .complete: // "OK" indicates completion (hash verified, partition set). offset = nextOffset - self.transferProgress = 1.0 - self.otaStatus = .completed - self.statusMessage = "Success! Rebooting..." - Logger.services.info("OTA Success (OK received on last chunk)") + markUploadCompleted(logMessage: "OTA Success (OK received on last chunk)") } } if self.otaStatus != .completed, offset >= fileSize { guard let terminalData = try await withTimeout(seconds: ESP32OTAProtocol.terminalResponseTimeout, operation: { await iterator.next() }) else { throw BLEOTAFailure.disconnected } guard let terminalResponse = String(data: terminalData, encoding: .utf8) else { throw BLEOTAFailure.unexpectedResponse("Encoding Error") } try ESP32OTAProtocol.validateTerminalResponse(terminalResponse) - self.transferProgress = 1.0 - self.otaStatus = .completed - self.statusMessage = "Success! Rebooting..." - Logger.services.info("OTA Success (OK received after final ACK)") + markUploadCompleted(logMessage: "OTA Success (OK received after final ACK)") }Add the helper under
// MARK: - Helpers:private func markUploadCompleted(logMessage: String) { self.transferProgress = 1.0 self.otaStatus = .completed self.statusMessage = "Success! Rebooting..." Logger.services.info(logMessage) }🤖 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 `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/BLE/ESP32BLEOTAViewModel.swift around lines 193 - 219, The OTA completion handling is duplicated between the `.complete` branch and the terminal-response success path in `ESP32BLEOTAViewModel`, which can drift over time. Extract the shared completion-state updates into a small helper method (for example, a helper near the existing OTA helpers) that sets `transferProgress`, `otaStatus`, and `statusMessage`, then logs the provided success message. Replace both success paths with calls to that helper, keeping only the branch-specific log text differences.
183-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider updating
docs/user/firmware.mdfor this OTA reliability change.This changes user-visible OTA completion/failure detection (e.g. how "success" vs. stalled/failed transfers are now determined). As per path instructions, "Update docs/user/firmware.md when code in Meshtastic/Views/Settings/Firmware/ changes in a user-visible way."
🤖 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 `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/BLE/ESP32BLEOTAViewModel.swift around lines 183 - 219, The OTA completion handling in ESP32BLEOTAViewModel now changes when a transfer is considered successful or stalled, so update the user-facing firmware documentation accordingly. Add a note in docs/user/firmware.md describing the new success/failure detection behavior for BLE OTA, and make sure the description matches the flow in ESP32BLEOTAViewModel’s chunk handling and terminal response validation.Source: Path instructions
MeshtasticTests/FirmwareAndAPITests.swift (1)
6-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for whitespace-trimming behavior.
All tests use exact
"ACK"/"OK"strings; none verify thatchunkDecision/validateTerminalResponsecorrectly trim surrounding whitespace/newlines, which is the key new parsing behavior versus the old raw string comparison.✅ Suggested additional tests
`@Test` func chunkDecisionTrimsWhitespaceAroundResponse() throws { let decision = try ESP32OTAProtocol.chunkDecision(response: " ACK\r\n", nextOffset: 509, fileSize: 1_000) `#expect`(decision == .advance) } `@Test` func validateTerminalResponseTrimsWhitespace() throws { try ESP32OTAProtocol.validateTerminalResponse(" OK \n") }🤖 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 `@MeshtasticTests/FirmwareAndAPITests.swift` around lines 6 - 47, Add coverage for the new whitespace-trimming parsing in ESP32OTAProtocol by extending ESP32BLEOTATerminalResponseTests with cases that pass responses containing surrounding spaces/newlines; verify chunkDecision still returns .advance for a trimmed "ACK" and validateTerminalResponse accepts a trimmed "OK". Keep the tests focused on the existing symbols chunkDecision and validateTerminalResponse so they fail if raw-string comparison is reintroduced.
🤖 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 `@Meshtastic/Views/Settings/Firmware/ESP32` OTA/BLE/ESP32BLEOTAViewModel.swift:
- Around line 181-186: The BLE OTA response handling in ESP32BLEOTAViewModel is
doing a redundant trim before calling ESP32OTAProtocol.chunkDecision, which
already normalizes the response internally. Update the code around the chunk
processing switch to pass respStr directly into chunkDecision and remove the
local trimmed variable so trimming stays encapsulated in the protocol helper.
- Around line 193-219: The OTA completion handling is duplicated between the
`.complete` branch and the terminal-response success path in
`ESP32BLEOTAViewModel`, which can drift over time. Extract the shared
completion-state updates into a small helper method (for example, a helper near
the existing OTA helpers) that sets `transferProgress`, `otaStatus`, and
`statusMessage`, then logs the provided success message. Replace both success
paths with calls to that helper, keeping only the branch-specific log text
differences.
- Around line 183-219: The OTA completion handling in ESP32BLEOTAViewModel now
changes when a transfer is considered successful or stalled, so update the
user-facing firmware documentation accordingly. Add a note in
docs/user/firmware.md describing the new success/failure detection behavior for
BLE OTA, and make sure the description matches the flow in
ESP32BLEOTAViewModel’s chunk handling and terminal response validation.
In `@MeshtasticTests/FirmwareAndAPITests.swift`:
- Around line 6-47: Add coverage for the new whitespace-trimming parsing in
ESP32OTAProtocol by extending ESP32BLEOTATerminalResponseTests with cases that
pass responses containing surrounding spaces/newlines; verify chunkDecision
still returns .advance for a trimmed "ACK" and validateTerminalResponse accepts
a trimmed "OK". Keep the tests focused on the existing symbols chunkDecision and
validateTerminalResponse so they fail if raw-string comparison is reintroduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77540d00-7175-41f1-ab3f-0b8117527491
📒 Files selected for processing (3)
Meshtastic/Views/Settings/Firmware/ESP32 OTA/BLE/ESP32BLEOTAViewModel.swiftMeshtastic/Views/Settings/Firmware/ESP32 OTA/Helpers/OTAEnums.swiftMeshtasticTests/FirmwareAndAPITests.swift
5561f29 to
ec62228
Compare
2 participants
Summary
Repro
Some ESP32 BLE OTA status streams can end the data phase with ACK for the final chunk, then send OK only after firmware verification. Before this change, the final ACK advanced offset to the firmware size, the upload loop exited, and the view model reported
Stream ended without OKwithout waiting for that terminal OK.Validation
MeshtasticTests/ESP32BLEOTATerminalResponseTests: 9 passed.Meshtasticon iPhone 17 succeeded.