Skip to content

feat(hid): exclusive HID open for held bootloader sessions - #274

Merged
cptkoolbeenz merged 5 commits into
mainfrom
feat/hid-exclusive-open
Jun 28, 2026
Merged

cptkoolbeenz merged 5 commits into
mainfrom
feat/hid-exclusive-open

Conversation

@cptkoolbeenz

Copy link
Copy Markdown
Member

What

Adds HidLibraryTransport.ExclusiveAccess (default false). When set, the HID device is opened exclusively so no other user-mode opener can open or write to it while this transport holds the handle:

  • Windows: dwShareMode=0 via HidSharp OpenConfiguration + OpenOption.Exclusive.
  • macOS: IOKit kIOHIDOptionsTypeSeizeDevice.

IHidTransportDevice.Open() becomes Open(bool exclusive) across both platform backends and the test fakes.

Best-effort: a refused exclusive open falls back to a shared open, so any flow that works today is never regressed.

Why

Enables the desktop to grab and hold a sitting PIC32 HID bootloader and to lock other openers out for the duration of a flash. The bootloader's inbound CRC gate is disabled in fielded firmware, so a stray SOH…EOT frame from another opener (e.g. a discovery loop) could be mis-parsed as an ERASE; the exclusive handle guards against that. A vendor-defined top-level collection (Usage Page 0xFF00) permits exclusive access, unlike the system keyboard/mouse collections hidclass keeps shared.

This is the minimal Core change that the upcoming desktop grab-and-hold-the-bootloader feature depends on.

Testing

  • Core build clean; 1272/1274 tests pass (2 pre-existing skips).
  • New unit tests cover exclusive open + the shared-open fallback.
  • Hardware-validated end-to-end via the desktop flash path against a live PIC32 HID bootloader (exclusive open succeeded on the real vendor collection; no fallback needed).

🤖 Generated with Claude Code

Add HidLibraryTransport.ExclusiveAccess (default false). When set, the HID
device is opened exclusively — Windows dwShareMode=0 via HidSharp
OpenConfiguration, macOS IOKit kIOHIDOptionsTypeSeizeDevice — so no other
user-mode opener can open or write to the device while the handle is held.
Best-effort: a refused exclusive open falls back to a shared open so existing
flows are never regressed. IHidTransportDevice.Open() becomes Open(bool exclusive)
across both platform backends and the test fakes.

Enables the desktop to grab and hold a sitting PIC32 HID bootloader and to lock
out other openers during a flash (the bootloader's CRC gate is disabled, so a
stray frame from another opener could be mis-parsed as an ERASE).
@cptkoolbeenz
cptkoolbeenz requested a review from a team as a code owner June 28, 2026 16:41
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add best-effort exclusive HID opens for bootloader flash sessions
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

Description

• Add an opt-in ExclusiveAccess flag to request exclusive HID handles during ConnectAsync.
• Implement best-effort exclusive open on Windows (HidSharp) and macOS (IOKit seize), with shared
 fallback.
• Update HID transport seam and add unit tests to validate exclusive vs shared open behavior.
Diagram

graph TD
A["Desktop flash flow"] --> B["HidLibraryTransport"] --> C["IHidTransportDevice.Open(exclusive)"]
C --> D["Windows: HidSharp TryOpen"]
C --> E["macOS: IOHIDDeviceOpen(seize)"]
B --> F["Core unit tests (fakes)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Make exclusivity a ConnectAsync parameter (not a mutable property)
  • ➕ Avoids hidden state on the transport instance; call sites must choose per connection.
  • ➕ Makes it harder to accidentally reuse a transport with the wrong open mode.
  • ➖ API signature change to ConnectAsync (and any interface) ripples further than a property.
  • ➖ Harder to configure via DI/options patterns if this transport is created once and reused.
2. Always attempt exclusive open for vendor-collection devices
  • ➕ No new configuration surface; bootloader safety is automatic when applicable.
  • ➕ Reduces chance a caller forgets to enable ExclusiveAccess.
  • ➖ Heuristics can be brittle (usage-page checks may not be available everywhere).
  • ➖ May surprise existing consumers that intentionally expect sharing semantics.
3. Implement a higher-level coordination lock (named mutex / lockfile)
  • ➕ Coordinates multiple instances of the same app even if OS exclusivity is not available.
  • ➕ Can provide clearer user-facing error messages when contention occurs.
  • ➖ Does not prevent other non-cooperating processes from opening/writing to the HID device.
  • ➖ Adds cross-platform locking complexity and still may not protect against stray writes.

Recommendation: The PR’s approach (opt-in flag with best-effort exclusive open and shared fallback) is a good minimal Core change: it provides real OS-enforced protection against stray writes during bootloader flashing while preserving existing behavior by default. If this expands beyond the bootloader use case, consider shifting to a per-connect parameter to reduce mutable transport state.

Files changed (5) +98 / -10

Enhancement (3) +64 / -8
HidLibraryPlatform.csExtend HID transport device seam to Open(bool exclusive) with HidSharp exclusive attempt +36/-5

Extend HID transport device seam to Open(bool exclusive) with HidSharp exclusive attempt

• Updates IHidTransportDevice.Open to accept an exclusivity request and implements best-effort exclusive open for the HidSharp backend using OpenConfiguration/OpenOption.Exclusive, falling back to shared open when necessary.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs

HidLibraryTransport.csAdd ExclusiveAccess flag and pass it through ConnectAsync +12/-1

Add ExclusiveAccess flag and pass it through ConnectAsync

• Introduces HidLibraryTransport.ExclusiveAccess (default false) with documentation explaining bootloader safety rationale. ConnectAsync now calls device.Open(ExclusiveAccess) so the transport can request exclusive handles when desired.

src/Daqifi.Core/Communication/Transport/HidLibraryTransport.cs

MacOsHidPlatform.csAdd macOS exclusive open via IOHID seize with shared fallback +16/-2

Add macOS exclusive open via IOHID seize with shared fallback

• Extends the macOS IOKit backend to support Open(bool exclusive) by using kIOHIDOptionsTypeSeizeDevice when requested. Implements best-effort fallback to a shared open when seize is refused and adds the missing native constant.

src/Daqifi.Core/Communication/Transport/MacOsHidPlatform.cs

Tests (2) +34 / -2
HidLibraryDeviceEnumeratorTests.csUpdate enumerator fake to match new Open(exclusive) signature +1/-1

Update enumerator fake to match new Open(exclusive) signature

• Adjusts the FakeHidTransportDevice implementation to accept the new Open(bool exclusive) method signature while keeping test behavior the same.

src/Daqifi.Core.Tests/Communication/Transport/HidLibraryDeviceEnumeratorTests.cs

HidLibraryTransportTests.csAdd tests for default shared open and ExclusiveAccess behavior +33/-1

Add tests for default shared open and ExclusiveAccess behavior

• Adds unit coverage ensuring ExclusiveAccess defaults to false (shared open) and that setting the flag threads an exclusive-open request into the device open call. Enhances the fake device to capture the last requested exclusivity.

src/Daqifi.Core.Tests/Communication/Transport/HidLibraryTransportTests.cs

@qodo-code-review

qodo-code-review Bot commented Jun 28, 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


Remediation recommended

1. TryOpen result ignored ✓ Resolved 🐞 Bug ≡ Correctness
Description
In HidLibraryTransportDevice.Open(bool), the shared-open fallback ignores the boolean return value
from _device.TryOpen(out sharedStream) and treats any non-null sharedStream as a successful open.
This is inconsistent with the exclusive-open path (which checks the bool) and can misclassify a
failed open attempt, leading to harder-to-diagnose downstream failures when later I/O is attempted.
Code

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[R168-171]

+            try
+            {
+                _device.TryOpen(out sharedStream);
+            }
Relevance

⭐⭐⭐ High

Team accepts defensive transport correctness fixes; similar hardening merged in PR #240 and HID
robustness in #263.

PR-#240
PR-#263

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The shared-open fallback currently does not use the TryOpen boolean return, while the exclusive-open
attempt in the same method explicitly checks it, indicating it is part of the success contract and
should be honored consistently.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[164-177]
src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[147-152]

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 shared-open fallback calls `_device.TryOpen(out sharedStream)` but ignores the returned `bool`. The exclusive-open path treats the boolean return as meaningful (`if (_device.TryOpen(...) && stream != null)`), so the shared-open path should do the same to avoid accepting an unsuccessful open attempt.

### Issue Context
This code is in the shared-open fallback inside `HidLibraryTransportDevice.Open(bool exclusive)`.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[164-177]

### Suggested change
Update the shared-open attempt to capture and validate the `bool` return, e.g.:
- `var ok = _device.TryOpen(out sharedStream);`
- treat the open as success only when `ok && sharedStream != null`
- otherwise proceed to the existing failure handling (including preserving `sharedOpenError` / `exclusiveOpenError`).

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


2. Shared open bypasses wrapping ✓ Resolved 🐞 Bug ☼ Reliability
Description
In HidLibraryTransportDevice.Open(bool), the shared-open fallback calls _device.TryOpen(...) without
a try/catch; if it throws, the method bypasses the intended IOException("Failed to open HID
device.", exclusiveOpenError) path and drops the previously captured exclusive-open exception
context. This results in less diagnosable double-failure scenarios (ConnectAsync will only see/wrap
the shared-open thrown exception).
Code

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[R162-167]

+        if (opened == null)
        {
-            throw new IOException("Failed to open HID device.");
+            if (!_device.TryOpen(out var sharedStream) || sharedStream == null)
+            {
+                // Chain the exclusive-open failure (if any) so a double failure preserves the root cause.
+                throw new IOException("Failed to open HID device.", exclusiveOpenError);
Relevance

⭐⭐⭐ High

Team often wraps/normalizes thrown transport errors for diagnostics (accepted exception
translation/wrapping in PRs #240, #237).

PR-#240
PR-#237
PR-#263

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The method explicitly handles thrown exceptions from the exclusive TryOpen(...) (storing
exclusiveOpenError), but the shared fallback TryOpen(out ...) is not inside a try/catch;
therefore any thrown shared-open exception will escape directly and the code path that chains
exclusiveOpenError into the final IOException is skipped.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[143-167]
src/Daqifi.Core/Communication/Transport/HidLibraryTransport.cs[135-143]

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

## Issue description
`HidLibraryTransportDevice.Open(bool exclusive)` catches exceptions only from the *exclusive* `TryOpen(...)` attempt. The shared-open fallback `TryOpen(out ...)` is not wrapped, so if it throws, the method bypasses the final `IOException("Failed to open HID device.", exclusiveOpenError)` and loses the exclusive-open failure context.

## Issue Context
The code comment explicitly anticipates `TryOpen` can throw for refusal cases, and the method already stores an `exclusiveOpenError` to chain on a double-failure. That chaining currently only happens when the shared open fails via `TryOpen` returning `false`/`null`, not when it throws.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[143-167]

## Suggested fix
- Wrap the shared `_device.TryOpen(out var sharedStream)` in a `try/catch (Exception sharedEx)`.
- If it throws and `exclusiveOpenError != null`, throw a new `IOException("Failed to open HID device.", new AggregateException(exclusiveOpenError, sharedEx))` (or equivalent) so both causes are preserved.
- If it throws and `exclusiveOpenError == null`, throw `new IOException("Failed to open HID device.", sharedEx)` to keep the method’s exception surface consistent.

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



Informational

3. Exclusive fallback skips on throw ✓ Resolved 🐞 Bug ☼ Reliability
Description
In HidLibraryTransportDevice.Open(bool exclusive), the shared-open fallback only runs when the
exclusive TryOpen returns false; if the exclusive TryOpen throws, ConnectAsync fails without
attempting a shared open (contrary to the method’s documented best-effort fallback behavior). This
can turn some exclusive-open failures into hard connection failures even though a shared open might
have worked.
Code

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[R128-159]

+        HidStream? opened = null;
+
+        if (exclusive)
        {
-            throw new IOException("Failed to open HID device.");
+            // A2 (stray-write guard): open the bootloader's vendor collection exclusively
+            // (Windows dwShareMode=0) so no other user-mode opener — the desktop's own HID discovery
+            // loop, a second app instance, anything — can open or write to the device while this
+            // handle is held. The PIC32 bootloader's CRC check is disabled, so a stray SOH…EOT frame
+            // from another opener could be mis-parsed as an ERASE; the exclusive handle guards
+            // against that. A vendor-defined top-level collection (Usage Page 0xFF00) permits
+            // exclusive access, unlike the system keyboard/mouse collections hidclass keeps shared.
+            var exclusiveConfig = new OpenConfiguration();
+            exclusiveConfig.SetOption(OpenOption.Exclusive, true);
+
+            // Best-effort: a refused exclusive open (another handle already open — e.g. a transient
+            // discovery handle not yet released) falls through to the shared open below, so a flash
+            // that works today is never regressed by the added guard.
+            if (_device.TryOpen(exclusiveConfig, out var exclusiveStream) && exclusiveStream != null)
+            {
+                opened = exclusiveStream;
+            }
+        }
+
+        if (opened == null)
+        {
+            if (!_device.TryOpen(out var sharedStream) || sharedStream == null)
+            {
+                throw new IOException("Failed to open HID device.");
+            }
+
+            opened = sharedStream;
        }
Relevance

⭐⭐⭐ High

Team accepts reliability hardening via exception-handling to preserve best-effort behavior (seen in
PRs 180, 237, 240).

PR-#180
PR-#237
PR-#240

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The HidSharp backend’s exclusive open path calls _device.TryOpen(exclusiveConfig, ...) without a
try/catch; any exception escapes Open(bool) and prevents reaching the shared-open fallback block.
ConnectAsync then catches that exception and fails the connection rather than retrying shared.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[121-162]
src/Daqifi.Core/Communication/Transport/HidLibraryTransport.cs[135-143]

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

### Issue description
`HidLibraryTransportDevice.Open(bool exclusive)` intends to be best-effort (exclusive first, then fall back to shared), but the exclusive `_device.TryOpen(exclusiveConfig, ...)` call is not exception-safe. If it throws, the method exits before attempting the shared open.

### Issue Context
`HidLibraryTransport.ConnectAsync` wraps any exception from `device.Open(...)` and aborts the connect, so the fallback must happen inside `Open(bool exclusive)` to uphold the documented behavior.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs[121-162]

### Suggested change
- Wrap only the exclusive `TryOpen(exclusiveConfig, ...)` attempt in a narrow `try/catch`.
- On exception, proceed to the existing shared-open attempt.
- (Optional) If the shared open also fails, include the original exclusive exception as an inner exception to preserve diagnostics.

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


Grey Divider

Qodo Logo

…s a throwing TryOpen

The shared-open fallback only ran when the exclusive TryOpen returned false; if it
threw, ConnectAsync failed hard, contradicting the documented best-effort fallback.
Wrap the exclusive TryOpen so a throw also falls through to the shared open.
@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

qodo-code-review Bot commented Jun 28, 2026 •

Copy link
Copy Markdown

PR Code Suggestions ✨

Latest suggestions up to e141c34

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve failure causes on false returns

Create synthetic exceptions when TryOpen returns false to preserve diagnostic
information.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs [24-91]

 if (exclusive)
 {
-    // A2 (stray-write guard): open the bootloader's vendor collection exclusively
-    // (Windows dwShareMode=0) so no other user-mode opener — the desktop's own HID discovery
-    // loop, a second app instance, anything — can open or write to the device while this
-    // handle is held. The PIC32 bootloader's CRC check is disabled, so a stray SOH…EOT frame
-    // from another opener could be mis-parsed as an ERASE; the exclusive handle guards
-    // against that. A vendor-defined top-level collection (Usage Page 0xFF00) permits
-    // exclusive access, unlike the system keyboard/mouse collections hidclass keeps shared.
     var exclusiveConfig = new OpenConfiguration();
     exclusiveConfig.SetOption(OpenOption.Exclusive, true);
 
-    // Best-effort: a refused exclusive open falls through to the shared open below, so a flash
-    // that works today is never regressed by the added guard. This covers BOTH ways HidSharp can
-    // refuse — TryOpen returning false AND TryOpen throwing (e.g. a sharing-violation surfaced as
-    // an exception) — otherwise an exclusive-open throw would become a hard connection failure.
     try
     {
         if (_device.TryOpen(exclusiveConfig, out var exclusiveStream) && exclusiveStream != null)
         {
             opened = exclusiveStream;
         }
         else
         {
-            // TryOpen reported failure; dispose any partial stream so it isn't leaked.
             exclusiveStream?.Dispose();
+            exclusiveOpenError ??= new IOException("Exclusive HID open was refused (TryOpen returned false/null).");
         }
     }
     catch (Exception ex)
     {
-        // Exclusive open threw; remember why and leave opened == null so the shared open below
-        // is attempted. The cause is chained into the final throw if the shared open also fails.
         exclusiveOpenError = ex;
     }
 }
 
 if (opened == null)
 {
-    // The shared open can also throw (not just return false); catch it so a throwing shared
-    // fallback still routes through our normalized IOException and never drops the exclusive cause.
     HidStream? sharedStream = null;
     Exception? sharedOpenError = null;
     try
     {
         if (!_device.TryOpen(out sharedStream) || sharedStream == null)
         {
-            // TryOpen reported failure; dispose any partial stream and fall through to the throw.
             sharedStream?.Dispose();
             sharedStream = null;
+            sharedOpenError ??= new IOException("Shared HID open failed (TryOpen returned false/null).");
         }
     }
     catch (Exception ex)
     {
         sharedOpenError = ex;
     }
 
     if (sharedStream == null)
     {
-        // Both opens failed — preserve whatever cause(s) we have. When both threw, chain both via
-        // an AggregateException so neither root cause is lost for diagnosis.
         var cause = exclusiveOpenError != null && sharedOpenError != null
             ? new AggregateException(exclusiveOpenError, sharedOpenError)
             : sharedOpenError ?? exclusiveOpenError;
         throw new IOException("Failed to open HID device.", cause);
     }
 
     opened = sharedStream;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 4

__

Why: This suggestion provides a minor improvement for diagnostics by creating synthetic exceptions when TryOpen returns false, ensuring the failure reason is always captured in the inner exception.

Low
  • More

Previous suggestions

Suggestions up to commit 8e24081
CategorySuggestion                                                                                                                                    Impact
Possible issue
Respect open return values

Check the boolean return value from TryOpen and dispose of the stream if the
open operation fails.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs [24-81]

 if (exclusive)
 {
     ...
     try
     {
-        if (_device.TryOpen(exclusiveConfig, out var exclusiveStream) && exclusiveStream != null)
+        var ok = _device.TryOpen(exclusiveConfig, out var exclusiveStream);
+        if (ok && exclusiveStream != null)
         {
             opened = exclusiveStream;
+        }
+        else
+        {
+            exclusiveStream?.Dispose();
         }
     }
     catch (Exception ex)
     {
         ...
         exclusiveOpenError = ex;
     }
 }
 
 if (opened == null)
 {
     ...
     HidStream? sharedStream = null;
+    var sharedOpened = false;
     Exception? sharedOpenError = null;
     try
     {
-        _device.TryOpen(out sharedStream);
+        sharedOpened = _device.TryOpen(out sharedStream);
     }
     catch (Exception ex)
     {
         sharedOpenError = ex;
     }
 
-    if (sharedStream == null)
+    if (!sharedOpened || sharedStream == null)
     {
+        sharedStream?.Dispose();
         ...
         throw new IOException("Failed to open HID device.", cause);
     }
 
     opened = sharedStream;
 }
Suggestion importance[1-10]: 7

__

Why: The PR dropped the boolean check for TryOpen in the shared fallback path which existed in the original code, potentially leading to resource leaks or incorrectly treating failed opens as successful.

Medium
General
Avoid misleading error details

Initialize seizeResult to success and only populate it with the IOHIDDeviceOpen
result during an exclusive open attempt.

src/Daqifi.Core/Communication/Transport/MacOsHidPlatform.cs [15-33]

+var seizeResult = NativeMethods.kIOReturnSuccess;
+
 var options = exclusive
     ? NativeMethods.kIOHIDOptionsTypeSeizeDevice
     : NativeMethods.kIOHIDOptionsTypeNone;
+
 var result = NativeMethods.IOHIDDeviceOpen(_deviceRef, options);
-var seizeResult = result;
-if (result != NativeMethods.kIOReturnSuccess && exclusive)
+
+if (exclusive)
 {
-    // Refused seize falls back to a shared open so a flash that works today is not regressed.
-    result = NativeMethods.IOHIDDeviceOpen(_deviceRef, NativeMethods.kIOHIDOptionsTypeNone);
+    seizeResult = result;
+
+    if (result != NativeMethods.kIOReturnSuccess)
+    {
+        // Refused seize falls back to a shared open so a flash that works today is not regressed.
+        result = NativeMethods.IOHIDDeviceOpen(_deviceRef, NativeMethods.kIOHIDOptionsTypeNone);
+    }
 }
 
 if (result != NativeMethods.kIOReturnSuccess)
 {
-    // On a double failure surface both codes so the seize and the shared fallback are distinguishable.
     throw new IOException(
         exclusive
             ? $"IOHIDDeviceOpen failed (seize IOReturn=0x{seizeResult:X8}, shared IOReturn=0x{result:X8})."
             : $"IOHIDDeviceOpen failed (IOReturn=0x{result:X8}).");
 }
Suggestion importance[1-10]: 4

__

Why: This is a minor readability and maintainability improvement that prevents potentially misleading variables from being populated in non-exclusive open attempts.

Low
Suggestions up to commit 9d266d0
CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve exclusive-open failure details

Capture the exception thrown during the exclusive-open attempt to preserve
diagnostic information.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs [39-49]

+Exception? exclusiveOpenError = null;
+
 try
 {
     if (_device.TryOpen(exclusiveConfig, out var exclusiveStream) && exclusiveStream != null)
     {
         opened = exclusiveStream;
     }
 }
-catch (Exception)
+catch (Exception ex)
 {
     // Exclusive open threw; leave opened == null so the shared open below is attempted.
+    exclusiveOpenError = ex;
 }
Suggestion importance[1-10]: 7

__

Why: Capturing the exclusive-open exception improves error observability for debugging when both open attempts fail.

Medium
Chain failure causes on open

Include the exclusive-open exception as an inner exception when throwing the
shared-open failure.

src/Daqifi.Core/Communication/Transport/HidLibraryPlatform.cs [52-60]

 if (opened == null)
 {
     if (!_device.TryOpen(out var sharedStream) || sharedStream == null)
     {
-        throw new IOException("Failed to open HID device.");
+        throw new IOException(
+            "Failed to open HID device.",
+            exclusiveOpenError);
     }
 
     opened = sharedStream;
 }
Suggestion importance[1-10]: 4

__

Why: Chaining the exception enhances diagnostics, but the suggested code fails to compile independently because exclusiveOpenError is undefined in this snippet.

Low
General
Preserve both macOS open codes

Capture both the exclusive seize and shared-open failure codes to include in the
thrown exception message.

src/Daqifi.Core/Communication/Transport/MacOsHidPlatform.cs [18-23]

 var result = NativeMethods.IOHIDDeviceOpen(_deviceRef, options);
+var seizeResult = result;
+
 if (result != NativeMethods.kIOReturnSuccess && exclusive)
 {
     // Refused seize falls back to a shared open so a flash that works today is not regressed.
     result = NativeMethods.IOHIDDeviceOpen(_deviceRef, NativeMethods.kIOHIDOptionsTypeNone);
 }
Suggestion importance[1-10]: 4

__

Why: Preserving the seize error code is valuable for diagnostics, but the suggested code is incomplete because it does not actually update the thrown exception.

Low

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 9d266d0

When both the exclusive and shared opens fail, surface the root cause:
- HidSharp: capture the exclusive-open exception and chain it as the inner
  exception of the final IOException.
- macOS: include both the seize and shared-open IOReturn codes in the message.
@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

No code suggestions found for the PR.

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 60e4fcc

The shared-open fallback could itself throw, bypassing the normalized
IOException and dropping the captured exclusive-open cause. Wrap it and, on a
double failure, chain both causes via AggregateException so neither is lost.
@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Persistent suggestions updated to latest commit 8e24081

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 8e24081

…dy macOS codes

- HidSharp: check the TryOpen bool on both the exclusive and shared opens (a false
  return with a non-null stream was misclassified as success); dispose any partial
  stream so it isn't leaked.
- macOS: only populate seizeResult during an exclusive attempt so the error message
  never reports a misleading seize code on a shared-only open.
@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Persistent suggestions updated to latest commit e141c34

@qodo-code-review

Copy link
Copy Markdown

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

@cptkoolbeenz

Copy link
Copy Markdown
Member Author

Re: /improve suggestion "Preserve failure causes on false returns" — declining, with rationale.

The bug pass (/agentic_review) is at 0, and the substantive diagnostic suggestions from earlier passes are applied (capture + chain the exclusive-open exception; wrap the shared open; respect the TryOpen bool; dispose leaked streams; surface both macOS IOReturn codes).

This last suggestion proposes synthesizing IOExceptions when TryOpen returns false. Declining because:

  1. A false return carries no underlying exception — there's no real cause to preserve. The outer IOException("Failed to open HID device.") already states the condition clearly, so a synthetic "TryOpen returned false/null" inner adds string noise, not diagnostic signal.
  2. The suggested diff also deletes the explanatory A2 / best-effort comments, which document why the exclusive open + fallback exist (the disabled-CRC stray-ERASE guard). Those are worth keeping.
  3. The genuine variant of this concern — a throwing open dropping context — is already handled (exclusive and shared throws are captured and chained via AggregateException on a double failure).

Treating the bug pass (0) as the convergence signal here.

@cptkoolbeenz
cptkoolbeenz merged commit 93e4bf2 into main Jun 28, 2026
1 check passed
@cptkoolbeenz
cptkoolbeenz deleted the feat/hid-exclusive-open branch June 28, 2026 18:59
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.

1 participant