[tylerkron]

Summary

• DaqifiStreamingDevice.PwmFrequencyHz now defaults to a new DefaultPwmFrequencyHz (1000 Hz) instead of 0
• IDigitalChannel.PwmDutyCyclePercent now defaults to 50 and clamps to 1-100 on assignment (Math.Clamp)
• Daqifi.Mcp.DaqifiAgent.SetPwmOutputAsync drops its own "PwmFrequencyHz == 0 means unset" guard, since the property is never 0 anymore, and always sends a frequency (caller-supplied or the current commandable bookkeeping value)

Why

Core tracked PWM frequency and duty as bookkeeping that started at 0, which is not a commandable value. PwmFrequencyHz was documented as "0 when none has been set this session" — but 0 is not a commandable frequency, and Core already defines MinPwmFrequencyHz/MaxPwmFrequencyHz without enforcing them as a default. Every consumer (daqifi-desktop, and this repo's own Daqifi.Mcp agent) had to paper over the gap with its own "0 means unset, don't send it" special-casing.

Scope note: SetPwmFrequency/SetPwmDutyCycle — the methods that actually command the hardware — keep their existing throw-on-invalid-range behavior unchanged, including the duty=0 firmware-trap rejection (duty 0 is stored by firmware but never applied, so it's deliberately rejected rather than silently reinterpreted). The issue asks for commandable defaults + clamping on the bookkeeping properties themselves (PwmFrequencyHz, PwmDutyCyclePercent), not a change to command validation, and duty=0's rejection is a business rule distinct from ordinary range clamping.

Closes #306

Test plan

• New unit tests in DaqifiStreamingDeviceChannelManagementTests covering the new default values (PwmFrequencyHz, PwmDutyCyclePercent) and duty clamping on assignment (-5→1, 0→1, 150→100, etc.)
• Full dotnet test suite: Daqifi.Core.Tests (1484/1484, net9.0 + net10.0) and Daqifi.Mcp.Tests (23/23) all passing
• Verified on physical hardware: built a throwaway harness against this branch's Daqifi.Core.csproj via ProjectReference, connected to a Nyquist device on COM3, and confirmed:
  • PwmFrequencyHz reads 1000 Hz before any SetPwmFrequency call
  • PwmDutyCyclePercent reads 50 before any set, and clamps direct assignments (500→100, -20→1)
  • Real PWM output was successfully commanded and driven on a PWM-capable channel using only the commandable defaults, without ever needing an explicit SetPwmFrequency call first

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

PWM: commandable defaults for frequency/duty; clamp duty bookkeeping on set

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Default PWM frequency bookkeeping to 1000 Hz (never 0/unset)
• Default duty bookkeeping to 50% and clamp assignments to 1–100%
• Simplify MCP PWM command flow to always send a valid frequency

Diagram

graph TD
  T[["MCP Tool: set_pwm_output"]] --> A(["DaqifiAgent.SetPwmOutputAsync"]) --> S(["IStreamingDevice / DaqifiStreamingDevice"]) --> H{{"Device firmware"}}
  S --> C(["DigitalChannel bookkeeping"])
  subgraph Legend
    direction LR
    _tool[["Tool/API"]] ~~~ _svc(["Service/Agent"]) ~~~ _mod(["Core module"]) ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Track “unset” with nullable/flag instead of defaults

• ➕ Preserves the semantic distinction between “never set this session” vs “set to default”
• ➕ Avoids implying the device is truly running at the default value
• ➖ Pushes complexity into every consumer (same problem as today)
• ➖ Requires widening API surface (nullable int or additional property) and more branching

2. Make defaults configurable via options/config

• ➕ Allows product-specific defaults without code changes
• ➕ Keeps commandable invariants while remaining flexible
• ➖ Introduces configuration plumbing and precedence rules
• ➖ More surface area to test and document for a small gain

Recommendation: Keep the PR’s approach: commandable defaults + clamping in bookkeeping properties removes repeated consumer-side special-casing while keeping hardware-command validation behavior unchanged. The remaining ambiguity (“bookkeeping default does not prove device state”) is already addressed via updated documentation and is preferable to reintroducing an “unset” sentinel.

Files changed (7) +85 / -24

Enhancement (2) +39 / -7

DigitalChannel.csDefault duty to 50% and clamp duty bookkeeping to 1–100 +29/-4

Default duty to 50% and clamp duty bookkeeping to 1–100

• Introduces explicit min/max/default constants for PWM duty bookkeeping. Updates PwmDutyCyclePercent setter to clamp values into the commandable range, and initializes the backing field to 50 instead of 0.

src/Daqifi.Core/Channel/DigitalChannel.cs

DaqifiStreamingDevice.csAdd DefaultPwmFrequencyHz and initialize PwmFrequencyHz to 1000 +10/-3

Add DefaultPwmFrequencyHz and initialize PwmFrequencyHz to 1000

• Adds a DefaultPwmFrequencyHz constant (1000 Hz) and changes PwmFrequencyHz bookkeeping to default to that value instead of 0, ensuring it is always commandable.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Bug fix (1) +4 / -11

DaqifiAgent.csAlways send a PWM frequency (explicit or default) in SetPwmOutputAsync +4/-11

Always send a PWM frequency (explicit or default) in SetPwmOutputAsync

• Removes the guard that previously treated frequency=0 and device frequency=0 as an error. Updates command sequencing to always call SetPwmFrequency using either the caller-provided frequency or the core session-default/current bookkeeping value.

src/Daqifi.Mcp/DaqifiAgent.cs

Tests (1) +33 / -0

DaqifiStreamingDeviceChannelManagementTests.csAdd tests for PWM defaults and duty clamping +33/-0

Add tests for PWM defaults and duty clamping

• Adds coverage asserting the new default duty (50%) and default device PWM frequency (DefaultPwmFrequencyHz). Adds parameterized tests ensuring duty bookkeeping clamps assignments into 1–100.

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

Documentation (3) +9 / -6

IDigitalChannel.csDocument duty bookkeeping defaults and clamping semantics +4/-3

Document duty bookkeeping defaults and clamping semantics

• Updates interface documentation to reflect that PwmDutyCyclePercent defaults to 50 and clamps assignments to 1–100, while hardware changes still require SetPwmDutyCycle.

src/Daqifi.Core/Channel/IDigitalChannel.cs

IStreamingDevice.csUpdate PWM frequency bookkeeping documentation (no more 0/unset) +4/-2

Update PWM frequency bookkeeping documentation (no more 0/unset)

• Rewords the PwmFrequencyHz contract to clarify it is session bookkeeping that defaults to a commandable value rather than using 0 to represent “unset”.

src/Daqifi.Core/Device/IStreamingDevice.cs

DaqifiTools.csUpdate tool parameter description for new session default frequency +1/-1

Update tool parameter description for new session default frequency

• Adjusts the frequencyHz parameter description to reflect that passing 0 keeps the current session frequency, which now defaults to 1000 Hz until explicitly set.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 feat: give PWM frequency and duty commandable defaults and clamp them on set

1. Redundant frequency command ✓ Resolved 🐞 Bug ➹ Performance

Description

DaqifiAgent.SetPwmOutputAsync now always calls SetPwmFrequency, even when the caller passes
frequencyHz=0, causing an extra SCPI/device command on duty-only updates. This increases
latency/device traffic and needlessly reprograms the device-wide PWM timer for calls that only
intend to change duty or re-enable PWM.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R292-294]

            streaming.SetPwmDutyCycle(ch, dutyCyclePercent);
-            if (frequencyHz != 0)
-            {
-                streaming.SetPwmFrequency(frequencyHz);
-            }
+            streaming.SetPwmFrequency(frequencyHz != 0 ? frequencyHz : streaming.PwmFrequencyHz);
            streaming.SetPwmEnabled(ch, true);

Relevance

⭐⭐ Medium

No clear history on avoiding redundant PWM frequency sends; PR #281 previously skipped Send() when
frequencyHz==0.

PR-#281

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff shows SetPwmOutputAsync now unconditionally invokes SetPwmFrequency;
DaqifiStreamingDevice.SetPwmFrequency always performs a Send(), so this translates directly into
additional device I/O per call when frequencyHz is 0.

src/Daqifi.Mcp/DaqifiAgent.cs[278-301]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[629-649]

Agent prompt

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

## Issue description
`DaqifiAgent.SetPwmOutputAsync` always calls `streaming.SetPwmFrequency(...)` even when the caller passes `frequencyHz = 0` (meaning “use current session frequency”). `SetPwmFrequency` always sends a SCPI command, so this adds an avoidable device round-trip whenever callers update duty (or simply re-enable PWM) without changing frequency.

## Issue Context
This PR intentionally makes `Core.PwmFrequencyHz` default to a commandable value (1000 Hz), so the previous “unset == 0” guard is gone. That’s fine, but we can still avoid sending the frequency command repeatedly when it hasn’t changed.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[278-301]

### Suggested approach
- Compute `desiredFrequency = (frequencyHz != 0) ? frequencyHz : streaming.PwmFrequencyHz`.
- Track the last frequency actually *sent* per connected device id inside `DaqifiAgent` (e.g., `ConcurrentDictionary<string,int>` or dictionary guarded by `_gate`).
- Only call `streaming.SetPwmFrequency(desiredFrequency)` when:
 - there is no entry yet for that device (first PWM use this session), or
 - `desiredFrequency` differs from the last-sent value.
- Update the stored last-sent value when you do send.

This preserves “first use sets a commandable default” while avoiding redundant frequency reprogramming for duty-only updates.

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

---

2. SetPwmFrequency throws not clamps 📎 Requirement gap ≡ Correctness

Description

Core still rejects out-of-range PWM frequencies by throwing instead of clamping to
[MinPwmFrequencyHz, MaxPwmFrequencyHz]. This violates the requirement that PWM frequency be
clamped on set so invalid values cannot be stored or used by consumers.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R529-540]

        /// <summary>
-        /// Gets the last commanded device-wide PWM frequency in hertz, or 0 when none has been
-        /// set this session. Local bookkeeping mirroring <see cref="SetPwmFrequency"/>.
+        /// Default device-wide PWM frequency, in hertz, used until a frequency has been
+        /// commanded via <see cref="SetPwmFrequency"/>.
        /// </summary>
-        public int PwmFrequencyHz { get; private set; }
+        public const int DefaultPwmFrequencyHz = 1_000;
+
+        /// <summary>
+        /// Gets the last commanded device-wide PWM frequency in hertz. Local bookkeeping
+        /// mirroring <see cref="SetPwmFrequency"/>; defaults to <see cref="DefaultPwmFrequencyHz"/>
+        /// (a commandable value) until a frequency has been set this session.
+        /// </summary>
+        public int PwmFrequencyHz { get; private set; } = DefaultPwmFrequencyHz;

Relevance

⭐ Low

PR #281 intentionally validates PWM frequency and throws on out-of-range as a firmware guardrail,
not clamping.

PR-#281

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2 requires clamping PWM frequency to Core’s min/max on set. In SetPwmFrequency,
Core throws for out-of-range values rather than clamping, so assignments outside the allowed range
are not coerced as required.

PWM frequency is clamped to Core’s documented min/max on set
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[630-649]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[517-540]

Agent prompt

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

## Issue description
`SetPwmFrequency(int frequencyHz)` currently throws `ArgumentOutOfRangeException` when `frequencyHz` is outside `[MinPwmFrequencyHz, MaxPwmFrequencyHz]`, but the compliance requirement expects Core to clamp the assigned value to that range before storing/sending.

## Issue Context
The PR introduces a commandable default frequency (`DefaultPwmFrequencyHz`) but does not add clamping behavior for subsequent assignments, meaning callers can still fail rather than being safely coerced to a supported frequency.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[630-649]

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

---

[tylerkron]

Re: finding #2, "SetPwmFrequency throws not clamps" (optional/low relevance) — leaving this as-is intentionally, not fixing.

SetPwmFrequency/SetPwmDutyCycle are the methods that actually command real hardware, and both already enforce [MinPwmFrequencyHz, MaxPwmFrequencyHz] / 1-100 by throwing before ever sending an SCPI command — a stricter guarantee than clamping, since an invalid request never reaches the device at all. Silently clamping there instead would mean a caller's out-of-range request gets silently reinterpreted and sent as a different value than what was asked for, which is worse UX for a direct hardware command and, per this repo's own history (PR #281, cited by Qodo itself), was a deliberate choice: it's a firmware guardrail, not just generic range validation. It's also the same reasoning SetPwmDutyCycle already applies to duty=0 — that's rejected outright (a firmware trap, not silently bumped to 1) via the identical pattern.

Issue #306 names PwmFrequencyHz and IDigitalChannel.PwmDutyCyclePercent specifically — the local bookkeeping properties — as the things that should get commandable defaults and clamp-on-assignment, which is exactly what this PR does. I read that as intentionally distinct from changing the command-methods' validation, and kept SetPwmFrequency/SetPwmDutyCycle's existing throw behavior (and its passing tests) untouched.

Happy to revisit if the intent was actually to change the command methods too, but as written I don't think clamping a live hardware command is the right behavior here.