Skip to content

feat(firmware): standalone bootloader health-check and soft-reset (closes #299) - #375

Merged
tylerkron merged 8 commits into
mainfrom
feature/standalone-bootloader-diagnostics
Jul 24, 2026
Merged

feat(firmware): standalone bootloader health-check and soft-reset (closes #299)#375
tylerkron merged 8 commits into
mainfrom
feature/standalone-bootloader-diagnostics

Conversation

@tylerkron

@tylerkron tylerkron commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Closes #299.

The problem

FirmwareUpdateService already knows how to talk to the PIC32 HID bootloader — wait for it to enumerate, connect over HID, read its version, and issue a JMP_TO_APP soft reset. But every one of those steps is private, reachable only by running a full UpdateFirmwareAsync.

So today, the only way to answer "is this bootloader session healthy?" is to hand over a real .hex file and erase/reprogram flash. There is no way to just look.

daqifi-desktop#630 needs to ask that question before committing to a flash. Its recovery / manual-bootloader dialog wants to probe a device the moment it's grabbed, and to force a clean USB re-enumeration when the session looks wrong — neither of which should go anywhere near flash contents.

The fix

Two methods on a new narrow interface, IPic32BootloaderDiagnostics, implemented by FirmwareUpdateService:

Method What it does
CheckBootloaderHealthAsync(string? targetDevicePath, CancellationToken) Connects to the bootloader and reads its version as a health check. Returns the version string.
ResetBootloaderAsync(string? targetDevicePath, CancellationToken) Issues a JMP_TO_APP soft reset to force a clean USB re-enumeration.

Neither erases nor programs flash. Both reuse the existing private plumbing — WaitForBootloaderDeviceAsync, ConnectToBootloaderWithRetryAsync, RequestBootloaderVersionAsync, and the bootloader protocol's CreateJumpToApplicationMessage — so there is no duplicated HID transport handling.

A separate interface (rather than new members on IFirmwareUpdateService) keeps the update interface focused on updates and stays additive: no existing implementer or caller breaks.

How it works

A new RunBootloaderDiagnosticAsync helper wraps both operations. It:

  • serializes on the same _operationLock and HID transport the full update flow uses, so a diagnostic can never interleave with an in-flight update;
  • keeps the Idle-only gate, and rejects reentrancy from an update's synchronous progress / state-change callback (unlike the read-only CheckWifiFirmwareStatusAsync probe, a diagnostic owns the HID connect/version/reset exchange);
  • always releases the HID handle in a finally, so a later update or diagnostic starts from a clean transport;
  • does not drive the update state machine — CurrentState stays Idle, because a health check is not a firmware update.

Each step is bounded by the matching per-state timeout (WaitingForBootloader, Connecting, JumpingToApp), and failures throw the existing FirmwareUpdateException carrying FailedState and RecoveryGuidance for the phase where they occurred.

Tests

20 new xUnit tests in FirmwareUpdateServiceTests cover: healthy version read, connect-by-path targeting, invalid-version failure, no-bootloader timeout, whitespace-path / ObjectDisposedException / cancellation guards on both methods, the JMP_TO_APP write, reset failure, JumpingToApp timeout enforcement on a hung write, service reusability after a diagnostic, failure-message wording, callback-reentrancy rejection, and the concurrent-call-waits contract.

Full suite green on net9.0 and net10.0 — 1783 tests, 1781 passed, 2 skipped. Release build is 0-warning on both targets.

Hardware validation

Validated end to end on a real Nyquist (Nq1, firmware 3.7.2) over USB on macOS, driving the new API through the native IOKit HID backend rather than a mocked transport. No flash was erased or programmed — the loop is SYSTem:FORceBoot → probe → JMP_TO_APP, none of which touches flash contents.

Happy path:

  • Device forced into bootloader mode re-enumerates as HID 04D8:003C ("USB HID Bootloader").
  • CheckBootloaderHealthAsync() returns the live bootloader version 1.4 in ~20 ms.
  • A second call on the same service instance succeeds, and so does a call from a fresh service instance — the real proof that the HID handle is released at the OS level, which a mocked transport can't demonstrate.
  • CheckBootloaderHealthAsync(devicePath) succeeds against the real device path.
  • ResetBootloaderAsync() issues JMP_TO_APP in ~115 ms; the HID bootloader disappears and the serial port re-enumerates.
  • After the reset the device answers SCPI again with firmware version and serial number unchanged — flash confirmed untouched.
  • CurrentState stays Idle across every diagnostic.

Failure paths:

  • No bootloader present → FirmwareUpdateException with FailedState == WaitingForBootloader, TimeoutException inner, populated RecoveryGuidance, and the WaitingForBootloader timeout honored exactly (4.0 s configured → 4.0 s elapsed over 16 real HID enumeration polls).
  • A bogus targetDevicePath does not silently fall back to the first enumerated bootloader — it fails in WaitingForBootloader and names the requested path in the message.
  • Whitespace targetDevicePath is rejected up front by both methods, before any polling.
  • CurrentState stays Idle after a failed diagnostic, and the transport is left disconnected.

Cancellation and stability:

  • A token canceled ~1 s into a health check (device in app mode, so mid-poll) unwinds in 1.0 s with OperationCanceledException — not at the 30 s state timeout — confirming the token threads through the real IOKit enumeration loop, so callers can bound the wait as the docs advise.
  • 3 back-to-back force → health-check → JMP_TO_APP cycles all passed, every cycle read bootloader version 1.4, and firmware version + serial number stayed unchanged throughout — no HID-handle leak or cumulative wedge across repeated probes (the daqifi-desktop#630 BootloaderWatcher usage pattern).

Not merging — opened for review.

🤖 Generated with Claude Code

…set (closes #299)

Add IPic32BootloaderDiagnostics with CheckBootloaderHealthAsync and
ResetBootloaderAsync, implemented on FirmwareUpdateService, so consumers
(e.g. daqifi-desktop's recovery/manual bootloader dialog) can probe or
reset a bootloader session without kicking off a full erase/program flash.

Both reuse the existing private connect/retry/version/soft-reset plumbing
via a new RunBootloaderDiagnosticAsync helper that serializes on the same
operation lock and HID transport as the full update flow, rejects
reentrancy from an in-flight update, and always releases the HID handle.
Unlike an update these do not drive the update state machine (CurrentState
stays Idle). Failures throw FirmwareUpdateException with RecoveryGuidance.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron
tylerkron requested a review from a team as a code owner July 20, 2026 23:33
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add standalone PIC32 bootloader health check and soft reset

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add IPic32BootloaderDiagnostics for bootloader version probing and JMP_TO_APP soft reset.
• Implement diagnostics in FirmwareUpdateService with shared locking and guaranteed HID disconnect.
• Add xUnit tests covering targeting, failures, and service reusability after diagnostics.
Diagram

graph TD
  A["Consumer app"] --> B[["IPic32BootloaderDiagnostics"]] --> C["FirmwareUpdateService"] --> D["RunBootloaderDiagnosticAsync"] --> E["HID transport"] --> F{{"PIC32 HID bootloader"}}
  D --> G["Bootloader protocol"]
  subgraph Legend
    direction LR
    _comp["Component"] ~~~ _iface[["Interface"]] ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Add methods to IFirmwareUpdateService
  • ➕ Single entry-point interface for firmware-related operations
  • ➕ No need for consumers to depend on an additional interface
  • ➖ Potential breaking change for other implementers
  • ➖ Conflates update orchestration with lightweight diagnostics, expanding the surface area of a core interface
2. Create a separate BootloaderDiagnosticsService class
  • ➕ Hard separation of concerns and clearer ownership boundaries
  • ➕ Could simplify FirmwareUpdateService responsibilities long-term
  • ➖ Would require exposing or duplicating private bootloader plumbing (connect/retry/version/reset)
  • ➖ Higher maintenance risk due to duplicated HID/protocol handling
3. Expose lower-level HID/bootloader primitives publicly
  • ➕ Maximum flexibility for advanced clients and tooling
  • ➕ Enables custom diagnostic sequences beyond version/reset
  • ➖ Leaky abstraction; pushes protocol/transport complexity to callers
  • ➖ Increases coupling and makes it harder to preserve safety/serialization guarantees

Recommendation: The chosen approach (a new narrow IPic32BootloaderDiagnostics implemented by FirmwareUpdateService) is the best tradeoff: it avoids breaking existing IFirmwareUpdateService implementers, keeps the update interface focused, and reuses proven internal HID/protocol plumbing while preserving the same serialization and cleanup guarantees via RunBootloaderDiagnosticAsync.

Files changed (3) +412 / -1

Enhancement (2) +220 / -1
FirmwareUpdateService.csImplement standalone bootloader health check and soft reset +160/-1

Implement standalone bootloader health check and soft reset

• Implements IPic32BootloaderDiagnostics on FirmwareUpdateService, adding CheckBootloaderHealthAsync (version probe) and ResetBootloaderAsync (JMP_TO_APP soft reset). Adds RunBootloaderDiagnosticAsync to serialize diagnostics on the existing operation lock, enforce Idle-only gating and no reentrancy, and always disconnect the HID transport in a finally block while surfacing failures as FirmwareUpdateException with phase-appropriate FailedState/RecoveryGuidance.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs

IPic32BootloaderDiagnostics.csAdd IPic32BootloaderDiagnostics interface +60/-0

Add IPic32BootloaderDiagnostics interface

• Adds a new interface defining two lightweight bootloader operations that intentionally avoid the full UpdateFirmwareAsync flow: a version-based health check and a JMP_TO_APP soft reset. Documents targeting behavior, guaranteed disconnect semantics, and failure reporting via FirmwareUpdateException.

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs

Tests (1) +192 / -0
FirmwareUpdateServiceTests.csAdd xUnit coverage for standalone bootloader diagnostics +192/-0

Add xUnit coverage for standalone bootloader diagnostics

• Introduces helper setup for diagnostics-focused service instances and adds new tests for CheckBootloaderHealthAsync and ResetBootloaderAsync. Covers success paths, target device path selection, invalid responses, timeout/no-enumeration failures, disposal/argument guards, JMP_TO_APP write behavior, and reusability after diagnostics.

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

@qodo-code-review

qodo-code-review Bot commented Jul 20, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Broken XML doc cref ✗ Dismissed 🐞 Bug ≡ Correctness
Description
IPic32BootloaderDiagnostics contains a <see cref="..."/> with a method signature including
string and nullable ?, which is not a valid XML documentation ID and will produce an
unresolved/malformed-cref warning. Because Daqifi.Core enables GenerateDocumentationFile and
TreatWarningsAsErrors, this warning will fail the build.
Code

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[R4-6]

+/// Lightweight PIC32 HID-bootloader diagnostics that run <em>outside</em> the full
+/// <see cref="IFirmwareUpdateService.UpdateFirmwareAsync(Daqifi.Core.Device.IStreamingDevice, string, System.IProgress{FirmwareUpdateProgress}?, System.Threading.CancellationToken)"/>
+/// flow: a version health check and a <c>JMP_TO_APP</c> soft reset, neither of which
Relevance

⭐⭐⭐ High

TreatWarningsAsErrors enforced (#225); broken XML cref would fail build, so they’ll fix it.

PR-#225
PR-#163

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new interface introduces a cref containing nullable ? and a non-doc-id style signature, and
the core project build is configured to generate documentation and treat warnings as errors—making
invalid cref warnings build-breaking.

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[4-6]
src/Daqifi.Core/Daqifi.Core.csproj[3-8]

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

## Issue description
The XML doc `<see cref="..."/>` in `IPic32BootloaderDiagnostics` uses a signature format (including nullable `?`) that the compiler cannot resolve to a documentation ID. With documentation generation enabled and warnings treated as errors, this breaks compilation.

## Issue Context
The core project is configured to generate XML docs and fail on warnings.

## Fix Focus Areas
- src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[4-6]
- src/Daqifi.Core/Daqifi.Core.csproj[3-8]

## Proposed fix
Update the `cref` to a resolvable documentation ID.

Two safe options:
1) Use an explicit doc-id (recommended to avoid overload ambiguity), e.g.:
```xml
/// <see cref="M:Daqifi.Core.Firmware.IFirmwareUpdateService.UpdateFirmwareAsync(Daqifi.Core.Device.IStreamingDevice,System.String,System.IProgress{Daqifi.Core.Firmware.FirmwareUpdateProgress},System.Threading.CancellationToken)"/>
```
(Ensure no nullable `?` appears in the signature.)

2) If you don’t need a direct link to a specific overload, replace with a non-overload-specific reference (e.g., `<see cref="IFirmwareUpdateService"/>`) and mention `UpdateFirmwareAsync` in plain text.

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



Remediation recommended

2. Undocumented diagnostic exceptions ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New
Description
IPic32BootloaderDiagnostics documents (and remarks imply) that failures throw
FirmwareUpdateException, but FirmwareUpdateService diagnostics also intentionally throw
ArgumentException (whitespace path), ObjectDisposedException (disposed service),
InvalidOperationException (reentrancy/non-idle), and can propagate OperationCanceledException. This
mismatch can mislead consumers of the new public API into handling the wrong exception surface.
Code

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[R11-15]

+/// Implementations share the same HID transport and operation serialization as the full
+/// update flow, so these operations cannot run concurrently with — nor be re-entered from a
+/// callback of — an in-flight update. Both throw <see cref="FirmwareUpdateException"/>
+/// (carrying <see cref="FirmwareUpdateException.RecoveryGuidance"/>) on failure, consistent
+/// with the full update flow.
Relevance

⭐⭐⭐ High

Team often fixes misleading XML/docs to match behavior and exception surfaces (accepted in PRs #357,
#98, #240).

PR-#357
PR-#98
PR-#240

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The interface remarks and per-method exception docs only mention FirmwareUpdateException, but the
implementation explicitly throws other exception types before wrapping operational errors; the new
unit tests also assert these behaviors, confirming they are intentional and part of the observable
API surface.

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[11-15]
src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[31-59]
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[442-467]
src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[3237-3254]

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

## Issue description
`IPic32BootloaderDiagnostics` currently implies diagnostics throw only `FirmwareUpdateException` on failure, but the implementation also throws other deterministic exceptions (argument validation, disposal, invalid operation/reentrancy, and cancellation propagation). This is a public API contract/documentation mismatch.

## Issue Context
- Operational bootloader failures (enumeration/connect/version/reset) are wrapped in `FirmwareUpdateException`.
- Precondition/lifecycle/concurrency failures are thrown directly (`ArgumentException`, `ObjectDisposedException`, `InvalidOperationException`), and cancellation can propagate as `OperationCanceledException`.

## Fix Focus Areas
- src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[3-16]
- src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[31-59]

## Suggested changes
- Update the interface `<remarks>` to clarify: `FirmwareUpdateException` is used for bootloader operation failures; validation/lifecycle/concurrency/cancellation exceptions may be thrown directly.
- Add `<exception>` tags for:
 - `ArgumentException` (whitespace `targetDevicePath`)
 - `ObjectDisposedException` (service disposed)
 - `InvalidOperationException` (called while another firmware operation is in-flight / non-idle / reentrancy)
 - `OperationCanceledException` (when `cancellationToken` is canceled)
- Keep existing `FirmwareUpdateException` documentation for device/protocol failures.

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


3. Reset lacks state timeout ✓ Resolved 🐞 Bug ☼ Reliability
Description
ResetBootloaderAsync performs the JMP_TO_APP HID write without ExecuteWithStateTimeoutAsync,
so the configured JumpingToApplicationTimeout is not enforced for this diagnostic step. If the HID
write blocks or does not honor cancellation promptly, the reset call can run longer than intended
and appear hung to callers.
Code

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[R402-406]

+                    failedState = FirmwareUpdateState.JumpingToApp;
+                    failedOperation = "issue JMP_TO_APP soft reset";
+                    await _hidTransport
+                        .WriteAsync(_bootloaderProtocol.CreateJumpToApplicationMessage(), ct)
+                        .ConfigureAwait(false);
Relevance

⭐⭐⭐ High

Team hardens blocking I/O with timeouts/cancellation (accepted PRs #326/#358); will enforce
JumpingToApp timeout too.

PR-#326
PR-#358
PR-#312

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The service has an explicit JumpingToApplicationTimeout and the main update flow enforces it via
ExecuteWithStateTimeoutAsync for JumpingToApp, but the new reset diagnostic path does not apply
that mechanism to its HID write.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[376-421]
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[624-631]
src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[55-59]

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

## Issue description
`ResetBootloaderAsync` sets `failedState = FirmwareUpdateState.JumpingToApp` but calls `_hidTransport.WriteAsync(...)` directly, bypassing the service’s per-state timeout enforcement (`ExecuteWithStateTimeoutAsync`). This means `JumpingToApplicationTimeout` is not applied to the reset write.

## Issue Context
The full update flow consistently uses `ExecuteWithStateTimeoutAsync` to enforce state budgets, including the `JumpingToApp` state.

## Fix Focus Areas
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[402-406]
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[624-631]
- src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[55-59]

## Proposed fix
Wrap the `JMP_TO_APP` write in `ExecuteWithStateTimeoutAsync` using `FirmwareUpdateState.JumpingToApp` and the existing `failedOperation` string, e.g.:
```csharp
failedState = FirmwareUpdateState.JumpingToApp;
failedOperation = "issue JMP_TO_APP soft reset";

await ExecuteWithStateTimeoutAsync(
   FirmwareUpdateState.JumpingToApp,
   failedOperation,
   innerCt => _hidTransport.WriteAsync(
       _bootloaderProtocol.CreateJumpToApplicationMessage(),
       innerCt),
   ct).ConfigureAwait(false);
```
This preserves the diagnostic’s existing error mapping while enforcing the configured timeout.

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


Grey Divider

Previous review results

Review updated until commit 513287f

Results up to commit 7277f41 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Broken XML doc cref ✗ Dismissed 🐞 Bug ≡ Correctness
Description
IPic32BootloaderDiagnostics contains a <see cref="..."/> with a method signature including
string and nullable ?, which is not a valid XML documentation ID and will produce an
unresolved/malformed-cref warning. Because Daqifi.Core enables GenerateDocumentationFile and
TreatWarningsAsErrors, this warning will fail the build.
Code

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[R4-6]

+/// Lightweight PIC32 HID-bootloader diagnostics that run <em>outside</em> the full
+/// <see cref="IFirmwareUpdateService.UpdateFirmwareAsync(Daqifi.Core.Device.IStreamingDevice, string, System.IProgress{FirmwareUpdateProgress}?, System.Threading.CancellationToken)"/>
+/// flow: a version health check and a <c>JMP_TO_APP</c> soft reset, neither of which
Relevance

⭐⭐⭐ High

TreatWarningsAsErrors enforced (#225); broken XML cref would fail build, so they’ll fix it.

PR-#225
PR-#163

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new interface introduces a cref containing nullable ? and a non-doc-id style signature, and
the core project build is configured to generate documentation and treat warnings as errors—making
invalid cref warnings build-breaking.

src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[4-6]
src/Daqifi.Core/Daqifi.Core.csproj[3-8]

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

## Issue description
The XML doc `<see cref="..."/>` in `IPic32BootloaderDiagnostics` uses a signature format (including nullable `?`) that the compiler cannot resolve to a documentation ID. With documentation generation enabled and warnings treated as errors, this breaks compilation.

## Issue Context
The core project is configured to generate XML docs and fail on warnings.

## Fix Focus Areas
- src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs[4-6]
- src/Daqifi.Core/Daqifi.Core.csproj[3-8]

## Proposed fix
Update the `cref` to a resolvable documentation ID.

Two safe options:
1) Use an explicit doc-id (recommended to avoid overload ambiguity), e.g.:
```xml
/// <see cref="M:Daqifi.Core.Firmware.IFirmwareUpdateService.UpdateFirmwareAsync(Daqifi.Core.Device.IStreamingDevice,System.String,System.IProgress{Daqifi.Core.Firmware.FirmwareUpdateProgress},System.Threading.CancellationToken)"/>
```
(Ensure no nullable `?` appears in the signature.)

2) If you don’t need a direct link to a specific overload, replace with a non-overload-specific reference (e.g., `<see cref="IFirmwareUpdateService"/>`) and mention `UpdateFirmwareAsync` in plain text.

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



Remediation recommended
2. Reset lacks state timeout ✓ Resolved 🐞 Bug ☼ Reliability
Description
ResetBootloaderAsync performs the JMP_TO_APP HID write without ExecuteWithStateTimeoutAsync,
so the configured JumpingToApplicationTimeout is not enforced for this diagnostic step. If the HID
write blocks or does not honor cancellation promptly, the reset call can run longer than intended
and appear hung to callers.
Code

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[R402-406]

+                    failedState = FirmwareUpdateState.JumpingToApp;
+                    failedOperation = "issue JMP_TO_APP soft reset";
+                    await _hidTransport
+                        .WriteAsync(_bootloaderProtocol.CreateJumpToApplicationMessage(), ct)
+                        .ConfigureAwait(false);
Relevance

⭐⭐⭐ High

Team hardens blocking I/O with timeouts/cancellation (accepted PRs #326/#358); will enforce
JumpingToApp timeout too.

PR-#326
PR-#358
PR-#312

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The service has an explicit JumpingToApplicationTimeout and the main update flow enforces it via
ExecuteWithStateTimeoutAsync for JumpingToApp, but the new reset diagnostic path does not apply
that mechanism to its HID write.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[376-421]
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[624-631]
src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[55-59]

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

## Issue description
`ResetBootloaderAsync` sets `failedState = FirmwareUpdateState.JumpingToApp` but calls `_hidTransport.WriteAsync(...)` directly, bypassing the service’s per-state timeout enforcement (`ExecuteWithStateTimeoutAsync`). This means `JumpingToApplicationTimeout` is not applied to the reset write.

## Issue Context
The full update flow consistently uses `ExecuteWithStateTimeoutAsync` to enforce state budgets, including the `JumpingToApp` state.

## Fix Focus Areas
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[402-406]
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[624-631]
- src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[55-59]

## Proposed fix
Wrap the `JMP_TO_APP` write in `ExecuteWithStateTimeoutAsync` using `FirmwareUpdateState.JumpingToApp` and the existing `failedOperation` string, e.g.:
```csharp
failedState = FirmwareUpdateState.JumpingToApp;
failedOperation = "issue JMP_TO_APP soft reset";

await ExecuteWithStateTimeoutAsync(
   FirmwareUpdateState.JumpingToApp,
   failedOperation,
   innerCt => _hidTransport.WriteAsync(
       _bootloaderProtocol.CreateJumpToApplicationMessage(),
       innerCt),
   ct).ConfigureAwait(false);
```
This preserves the diagnostic’s existing error mapping while enforcing the configured timeout.

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


Qodo Logo

Comment thread src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs
Comment thread src/Daqifi.Core/Firmware/FirmwareUpdateService.cs
…soft reset

ResetBootloaderAsync issued the JMP_TO_APP HID write directly, bypassing
ExecuteWithStateTimeoutAsync, so the configured JumpingToApplicationTimeout
was not applied to the standalone soft-reset step. A blocking write (or one
that ignores cancellation) could run unbounded and appear hung to callers.
Wrap the write in ExecuteWithStateTimeoutAsync(JumpingToApp) to match the
full update flow, preserving the existing JumpingToApp error mapping.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread src/Daqifi.Core/Firmware/IPic32BootloaderDiagnostics.cs Outdated
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 4a94caa

…BootloaderDiagnostics

The diagnostic methods also throw ArgumentException (whitespace path),
ObjectDisposedException (disposed service), InvalidOperationException
(reentrancy / non-idle), and propagate OperationCanceledException — none
of which were on the interface contract. Add <exception> tags and clarify
<remarks> so consumers handle the right surface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron

Copy link
Copy Markdown
Contributor Author

Valid — fixed in the latest commit. IPic32BootloaderDiagnostics now documents the full throw surface: the <remarks> clarifies that only bootloader operation failures (enumeration/connect/version/reset) are wrapped in FirmwareUpdateException, and both methods gained <exception> tags for ArgumentException (whitespace path), ObjectDisposedException (disposed service), InvalidOperationException (in-flight/reentrancy/non-idle), and OperationCanceledException (canceled token). Doc-only change; Release build stays 0-warning on net9.0+net10.0 and the full suite is green.

/agentic_review

tylerkron and others added 3 commits July 22, 2026 16:01
… failed"

A health check or soft reset is what a consumer runs *instead of* starting
an update, but both routed through CreateFirmwareUpdateException, which
hard-codes "Firmware update failed in state 'X' while Y." A recovery dialog
would tell a user their firmware update failed when none was ever attempted.
Confirmed on a real Nq1: probing with no bootloader present emitted
"Firmware update failed in state 'WaitingForBootloader'".

CreateFirmwareUpdateException gains a failureSubject parameter defaulting to
"Firmware update", so the update flow's wording is unchanged (pinned by a new
regression test); the diagnostics pass "Bootloader health check" and
"Bootloader soft reset".

Also corrects the IPic32BootloaderDiagnostics throw contract, which claimed
InvalidOperationException is raised "when another firmware operation is in
flight". Only reentrancy from an in-flight operation's own synchronous
callback throws — a concurrent call from a separate execution context waits
on the shared lock and then proceeds. Documents two further behaviours callers
need: the 45s default WaitingForBootloaderTimeout means a probe against a
device that is not in bootloader mode blocks that long, and a failed health
check does not imply an update would fail, since the check deliberately skips
the #298 JMP_TO_APP self-heal the update flow applies.

Adds 8 tests: message wording for both diagnostics plus an update-flow
regression guard, ResetBootloaderAsync disposed-guard symmetry, cancellation
for both methods, callback-reentrancy rejection, and the concurrent-call-waits
semantics the docs now promise.

Bench-tested end to end on a real Nq1 (fw 3.7.2) — no flash written:
FORceBoot -> HID 04D8:003C -> health check returns bootloader version 1.4 in
~18ms -> repeat on the same service and from a fresh instance both succeed
(the real proof the HID handle is released at OS level) -> connect-by-path
targeting works, bogus path correctly refuses to fall back -> JMP_TO_APP in
~117ms -> device returns to app mode with firmware version and serial number
unchanged. Full suite green on net9.0 and net10.0 (1783 tests, 1781 passed,
2 skipped), 0 warnings.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@tylerkron
tylerkron merged commit 208592d into main Jul 24, 2026
1 check passed
@tylerkron
tylerkron deleted the feature/standalone-bootloader-diagnostics branch July 24, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: expose standalone bootloader health-check and soft-reset outside the full update flow

1 participant