fix(mcp): stop PwmResult fabricating uncommanded PWM state

[tylerkron]

Summary

Fixes #450.

PwmResult (returned by set_pwm_output/disable_pwm) reported DutyCyclePercent/FrequencyHz straight out of Core's session-default seeds (DigitalChannel.PwmDutyCyclePercent = 50, ChannelControlOperations.PwmFrequencyHz = 1000) whenever nothing had actually been commanded, so a caller couldn't tell "this is the device's PWM configuration" from "this is a constant Core made up". The DTO's own doc promised a 0 sentinel for "none set" that was unreachable (the field is always seeded with a commandable value).

Changes

• PwmResult.DutyCyclePercent/FrequencyHz are now int?, null until a value has actually been commanded this session via set_pwm_output. Tracked in DaqifiAgent with two ConditionalWeakTable<,> keyed by channel/device identity, so a fresh connection (fresh channel/device instances) starts clean with no explicit eviction logic needed.
• disable_pwm no longer sends PWM:CHannel:ENable to a channel that isn't IsPwmCapable: such a channel can never have had PWM armed (the half-armed state the command exists to recover from is only reachable on capable channels), so the send only cost the device a spurious -200,"Execution error" for no effect.
• Updated the DTO/tool doc comments to describe the new null semantics and the disable_pwm no-op.

Testing

• dotnet test Daqifi.Core.sln — all 2870 tests pass.
• Verified end-to-end against the bench Nq1 (fw 3.7.2) via a throwaway harness driving DaqifiAgent directly against the local build:
  • disable_pwm(channel=1) (non-capable, never commanded): enabled=False duty=null freq=null (was duty=50 freq=1000 fabricated before the fix; no -200 sent now).
  • disable_pwm(channel=5) (capable, never commanded): duty=null freq=null.
  • set_pwm_output(channel=5, duty=30, freq=25): duty=30 freq=25 (real commanded values).
  • disable_pwm(channel=5) after commanding: duty=30 freq=25 (correctly still reported — it was actually commanded).
  • disable_pwm(channel=4) (capable, own duty never commanded, but frequency is device-wide and was committed by channel 5): duty=null freq=25 — confirms duty is tracked per-channel and frequency per-device, matching the hardware's shared-timer semantics.

Related

Split out of the same bench pass as #449.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix PWM result reporting when duty/frequency were never commanded

🐞 Bug fix 📝 Documentation 🕐 20-40 Minutes

AI Description

• Make PWM duty/frequency results null until explicitly commanded this session.
• Track “commanded” PWM duty per-channel and frequency per-device in DaqifiAgent.
• Avoid sending disable PWM SCPI on non-PWM-capable channels; update tool/DTO docs.

Diagram

graph TD
  T(["disable_pwm / set_pwm_output"]) --> A["DaqifiAgent"] --> C["Daqifi.Core PWM APIs"] --> D[("DAQiFi device")]
  A --> R["PwmResult (nullable duty/freq)"]
  subgraph Legend
    direction LR
    _tool(["Tool"]) ~~~ _mod["Module/DTO"] ~~~ _dev[("Device")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Represent “unset PWM” in Core (preferred long-term)

• ➕ Single source of truth across all callers, not just MCP
• ➕ Avoids agent-side state tracking keyed by object identity
• ➖ Larger change surface in Daqifi.Core; higher regression risk
• ➖ May require protocol/device query support to distinguish defaults vs actual state

2. Track commanded state in dictionaries keyed by (deviceId, channel)

• ➕ Simple, explicit keys; independent of object identity semantics
• ➖ Requires explicit eviction/reset on disconnect and reconnection
• ➖ Higher risk of stale state across sessions if lifecycle hooks are missed

3. Query device state on demand for PWM config

• ➕ Would reflect actual device configuration rather than session history
• ➖ Adds SCPI round-trips/latency and may not be supported consistently
• ➖ Does not solve “never commanded” semantics if device has no reliable sentinel

Recommendation: The current agent-side approach is a good, low-risk fix for MCP semantics: it avoids fabricating values from Core’s seeded defaults while keeping existing Core behavior unchanged. Using ConditionalWeakTable keyed by channel/device identity is a pragmatic way to scope the tracking to a connection lifetime without adding explicit eviction logic; a Core-level representation of “unset” could be considered later if multiple entrypoints need the same semantics.

Files changed (3) +45 / -12

Bug fix (2) +44 / -11

DaqifiAgent.csTrack commanded PWM duty/frequency and guard disable on non-capable channels +34/-4

Track commanded PWM duty/frequency and guard disable on non-capable channels

• Adds session-scoped tracking for whether PWM duty (per-channel) and frequency (per-device) were actually commanded via SetPwmOutputAsync. DisablePwmAsync now avoids sending SetPwmEnabled for non-PWM-capable channels and returns a PwmResult that reflects whether values were commanded rather than Core’s seeded defaults.

src/Daqifi.Mcp/DaqifiAgent.cs

Dtos.csMake PwmResult duty/frequency nullable with explicit “uncommanded” semantics +10/-7

Make PwmResult duty/frequency nullable with explicit “uncommanded” semantics

• Updates PwmResult to use nullable int fields for DutyCyclePercent and FrequencyHz and documents the new meaning. Extends the factory method to accept flags indicating whether duty/frequency were commanded and emits null when they were not.

src/Daqifi.Mcp/Dtos.cs

Documentation (1) +1 / -1

DaqifiTools.csClarify disable_pwm no-op behavior for non-PWM-capable channels +1/-1

Clarify disable_pwm no-op behavior for non-PWM-capable channels

• Updates the MCP tool description to state that disable_pwm is safe to call on any digital channel and is a no-op for channels that are not PWM-capable.

src/Daqifi.Mcp/Tools/DaqifiTools.cs

[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. Invalid AddOrUpdate call ✗ Dismissed 🐞 Bug ≡ Correctness

Description

DaqifiAgent.SetPwmOutputAsync calls ConditionalWeakTable.AddOrUpdate, which is not a
ConditionalWeakTable API in .NET, so this change will not compile. This affects both the duty and
frequency tracking updates.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R328-329]

            streaming.SetPwmDutyCycle(ch, dutyCyclePercent);
+            _pwmDutyCommanded.AddOrUpdate(ch, PwmCommandedMarker);

Relevance

●●● Strong

Build-breaking API misuse; team consistently accepts straightforward compile-fix feedback.

PR-#277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR-added code invokes AddOrUpdate on two ConditionalWeakTable instances; this API is not
part of ConditionalWeakTable and there is no in-repo implementation of such a method for these
tables, making this a build-breaking change.

src/Daqifi.Mcp/DaqifiAgent.cs[323-340]

Agent prompt

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

### Issue description
`ConditionalWeakTable<TKey,TValue>` does not have an `AddOrUpdate` method. The PR calls `_pwmDutyCommanded.AddOrUpdate(...)` and `_pwmFrequencyCommanded.AddOrUpdate(...)`, which will fail to compile.

### Issue Context
You only need “presence” semantics (a boolean marker), so you can use `GetValue(key, createValueCallback)` to ensure a marker exists, or use `TryGetValue` + `Add` with `Remove` on duplicates.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[326-336]

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

---

2. disable_pwm loses recovery ✓ Resolved 🐞 Bug ≡ Correctness

Description

DisablePwmAsync now skips sending SetPwmEnabled(..., false) for non-PWM-capable digital channels,
which removes the only recovery path Core documents for a non-capable channel getting stuck
“PWM-active” after an enable attempt. This can leave the channel wedged (ignoring digital writes)
with no MCP-level way to clear it.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R359-362]

+            if (ch is IDigitalChannel { IsPwmCapable: true })
+            {
+                streaming.SetPwmEnabled(ch, false);
+            }

Relevance

●● Moderate

Correctness claim vs PR intent; no close precedent on disable_pwm recovery behavior for non-capable
channels.

PR-#322

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR change gates the disable SCPI send on PWM capability, but Core explicitly states disabling
must be accepted on any digital channel as the only recovery from the firmware’s half-armed PWM
state when an enable hits a non-capable channel.

src/Daqifi.Mcp/DaqifiAgent.cs[351-366]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[175-193]

Agent prompt

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

## Issue description
`DisablePwmAsync` now gates `streaming.SetPwmEnabled(ch, false)` behind `IsPwmCapable`, but Core’s `SetPwmEnabled` contract explicitly allows *disabling* on any digital channel as the recovery operation for the firmware’s half-armed PWM state on non-capable channels.

## Issue Context
Core’s PWM enable path blocks enabling on non-capable channels because the firmware can mark the channel PWM-active before failing its capability check, and *disabling is the only recovery command*.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[351-366]

## Suggested fix
- Remove the `IsPwmCapable` guard and always call `streaming.SetPwmEnabled(ch, false)` for digital channels.
- If the original motivation was avoiding a spurious SCPI error on some firmwares, keep the unconditional call but catch/translate the specific “execution error” failure for the non-capable/no-op case (so recovery still works when needed, without failing the tool call).

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

---

3. Unobservable device errors ✓ Resolved 🐞 Bug ◔ Observability ⭐ New

Description

The updated disable_pwm tool description says it “may return a device-side error,” but the disable
path sends the PWM-disable command without querying/confirming the device error queue, so SCPI
execution errors are typically not surfaced as a failed MCP tool call or in the returned PwmResult.
This makes the new wording misleading/ambiguous for callers who might expect an error response to
indicate whether the channel was never armed.

Code

src/Daqifi.Mcp/Tools/DaqifiTools.cs[106]

+    [Description("Stop PWM output on a digital channel. The pin is left high-impedance (not driven); use set_digital_direction/set_digital_output to drive it digitally again. Allowed on any digital channel, including one that isn't PWM-capable — this is the only recovery path for the firmware's half-armed PWM state, and may return a device-side error if the channel was never actually armed.")]

Relevance

●●● Strong

Team often accepts changes to surface/clarify SCPI device-side errors; current doc implies
caller-visible errors.

PR-#266
PR-#455

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The updated tool docs promise/strongly imply a caller-visible error outcome, but the call chain
returns a PwmResult and does not include any synchronous mechanism that would surface SCPI execution
errors to the MCP caller in this path (no error-queue query/confirmation).

src/Daqifi.Mcp/Tools/DaqifiTools.cs[105-112]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[161-178]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[174-217]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[151-174]

Agent prompt

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

### Issue description
`disable_pwm`’s Description text says it “may return a device-side error if the channel was never actually armed.” However, the current implementation path for disabling PWM does not synchronously observe SCPI execution errors (it sends a command without confirming/reading the error queue), and the tool returns a `PwmResult` with no error field. This makes the new docs ambiguous and can mislead callers into expecting a caller-visible error response.

### Issue Context
- `disable_pwm` wraps `DaqifiAgent.DisablePwmAsync(...)` via `GuardAsync`, which only translates *thrown* exceptions.
- Core’s PWM disable is executed via `_host.Send(...)` (fire-and-forget), with no confirmation/error-queue read in this path.

### Fix Focus Areas
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[106-106]
- src/Daqifi.Mcp/DaqifiAgent.cs[345-351]

### Suggested fix
Update the tool description (and matching XML comment) to explicitly state that disabling PWM on a non-PWM-capable channel is allowed as a recovery command, and that if the firmware rejects it, the error may only be recorded in the device’s SCPI error queue and is not guaranteed to surface as a tool-call failure/result field. (Alternatively, if caller-visible failures are desired, switch this path to a confirming text exchange that drains/reads `SYSTem:ERRor?` after the command.)

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

---

4. Stale PWM state after reconnect ✗ Dismissed 🐞 Bug ☼ Reliability

Description

The new "commanded" tracking tables are never reset when Core performs an automatic reconnect on the
same device instance, so DisablePwmAsync can return non-null DutyCyclePercent/FrequencyHz after a
reconnect even though Core explicitly does not restore PWM state on reconnect. This reintroduces
“fabricated” PWM state specifically in reconnection scenarios.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R54-60]

+    /// <summary>
+    /// Which digital channels have had a PWM duty cycle actually commanded through
+    /// <see cref="SetPwmOutputAsync"/> this session, as opposed to Core's uncommanded
+    /// <see cref="DigitalChannel"/> default (#450). Keyed by channel identity so a fresh
+    /// connection — which gets fresh channel instances — starts clean without explicit eviction.
+    /// </summary>
+    private readonly ConditionalWeakTable<IChannel, object> _pwmDutyCommanded = new();

Relevance

●●● Strong

Reliability bug aligned with #450 intent; team has accepted MCP fixes for channel/repopulation edge
cases.

PR-#277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Core supports automatic reconnect on the same device instance and explicitly states PWM state is not
restored. Core also documents that Disconnect does not clear channels, and channel repopulation code
may reuse existing channel instances, so the agent’s identity-keyed markers can persist across
reconnect and then be read back when constructing PwmResult in DisablePwmAsync.

src/Daqifi.Mcp/DaqifiAgent.cs[54-68]
src/Daqifi.Mcp/DaqifiAgent.cs[357-367]
src/Daqifi.Core/Device/DaqifiDevice.cs[2732-2750]
src/Daqifi.Core/Device/DaqifiDevice.cs[3732-3737]
src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs[221-245]

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 agent’s “PWM commanded this session” state survives Core automatic reconnects, but Core documents that PWM state is *not* restored on reconnect. This means after a drop/reconnect the agent can again report duty/frequency as if they were commanded “this session” when they were not.

### Issue Context
Core reconnects the *same* device instance (`ReconnectOptions`) and does not clear channels on disconnect; status repopulation may reuse channel objects. Because the agent uses long-lived tables keyed by `IStreamingDevice` and `IChannel`, previously-set markers can remain true across a reconnect.

A robust approach is to track PWM-commanded state per device-session object that you can discard on reconnect boundaries:
- Replace the two global `ConditionalWeakTable<,>` markers with a single per-device/session state container (e.g., `ConditionalWeakTable<IStreamingDevice, PwmCommandSession>` where `PwmCommandSession` contains `HashSet<int> DutyCommandedChannels` and `bool FrequencyCommanded`).
- Subscribe once per connected device to `device.StatusChanged` (or `device.Reconnected`) and when the device transitions away from `Connected` (or upon `Reconnected`), remove/reset that device’s `PwmCommandSession` so the next PWM tool call starts clean.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[54-70]
- src/Daqifi.Mcp/DaqifiAgent.cs[313-340]
- src/Daqifi.Mcp/DaqifiAgent.cs[351-367]

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

---

5. Unused out markers ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

DisablePwmAsync declares dutyMarker and frequencyMarker from TryGetValue(..., out var ...) but
never uses them, producing unused-local warnings. This adds avoidable noise and can become a build
break in warning-as-error configurations.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R364-365]

+            var dutyCommanded = _pwmDutyCommanded.TryGetValue(ch, out var dutyMarker);
+            var frequencyCommanded = _pwmFrequencyCommanded.TryGetValue(streaming, out var frequencyMarker);

Relevance

●●● Strong

Team frequently fixes unused locals/fields to avoid TreatWarningsAsErrors build noise; discard out
vars.

PR-#423

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The introduced out var locals are never read; the repository also contains projects configured
with TreatWarningsAsErrors=true, so keeping MCP code warning-free is beneficial.

src/Daqifi.Mcp/DaqifiAgent.cs[357-366]
src/Daqifi.Mcp.Tests/Daqifi.Mcp.Tests.csproj[3-9]

Agent prompt

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

## Issue description
`TryGetValue` uses `out var dutyMarker` / `out var frequencyMarker`, but those locals are never referenced.

## Issue Context
Only the boolean return values are used (`dutyCommanded`, `frequencyCommanded`), so the `out` values should be discarded.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[364-365]

## Suggested fix
Replace:
```csharp
var dutyCommanded = _pwmDutyCommanded.TryGetValue(ch, out var dutyMarker);
var frequencyCommanded = _pwmFrequencyCommanded.TryGetValue(streaming, out var frequencyMarker);
```
with:
```csharp
var dutyCommanded = _pwmDutyCommanded.TryGetValue(ch, out _);
var frequencyCommanded = _pwmFrequencyCommanded.TryGetValue(streaming, out _);
```

ⓘ 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 bbd4015

Results up to commit fe7f64e ⚖️ Balanced

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

1. disable_pwm loses recovery ✓ Resolved 🐞 Bug ≡ Correctness

Description

DisablePwmAsync now skips sending SetPwmEnabled(..., false) for non-PWM-capable digital channels,
which removes the only recovery path Core documents for a non-capable channel getting stuck
“PWM-active” after an enable attempt. This can leave the channel wedged (ignoring digital writes)
with no MCP-level way to clear it.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R359-362]

+            if (ch is IDigitalChannel { IsPwmCapable: true })
+            {
+                streaming.SetPwmEnabled(ch, false);
+            }

Relevance

●● Moderate

Correctness claim vs PR intent; no close precedent on disable_pwm recovery behavior for non-capable
channels.

PR-#322

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR change gates the disable SCPI send on PWM capability, but Core explicitly states disabling
must be accepted on any digital channel as the only recovery from the firmware’s half-armed PWM
state when an enable hits a non-capable channel.

src/Daqifi.Mcp/DaqifiAgent.cs[351-366]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[175-193]

Agent prompt

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

## Issue description
`DisablePwmAsync` now gates `streaming.SetPwmEnabled(ch, false)` behind `IsPwmCapable`, but Core’s `SetPwmEnabled` contract explicitly allows *disabling* on any digital channel as the recovery operation for the firmware’s half-armed PWM state on non-capable channels.

## Issue Context
Core’s PWM enable path blocks enabling on non-capable channels because the firmware can mark the channel PWM-active before failing its capability check, and *disabling is the only recovery command*.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[351-366]

## Suggested fix
- Remove the `IsPwmCapable` guard and always call `streaming.SetPwmEnabled(ch, false)` for digital channels.
- If the original motivation was avoiding a spurious SCPI error on some firmwares, keep the unconditional call but catch/translate the specific “execution error” failure for the non-capable/no-op case (so recovery still works when needed, without failing the tool call).

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

---

2. Unused out markers ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

DisablePwmAsync declares dutyMarker and frequencyMarker from TryGetValue(..., out var ...) but
never uses them, producing unused-local warnings. This adds avoidable noise and can become a build
break in warning-as-error configurations.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R364-365]

+            var dutyCommanded = _pwmDutyCommanded.TryGetValue(ch, out var dutyMarker);
+            var frequencyCommanded = _pwmFrequencyCommanded.TryGetValue(streaming, out var frequencyMarker);

Relevance

●●● Strong

Team frequently fixes unused locals/fields to avoid TreatWarningsAsErrors build noise; discard out
vars.

PR-#423

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The introduced out var locals are never read; the repository also contains projects configured
with TreatWarningsAsErrors=true, so keeping MCP code warning-free is beneficial.

src/Daqifi.Mcp/DaqifiAgent.cs[357-366]
src/Daqifi.Mcp.Tests/Daqifi.Mcp.Tests.csproj[3-9]

Agent prompt

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

## Issue description
`TryGetValue` uses `out var dutyMarker` / `out var frequencyMarker`, but those locals are never referenced.

## Issue Context
Only the boolean return values are used (`dutyCommanded`, `frequencyCommanded`), so the `out` values should be discarded.

## Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[364-365]

## Suggested fix
Replace:
```csharp
var dutyCommanded = _pwmDutyCommanded.TryGetValue(ch, out var dutyMarker);
var frequencyCommanded = _pwmFrequencyCommanded.TryGetValue(streaming, out var frequencyMarker);
```
with:
```csharp
var dutyCommanded = _pwmDutyCommanded.TryGetValue(ch, out _);
var frequencyCommanded = _pwmFrequencyCommanded.TryGetValue(streaming, out _);
```

ⓘ 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 fe7f64e

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1c300ef

[tylerkron]

/agentic_review

[qodo-code-review]

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