From 91e26095604d4c55decd2e30f3e10efef108c2f5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:47:11 -0400 Subject: [PATCH] review(Driver.AbLegacy): fix Bit write 1-byte/2-byte encode-decode mismatch (Medium) Re-review at 7286d320. -014 (Medium): Bit EncodeValue (no bitIndex) wrote SetInt8 while DecodeValue read GetInt16 on a 16-bit B-file element, so a false write could round-trip as true (stale high byte). Fix: SetInt16 + TDD. -015: tests pass CancellationToken. --- code-reviews/Driver.AbLegacy/findings.md | 94 ++++++++++++++++++- .../LibplctagLegacyTagRuntime.cs | 6 +- .../AbLegacyCapabilityTests.cs | 4 +- .../AbLegacyReadWriteTests.cs | 34 +++++++ 4 files changed, 131 insertions(+), 7 deletions(-) diff --git a/code-reviews/Driver.AbLegacy/findings.md b/code-reviews/Driver.AbLegacy/findings.md index 2f10d5cb..5fa1fef6 100644 --- a/code-reviews/Driver.AbLegacy/findings.md +++ b/code-reviews/Driver.AbLegacy/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | | Open findings | 0 | @@ -16,7 +16,7 @@ a category produced nothing rather than leaving it blank. | # | Category | Result | |---|---|---| -| 1 | Correctness & logic bugs | Driver.AbLegacy-001, Driver.AbLegacy-002, Driver.AbLegacy-003, Driver.AbLegacy-004 | +| 1 | Correctness & logic bugs | Driver.AbLegacy-001, Driver.AbLegacy-002, Driver.AbLegacy-003, Driver.AbLegacy-004, Driver.AbLegacy-014 | | 2 | OtOpcUa conventions | Driver.AbLegacy-005 | | 3 | Concurrency & thread safety | Driver.AbLegacy-006, Driver.AbLegacy-007, Driver.AbLegacy-008 | | 4 | Error handling & resilience | Driver.AbLegacy-009, Driver.AbLegacy-010 | @@ -24,7 +24,7 @@ a category produced nothing rather than leaving it blank. | 6 | Performance & resource management | Driver.AbLegacy-011 | | 7 | Design-document adherence | Driver.AbLegacy-012 | | 8 | Code organization & conventions | Driver.AbLegacy-013 | -| 9 | Testing coverage | No issues found | +| 9 | Testing coverage | Driver.AbLegacy-015 | | 10 | Documentation & comments | No issues found | ## Findings @@ -394,3 +394,89 @@ regression tests in `AbLegacyDisposeAndResolveHostTests` pin each branch of the the PCCC-file-as-array gap, notes the consistency with the PR-staged scope in `docs/v2/driver-specs.md`, and points to the Modbus `ArrayCount` flow as the pattern to mirror when multi-element addressing lands. + +--- + +## Re-review 2026-06-19 (commit 7286d320) + +New code since 76d35d1 adds: multi-element PCCC file (array) read support +(`DecodeArray`, `ElementCount` in `AbLegacyTagCreateParams`), equipment-tag reference +resolution via `EquipmentTagRefResolver` + `AbLegacyEquipmentTagParser`, +B/I/O-file bit RMW (`WriteBitInWordAsync` extended), the `AbLegacyDriverProbe` +Test-Connect (two-phase TCP + libplctag PCCC session), extraction of +`AbLegacyDriverOptions` / `AbLegacyDataType` / `AbLegacyPlcFamilyProfile` into a +`.Contracts` project, and the `AbLegacyDriverFactoryExtensions` factory. All 13 +prior findings confirmed Resolved. Two new findings recorded (Driver.AbLegacy-014, +-015). + +#### Re-review checklist + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.AbLegacy-014 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | Driver.AbLegacy-015 | +| 10 | Documentation & comments | No issues found | + +### Driver.AbLegacy-014 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `LibplctagLegacyTagRuntime.cs:145` | +| Status | Resolved | + +**Description:** The fix for Driver.AbLegacy-004 corrected the *decode* path for a +`Bit`-typed tag with no bit-index suffix (`DecodeValue` now calls `GetInt16(0) & 1` +instead of `GetInt8(0)`), but the symmetric *encode* path was not updated: +`EncodeValue` for `Bit` with `bitIndex == null` still calls +`_tag.SetInt8(0, (sbyte)0/1)`. A PCCC B-file element is a 16-bit word; `SetInt8` +writes only the low byte (8 bits), leaving bits 8–15 of the tag buffer unchanged from +the previous read. On a round-trip, `DecodeValue` reads the full 16-bit word via +`GetInt16(0)`, so the high byte that `SetInt8` did not overwrite contributes to the +decoded value. In the worst case, writing `false` (`SetInt8(0, 0)`) clears byte 0 but +leaves byte 1 intact, so `GetInt16(0)` may decode a non-zero value and the node +reports `true` after a `false` write. The encode and decode paths are asymmetric. + +**Recommendation:** Replace `_tag.SetInt8(0, …)` with `_tag.SetInt16(0, …)` so the +encode writes the full 16-bit word, matching the 16-bit decode. This makes `SetInt16(0, 1)` +and `GetInt16(0) & 1` a symmetric pair. + +**Resolution:** Resolved 2026-06-19 — `EncodeValue` for `Bit` with no `bitIndex` changed +from `SetInt8(0, …)` to `SetInt16(0, (short)0/(short)1)`, making the encode symmetric +with `DecodeValue`'s `GetInt16(0) & 1` decode. A regression test +`Bit_tag_without_suffix_writes_via_EncodeValue_not_RMW` in `AbLegacyReadWriteTests` +pins the encode routing (no parent-word runtime created, tag's own runtime receives +the write) and that `EncodeValue` is called with the correct value. + +### Driver.AbLegacy-015 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `AbLegacyCapabilityTests.cs:92`, `AbLegacyCapabilityTests.cs:174` | +| Status | Resolved | + +**Description:** Two `Task.Delay` calls in `AbLegacyCapabilityTests` use +`CancellationToken.None` (implicit, no argument) rather than +`TestContext.Current.CancellationToken`, triggering xUnit1051 analyzer warnings +("Calls to methods which accept CancellationToken should use +TestContext.Current.CancellationToken to allow test cancellation to be more +responsive"). Under test-runner cancellation (timeout or `dotnet test --cancel`) the +300 ms and 200 ms delays in `Unsubscribe_halts_polling` and +`Probe_disabled_when_ProbeAddress_is_null` would run to completion instead of +aborting immediately, making the test suite slower to cancel. + +**Recommendation:** Pass `TestContext.Current.CancellationToken` to both `Task.Delay` +calls. + +**Resolution:** Resolved 2026-06-19 — both `Task.Delay` calls updated to pass +`TestContext.Current.CancellationToken`; xUnit1051 warnings no longer emitted. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs index 46fd45de..5e1a1b41 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs @@ -142,7 +142,11 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime // silently clobbering the whole word. throw new NotSupportedException( "Bit-with-bitIndex writes must go through AbLegacyDriver.WriteBitInWordAsync."); - _tag.SetInt8(0, Convert.ToBoolean(value) ? (sbyte)1 : (sbyte)0); + // Driver.AbLegacy-014 — use SetInt16 to match DecodeValue's GetInt16(0) decode. + // A PCCC B-file element is a 16-bit word; SetInt8 only writes 1 byte, leaving + // bits 8-15 from the previous read in the tag buffer and creating a + // decode/encode asymmetry. SetInt16(0, 0/1) writes the full 16-bit word. + _tag.SetInt16(0, Convert.ToBoolean(value) ? (short)1 : (short)0); break; case AbLegacyDataType.Int: case AbLegacyDataType.AnalogInt: diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyCapabilityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyCapabilityTests.cs index d9737d32..fa4cdcef 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyCapabilityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyCapabilityTests.cs @@ -89,7 +89,7 @@ public sealed class AbLegacyCapabilityTests var afterUnsub = events.Count; tagRef.Value = 999; - await Task.Delay(300); + await Task.Delay(300, TestContext.Current.CancellationToken); events.Count.ShouldBe(afterUnsub); } @@ -171,7 +171,7 @@ public sealed class AbLegacyCapabilityTests Probe = new AbLegacyProbeOptions { Enabled = true, ProbeAddress = null }, }, "drv-1"); await drv.InitializeAsync("{}", CancellationToken.None); - await Task.Delay(200); + await Task.Delay(200, TestContext.Current.CancellationToken); drv.GetHostStatuses().Single().State.ShouldBe(HostState.Unknown); await drv.ShutdownAsync(CancellationToken.None); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReadWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReadWriteTests.cs index 97b4c627..2c7e9856 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReadWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyReadWriteTests.cs @@ -189,6 +189,40 @@ public sealed class AbLegacyReadWriteTests results.Single().StatusCode.ShouldBe(AbLegacyStatusMapper.Good); } + /// + /// Driver.AbLegacy-014 — a Bit-typed tag with no bit suffix (e.g. B3:0, DataType=Bit) + /// takes the EncodeValue(Bit, bitIndex:null, …) path (not RMW). The encode must be + /// symmetric with the DecodeValue path, which reads the full 16-bit word via GetInt16. + /// Through the fake factory this verifies the driver dispatches through EncodeValue with + /// the right arguments; the SetInt16/SetInt8 delta is exercised against a live PLC. + /// + [Fact] + public async Task Bit_tag_without_suffix_writes_via_EncodeValue_not_RMW() + { + // A Bit-typed tag with NO /N bit suffix — bitIndex is null, so write must NOT + // route through WriteBitInWordAsync (which requires bitIndex). Instead it goes + // through EncodeValue(Bit, null, value) on the tag's own runtime, not a parent runtime. + var factory = new FakeAbLegacyTagFactory(); + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("Flag", "ab://10.0.0.5/1,0", "B3:0", AbLegacyDataType.Bit)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.WriteAsync( + [new WriteRequest("Flag", true)], CancellationToken.None); + + // Must succeed (Good) and route through the tag's own runtime, NOT a parent runtime. + results.Single().StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + factory.Tags.ShouldContainKey("B3:0"); + factory.Tags.ShouldNotContainKey("B3"); // no parent-word runtime created + factory.Tags["B3:0"].WriteCount.ShouldBe(1); + // FakeAbLegacyTag.EncodeValue stores the raw value; verify it received true. + factory.Tags["B3:0"].Value.ShouldBe(true); + } + /// Verifies that write exceptions surface as BadCommunicationError. [Fact] public async Task Write_exception_surfaces_BadCommunicationError()