Skip to content

feat(device): modernize device interfaces — async + CancellationToken on IStreamingDevice, disposability on IDevice - #469

Merged
tylerkron merged 4 commits into
mainfrom
claude/github-issue-460-42bbdf
Aug 7, 2026
Merged

feat(device): modernize device interfaces — async + CancellationToken on IStreamingDevice, disposability on IDevice#469
tylerkron merged 4 commits into
mainfrom
claude/github-issue-460-42bbdf

Conversation

@tylerkron

Copy link
Copy Markdown
Contributor

Summary

Closes #460. Follow-on to #333 (not yet merged): that ticket promotes missing members and fixes factory return types; this one is the shape of the contracts — the three inconsistencies the issue calls out.

  1. IStreamingDevice async surface. Every streaming/channel/DIO/PWM/analog-output/reboot method now has a cancellable ...Async twin declared directly on the interface (StartStreamingAsync, EnableChannelAsync, SetDioValueAsync, SetPwmEnabledAsync, SetAnalogOutputAsync, RebootAsync, …). IStreamingDevice also now extends IConfirmingDeviceAdministration, so the confirming ADC-calibration/voltage-precision calls are reachable directly off an IStreamingDevice reference — no separate cast.
  2. IDevice.ConnectAsync/DisconnectAsync are genuine abstract members now, not default-interface-method shims that quietly wrapped Connect()/Disconnect(). Every implementer must honor the CancellationToken itself.
  3. IDevice extends IAsyncDisposable. A consumer holding only the interface can now await using/await device.DisposeAsync() without a cast to DaqifiDevice.

Sync methods are unchanged and remain the primary API for existing callers — nothing was removed.

Design notes

  • Most of the new ...Async surface (streaming/channel/DIO/PWM/output/reboot) has no genuine async machinery underneath todayDaqifiStreamingDevice's implementations are fire-and-forget synchronous writes. So the interface's default-interface-method body for each is a thin, cancellable wrapper over the sync call — that's an honest implementation of what's actually happening, not a stopgap (unlike the old ConnectAsync/DisconnectAsync shims, which hid genuinely async machinery that already existed in DaqifiDevice). DaqifiStreamingDevice also implements each of these explicitly as a regular class member (not relying purely on the DIM default), so they're callable directly on the concrete type the same way the existing sync methods are — a DIM-only default is only reachable through the interface type.
  • Calibration is different: DaqifiStreamingDevice already runs real async machinery for those (drain-error-queue + confirm), on IConfirmingDeviceAdministration. Making IStreamingDevice extend that interface (rather than duplicating differently-behaved members with the same names) reuses it directly with zero code changes to the concrete class.

Breaking changes

Any IDevice/IStreamingDevice implementer outside this repo needs to add:

  • ConnectAsync, DisconnectAsync (previously optional via DIM)
  • DisposeAsync (IAsyncDisposable)
  • The 9 IConfirmingDeviceAdministration calibration members (previously only reachable via is IConfirmingDeviceAdministration cast)

Per the issue, batch this into the same release note as #333 when both ship.

Testing

  • Updated the five IStreamingDevice test fakes in FirmwareUpdateServiceTests.cs / LanChipInfoProviderExtensionsTests.cs for the new interface shape.
  • Added DaqifiStreamingDeviceAsyncSurfaceTests.cs: delegation + cancellation behavior for the new ...Async methods, IDevice.ConnectAsync/DisconnectAsync reachable through the interface, IDevice : IAsyncDisposable, and a minimal IStreamingDevice implementer proving the default-interface-method bodies work when reached only through the interface type.
  • dotnet build — solution builds clean (0 warnings, 0 errors).
  • dotnet test — full suite passes: 2846 passed, 2 skipped (real-hardware tests), 0 failed, both net9.0 and net10.0. Plus Daqifi.Mcp.Tests (23 passed).
  • dotnet format --verify-no-changes — no new formatting issues introduced (pre-existing drift in files this PR doesn't touch).

No bench hardware test was needed: this PR only reshapes interface contracts and adds thin wrappers over already-tested code paths (DaqifiStreamingDevice's sync methods, DaqifiDevice's existing ConnectAsync/DisconnectAsync/DisposeAsync), all covered by the unit suite above.

Docs

  • docs/DEVICE_INTERFACES.mdIDevice/IStreamingDevice core-interface sections, the "Connecting and disconnecting without blocking" section, and Channel Management examples.
  • README.md — pointer to the async surface from the Digital output quick recipe.

🤖 Generated with Claude Code

…cDisposable on IDevice

Follow-on to #333, closes #460.

- IDevice extends IAsyncDisposable. ConnectAsync/DisconnectAsync are now
  genuine abstract interface members instead of default-interface-method
  shims that quietly wrapped Connect()/Disconnect() — every implementer
  must honor the CancellationToken itself.
- IStreamingDevice extends IConfirmingDeviceAdministration, so the
  confirming ADC-calibration/voltage-precision calls are reachable
  directly off an IStreamingDevice reference with no cast.
- IStreamingDevice gains cancellable ...Async twins for every
  streaming/channel/DIO/PWM/analog-output/reboot method
  (StartStreamingAsync, EnableChannelAsync, SetDioValueAsync,
  SetPwmEnabledAsync, SetAnalogOutputAsync, RebootAsync, etc.), with
  default-interface-method bodies that thinly wrap the synchronous
  call — most of this surface has no genuine async machinery
  underneath today, so that default is an honest implementation, not
  a stopgap. DaqifiStreamingDevice implements them explicitly as
  regular class members too, so they're callable directly on the
  concrete type the same way the existing sync methods are.
- Sync methods are unchanged and remain the primary API for existing
  callers.
- Updated the five IStreamingDevice test fakes in
  FirmwareUpdateServiceTests.cs / LanChipInfoProviderExtensionsTests.cs
  for the new interface shape (ConnectAsync/DisconnectAsync,
  DisposeAsync, the 9 confirming calibration members).
- README.md and docs/DEVICE_INTERFACES.md updated.

Breaking change: IDevice/IStreamingDevice implementers must add
ConnectAsync, DisconnectAsync, DisposeAsync, and the
IConfirmingDeviceAdministration calibration members. Batch into the
same release note as #333 per the issue.
@tylerkron
tylerkron requested a review from a team as a code owner August 7, 2026 15:33
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Modernize device interfaces: cancellable async streaming ops + IAsyncDisposable

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add cancellable ...Async twins across IStreamingDevice streaming/channel/DIO/PWM/DAC/reboot
 operations.
• Make IDevice async connect/disconnect real abstract members and add IAsyncDisposable for
 await using.
• Update docs and test fakes; add unit coverage validating cancellation and sync-equivalence.
Diagram

classDiagram
  class IAsyncDisposable<<interface>>
  class IDevice<<interface>>
  class IConfirmingDeviceAdministration<<interface>>
  class IStreamingDevice<<interface>>
  class DaqifiDevice
  class DaqifiStreamingDevice

  IDevice --|> IAsyncDisposable : extends
  IStreamingDevice --|> IDevice : extends
  IStreamingDevice --|> IConfirmingDeviceAdministration : extends

  DaqifiDevice ..|> IDevice : implements
  DaqifiStreamingDevice --|> DaqifiDevice : inherits
  DaqifiStreamingDevice ..|> IStreamingDevice : implements
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Expose async wrappers via extension methods instead of interface members
  • ➕ Avoids adding many members to the public interface surface
  • ➕ No breaking change for third-party implementers of IStreamingDevice
  • ➖ Not discoverable/intellisense-friendly compared to interface members
  • ➖ Cannot be overridden by implementers with genuinely async transports
  • ➖ Doesn’t solve the ‘interface-only consumer’ goal as cleanly
2. Require implementers to implement all new `...Async` members (no DIM defaults)
  • ➕ Forces true cancellation/async semantics where needed
  • ➕ Eliminates DIM-only behavior differences between interface vs concrete types
  • ➖ Much higher breaking impact and boilerplate across implementers
  • ➖ Many operations are inherently fire-and-forget today; required async adds little value
3. Introduce an abstract base class providing the async wrappers
  • ➕ Centralizes the wrapper logic without DIM defaults
  • ➕ Keeps interface smaller while still providing a default implementation path
  • ➖ Doesn’t help consumers coding to interfaces unless combined with interface changes
  • ➖ Limits implementers that already have their own inheritance hierarchy

Recommendation: The PR’s approach is the best fit for the stated goals: it standardizes a cancellable async surface for interface-only consumers while keeping existing sync APIs intact. Using DIM defaults for fire-and-forget operations is pragmatic, and explicitly implementing the async members on DaqifiStreamingDevice mitigates the DIM-only discoverability/call-site limitation. The key follow-up is ensuring downstream implementers understand the breaking requirements (ConnectAsync/DisconnectAsync, DisposeAsync, and the confirming admin surface via inheritance).

Files changed (9) +989 / -36

Enhancement (3) +356 / -30
DaqifiStreamingDevice.csImplement explicit cancellable '...Async' wrappers for streaming/channel/DIO/PWM/DAC/reboot +105/-1

Implement explicit cancellable '...Async' wrappers for streaming/channel/DIO/PWM/DAC/reboot

• Stops explicitly listing 'IConfirmingDeviceAdministration' in the class declaration because 'IStreamingDevice' now inherits it. Adds concrete '...Async(CancellationToken)' methods that check cancellation then delegate to existing synchronous operations for streaming control, channel enable/disable, DIO, PWM, DAC output, and reboot.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

IDevice.csMake async connect/disconnect abstract and add 'IAsyncDisposable' to 'IDevice' +17/-28

Make async connect/disconnect abstract and add 'IAsyncDisposable' to 'IDevice'

• Changes 'IDevice' to extend 'IAsyncDisposable' so consumers can dispose via interface references. Removes default-interface-method shims for 'ConnectAsync'/'DisconnectAsync', making them required members that must honor the provided 'CancellationToken'.

src/Daqifi.Core/Device/IDevice.cs

IStreamingDevice.csAdd cancellable async twins and extend confirming admin interface +234/-1

Add cancellable async twins and extend confirming admin interface

• Extends 'IStreamingDevice' from 'IConfirmingDeviceAdministration' and adds cancellable '...Async' methods for streaming control, channel management, DIO, PWM, analog output, and reboot. Provides default-interface-method implementations that check cancellation then call the existing synchronous methods, with guidance for implementers to override when true async behavior exists.

src/Daqifi.Core/Device/IStreamingDevice.cs

Tests (3) +572 / -0
DaqifiStreamingDeviceAsyncSurfaceTests.csAdd unit tests for new cancellable async interface surface +425/-0

Add unit tests for new cancellable async interface surface

• Introduces a comprehensive test suite validating that new '...Async' methods honor pre-cancelled tokens and otherwise behave like their synchronous counterparts. Includes coverage for DIM default implementations (via a minimal implementer) and for 'IDevice' async connect/disconnect and async disposal behavior through interface references.

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

FirmwareUpdateServiceTests.csUpdate streaming-device fakes to satisfy new 'IDevice'/'IStreamingDevice' contracts +118/-0

Update streaming-device fakes to satisfy new 'IDevice'/'IStreamingDevice' contracts

• Adds 'ConnectAsync', 'DisconnectAsync', and 'DisposeAsync' implementations to several test doubles implementing 'IDevice'/'IStreamingDevice'. Adds async confirming-admin member stubs required by 'IStreamingDevice' now extending 'IConfirmingDeviceAdministration'.

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

LanChipInfoProviderExtensionsTests.csUpdate test fake to implement async connect/disconnect and async disposal +29/-0

Update test fake to implement async connect/disconnect and async disposal

• Extends the test fake implementing 'IStreamingDevice' to include 'ConnectAsync', 'DisconnectAsync', and 'DisposeAsync', plus confirming-admin async stubs required by the updated interface hierarchy.

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

Documentation (3) +61 / -6
README.mdDocument new 'IStreamingDevice ...Async' twins in usage examples +4/-0

Document new 'IStreamingDevice ...Async' twins in usage examples

• Adds a note clarifying that the streaming/device operations shown in the README now have cancellable '...Async' counterparts on 'IStreamingDevice', and points readers to the interface docs for the full list.

README.md

DEVICE_INTERFACES.mdUpdate interface docs for async connect/disconnect, async disposal, and streaming async API +50/-1

Update interface docs for async connect/disconnect, async disposal, and streaming async API

• Expands the interface documentation to describe 'IDevice.ConnectAsync/DisconnectAsync' as true abstract members and 'IDevice : IAsyncDisposable'. Adds examples and rationale for the new cancellable 'IStreamingDevice ...Async' methods and for 'IStreamingDevice' extending 'IConfirmingDeviceAdministration'.

docs/DEVICE_INTERFACES.md

IConfirmingDeviceAdministration.csClarify documentation now that 'IStreamingDevice' extends confirming admin interface +7/-5

Clarify documentation now that 'IStreamingDevice' extends confirming admin interface

• Updates remarks to reflect that 'IStreamingDevice' now includes these confirming calibration operations directly, while still noting that callers with only 'IDevice' may need capability checks.

src/Daqifi.Core/Device/IConfirmingDeviceAdministration.cs

@qodo-code-review

qodo-code-review Bot commented Aug 7, 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. RebootAsync blocks thread ✓ Resolved 🐞 Bug ☼ Reliability
Description
DaqifiStreamingDevice.RebootAsync synchronously calls Reboot(), which performs a blocking
Disconnect() and can stall the caller thread (including UI threads) for the full teardown wait. This
contradicts the interface contract/documentation that RebootAsync is non-blocking and undermines the
purpose of the new async surface.
Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R863-866]

+        public Task RebootAsync(CancellationToken cancellationToken = default)
+        {
+            cancellationToken.ThrowIfCancellationRequested();
+            Reboot();
Relevance

●●● Strong

Team enforces non-blocking async contracts; should use DisconnectAsync/avoid UI-thread stalls,
consistent with recent async reliability fixes.

PR-#421
PR-#418
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new RebootAsync implementation directly invokes Reboot() synchronously. Reboot() ultimately
calls the host's synchronous Disconnect(), and the base device explicitly documents Disconnect() as
blocking and recommends DisconnectAsync for non-blocking teardown; therefore RebootAsync can block
for the same reasons.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[859-868]
src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[77-90]
src/Daqifi.Core/Device/DaqifiDevice.cs[1451-1467]
src/Daqifi.Core/Device/IStreamingDevice.cs[366-375]

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

## Issue description
`DaqifiStreamingDevice.RebootAsync(...)` is documented/positioned as a non-blocking async API, but it immediately calls the synchronous `Reboot()` implementation. That reboot path performs a synchronous `Disconnect()` which can block for seconds (lock acquisition + teardown), so `RebootAsync` can still freeze the caller thread.

## Issue Context
- `DeviceAdministrationOperations.Reboot()` sends the reboot SCPI command and then calls `_host.Disconnect()` (sync).
- `DaqifiDevice.Disconnect()` explicitly blocks (and advises callers to use `DisconnectAsync()` for non-blocking teardown).
- `IStreamingDevice.RebootAsync` summary says it is "without blocking the calling thread".

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[847-868]

### Implementation direction
- Change `DaqifiStreamingDevice.RebootAsync` to avoid calling `Reboot()`.
- Instead:
 - Validate connection (same behavior as current reboot).
 - Send the reboot command (`Send(ScpiMessageProducer.RebootDevice)`).
 - Then `await DisconnectAsync(cancellationToken).ConfigureAwait(false)` to ensure teardown is non-blocking.
- Keep the existing synchronous `Reboot()` unchanged for callers that explicitly want the blocking behavior.

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



Informational

2. Canceled reboot test incomplete ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New
Description
RebootAsync_CanceledToken_OnDisconnectedDevice_ThrowsOperationCanceled_NotDeviceNotConnected only
asserts that OperationCanceledException is thrown, but it does not assert that no reboot command (or
other side effects) occurred before throwing. This test could still pass if a future refactor sends
the reboot SCPI command and then throws due to cancellation, violating the intended “do nothing when
pre-canceled” contract.
Code

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceAsyncSurfaceTests.cs[R259-263]

+            var device = new TestableDaqifiStreamingDevice("TestDevice");
+            Assert.False(device.IsConnected);
+
+            await Assert.ThrowsAsync<OperationCanceledException>(
+                () => device.RebootAsync(CanceledToken()));
Relevance

●●● Strong

Team often accepts strengthening tests to assert intended behavior and prevent false-positive
passes.

PR-#381
PR-#416

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test only asserts the exception, while the adjacent canceled-token reboot test asserts both
the exception and that no messages were sent, indicating that avoiding side effects is part of the
intended contract for pre-canceled tokens.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceAsyncSurfaceTests.cs[253-264]
src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceAsyncSurfaceTests.cs[241-251]

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 new test `RebootAsync_CanceledToken_OnDisconnectedDevice_ThrowsOperationCanceled_NotDeviceNotConnected` only checks exception precedence, but doesn’t verify the “no side effects when pre-canceled” behavior (e.g., no reboot message sent).

### Issue Context
A neighboring canceled-token test already asserts `SentMessages` is empty, which is the intended invariant for pre-canceled calls.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceAsyncSurfaceTests.cs[253-265]

### Suggested change
After the `ThrowsAsync` assertion, add assertions such as:
- `Assert.Empty(device.SentMessages);`
- `Assert.False(device.IsConnected);` (optional, but aligns with the initial state)

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


3. Cancellation masked by validation ✓ Resolved 🐞 Bug ≡ Correctness
Description
DaqifiStreamingDevice.RebootAsync throws DeviceNotConnectedException before observing a pre-canceled
CancellationToken, so a canceled call on a disconnected device won’t surface as
OperationCanceledException. This is inconsistent with the rest of the new async wrappers (which
prioritize cancellation) and can cause callers to misclassify cancellation as an operational error.
Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R877-880]

+            if (!IsConnected)
+            {
+                throw new DeviceNotConnectedException();
+            }
Relevance

●●● Strong

Team previously prioritized cancellation checks before IsConnected/validation in async device
methods; this matches that precedent.

PR-#329
PR-#381

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The interface documents RebootAsync as cancellable (throws OperationCanceledException) and observes
the token before sending; the new override checks connectivity first, so a pre-canceled token can’t
take precedence when disconnected.

src/Daqifi.Core/Device/IStreamingDevice.cs[369-387]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[875-883]

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

## Issue description
`DaqifiStreamingDevice.RebootAsync` currently validates `IsConnected` before checking `cancellationToken.ThrowIfCancellationRequested()`, which means a pre-canceled call when the device is disconnected throws `DeviceNotConnectedException` instead of `OperationCanceledException`.

## Issue Context
This method is part of the new async surface where most `...Async` wrappers check cancellation before doing any other work/validation.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[877-883]

## Suggested change
Move `cancellationToken.ThrowIfCancellationRequested();` to the top of the method (before the `IsConnected` check), or explicitly decide and document that connection validation takes precedence over cancellation for this method.

## Optional test
Add a unit test asserting that `RebootAsync(CanceledToken())` throws `OperationCanceledException` even when `IsConnected == false`.

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


To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Previous review results

Review updated until commit b0d1fa2

Results up to commit 2504b66 ⚖️ Balanced


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


Action required
1. RebootAsync blocks thread ✓ Resolved 🐞 Bug ☼ Reliability
Description
DaqifiStreamingDevice.RebootAsync synchronously calls Reboot(), which performs a blocking
Disconnect() and can stall the caller thread (including UI threads) for the full teardown wait. This
contradicts the interface contract/documentation that RebootAsync is non-blocking and undermines the
purpose of the new async surface.
Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R863-866]

+        public Task RebootAsync(CancellationToken cancellationToken = default)
+        {
+            cancellationToken.ThrowIfCancellationRequested();
+            Reboot();
Relevance

●●● Strong

Team enforces non-blocking async contracts; should use DisconnectAsync/avoid UI-thread stalls,
consistent with recent async reliability fixes.

PR-#421
PR-#418
PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new RebootAsync implementation directly invokes Reboot() synchronously. Reboot() ultimately
calls the host's synchronous Disconnect(), and the base device explicitly documents Disconnect() as
blocking and recommends DisconnectAsync for non-blocking teardown; therefore RebootAsync can block
for the same reasons.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[859-868]
src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[77-90]
src/Daqifi.Core/Device/DaqifiDevice.cs[1451-1467]
src/Daqifi.Core/Device/IStreamingDevice.cs[366-375]

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

## Issue description
`DaqifiStreamingDevice.RebootAsync(...)` is documented/positioned as a non-blocking async API, but it immediately calls the synchronous `Reboot()` implementation. That reboot path performs a synchronous `Disconnect()` which can block for seconds (lock acquisition + teardown), so `RebootAsync` can still freeze the caller thread.

## Issue Context
- `DeviceAdministrationOperations.Reboot()` sends the reboot SCPI command and then calls `_host.Disconnect()` (sync).
- `DaqifiDevice.Disconnect()` explicitly blocks (and advises callers to use `DisconnectAsync()` for non-blocking teardown).
- `IStreamingDevice.RebootAsync` summary says it is "without blocking the calling thread".

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[847-868]

### Implementation direction
- Change `DaqifiStreamingDevice.RebootAsync` to avoid calling `Reboot()`.
- Instead:
 - Validate connection (same behavior as current reboot).
 - Send the reboot command (`Send(ScpiMessageProducer.RebootDevice)`).
 - Then `await DisconnectAsync(cancellationToken).ConfigureAwait(false)` to ensure teardown is non-blocking.
- Keep the existing synchronous `Reboot()` unchanged for callers that explicitly want the blocking behavior.

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


Results up to commit 70cfbd4 ⚖️ Balanced


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


Informational
1. Cancellation masked by validation ✓ Resolved 🐞 Bug ≡ Correctness
Description
DaqifiStreamingDevice.RebootAsync throws DeviceNotConnectedException before observing a pre-canceled
CancellationToken, so a canceled call on a disconnected device won’t surface as
OperationCanceledException. This is inconsistent with the rest of the new async wrappers (which
prioritize cancellation) and can cause callers to misclassify cancellation as an operational error.
Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R877-880]

+            if (!IsConnected)
+            {
+                throw new DeviceNotConnectedException();
+            }
Relevance

●●● Strong

Team previously prioritized cancellation checks before IsConnected/validation in async device
methods; this matches that precedent.

PR-#329
PR-#381

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The interface documents RebootAsync as cancellable (throws OperationCanceledException) and observes
the token before sending; the new override checks connectivity first, so a pre-canceled token can’t
take precedence when disconnected.

src/Daqifi.Core/Device/IStreamingDevice.cs[369-387]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[875-883]

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

## Issue description
`DaqifiStreamingDevice.RebootAsync` currently validates `IsConnected` before checking `cancellationToken.ThrowIfCancellationRequested()`, which means a pre-canceled call when the device is disconnected throws `DeviceNotConnectedException` instead of `OperationCanceledException`.

## Issue Context
This method is part of the new async surface where most `...Async` wrappers check cancellation before doing any other work/validation.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[877-883]

## Suggested change
Move `cancellationToken.ThrowIfCancellationRequested();` to the top of the method (before the `IsConnected` check), or explicitly decide and document that connection validation takes precedence over cancellation for this method.

## Optional test
Add a unit test asserting that `RebootAsync(CanceledToken())` throws `OperationCanceledException` even when `IsConnected == false`.

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


Qodo Logo

Comment thread src/Daqifi.Core/Device/DaqifiStreamingDevice.cs Outdated
…cking

Qodo review (PR #469): RebootAsync delegated to the synchronous Reboot(),
which tears down through the blocking DaqifiDevice.Disconnect() — up to
a 10s stall on the caller thread, contradicting the documented
non-blocking contract.

RebootAsync now sends the reboot command directly and awaits
DaqifiDevice.DisconnectAsync instead, mirroring how ConnectAsync/
DisconnectAsync are genuinely async on the base class. The synchronous
Reboot() is unchanged for callers that want the blocking behavior.
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Comment thread src/Daqifi.Core/Device/DaqifiStreamingDevice.cs
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 70cfbd4

…tion

Qodo review (PR #469, round 2): RebootAsync checked IsConnected before
the cancellation token, so a pre-cancelled call against a disconnected
device surfaced DeviceNotConnectedException instead of
OperationCanceledException — inconsistent with every other ...Async
member on the class, which all check cancellation first.

Swapped the order and added a regression test.
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 66a019b

… test

Qodo review (PR #469, round 3): the disconnected+pre-cancelled RebootAsync
test only checked the exception type, not that cancellation actually
short-circuited before any side effect. Adds the same
empty-SentMessages/still-disconnected assertions the sibling
cancellation tests already make.
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit b0d1fa2

@tylerkron

Copy link
Copy Markdown
Contributor Author

Bench test

Ran a real-hardware check of `RebootAsync` on the bench Nq1 (fw 3.7.2, USB CDC, `/dev/cu.usbmodem1101`) against this branch's `Daqifi.Core` — this is the one behavior in the PR a fake-transport unit test can't exercise (real link-drop timing racing the async teardown).

Result: pass.

Connecting to /dev/cu.usbmodem1101...
Connected in 4742 ms. IsConnected=True, Name=DAQiFi Device
Pre-reboot: IsConnected=True, FirmwareVersion=3.7.2, PartNumber=Nq1
Calling RebootAsync()...
RebootAsync() returned in 509 ms. IsConnected=False
PASS: RebootAsync returned promptly (< 3000 ms), confirming it did not block on the sync teardown path.
Waiting for the device to drop off and come back after reboot...
Port disappeared after 403 ms (device is restarting).
PASS: reconnected after reboot. IsConnected=True, Name=DAQiFi Device
ALL CHECKS PASSED.
  • RebootAsync() returned in ~500ms — clear of the ~10s blocking teardown wait it was exposed to before the round-1 fix
  • Device genuinely dropped off (port disappeared within ~400ms) and reconnected cleanly afterward, firmware responding normally

No code changes resulted from this run.

@tylerkron
tylerkron added this pull request to the merge queue Aug 7, 2026
Merged via the queue into main with commit ed0760b Aug 7, 2026
1 check passed
@tylerkron
tylerkron deleted the claude/github-issue-460-42bbdf branch August 7, 2026 17:10
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: modernize the device interfaces — async + CancellationToken on IStreamingDevice, disposability on IDevice

1 participant