feat(device): DeviceFeature requirement table + Supports() seam (closes #256)

[tylerkron]

Closes #256.

Why now

ADR 0001 deferred the version-gating layer until daqifi-core consumed a command introduced above the v3.5.0 floor. #347 did exactly that — SD file transfer over WiFi, gated at firmware >= v3.7.0 — implemented as a bespoke inline version compare in DaqifiStreamingDevice. That was the trigger condition, so this generalizes it into the seam the ADR specified and routes the existing gates through it.

What changed

DeviceFeatureRequirements (new, internal) — a static DeviceFeature -> FeatureRequirement table carrying a minimum firmware version, a board allow-list, and hardware flags. Seeded from ADR 0001's firmware audit and covering all four current features: AnalogOutput (NQ3-only, no version gate), SdStorageQuery (floor + SD hardware), CapabilityDocument (v3.5.0), SdFileTransferOverWifi (v3.7.0 + SD + WiFi). SdOverWifiMinFirmware moved here from DaqifiStreamingDevice.

DaqifiDevice.Supports(DeviceFeature) / EnsureSupported(DeviceFeature) — evaluated live against Metadata on every call, never cached, since the board variant and firmware version arrive in separate status-message branches.

Both hand-rolled gates now route through the seam. EnsureSdFileTransferSupportedOnTransport() keeps only the transport predicate (which feature applies over WiFi vs. USB) and delegates the support question; the SD-storage-query -113 backstop builds its typed exception from the table via CreateFeatureNotSupportedException. No bespoke feature checks remain in DaqifiStreamingDevice.

The one design call worth reviewing

The two axes fail differently, deliberately:

• Firmware version fails closed — absent or unparseable reports unsupported, preserving feat(sd): allow SD file transfer over WiFi on firmware >= v3.7.0 #347's conservative rule. Dispatching an SD command over WiFi to pre-v3.7.0 firmware stalls on the shared SPI bus, so an unknown version is not treated as permission.
• Board and hardware requirements are evaluated only once the board is known. While DeviceType is Unknown, FromDeviceType has not run and Capabilities holds all-false defaults that mean "not yet known", not "hardware absent". Reading them as violations would newly refuse SD-over-WiFi on a device that has merely not reported its part number yet — a behavior regression against feat(sd): allow SD file transfer over WiFi on firmware >= v3.7.0 #347. The wire-level -113 remains the backstop for that window. The cost asymmetry justifies the split: a DAC command on an NQ1 returns a clean SCPI error, whereas an SD command over WiFi on old firmware stalls the bus.

Supports() landed on DaqifiDevice, not on DeviceCapabilities as ADR 0001 sketched — DeviceCapabilities is board-derived and never sees the firmware version, so it structurally cannot answer a version gate. This also matches where its siblings already live (MinSupportedFirmware, IsFirmwareVersionSupported, Metadata), none of which are on IDevice. The ADR is updated with an implementation note recording both decisions.

Testing

• 32 new table-driven tests across every axis: below / at / above minimum, absent, unparseable, Int32-overflow, pre-release, wrong board, missing hardware, unknown board, plus a guard asserting the table stays exhaustive as DeviceFeature grows.
• Full suite green: 1900 passed, 0 failed on net9.0 and net10.0; 0 build warnings under TreatWarningsAsErrors.
• Bench-tested on a Nyquist1 (FW 3.7.2) over USB with the example CLI built against this branch: 10 Hz stream (23 samples, clean stop), --sd-list (20 files, exit 0), --sd-storage (exit 0). Both refactored call sites exercised on real hardware. The below-gate SD-over-WiFi branch only exists on pre-v3.7.0 firmware and is covered by unit tests, not the bench.

Out of scope

Part 2 of #256 — the CONFigure:CAPabilities:JSON? / :APIVersion? device self-description reader (firmware #327) — stays deferred, as the issue specifies.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Add DeviceFeature requirements table and Supports() feature-gating seam

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add a central DeviceFeatureRequirements table and DaqifiDevice.Supports/EnsureSupported seam.
• Route SD-over-WiFi and SD-storage-query gates through the seam (no bespoke checks).
• Add exhaustive unit coverage and document the finalized ADR 0001 implementation details.

Diagram

graph TD
  DSD["DaqifiStreamingDevice"] --> DD["DaqifiDevice"] --> DFR["DeviceFeatureRequirements"] --> FV["FirmwareVersion"]
  DD --> DM["DeviceMetadata"]
  DD --> FNSE["FeatureNotSupportedException"]
  DSD --> FNSE
  T["DeviceFeatureSupportTests"] --> DD --> DFR

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Encode requirements as DeviceFeature enum attributes

• ➕ Keeps requirements co-located with the enum members
• ➕ Can reduce the need for an explicit dictionary table
• ➖ Requires reflection (or source generation) and complicates AOT/linking scenarios
• ➖ Harder to express shared constants (e.g., v3.7.0) cleanly
• ➖ Still needs the same nuanced runtime rules (fail-closed version, skip board/hardware when unknown)

2. Introduce an injectable IDeviceFeatureGate service

• ➕ Avoids static table; easier to swap policies per environment/testing
• ➕ Makes dependencies explicit and can simplify future expansions
• ➖ More plumbing for a small, internal concern
• ➖ Risk of scattering call sites unless DaqifiDevice remains the primary API

3. Defer gating to live capability document (#327) only

• ➕ Eventually more accurate than board-derived capabilities defaults
• ➕ Could remove some board/hardware heuristics
• ➖ Does not solve today’s above-floor firmware gate use case
• ➖ Still needs a conservative pre-capability window; the seam is still useful

Recommendation: Keep the PR’s approach: a single internal requirements table plus DaqifiDevice.Supports/EnsureSupported as the sole gate API. It matches ADR 0001’s intent, eliminates bespoke version compares, and correctly handles the split arrival of board vs. firmware metadata (live evaluation + fail-closed version gating + deferred board/hardware checks while DeviceType is Unknown). The alternatives add reflection/plumbing or don’t address the immediate above-floor gating trigger.

Files changed (5) +600 / -43

Enhancement (2) +251 / -0

DaqifiDevice.csAdd Supports(DeviceFeature), EnsureSupported, and exception factory +119/-0

Add Supports(DeviceFeature), EnsureSupported, and exception factory

• Implements a live-evaluated feature gating API on DaqifiDevice backed by DeviceFeatureRequirements. Enforces version gates conservatively (absent/unparseable => unsupported) while skipping board/hardware requirements until the board is known, and centralizes typed exception creation for both pre-checks and wire-level -113 backstops.

src/Daqifi.Core/Device/DaqifiDevice.cs

DeviceFeatureRequirements.csIntroduce DeviceFeature→FeatureRequirement requirement table +132/-0

Introduce DeviceFeature→FeatureRequirement requirement table

• Adds a static internal requirements table defining per-feature minimum firmware versions, optional board allow-lists, and hardware requirements (SD/WiFi). Includes shared version constants (e.g., SD-over-WiFi v3.7.0) and fails fast via ArgumentOutOfRangeException if a DeviceFeature member lacks a table entry.

src/Daqifi.Core/Device/DeviceFeatureRequirements.cs

Refactor (1) +18 / -31

DaqifiStreamingDevice.csRoute SD-over-WiFi and -113 SD storage backstop through the Supports seam +18/-31

Route SD-over-WiFi and -113 SD storage backstop through the Supports seam

• Removes the bespoke inline firmware version comparison for SD file transfer over WiFi and delegates to EnsureSupported(DeviceFeature.SdFileTransferOverWifi), keeping only the transport predicate. Updates the SD storage query -113 handler to throw a typed exception built via CreateFeatureNotSupportedException, sourcing required version/board from the shared table.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (1) +288 / -0

DeviceFeatureSupportTests.csAdd unit tests for feature requirements table and Supports/EnsureSupported +288/-0

Add unit tests for feature requirements table and Supports/EnsureSupported

• Introduces comprehensive tests validating table exhaustiveness and behavior across firmware boundaries, parse failures (fail-closed), board allow-lists, hardware flags, and DeviceType.Unknown semantics. Also verifies EnsureSupported exception fields (feature, required/actual version, board nullability).

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

Documentation (1) +43 / -12

0001-firmware-feature-gating.mdDocument Supports() seam + requirement table as built (ADR 0001) +43/-12

Document Supports() seam + requirement table as built (ADR 0001)

• Updates follow-ups and decision text to reflect that the previously-deferred feature/version table and Supports() seam are now implemented. Adds an implementation note explaining why Supports() lives on DaqifiDevice and how version vs. board/hardware axes fail differently.

docs/adr/0001-firmware-feature-gating.md

[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. Misleading firmware requirement ✓ Resolved 🐞 Bug ≡ Correctness

Description

DaqifiDevice.EnsureSupported always builds FeatureNotSupportedException with the table’s MinVersion,
even when Supports(feature) failed due to board or hardware requirements. This can incorrectly imply
a firmware upgrade is needed (via RequiredVersion/message) when the device already meets the version
constraint but lacks required hardware/board support.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R156-185]

+        public void EnsureSupported(DeviceFeature feature)
+        {
+            if (!Supports(feature))
+            {
+                throw CreateFeatureNotSupportedException(feature);
+            }
+        }
+
+        /// <summary>
+        /// Builds the typed <see cref="FeatureNotSupportedException"/> for <paramref name="feature"/>,
+        /// populating the required version from the requirement table and the actual version and
+        /// board from the current <see cref="Metadata"/>.
+        /// </summary>
+        /// <remarks>
+        /// Used by <see cref="EnsureSupported"/> and by the wire-level <c>-113</c> backstop, which
+        /// throws on the firmware's authoritative answer rather than on a table lookup and so needs
+        /// to build the exception without first re-testing <see cref="Supports"/>.
+        /// </remarks>
+        /// <param name="feature">The feature the device does not support.</param>
+        /// <returns>The exception to throw.</returns>
+        /// <exception cref="ArgumentOutOfRangeException">
+        /// Thrown when <paramref name="feature"/> has no entry in the requirement table.
+        /// </exception>
+        protected FeatureNotSupportedException CreateFeatureNotSupportedException(DeviceFeature feature)
+        {
+            return new FeatureNotSupportedException(
+                feature,
+                DeviceFeatureRequirements.For(feature).MinVersion,
+                Metadata.FirmwareVersion,
+                Metadata.DeviceType == DeviceType.Unknown ? null : Metadata.DeviceType);

Relevance

⭐⭐⭐ High

Team has accepted fixes for misleading FeatureNotSupportedException fields/messages; likely to
refine RequiredVersion for non-version failures.

PR-#288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Supports() can fail due to board/hardware checks, but CreateFeatureNotSupportedException()
always injects MinVersion into the exception. The exception message builder interprets any
non-null RequiredVersion as a firmware upgrade requirement, so failures caused by board/hardware
can be reported as (incorrectly) requiring newer firmware.

src/Daqifi.Core/Device/DaqifiDevice.cs[107-141]
src/Daqifi.Core/Device/DaqifiDevice.cs[156-186]
src/Daqifi.Core/Device/DeviceFeatureRequirements.cs[41-100]
src/Daqifi.Core/Device/FeatureNotSupportedException.cs[60-79]

Agent prompt

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

### Issue description
`EnsureSupported()` throws `FeatureNotSupportedException` via `CreateFeatureNotSupportedException(feature)`, which unconditionally populates `RequiredVersion` from the requirement table. Since `Supports()` can fail due to board allow-list or hardware flags (not just version), the exception can claim `Requires firmware >= X` even when the firmware already satisfies X.

### Issue Context
- `Supports()` returns `false` on non-version failures (board/hardware).
- `FeatureNotSupportedException.BuildMessage()` renders any non-null `RequiredVersion` as `"Requires firmware >= ..."`, so populating it when version is not the failing axis is misleading.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[107-186]
- src/Daqifi.Core/Device/FeatureNotSupportedException.cs[60-79]
- src/Daqifi.Core/Device/DeviceFeatureRequirements.cs[41-100]

### Suggested fix approach
1. Refactor support evaluation into a single internal helper (e.g., `EvaluateSupport(DeviceFeature feature, out SupportFailure failure)`), used by both `Supports()` and `EnsureSupported()`.
2. When constructing the exception, set `requiredVersion` **only if** the version check failed (unparseable/absent, or parsed < MinVersion). Otherwise pass `null` for `requiredVersion`.
3. (Optional but best) Extend `FeatureNotSupportedException` to carry/print non-version reasons (e.g., required hardware flags / allowed boards) so diagnostics remain actionable when the version is OK.

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

---

2. Mutable requirements arrays ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

DeviceFeatureRequirements stores board allow-lists as mutable DeviceType[] and returns them by
reference in FeatureRequirement.Boards. Because these types are internal (and visible to the test
assembly), any internal/friend-assembly code can accidentally mutate the array and silently change
Supports() behavior process-wide.

Code

src/Daqifi.Core/Device/DeviceFeatureRequirements.cs[R41-80]

+    internal readonly record struct FeatureRequirement(
+        FirmwareVersion? MinVersion,
+        DeviceType[]? Boards,
+        HardwareRequirement Hardware);
+
+    /// <summary>
+    /// The <see cref="DeviceFeature"/> → <see cref="FeatureRequirement"/> table behind
+    /// <see cref="DaqifiDevice.Supports"/> (ADR 0001, docs/adr/0001-firmware-feature-gating.md).
+    /// Sourced from the firmware audit table in that ADR; when daqifi-core starts consuming a new
+    /// firmware-gated command, its <see cref="DeviceFeature"/> member and its entry here are added
+    /// together — <see cref="For"/> throws for a member with no entry, and
+    /// <c>DeviceFeatureRequirementsTests</c> asserts the table stays exhaustive.
+    /// </summary>
+    internal static class DeviceFeatureRequirements
+    {
+        /// <summary>
+        /// Minimum firmware for SD-card access (file transfer and the storage-space query) over a
+        /// WiFi/TCP connection. Firmware <c>#598/#599</c> (first released <b>v3.7.0</b>) route the
+        /// SD reply to the requesting interface; before that the SD card and WiFi contend for the
+        /// shared SPI bus, so these operations were USB-only.
+        /// </summary>
+        internal static readonly FirmwareVersion SdOverWifiMinFirmware = new(3, 7, 0, null, 0);
+
+        /// <summary>
+        /// Minimum firmware for the capability document (<c>CONFigure:CAPabilities:JSON?</c> /
+        /// <c>:APIVersion?</c>), first released in v3.5.0 (firmware #327/#343).
+        /// </summary>
+        internal static readonly FirmwareVersion CapabilityDocumentMinFirmware = new(3, 5, 0, null, 0);
+
+        private static readonly DeviceType[] Nyquist3Only = { DeviceType.Nyquist3 };
+
+        private static readonly IReadOnlyDictionary<DeviceFeature, FeatureRequirement> Table =
+            new Dictionary<DeviceFeature, FeatureRequirement>
+            {
+                // Board-gated, not version-gated: the DAC commands shipped in v3.2.0 (below the
+                // floor) but the firmware rejects them on any board that isn't NQ3.
+                [DeviceFeature.AnalogOutput] = new(
+                    MinVersion: null,
+                    Boards: Nyquist3Only,
+                    Hardware: HardwareRequirement.None),

Relevance

⭐⭐⭐ High

Precedent: team accepted making exposed global allow-list collections truly immutable to prevent
in-process mutation.

PR-#318

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The requirements table uses a shared DeviceType[] instance and exposes it via the returned
FeatureRequirement, while Supports() uses that array for allow-list checks; mutating the array
changes gating results globally.

src/Daqifi.Core/Device/DeviceFeatureRequirements.cs[41-44]
src/Daqifi.Core/Device/DeviceFeatureRequirements.cs[70-80]
src/Daqifi.Core/Device/DaqifiDevice.cs[118-127]

Agent prompt

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

### Issue description
`FeatureRequirement.Boards` is a `DeviceType[]?` and the table stores shared array instances (e.g., `Nyquist3Only`). Arrays are mutable, so internal/friend-assembly code can accidentally modify the allow-list and globally alter feature gating.

### Issue Context
Although `DeviceFeatureRequirements` is `internal`, the test assembly is already granted access via `InternalsVisibleTo`, and future internal consumers could also mutate the array.

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceFeatureRequirements.cs[41-100]
- src/Daqifi.Core/Device/DaqifiDevice.cs[118-127]

### Suggested fix approach
- Change `Boards` from `DeviceType[]?` to an immutable/read-only type (e.g., `ImmutableArray<DeviceType>?`, `IReadOnlyList<DeviceType>?`, or `ReadOnlyMemory<DeviceType>?`).
- Alternatively, keep arrays privately but have `For()` return a defensive copy for `Boards`.
- Update `Supports()` to use the chosen representation (`Contains`/`IndexOf` equivalent) without exposing mutable storage.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 59572ff

[tylerkron]

Response to Qodo review

Both findings accepted and fixed in 8666481.

1. Misleading firmware requirement (🐞 Bug, Correctness) — fixed. Genuine bug. EnsureSupported took RequiredVersion from the table regardless of which axis failed, so a Nyquist1 on fw 3.7.0 with no SD card got "Requires firmware >= 3.7.0; the device reports '3.7.0'". Supports() and EnsureSupported() now share one EvaluateSupport() returning the failing axis, and a required version is reported only for a version failure. The -113 backstop still attributes to the version on purpose — there the firmware itself says it doesn't know the command.

2. Mutable requirements arrays (🐞 Bug, Maintainability) — fixed. Boards is now ImmutableArray<DeviceType>?. I'd dismissed this in self-review because the type is internal; the InternalsVisibleTo argument is what changed my mind.

Not done: extending FeatureNotSupportedException to print required hardware/allowed boards (your optional item 3 on finding 1). It's a public-API addition beyond #256's scope, and Board already appears in the message. Say the word and I'll open a follow-up.

Verification

• Full suite green: 1903 passed / 0 failed on net9.0 and net10.0, 0 warnings under TreatWarningsAsErrors.
• 3 new tests, including EnsureSupported_WhenHardwareIsTheFailingAxis_DoesNotClaimAFirmwareRequirement, which fails against the previous commit.
• Re-bench-tested on a Nyquist1 (FW 3.7.2) over USB after the fix: --sd-list exit 0 (20 files), --sd-storage exit 0.

[tylerkron]

/agentic_review

[qodo-code-review]

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