feat(firmware): attempt JMP_TO_APP soft-reset recovery on failed bootloader health check

[tylerkron]

Summary

On a PIC32 firmware-update failure during Connecting (the HID bootloader connect + RequestBootloaderVersionAsync health check), FirmwareUpdateService.RunPic32UpdateAsync previously only retried the disconnect/reconnect loop before giving up. It never attempted a JMP_TO_APP soft reset to recover an unhealthy/dirty bootloader session, even though Pic32BootloaderProtocol.CreateJumpToApplicationMessage() already exists and Core already uses it on the success path (JumpToApplicationAndReconnectAsync).

The observed real-world failure mode (daqifi-desktop#630) is a dirty HID bootloader handle left behind by another program (Microchip's HID bootloader PC tool, or historically desktop's own HID device finder). Once dirty, RequestBootloaderVersionAsync fails or returns garbage even though the physical device is present and enumerated.

Closes #298

Changes

• RunPic32UpdateAsync: the initial connect + version health check in Connecting is now wrapped in a try/catch. On failure (bad connect or a garbage/Error version response), it calls a new RecoverBootloaderHealthWithSoftResetAsync before giving up.
• RecoverBootloaderHealthWithSoftResetAsync:
  1. Writes CreateJumpToApplicationMessage() to the current HID handle, best-effort (guarded by IsConnected; a write failure falls through to the original failure rather than throwing a new unhandled exception).
  2. Disconnects and waits for HID re-enumeration, reusing WaitForBootloaderDeviceAsync (bounded by the existing WaitingForBootloaderTimeout).
  3. Reconnects (ConnectToBootloaderWithRetryAsync) and retries the version health check exactly once.
  4. If still unhealthy, rethrows the original pre-recovery exception, so the existing Connecting-state failure/guidance path and cleanup-eligibility logic (Connecting stays NotEligible — nothing has been erased yet) are completely unchanged.
• No changes to the erase-eligible failure paths (ErasingFlash/Programming/Verifying).

Test plan

• dotnet build Daqifi.Core.sln -c Release — 0 warnings/errors
• dotnet test Daqifi.Core.sln -c Release — 1394/1396 passed, 2 skipped (real-hardware-only tests); no new failures. (ContinuousDeviceFinderTests.Start_DiscoversDevice_RaisesDeviceDiscoveredAndPopulatesDevices is a known pre-existing intermittent flake, reproduces identically on main without this change.)
• New unit tests in FirmwareUpdateServiceTests:
  • UpdateFirmwareAsync_WhenBootloaderHealthCheckFails_SoftResetsAndCompletes — health check fails → soft reset issued → re-check succeeds → update proceeds through to Complete.
  • UpdateFirmwareAsync_WhenSoftResetRecoveryAlsoFails_FallsThroughToFailedWithGuidance — health check fails → soft reset recovery also fails → falls through to today's Failed behavior, original exception and Connecting recovery guidance preserved, no CleaningUp/Recovered detour.
• Bench-validated on real hardware: built the daqifi-core-example-app CLI against this branch (DaqifiCoreProjectPath) and ran a full --fw-update-latest PIC32 update against a real Nyquist (COM3, FW 3.7.2). State trace PreparingDevice → WaitingForBootloader → Connecting → ErasingFlash → Programming (43,207 records) → Verifying (3 CRC regions) → JumpingToApp → Complete; the device re-enumerated healthy afterward at the same firmware version. The HID handle was clean going in, so this confirms the modified Connecting step introduces no regression on the normal path (the new soft-reset branch is skipped entirely when the first health check succeeds) — it does not exercise the soft-reset recovery branch itself, which is covered deterministically by the two unit tests above instead.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Firmware: soft-reset recovery on PIC32 bootloader health-check failure

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Attempt one JMP_TO_APP soft reset when Connecting health check fails.
• Retry connect + version check after re-enumeration; preserve original failure semantics.
• Add unit tests for successful recovery and fall-through failure with guidance.

Diagram

graph TD
A(["Update start"]) --> B["Connect + version check"] --> C{"Health OK?"}
C -- "yes" --> D["Flash update"] --> F["Jump to app + complete"]
C -- "no" --> G["JMP_TO_APP soft reset + re-enum + retry"] --> H{"Recovered?"}
H -- "yes" --> D
H -- "no" --> I(["Fail (Connecting)"])
subgraph Legend
  direction LR
  _se(["Start/End"]) ~~~ _step["Operation"] ~~~ _dec{"Decision"}
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Always issue JMP_TO_APP before initial connect

• ➕ Maximizes chance of starting from a clean bootloader session
• ➕ Avoids needing special-case recovery logic
• ➖ Adds latency to every update, including healthy sessions
• ➖ May disrupt legitimate in-progress bootloader interactions

2. Recover by forcing HID handle recreation only (no device reset)

• ➕ Less intrusive than resetting the device
• ➕ Potentially faster if the issue is purely host-side
• ➖ May not clear device-side/firmware-side dirty state
• ➖ Harder to guarantee a clean re-enumeration boundary

3. Expand retry loop/backoff without JMP_TO_APP

• ➕ Simpler behavior; no additional commands sent to device
• ➕ May help transient timing issues
• ➖ Unlikely to fix the reported 'dirty handle' failure mode
• ➖ Can increase time-to-fail without improving success rate

Recommendation: Keep the PR’s approach: attempt JMP_TO_APP only on Connecting health-check failure, retry exactly once, and rethrow the original exception if still unhealthy. This targets the real-world dirty-handle scenario while avoiding added cost/risk on the normal success path and preserving existing failure-state semantics.

Files changed (2) +237 / -11

Bug fix (1) +105 / -11

FirmwareUpdateService.csAdd Connecting-state soft-reset recovery and single retry +105/-11

Add Connecting-state soft-reset recovery and single retry

• Wrap the initial HID connect + bootloader version health check in a try/catch. On non-cancellation failures, attempt a best-effort JMP_TO_APP soft reset, wait for re-enumeration, reconnect, and re-check the version once; if recovery fails, rethrow the original pre-recovery exception to preserve existing Connecting failure behavior.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs

Tests (1) +132 / -0

FirmwareUpdateServiceTests.csAdd unit tests for soft-reset recovery success/failure paths +132/-0

Add unit tests for soft-reset recovery success/failure paths

• Add a test that simulates an initial invalid version response followed by a successful post-reset health check and full update completion. Add a second test ensuring that if the post-reset health check still fails, the service remains in the existing Connecting failure path (original exception and guidance preserved, no cleanup/recovered detour).

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 feat: attempt JMP_TO_APP soft-reset recovery on a failed bootloader health check before giving up

1. Lost stack on rethrow ✓ Resolved 🐞 Bug ◔ Observability

Description

RecoverBootloaderHealthWithSoftResetAsync rethrows the original exception using `throw
originalFailure;`, which resets the exception’s stack trace to the recovery method and obscures
where the connect/version health check actually failed. This reduces troubleshooting quality because
the inner exception later wrapped into FirmwareUpdateException no longer points at the true failure
site.

Code

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[R1452-1493]

+        catch (Exception ex) when (ex is not OperationCanceledException)
+        {
+            _logger.LogWarning(
+                ex,
+                "JMP_TO_APP soft-reset write failed; the bootloader handle is likely already unusable.");
+            throw originalFailure;
+        }
+        finally
+        {
+            await SafeDisconnectHidAsync().ConfigureAwait(false);
+        }
+
+        try
+        {
+            var recoveredDevice = await ExecuteWithStateTimeoutAsync(
+                FirmwareUpdateState.WaitingForBootloader,
+                "wait for HID bootloader re-enumeration after soft reset",
+                ct => WaitForBootloaderDeviceAsync(targetDevicePath, targetLocationKey, ct),
+                cancellationToken).ConfigureAwait(false);
+
+            await ExecuteWithStateTimeoutAsync(
+                FirmwareUpdateState.Connecting,
+                "reconnect HID transport after soft reset",
+                ct => ConnectToBootloaderWithRetryAsync(recoveredDevice, targetDevicePath, targetLocationKey, ct),
+                cancellationToken).ConfigureAwait(false);
+
+            var version = await ExecuteWithStateTimeoutAsync(
+                FirmwareUpdateState.Connecting,
+                "request bootloader version after soft reset",
+                RequestBootloaderVersionAsync,
+                cancellationToken).ConfigureAwait(false);
+
+            _logger.LogInformation("Bootloader health restored after JMP_TO_APP soft reset.");
+            return version;
+        }
+        catch (Exception ex) when (ex is not OperationCanceledException)
+        {
+            _logger.LogWarning(
+                ex,
+                "Bootloader is still unhealthy after the JMP_TO_APP soft-reset recovery attempt.");
+            throw originalFailure;
+        }

Relevance

⭐⭐⭐ High

Team repeatedly accepted preserving diagnostic context/causes in exceptions; likely accept
stack-trace-preserving rethrow improvement.

PR-#274
PR-#237
PR-#273

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new recovery helper throws the previously caught exception instance (originalFailure) from a
different catch context, which resets its stack trace; that exception is then propagated into the
main failure path where it gets wrapped into a FirmwareUpdateException as the inner exception, so
the inner StackTrace is important for diagnosing the real failure site.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[1420-1493]
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[2210-2233]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RecoverBootloaderHealthWithSoftResetAsync` rethrows `originalFailure` via `throw originalFailure;` in two catch blocks. In C#, throwing a caught exception object like this resets its stack trace to the new throw site, which hides the original failure location (connect/version check).

### Issue Context
The method intentionally rethrows the *original* pre-recovery exception to preserve the existing Connecting-state guidance behavior. We should keep that behavior while preserving the original exception’s stack trace.

### Fix Focus Areas
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[1452-1493]

### Suggested fix
Use `ExceptionDispatchInfo` to rethrow without resetting the stack trace:

```csharp
using System.Runtime.ExceptionServices;
...
ExceptionDispatchInfo.Capture(originalFailure).Throw();
throw; // unreachable, for compiler
```

Apply this in both places currently doing `throw originalFailure;`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---