fix(device): block digital writes while PWM is active on the channel by tylerkron · Pull Request #473 · daqifi/daqifi-core

tylerkron · 2026-08-10T13:21:51Z

Summary

SetDioValue/SetDioDirection (and therefore the MCP set_digital_output/set_digital_direction tools) reported success and mirrored the commanded level/direction into local channel state while PWM was active on that channel, even though the firmware silently ignores the command. The pin keeps running its PWM waveform while every readable property in Core says it's a static driven level.

Confirmed on bench hardware (Nq1, fw 3.7.2, loopback rig) in #449: duty cycle and transition counts are unchanged after a "successful" digital write issued during PWM.

Fix

Adds a guard symmetric to the one SetPwmEnabled already has for the non-capable-channel case: SetDioValue/SetDioDirection now throw InvalidOperationException when IDigitalChannel.IsPwmEnabled is true, pointing the caller at SetPwmEnabled(channel, false) (the existing recovery path). No local mirroring happens, no SCPI is sent.

Also updates the set_digital_output/set_digital_direction/set_pwm_output MCP tool descriptions to document the precondition, per the issue's suggestion.

Testing

SetDioDirection_WhilePwmEnabled_ThrowsInvalidOperationException
SetDioValue_WhilePwmEnabled_ThrowsInvalidOperationExceptionAndDoesNotMirrorOutputValue
Full Daqifi.Core.Tests suite: 2855 passed
Full Daqifi.Mcp.Tests suite: 36 passed

Fixes #449

🤖 Generated with Claude Code

SetDioValue/SetDioDirection (and the MCP set_digital_output / set_digital_direction tools) reported success and mirrored the commanded level/direction into local channel state even while PWM was running on that channel. The firmware silently ignores the command, so the pin keeps outputting its PWM waveform while every readable property in Core claims a static driven level — confirmed on bench hardware (fw 3.7.2): duty/transition counts are unchanged after a "successful" digital write during PWM. Add a guard symmetric to the one SetPwmEnabled already has for the non-capable-channel case: SetDioValue/SetDioDirection now throw InvalidOperationException when IDigitalChannel.IsPwmEnabled is true, pointing the caller at SetPwmEnabled(channel, false). No local mirroring, no SCPI sent. Update the set_digital_output/set_digital_direction/set_pwm_output MCP tool descriptions to document the precondition. Fixes #449

qodo-code-review · 2026-08-10T13:23:21Z

PR Summary by Qodo

Fix: reject digital writes while PWM is enabled on a channel

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Throw InvalidOperationException on SetDioValue/SetDioDirection when PWM is active
• Prevent local state mirroring and SCPI writes that firmware would silently ignore
• Document the PWM precondition in MCP digital/PWM tool descriptions and add regression tests

Diagram

graph TD
  A["MCP Tools"] --> B["DaqifiAgent"] --> C["ChannelControlOperations"] --> D{"PWM enabled?"}
  D -->|"Yes"| E["Throw InvalidOperationException"]
  D -->|"No"| F["Mirror channel state"] --> G["Send SCPI"] --> H[("Device firmware")]
  subgraph Legend
    direction LR
    _svc(["Service/API"]) ~~~ _mod["Module/Logic"] ~~~ _dec{"Decision"} ~~~ _db[("Hardware/Device")]
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Auto-disable PWM on digital write

➕ One-call UX: callers can set digital level/direction without handling exceptions
➕ Avoids runtime errors in higher-level tooling
➖ Surprising side-effect (a digital write implicitly changes PWM configuration)
➖ Could disrupt timing-critical PWM use cases if called accidentally
➖ Harder to reason about state changes; violates principle of least astonishment

2. Allow call but do not mirror local state (best-effort send)

➕ Avoids lying in Core state while still attempting hardware control
➕ Maintains backward compatibility (no exception behavior change)
➖ Firmware still ignores the command, so behavior remains confusing
➖ Callers still see ‘success’ semantics unless additional signaling is introduced
➖ Adds ambiguity vs. a clear, actionable failure mode

3. Surface a richer error/result type (e.g., Result/Status)

➕ More expressive than exceptions; can encode ‘rejected due to PWM’ cleanly
➕ Could standardize device-operation outcomes across APIs
➖ Broad API change; larger refactor and adoption work
➖ Not aligned with existing exception-based guard patterns in Core

Recommendation: Keep the current explicit guard + InvalidOperationException. It is the clearest contract: digital writes are invalid while PWM is active, and callers must explicitly disable PWM first. This avoids silent divergence between Core state and real pin behavior, and it matches the existing ‘capability guard’ style already used by SetPwmEnabled.

Files changed (3) +53 / -3

Bug fix (1) +18 / -0

ChannelControlOperations.csGuard SetDioDirection/SetDioValue when PWM is enabled +18/-0

Guard SetDioDirection/SetDioValue when PWM is enabled
• Introduces EnsurePwmNotEnabled and calls it from SetDioDirection and SetDioValue. Prevents local state mirroring and SCPI writes when firmware would ignore commands during PWM, throwing an InvalidOperationException with recovery guidance.
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs

Tests (1) +32 / -0

DaqifiStreamingDeviceChannelManagementTests.csAdd regression coverage for DIO writes rejected during PWM +32/-0

Add regression coverage for DIO writes rejected during PWM
• Adds two unit tests asserting SetDioDirection/SetDioValue throw when PWM is enabled. Verifies no SCPI messages are sent and OutputValue is not mirrored on failure.
src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs

Documentation (1) +3 / -3

DaqifiTools.csDocument PWM precondition for digital-direction/output MCP tools +3/-3

Document PWM precondition for digital-direction/output MCP tools
• Updates tool descriptions for set_digital_direction, set_digital_output, and set_pwm_output to clarify that digital writes are rejected while PWM is enabled and that callers should disable PWM first.
src/Daqifi.Mcp/Tools/DaqifiTools.cs

qodo-code-review · 2026-08-10T13:28:58Z

Code Review by Qodo

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

1. ~~PWM check outside lock~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

RequirePwmDisabled is executed before device.RunExclusiveAsync, so PWM can be enabled/disabled by
another concurrent tool call between the check and the subsequent SetDioDirection/SetDioValue. This
can cause sporadic failures and can still surface Core’s SDK-oriented InvalidOperationException
(pointing to SetPwmEnabled) despite the MCP-specific guard message.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R276-279]

        var (device, streaming) = RequireStreaming(deviceId);
        var ch = RequireDigitalChannel(device, channel);
+        RequirePwmDisabled(ch);

Relevance

●●● Strong
Team often accepts fixes closing lock-check races; this is a concrete TOCTOU concurrency bug in MCP
guard.
PR-#381
PR-#248
PR-#411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new MCP PWM check runs outside the exclusive-operation critical section, but the actual DIO
operations run inside it; RunExclusiveAsync only enforces mutual exclusion within its delegate. Core
independently rejects DIO writes under PWM with an SDK-oriented exception message, so a race can
bypass the intended MCP-specific guidance.
src/Daqifi.Mcp/DaqifiAgent.cs[267-312]
src/Daqifi.Core/Device/DaqifiDevice.cs[817-867]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[183-190]

Agent prompt

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

### Issue description
`RequirePwmDisabled(ch)` is called *before* `device.RunExclusiveAsync(...)`. Because `RunExclusiveAsync` only serializes work inside its delegate, another concurrent tool call can toggle PWM between this pre-check and the actual digital write, leading to:
- false rejections (PWM disabled right after we throw), or
- leaking Core’s `InvalidOperationException` message (which references `SetPwmEnabled`, not MCP’s `disable_pwm`) if PWM becomes enabled after the pre-check.

### Issue Context
This PR added MCP-side validation to provide MCP-actionable guidance, but its current placement does not share the same critical section as the device operation it guards.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[278-281]
- src/Daqifi.Mcp/DaqifiAgent.cs[297-311]
- src/Daqifi.Mcp/DaqifiAgent.cs[656-671]

### Implementation guidance
- Move `RequirePwmDisabled(ch)` into the `RunExclusiveAsync` delegate for both `SetDigitalDirectionAsync` and `SetDigitalOutputAsync`, immediately before calling `streaming.SetDioDirection(...)` / `streaming.SetDioValue(...)`.
- Optionally keep the outer check for faster failure, but **also** re-check inside the exclusive delegate to guarantee consistent behavior under concurrency.
- Keep Core’s guard as defense-in-depth; the MCP check should be the user-facing one in normal races.

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

2. ~~DIO docs missing precondition~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SetDioDirection/SetDioValue now throw InvalidOperationException when PWM is enabled, but
IStreamingDevice’s public XML docs for those methods do not document this new
precondition/exception, risking unexpected runtime failures for SDK consumers.

Code

src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[R139-142]

+            EnsurePwmNotEnabled(channel);

            channel.Direction = direction;
            _host.Send(ScpiMessageProducer.SetDioPortDirection(

Relevance

●●● Strong
Team often updates XML docs to match new runtime behavior/exceptions; similar doc-mismatch fixes
were accepted.
PR-#321
PR-#357
PR-#388

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The implementation now calls EnsurePwmNotEnabled from both DIO methods, introducing a new
InvalidOperationException path. The public interface docs for SetDioDirection/SetDioValue currently
describe only basic usage and (for async) cancellation, with no mention of PWM-enabled rejection or
InvalidOperationException.
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[116-145]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[147-174]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]
src/Daqifi.Core/Device/IStreamingDevice.cs[179-225]

Agent prompt

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

### Issue description
`SetDioDirection` and `SetDioValue` now reject calls while `IDigitalChannel.IsPwmEnabled` is true (throwing `InvalidOperationException`). The public `IStreamingDevice` contract docs for these methods do not mention this new behavior.

### Issue Context
This PR intentionally changes runtime behavior for DIO operations under PWM. Public docs should reflect the new exception and the required recovery path (disable PWM first).

### Fix Focus Areas
- src/Daqifi.Core/Device/IStreamingDevice.cs[179-225]
- src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[116-174]
- src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]

### Suggested fix
Update `IStreamingDevice.SetDioDirection` and `IStreamingDevice.SetDioValue` XML docs to:
- State the precondition that PWM must be disabled on that channel.
- Add `<exception cref="InvalidOperationException">` describing the PWM-enabled case.
- Optionally mention the recovery call (`SetPwmEnabled(channel, false)`).

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

3. ~~MCP recovery message mismatch~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

EnsurePwmNotEnabled throws an InvalidOperationException whose message instructs callers to use
SetPwmEnabled(channel, false), but MCP surfaces that message verbatim and MCP users only have the
disable_pwm tool, making the failure guidance non-actionable in the MCP context.

Code

src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[R185-188]

+            if (channel is IDigitalChannel { IsPwmEnabled: true })
+            {
+                throw new InvalidOperationException(
+                    $"Channel {channel.ChannelNumber} has PWM enabled; digital direction/state commands are ignored by the firmware while PWM is running. Call SetPwmEnabled(channel, false) first.");

Relevance

●● Moderate
MCP user-facing guidance matters, but changing Core exception text vs MCP-layer translation is
subjective; no close precedent.
PR-#470

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Core now throws with a message hard-coding “Call SetPwmEnabled(channel, false) first”. MCP wraps and
rethrows exceptions using ex.Message, and the MCP recovery operation is exposed as the
disable_pwm tool—so the user-visible error text will reference a method name that is not an MCP
tool.
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[105-112]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[139-178]

Agent prompt

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

### Issue description
`EnsurePwmNotEnabled` throws an `InvalidOperationException` whose message explicitly says “Call SetPwmEnabled(channel, false) first.” In the MCP server, `GuardAsync` converts exceptions to `McpException(ex.Message)`, so MCP clients will see that message even though their recovery action is the `disable_pwm` tool.

### Issue Context
This PR intentionally blocks DIO writes while PWM is active. The error message should guide callers toward the correct recovery action in *both* SDK and MCP contexts.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[139-178]

### Suggested fix
Prefer a tool-agnostic message like: “Disable PWM on this channel first (e.g., SetPwmEnabled(channel, false)).”
Optionally, also translate this specific exception in the MCP layer to mention `disable_pwm` explicitly.

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

4. ~~Wrong issue reference~~ ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

A newly added comment in SetDigitalDirectionAsync cites issue “#473” even though the surrounding
implementation/docstrings (and this PR’s intent) reference “#449”, reducing traceability and
potentially misleading maintainers.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R279-282]

+        // The PWM check runs inside the same exclusive section as the write it guards, so no
+        // concurrent tool call can toggle PWM between the check and the send (#473) — an outer,
+        // unlocked check would leave that race open and could let Core's SDK-oriented exception
+        // (naming SetPwmEnabled, not an MCP tool) leak through instead of this guard's message.

Relevance

●●● Strong
Team frequently accepts fixes to misleading comments/docs for accuracy and traceability (similar
comment/doc clarifications accepted).
PR-#456
PR-#357

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new comment explicitly references “#473”, while the local MCP guard documentation in the same
file references “#449”, indicating the new reference is inconsistent with the intended issue
context.

src/Daqifi.Mcp/DaqifiAgent.cs[279-286]
src/Daqifi.Mcp/DaqifiAgent.cs[662-676]

Agent prompt

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

### Issue description
A new comment in `SetDigitalDirectionAsync` references `#473`, but the rest of the surrounding context (and this PR) consistently references `#449`. This breaks traceability and can misdirect future debugging/history lookups.

### Issue Context
The comment is describing the new in-lock PWM guard rationale, which is tied to the `#449` behavior change and the MCP-facing guard message.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[279-282]

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

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 114964d

Results up to commit 85bea30 ⚖️ Balanced

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

1. ~~DIO docs missing precondition~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SetDioDirection/SetDioValue now throw InvalidOperationException when PWM is enabled, but
IStreamingDevice’s public XML docs for those methods do not document this new
precondition/exception, risking unexpected runtime failures for SDK consumers.

Code

src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[R139-142]

+            EnsurePwmNotEnabled(channel);

            channel.Direction = direction;
            _host.Send(ScpiMessageProducer.SetDioPortDirection(

Relevance

●●● Strong
Team often updates XML docs to match new runtime behavior/exceptions; similar doc-mismatch fixes
were accepted.
PR-#321
PR-#357
PR-#388

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The implementation now calls EnsurePwmNotEnabled from both DIO methods, introducing a new
InvalidOperationException path. The public interface docs for SetDioDirection/SetDioValue currently
describe only basic usage and (for async) cancellation, with no mention of PWM-enabled rejection or
InvalidOperationException.
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[116-145]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[147-174]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]
src/Daqifi.Core/Device/IStreamingDevice.cs[179-225]

Agent prompt

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

### Issue description
`SetDioDirection` and `SetDioValue` now reject calls while `IDigitalChannel.IsPwmEnabled` is true (throwing `InvalidOperationException`). The public `IStreamingDevice` contract docs for these methods do not mention this new behavior.

### Issue Context
This PR intentionally changes runtime behavior for DIO operations under PWM. Public docs should reflect the new exception and the required recovery path (disable PWM first).

### Fix Focus Areas
- src/Daqifi.Core/Device/IStreamingDevice.cs[179-225]
- src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[116-174]
- src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]

### Suggested fix
Update `IStreamingDevice.SetDioDirection` and `IStreamingDevice.SetDioValue` XML docs to:
- State the precondition that PWM must be disabled on that channel.
- Add `<exception cref="InvalidOperationException">` describing the PWM-enabled case.
- Optionally mention the recovery call (`SetPwmEnabled(channel, false)`).

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

2. ~~MCP recovery message mismatch~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

EnsurePwmNotEnabled throws an InvalidOperationException whose message instructs callers to use
SetPwmEnabled(channel, false), but MCP surfaces that message verbatim and MCP users only have the
disable_pwm tool, making the failure guidance non-actionable in the MCP context.

Code

src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[R185-188]

+            if (channel is IDigitalChannel { IsPwmEnabled: true })
+            {
+                throw new InvalidOperationException(
+                    $"Channel {channel.ChannelNumber} has PWM enabled; digital direction/state commands are ignored by the firmware while PWM is running. Call SetPwmEnabled(channel, false) first.");

Relevance

●● Moderate
MCP user-facing guidance matters, but changing Core exception text vs MCP-layer translation is
subjective; no close precedent.
PR-#470

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Core now throws with a message hard-coding “Call SetPwmEnabled(channel, false) first”. MCP wraps and
rethrows exceptions using ex.Message, and the MCP recovery operation is exposed as the
disable_pwm tool—so the user-visible error text will reference a method name that is not an MCP
tool.
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[105-112]
src/Daqifi.Mcp/Tools/DaqifiTools.cs[139-178]

Agent prompt

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

### Issue description
`EnsurePwmNotEnabled` throws an `InvalidOperationException` whose message explicitly says “Call SetPwmEnabled(channel, false) first.” In the MCP server, `GuardAsync` converts exceptions to `McpException(ex.Message)`, so MCP clients will see that message even though their recovery action is the `disable_pwm` tool.

### Issue Context
This PR intentionally blocks DIO writes while PWM is active. The error message should guide callers toward the correct recovery action in *both* SDK and MCP contexts.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[176-190]
- src/Daqifi.Mcp/Tools/DaqifiTools.cs[139-178]

### Suggested fix
Prefer a tool-agnostic message like: “Disable PWM on this channel first (e.g., SetPwmEnabled(channel, false)).”
Optionally, also translate this specific exception in the MCP layer to mention `disable_pwm` explicitly.

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

Results up to commit ccc1855 ⚖️ Balanced

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

1. ~~PWM check outside lock~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

RequirePwmDisabled is executed before device.RunExclusiveAsync, so PWM can be enabled/disabled by
another concurrent tool call between the check and the subsequent SetDioDirection/SetDioValue. This
can cause sporadic failures and can still surface Core’s SDK-oriented InvalidOperationException
(pointing to SetPwmEnabled) despite the MCP-specific guard message.

Code

src/Daqifi.Mcp/DaqifiAgent.cs[R276-279]

        var (device, streaming) = RequireStreaming(deviceId);
        var ch = RequireDigitalChannel(device, channel);
+        RequirePwmDisabled(ch);

Relevance

●●● Strong
Team often accepts fixes closing lock-check races; this is a concrete TOCTOU concurrency bug in MCP
guard.
PR-#381
PR-#248
PR-#411

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new MCP PWM check runs outside the exclusive-operation critical section, but the actual DIO
operations run inside it; RunExclusiveAsync only enforces mutual exclusion within its delegate. Core
independently rejects DIO writes under PWM with an SDK-oriented exception message, so a race can
bypass the intended MCP-specific guidance.
src/Daqifi.Mcp/DaqifiAgent.cs[267-312]
src/Daqifi.Core/Device/DaqifiDevice.cs[817-867]
src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs[183-190]

Agent prompt

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

### Issue description
`RequirePwmDisabled(ch)` is called *before* `device.RunExclusiveAsync(...)`. Because `RunExclusiveAsync` only serializes work inside its delegate, another concurrent tool call can toggle PWM between this pre-check and the actual digital write, leading to:
- false rejections (PWM disabled right after we throw), or
- leaking Core’s `InvalidOperationException` message (which references `SetPwmEnabled`, not MCP’s `disable_pwm`) if PWM becomes enabled after the pre-check.

### Issue Context
This PR added MCP-side validation to provide MCP-actionable guidance, but its current placement does not share the same critical section as the device operation it guards.

### Fix Focus Areas
- src/Daqifi.Mcp/DaqifiAgent.cs[278-281]
- src/Daqifi.Mcp/DaqifiAgent.cs[297-311]
- src/Daqifi.Mcp/DaqifiAgent.cs[656-671]

### Implementation guidance
- Move `RequirePwmDisabled(ch)` into the `RunExclusiveAsync` delegate for both `SetDigitalDirectionAsync` and `SetDigitalOutputAsync`, immediately before calling `streaming.SetDioDirection(...)` / `streaming.SetDioValue(...)`.
- Optionally keep the outer check for faster failure, but **also** re-check inside the exclusive delegate to guarantee consistent behavior under concurrency.
- Keep Core’s guard as defense-in-depth; the MCP check should be the user-facing one in normal races.

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

- Document the new InvalidOperationException precondition on IStreamingDevice.SetDioDirection/SetDioValue (sync + async), so SDK consumers see it in the public contract, not just Core internals. - Add DaqifiAgent.RequirePwmDisabled so the MCP set_digital_direction/ set_digital_output tools fail fast with MCP-actionable guidance ("Call disable_pwm on this channel first") instead of surfacing Core's SDK-oriented message, which points at SetPwmEnabled — a method MCP callers have no tool for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tylerkron · 2026-08-10T14:51:59Z

/agentic_review

qodo-code-review · 2026-08-10T14:56:37Z

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

RequirePwmDisabled ran before device.RunExclusiveAsync, so a concurrent tool call could toggle PWM between the check and the actual SetDioDirection/SetDioValue send, letting Core's SDK-oriented exception (naming SetPwmEnabled, not an MCP tool) leak through instead of the MCP-specific message. Move the check into the same exclusive delegate as the write it guards, so both observe the same state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tylerkron · 2026-08-10T15:02:15Z

/agentic_review

qodo-code-review · 2026-08-10T15:06:34Z

Code review by qodo was updated up to the latest commit 0017f43

- Core's InvalidOperationException from EnsurePwmNotEnabled now also names the MCP disable_pwm tool alongside SetPwmEnabled, so the text is actionable for both SDK and MCP callers even in a path where the MCP-side guard doesn't get there first. - Fix a stray #473 (this PR) reference in a new comment to #449 (the originating issue), matching the file's existing convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tylerkron · 2026-08-10T15:13:15Z

/agentic_review

qodo-code-review · 2026-08-10T15:17:01Z

Code review by qodo was updated up to the latest commit 18ec370

tylerkron requested a review from a team as a code owner August 10, 2026 13:21

qodo-code-review Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs Outdated

Comment thread src/Daqifi.Core/Device/Internal/ChannelControlOperations.cs

qodo-code-review Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread src/Daqifi.Mcp/DaqifiAgent.cs

qodo-code-review Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread src/Daqifi.Mcp/DaqifiAgent.cs

Merge branch 'main' into fix/pwm-active-blocks-dio-writes

114964d

tylerkron added this pull request to the merge queue Aug 10, 2026

Merged via the queue into main with commit 2b9c2ee Aug 10, 2026
1 check passed

tylerkron deleted the fix/pwm-active-blocks-dio-writes branch August 10, 2026 16:49

qodo-code-review Bot mentioned this pull request Aug 12, 2026

fix(sdcard): an SD download now owns the device, so a concurrent command can't corrupt the file #506

Merged

Navigation Menu

Uh oh!

fix(device): block digital writes while PWM is active on the channel - #473

fix(device): block digital writes while PWM is active on the channel#473
tylerkron merged 5 commits into
mainfrom
fix/pwm-active-blocks-dio-writes

tylerkron commented Aug 10, 2026

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

qodo-code-review Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

tylerkron commented Aug 10, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

tylerkron commented Aug 10, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

tylerkron commented Aug 10, 2026

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

tylerkron commented Aug 10, 2026

Summary

Fix

Testing

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Uh oh!

Uh oh!

tylerkron commented Aug 10, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

tylerkron commented Aug 10, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

tylerkron commented Aug 10, 2026

Uh oh!

qodo-code-review Bot commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-code-review Bot commented Aug 10, 2026 •

edited

Loading