chore(device): add BootloaderSessionDevice so Core owns the bootloader stand-in

[tylerkron]

Summary

daqifi-desktop and daqifi-avalonia each carry a hand-written no-op IStreamingDevice for their manual bootloader-only firmware update dialogs. They are the same file — stripped of comments the two copies differ by 15 lines, and that delta is pure version drift (desktop, on Core 1.4.0, has ErrorOccurred and the add {}/remove {} accessor style; Avalonia, on 1.3.0, does not).

Core owns IStreamingDevice, but the only hand-written implementers live downstream. So every widening of that interface is a source break that surfaces later, in two repositories, and is invisible from inside this one — Core's own tests and the MCP consumer all work with the concrete DaqifiStreamingDevice. Measured against each app's current origin/main:

Adapter	vs v1.3.0	vs v1.4.0	vs v1.5.0	vs #476
daqifi-desktop	0	0 (its pin)	12	16
daqifi-avalonia	0 (its pin)	1	13	17

v1.5.0 added 12 members in one release (ConnectAsync/DisconnectAsync, DisposeAsync, 9 × IConfirmingDeviceAdministration.*Async); #476 added 4 more. Neither flagged the break.

This PR moves the stand-in into Core, so a member added to IStreamingDevice is resolved once — in the change that adds it — and caught by this repository's build.

Changes

BootloaderSessionDevice (src/Daqifi.Core/Device/) — a sealed, no-op IStreamingDevice for a device that is already sitting in its bootloader.

Three of its behaviours are load-bearing rather than incidental, because Pic32FirmwareUpdater.RunUpdateAsync touches the device before it ever reaches the bootloader. All three are documented on the type and covered by tests:

Member	Value	Why
IsConnected	starts true	The flow opens with FirmwareUpdateContext.EnsureDeviceConnected, which throws on a disconnected device
IsStreaming	always false	Lets the flow skip its StopStreaming() call
Send(...)	discards silently	The flow sends ForceBootloader unconditionally; throwing would abort a valid update

Connect/disconnect use an atomic compare-and-set, so StatusChanged fires exactly once per real transition when a dialog's teardown races the update flow's own Disconnect().

Two smaller judgement calls, both departures from what the apps do today:

• PwmFrequencyHz reports DaqifiStreamingDevice.DefaultPwmFrequencyHz, not 0. The apps return 0 citing a "none commanded this session" sentinel, but that comment predates the current contract — IStreamingDevice now documents this as defaulting to a commandable frequency, and 0 is not one.
• Metadata is a real empty instance, not null!. Callers read it without a guard.

Docs — a BootloaderSessionDevice subsection under Implementation Classes in docs/DEVICE_INTERFACES.md, including the load-bearing table and a note steering consumers away from hand-rolling their own stub.

Testing

• 24 unit tests. The load-bearing behaviours are tested against the real internal guard (FirmwareUpdateContext.EnsureDeviceConnected) rather than a restatement of it.
• A reflection sweep invokes every member of the IStreamingDevice surface — including inherited IDevice, IConfirmingDeviceAdministration, and IAsyncDisposable members — and asserts none throws. This is the guard that earns the type its place in Core: a member added later must be implemented here as a no-op, or CI fails.
  • Verified non-vacuous by mutation: making DisableAllChannels() throw NotImplementedException does fail the sweep. The test also asserts the discovered surface is >50 methods, so a reflection walk that silently stopped finding members can't pass while covering nothing.
• Full Daqifi.Core.Tests: 2890 passed, 2 skipped. Full Daqifi.Mcp.Tests: 43 passed.
• Clean build on both target frameworks with TreatWarningsAsErrors=true and XML doc generation on, so every cref in the new docs resolves.

No bench test. The change is purely additive — one new file plus a docs section, touching no existing code path — and nothing in Core constructs or calls this type. Exercising it for real means driving a manual bootloader update dialog, which lives in the apps and cannot consume this until it ships. The apps' existing adapters, whose behaviour this reproduces, are the field-proven reference.

Follow-up (not in this PR)

The consumer-side deletions in #477 land after this is released, as part of each app's Core version bump — they can't compile against an unreleased Core. Desktop's next bump is a 16-member adapter update either way; 12 of those are already on main from v1.5.0 independent of this change.

Closes #477

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Add BootloaderSessionDevice no-op IStreamingDevice to centralize bootloader updates

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add a Core-owned no-op IStreamingDevice for bootloader-only firmware update sessions.
• Make connection/teardown thread-safe and idempotent, matching PIC32 update flow expectations.
• Document the intended contract and add tests guarding future IStreamingDevice widening.

Diagram

graph TD
  A["Consumer app"] --> B["BootloaderSessionDevice"] --> C(("IStreamingDevice")) --> D["IFirmwareUpdateService"] --> E["Pic32FirmwareUpdater"]
  T["BootloaderSessionDeviceTests"] --> B
  X["DEVICE_INTERFACES.md"] --> B
  subgraph Legend
    direction LR
    _file["File / Doc"] ~~~ _mod["Module / Type"] ~~~ _svc(["Service"])
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce an abstract base class with default no-ops

• ➕ Reduces boilerplate for any future stub/test devices
• ➕ Can centralize cancellation/idempotency patterns once
• ➖ Adds inheritance into the public surface area
• ➖ Less explicit than a single-purpose BootloaderSessionDevice for this scenario

2. Use default interface methods on IStreamingDevice

• ➕ Avoids needing a full concrete class for no-op behavior
• ➕ New members could be added with non-breaking defaults
• ➖ Not always viable depending on target frameworks and style guidelines
• ➖ Can hide missing/incorrect behavior behind defaults in scenarios where explicit behavior matters

3. Add a firmware-update-specific session interface (separate from IStreamingDevice)

• ➕ More accurate domain model: bootloader sessions are not truly streaming devices
• ➕ Shrinks required contract for update-only flows
• ➖ Bigger API change and migration cost for callers
• ➖ Requires refactoring firmware update code paths and possibly consumers

Recommendation: Keep the PR’s approach: a dedicated, Core-owned BootloaderSessionDevice is the smallest change that fixes the real problem (downstream interface drift) while preserving the existing IFirmwareUpdateService contract. The added reflection sweep test is a practical guardrail that the alternative approaches would still need in some form, and the atomic connect/disconnect semantics directly address real teardown races without expanding the public API further.

Files changed (3) +825 / -0

Enhancement (1) +475 / -0

BootloaderSessionDevice.csAdd Core-owned no-op IStreamingDevice for bootloader sessions +475/-0

Add Core-owned no-op IStreamingDevice for bootloader sessions

• Introduces a sealed BootloaderSessionDevice implementing IStreamingDevice as a no-op for devices already in bootloader mode. Ensures IsConnected starts true, IsStreaming is always false, Send discards silently, and connect/disconnect are atomic to avoid duplicate StatusChanged notifications under races.

src/Daqifi.Core/Device/BootloaderSessionDevice.cs

Tests (1) +324 / -0

BootloaderSessionDeviceTests.csAdd tests for bootloader-session contract and interface widening guard +324/-0

Add tests for bootloader-session contract and interface widening guard

• Adds unit tests validating the PIC32 update flow expectations (connected-by-default, non-streaming, Send no-throw) plus connection transition semantics and cancellation behavior. Includes a reflection-based sweep that invokes all IStreamingDevice (and inherited) members to prevent future NotImplementedException regressions when the interface grows.

src/Daqifi.Core.Tests/Device/BootloaderSessionDeviceTests.cs

Documentation (1) +26 / -0

DEVICE_INTERFACES.mdDocument BootloaderSessionDevice usage and required behaviors +26/-0

Document BootloaderSessionDevice usage and required behaviors

• Adds a new documentation section describing BootloaderSessionDevice as the recommended bootloader-only IStreamingDevice stand-in. Captures the three load-bearing behaviors the PIC32 update flow relies on and provides a usage snippet.

docs/DEVICE_INTERFACES.md

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

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

1. ValueTask<T> not awaited ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The reflection sweep test only awaits Task and non-generic ValueTask, so a future IStreamingDevice
member returning ValueTask<T> could fault without being observed and the test would still pass. This
weakens the intended “future-widening guard” and can allow throwing/unimplemented async members to
evade CI.

Code

src/Daqifi.Core.Tests/Device/BootloaderSessionDeviceTests.cs[R310-313]

+            // Observe returned tasks so a faulted one is not silently dropped.
+            switch (result)
+            {
+                case Task task:

Relevance

●●● Strong

Team often accepts strengthening async tests to avoid silent failures/hangs; this is a small,
safety-focused fix.

PR-#440
PR-#364
PR-#411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test claims it awaits returned tasks so failures aren’t dropped, but its switch only handles
Task and non-generic ValueTask; a boxed ValueTask<T> will not match either case and therefore
won’t be awaited.

src/Daqifi.Core.Tests/Device/BootloaderSessionDeviceTests.cs[274-320]

Agent prompt

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

## Issue description
`BootloaderSessionDeviceTests.EveryInterfaceMember_IsInvokableWithoutThrowing` tries to await async results returned by reflected interface methods, but it only handles `Task` and non-generic `ValueTask`. If `IStreamingDevice` later grows a member returning `ValueTask<T>`, the boxed result will not match `case ValueTask valueTask`, so the test will not await it and may miss faults.

## Issue Context
This test is explicitly intended to catch interface widening regressions (members added to `IStreamingDevice` that throw / aren’t properly no-op). Not awaiting `ValueTask<T>` undermines that guarantee.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/BootloaderSessionDeviceTests.cs[310-319]

## Suggested fix
Extend the “Observe returned tasks” block to also detect and await `ValueTask<T>`:
- If `result` is non-null and `result.GetType().IsGenericType` with `GetGenericTypeDefinition() == typeof(ValueTask<>)`, use reflection to call `AsTask()` on the boxed value and `await` the resulting `Task`.
- Keep existing handling for `Task` and non-generic `ValueTask`.

Example approach (sketch):
```csharp
switch (result)
{
   case Task task:
       await task;
       break;
   case ValueTask vt:
       await vt;
       break;
   default:
       if (result is not null)
       {
           var t = result.GetType();
           if (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(ValueTask<>))
           {
               var asTask = (Task)t.GetMethod("AsTask")!.Invoke(result, null)!;
               await asTask;
           }
       }
       break;
}
```
(Adjust null-forgiving / guards to match repo style.)

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

---

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 55c71cc