Skip to content

feat: hardware-in-the-loop smoke test + /hil-smoke-test skill#219

Open
tylerkron wants to merge 3 commits into
mainfrom
feature/hil-smoke-test
Open

feat: hardware-in-the-loop smoke test + /hil-smoke-test skill#219
tylerkron wants to merge 3 commits into
mainfrom
feature/hil-smoke-test

Conversation

@tylerkron
Copy link
Copy Markdown
Contributor

Summary

Adds a developer-local hardware-in-the-loop (HIL) smoke test for confirming real-device USB/serial integration still works after touching the SDK. All existing tests are mock-based — this is the first piece of real-device coverage that lives in the repo.

Two pieces:

  • `src/Daqifi.Core.SmokeTest/` — a standalone console project (`net9.0;net10.0`) that discovers, connects, reads metadata, enables ADC channels, streams briefly, asserts a reasonable sample throughput, and tears down cleanly. Exit codes are stable and named (`10`=no device, `11`=connect fail, `12`=metadata missing, `13`=degraded samples, `20`=no samples, `99`=unexpected). Runnable without Claude — `dotnet run --project src/Daqifi.Core.SmokeTest`.

  • `.claude/skills/hil-smoke-test/SKILL.md` — a project-level Claude Code skill that orchestrates the binary, interprets exit codes with concrete remediation suggestions, retries transient failures once before reporting, and refuses out-of-scope destructive operations.

Design notes

  • Test logic in C#, not in the AI prompt. The skill is a thin orchestrator. The C# program is deterministic, diff-able, and could feed a future self-hosted CI runner without further changes.
  • Read-only w.r.t. persistent device state. No SD card writes, no network reconfig, no firmware updates. Each of those is destructive enough to warrant its own opt-in skill.
  • Always tears down. `StopStreaming` and `Disconnect` run in `finally` so a failed run doesn't leave the device streaming and poison the next attempt.
  • Tolerant sample-count threshold (~50% of `rate × duration`). Real broken streams produce zero samples; a half-rate run is "weird but alive" and should be investigated, not silently passed.

Test plan

  • `dotnet build src/Daqifi.Core.SmokeTest -c Release` — passes on net9.0 and net10.0 ✓ (verified locally)
  • `dotnet run --project src/Daqifi.Core.SmokeTest -- --help` — prints usage ✓ (verified locally)
  • Plug in a DAQiFi device, run `dotnet run --project src/Daqifi.Core.SmokeTest` — confirm exit 0 and a sane sample count. Not yet run against real hardware — expect the first hardware run may need a tweak (sample-count threshold or post-`StartStreaming` delay).
  • Disconnect the device, re-run — confirm exit code `10` (no device found).
  • Invoke via Claude Code: `smoke test the device` or `/hil-smoke-test` — confirm the skill builds + runs + summarizes.

Out of scope

Each of these is destructive or hardware-modifying and belongs in its own opt-in tool, not a smoke test:

  • SD card formatting / log operations
  • WiFi or LAN reconfiguration
  • PIC32 or WiFi-module firmware updates

🤖 Generated with Claude Code

Introduces a developer-local sanity check for confirming real-device
USB/serial integration still works after touching the SDK.

- src/Daqifi.Core.SmokeTest: standalone console project (net9.0;net10.0)
  that discovers, connects, reads metadata, streams briefly, asserts a
  reasonable sample throughput, and tears down cleanly. Stable named
  exit codes (10/11/12/13/20/99) so wrappers can react to failure mode
  without parsing message text.
- .claude/skills/hil-smoke-test/SKILL.md: project-level skill that
  orchestrates the binary, interprets exit codes with concrete
  remediation suggestions, and refuses out-of-scope destructive ops
  (SD card, firmware, network reconfig).

Read-only with respect to persistent device state. The test logic
lives in C# (deterministic, runnable without Claude); the skill is a
thin orchestrator so HIL coverage stays reproducible.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tylerkron tylerkron requested a review from a team as a code owner May 23, 2026 03:12
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Add hardware-in-the-loop smoke test + Claude Code skill

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add hardware-in-the-loop smoke test console app for real-device USB/serial validation
• Implement deterministic C# test logic with stable named exit codes for failure modes
• Create Claude Code skill to orchestrate test execution and interpret results
• Enable developer-local sanity checks after SDK modifications without destructive operations
Diagram
flowchart LR
  A["Developer touches SDK"] --> B["Run smoke test"]
  B --> C["SmokeTest.Program.cs"]
  C --> D["Discover device"]
  D --> E["Connect & init"]
  E --> F["Read metadata"]
  F --> G["Stream & verify samples"]
  G --> H["Exit code 0-99"]
  H --> I["SKILL.md orchestrator"]
  I --> J["Interpret & report to user"]

Loading

File Changes

1. src/Daqifi.Core.SmokeTest/Program.cs 🧪 Tests +342/-0

Hardware-in-the-loop smoke test implementation

• Implements 6-step hardware-in-the-loop workflow: discover → connect → metadata → enable channels →
 stream → verify samples
• Defines 7 stable exit codes (0, 2, 10, 11, 12, 13, 20, 99) for deterministic failure mode
 detection
• Includes configurable options parser for port, baud rate, stream rate, duration, and channel
 bitmask
• Enforces cleanup in finally block to prevent device state pollution between runs
• Tolerates ~50% sample threshold to distinguish real breakage from transient latency

src/Daqifi.Core.SmokeTest/Program.cs


2. .claude/skills/hil-smoke-test/SKILL.md 📝 Documentation +89/-0

Claude Code skill for test orchestration and diagnostics

• Defines Claude Code skill for orchestrating the smoke test binary and interpreting exit codes
• Provides remediation guidance for each exit code with most likely causes and troubleshooting steps
• Documents command-line flags and when to pass them (port, baud, rate, duration, channels)
• Specifies retry logic for transient failures (exit codes 10, 11, 13) and deterministic failures
 (0, 2, 12, 20, 99)
• Explicitly scopes out destructive operations (SD card, firmware, network) as out-of-scope

.claude/skills/hil-smoke-test/SKILL.md


3. Daqifi.Core.sln ⚙️ Configuration changes +7/-0

Register smoke test project in solution

• Register new Daqifi.Core.SmokeTest project in solution file
• Add project configuration entries for Debug and Release builds
• Nest project under src folder hierarchy

Daqifi.Core.sln


View more (1)
4. src/Daqifi.Core.SmokeTest/Daqifi.Core.SmokeTest.csproj ⚙️ Configuration changes +17/-0

Smoke test project file configuration

• Create new console application project targeting net9.0 and net10.0
• Enable implicit usings and nullable reference types
• Add project reference to Daqifi.Core for device communication APIs
• Mark as non-packable (internal tooling only)

src/Daqifi.Core.SmokeTest/Daqifi.Core.SmokeTest.csproj


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 23, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Auto mode ignores baud ✓ Resolved 🐞 Bug ≡ Correctness
Description
When --port=auto is used with a non-default --baud, the smoke test probes ports at the requested
baud but then connects via ConnectFromDeviceInfoAsync, which uses the factory’s serial DeviceInfo
path that defaults to 9600. This can make discovery succeed but the subsequent connect fail for the
same device.
Code

src/Daqifi.Core.SmokeTest/Program.cs[R75-112]

Evidence
The smoke test uses the provided baud for discovery, but the factory’s ConnectFromDeviceInfoAsync
serial path ultimately calls a ConnectSerialAsync overload that defaults to 9600, so the actual
connect baud can differ from the discovery baud.

src/Daqifi.Core.SmokeTest/Program.cs[72-112]
src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs[269-274]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[235-356]
src/Daqifi.Core/Device/DaqifiDeviceFactory.cs[131-137]

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

### Issue description
In auto-discovery mode the smoke test creates `SerialDeviceFinder(options.BaudRate)` but then connects with `DaqifiDeviceFactory.ConnectFromDeviceInfoAsync(deviceInfo)`. The factory’s serial-DeviceInfo connection path uses the overload `ConnectSerialAsync(portName, options)` which hardcodes the default baud (9600), ignoring the requested baud.

### Issue Context
This causes a mismatch between discovery baud and connection baud whenever the user specifies `--baud` (or when a device requires non-default baud), producing a false connect failure.

### Fix Focus Areas
- src/Daqifi.Core.SmokeTest/Program.cs[72-112]

### Suggested fix
In the `deviceInfo != null` branch, connect using the discovered serial port name *and* `options.BaudRate` (e.g. `ConnectSerialAsync(deviceInfo.PortName!, options.BaudRate)`), since this smoke test is serial-focused.
Optionally, if you want to keep using `ConnectFromDeviceInfoAsync`, extend the factory API to accept/propagate baud for serial DeviceInfo connections (larger change).

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


2. Throughput check ignores channels ✓ Resolved 🐞 Bug ≡ Correctness
Description
The degraded-samples threshold compares analogSampleCount to rate × duration, but
analogSampleCount is computed by summing msg.AnalogInData.Count, which grows with the number of
enabled channels. With >1 channel enabled (including the default bitmask 3), a significantly
degraded stream can incorrectly pass the threshold.
Code

src/Daqifi.Core.SmokeTest/Program.cs[R146-201]

Evidence
analogSampleCount is derived from msg.AnalogInData.Count (a repeated field), while channels are
enabled via a bitmask where each bit enables a channel; therefore the total element count is
channel-count dependent but the expected threshold is not.

src/Daqifi.Core.SmokeTest/Program.cs[141-201]
src/Daqifi.Core/Communication/Messages/DaqifiOutMessage.cs[211-220]
src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[331-353]

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 smoke test counts analog samples as the total number of elements in `DaqifiOutMessage.AnalogInData` across received messages, then compares that count against `rate × duration` (and a 50% threshold). Since ADC channels are enabled via a bitmask and `AnalogInData` is a repeated field, the element count typically scales with the number of enabled channels, making the threshold too lenient as channel count increases.

### Issue Context
Default `--channels=3` enables channels 0 and 1, which likely doubles `AnalogInData.Count` per time tick compared to a single channel. The current threshold therefore can mark degraded streams as healthy.

### Fix Focus Areas
- src/Daqifi.Core.SmokeTest/Program.cs[141-201]

### Suggested fix
Parse `options.ChannelBitmask` as an integer mask, compute `enabledChannels = popcount(mask)` (and error if 0 / invalid), then compare either:
- `analogSampleCount` vs `(rate × duration × enabledChannels)` (and corresponding 50% threshold), or
- `ticks = analogSampleCount / enabledChannels` vs `(rate × duration)`.
Also consider validating `--channels` is a valid decimal integer at argument-parse time so invalid masks become exit code 2 (BadArgs).

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



Remediation recommended

3. Broken Program.cs doc link ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The skill markdown links to src/Daqifi.Core.SmokeTest/Program.cs using a path relative to
.claude/skills/hil-smoke-test/, which does not exist at that location. This breaks navigation to
the referenced deterministic test logic.
Code

.claude/skills/hil-smoke-test/SKILL.md[R18-22]

Evidence
The markdown file location makes the current relative link resolve under
.claude/skills/hil-smoke-test/… instead of the repo-root src/… tree.

.claude/skills/hil-smoke-test/SKILL.md[18-22]

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

### Issue description
`SKILL.md` links to `src/Daqifi.Core.SmokeTest/Program.cs` using a relative path, but because the markdown file is under `.claude/skills/hil-smoke-test/`, that relative link resolves to a non-existent path.

### Issue Context
This is a documentation/navigation issue for reviewers and users reading the skill.

### Fix Focus Areas
- .claude/skills/hil-smoke-test/SKILL.md[18-22]

### Suggested fix
Change the link target to reach repo root, e.g. `../../../src/Daqifi.Core.SmokeTest/Program.cs`, or use a repo-root absolute link if your renderer supports it.

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


Grey Divider

Qodo Logo

Comment thread src/Daqifi.Core.SmokeTest/Program.cs
Comment thread src/Daqifi.Core.SmokeTest/Program.cs
tylerkron and others added 2 commits May 22, 2026 21:20
Three corrections from a self-review pass.

- csproj: TargetFrameworks net9.0;net10.0 → TargetFramework net9.0.
  Multi-targeting an exe forces every dotnet run to pass -f, which the
  skill's example commands did not — so the first invocation would fail
  with a confusing TFM-selection error. As a developer tool the smoke
  test only needs one runtime; pick the lib's minimum.
- Program.cs: drop the 100ms delay between EnableAdcChannels and
  StartStreaming and the comment that justified it. The producer queue
  already serializes Send calls; the README, DEVICE_INTERFACES, and
  end-to-end tests all sequence these back-to-back. The comment also
  asserted a specific firmware -200 behavior with no evidence behind it.
- Program.cs: help-text exit-code section reformatted as a single
  ordered column so codes read top-to-bottom in order.
- SKILL.md: drop the stale "-f net10.0" note now that the target is
  single.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three correctness fixes flagged by code review.

1. Auto mode honored --baud only for discovery, then connected through
   ConnectFromDeviceInfoAsync whose serial path resets the baud to the
   SDK default (9600). Collapse both branches into a single
   ConnectSerialAsync(port, baud) call so the user's baud is end-to-end.

2. The degraded-samples threshold compared AnalogInData total to
   rate × duration, but AnalogInData carries one element per enabled
   channel per tick — so the threshold was off by a factor of N
   (channels), letting a real ~50%-loss stream pass with default
   --channels=3. Parse --channels as int at parse time, derive the
   enabled-channel count via BitOperations.PopCount, and multiply
   expected by it. Invalid --channels values now surface as exit
   code 2 (BadArgs) instead of being passed verbatim to the device.

3. SKILL.md's relative link to Program.cs assumed repo-root resolution
   but lives at .claude/skills/hil-smoke-test/, so GitHub renders it
   broken. Use ../../../src/... to escape the subdirectory.

Also drop an unused ConnectAttemptCount constant noticed in passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tylerkron
Copy link
Copy Markdown
Contributor Author

@qodo-code-review thanks for the review — all three findings addressed in 6def15c.

# Finding Action
1 Auto mode ignores --baud Collapsed both connect branches into a single ConnectSerialAsync(portName, options.BaudRate) call so the user's baud is end-to-end. Detail in inline reply.
2 Throughput check ignores channels --channels now parsed as int at parse time (invalid → exit 2), enabled-channel count derived via BitOperations.PopCount, expected = rate × duration × channels. Detail in inline reply.
3 Broken Program.cs doc link Changed (src/Daqifi.Core.SmokeTest/Program.cs) to (../../../src/Daqifi.Core.SmokeTest/Program.cs) — the markdown lives under .claude/skills/hil-smoke-test/, so GitHub resolves the link relative to that directory.

One thing I deliberately did not do: extend ConnectFromDeviceInfoAsync to propagate baud through its serial path. That's a real library improvement — the factory currently drops back to 9600 for any caller using device-info connections — but it changes a public API surface, which is out of scope for this smoke-test PR. Tracking as a separate task.

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