From a914b73d57961c6adcb46840816a596572503011 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:34:34 -0400 Subject: [PATCH] review(Driver.AbCip): fix declared UDT array members read as scalar (Medium) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review at 7286d320. AbCip-016 (Medium): two cooperating defects made a declared array member (e.g. REAL[4]) read one scalar/null — fan-out dropped ElementCount/IsArray, and UdtMemberLayout.TryBuild ignored array members (mis-placing later members). Fix: thread array shape through fan-out + opt whole-UDT grouping out when any member is an array + TDD. AbCip-017 (severity-read StatusCode, Low) deferred. --- code-reviews/Driver.AbCip/findings.md | 112 +++++++++++++++++- .../AbCipDriver.cs | 9 +- .../AbCipUdtMemberLayout.cs | 8 ++ .../AbCipArrayTests.cs | 31 +++++ .../AbCipUdtMemberLayoutTests.cs | 25 ++++ 5 files changed, 181 insertions(+), 4 deletions(-) diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index ba5f8407..03f1c0f1 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -4,10 +4,10 @@ |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | -| Open findings | 0 | +| Open findings | 1 | ## Checklist coverage @@ -250,3 +250,109 @@ **Recommendation:** Sweep the module for PR-N forward references and "lands in PR X" notes that have been delivered; update them to describe present behavior. Where a comment marks genuinely unfinished work (e.g. `PlcTagHandle.ReleaseHandle`), convert it to a tracked TODO with an issue reference rather than a PR-number milestone. **Resolution:** Resolved 2026-05-23 — swept the module for stale PR-N forward references and replaced each with a description of present behaviour: `AbCipDriver.TemplateCache` summary, `AbCipDataType.cs` (PR 5 / PR 6 → references `CipTemplateObjectDecoder` + `AbCipTemplateCache`), `AbCipTagPath.cs` (PR 6 → references `AbCipTemplateCache`), `AbCipTemplateCache.cs` (the "lands with PR 6" remarks and the `AbCipUdtShape` summary), `IAbCipTagEnumerator.cs` (the `EmptyAbCipTagEnumeratorFactory`-defaults claim and the PR-5 stub line; `EmptyAbCipTagEnumerator` summary), `LibplctagTagEnumerator.cs` ("Task #178 closed the stub gap from PR 5"), `LibplctagTagRuntime.cs` (`Whole-UDT writes land in PR 6`), `AbCipDriverOptions.cs` (`Tags` summary, `ProbeTagPath` summary), and `AbCipPlcFamilyProfile.cs` ("Family-specific wire tests ship in PRs 9–12"). `PlcTagHandle.cs` was already deleted as part of Driver.AbCip-006's resolution. The only remaining "lands in" reference is the `AbCipDataType.Dt` ⇒ `Date/Time` mapping, which is product-domain wording, not a PR reference. + +## Re-review 2026-06-19 (commit 7286d320) + +Re-review at `7286d320` (the module is byte-identical at the working-tree HEAD `ae3f0719`). +Since the prior review at `76d35d1` the module grew substantially: the option/contract +records (`AbCipDriverOptions`, `AbCipDeviceOptions`, `AbCipTagDefinition`, +`AbCipStructureMember`, the `AbCipDataType` enum) moved out into a sibling +`Driver.AbCip.Contracts` project (**out of scope** for this module's review); a new +two-phase `AbCipDriverProbe` (`IDriverProbe`) and `AbCipDataTypeExtensions` landed; the +driver gained the `EquipmentTagRefResolver`, 1-D array support, controller-discovered +nested-UDT fan-out (Template Object + Symbol Object decoders), and string-enum +serialization in the probe (correctly using `JsonStringEnumConverter`, addressing the +systemic AdminUI enum-serialization hazard). All 15 prior findings remain Resolved and +were spot-checked as still in force. + +#### Checklist coverage (re-review) + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.AbCip-016 | +| 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 | No issues found (covered by Driver.AbCip-016 regression tests) | +| 10 | Documentation & comments | Driver.AbCip-017 | + +### Driver.AbCip-016 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `AbCipDriver.cs:276-282` (member fan-out), `AbCipUdtMemberLayout.cs:42-52` (declaration-only layout) | +| Status | Resolved | + +**Description:** A declared UDT member that is a 1-D array read its value as a single scalar +rather than as a typed array — a declared-type-vs-runtime-value mismatch in the same family +as Driver.AbCip-004. Two cooperating defects: + +1. **Member fan-out dropped the array shape.** `InitializeAsync` fans each + `AbCipStructureMember` out into a member-level `AbCipTagDefinition` + (`{tag}.{member}`) but constructed it without `ElementCount` / `IsArray`, so they + defaulted to `1` / `false`. Discovery (`DiscoverAsync`, line ~1033) DOES emit an array + node for an array member (`member.IsArray || member.ElementCount > 1` → `IsArray` + + `ArrayDim`). At read time the fanned-out runtime def therefore went through + `ReadSingleAsync` with `IsArrayTag(def) == false` and decoded a single scalar via + `DecodeValue`, not the `DecodeArray` path. The address space advertised, e.g., + `Motor.Setpoints : REAL[4]` as an array node, but a read returned one element (or `null`). + There was an existing discovery test (`Udt_array_member_discovers_as_IsArray_with_ArrayDim`) + but no test that *read* a declared UDT array member, so the mismatch went uncaught. + +2. **Declaration-only whole-UDT grouping mis-handled array members.** + `AbCipUdtMemberLayout.TryBuild` ignored each member's `ElementCount`, advancing the + layout cursor by the *scalar* element size only. A UDT with an array member would place + every *subsequent* member at a wrong offset, and `ReadGroupAsync` decodes one scalar per + member via `DecodeValueAt` (it cannot return an array). This only bites the opt-in + `EnableDeclarationOnlyUdtGrouping` path (default off), but it compounds the Driver.AbCip-003 + declaration-order risk. + +**Recommendation:** Thread `ElementCount` / `IsArray` from the `AbCipStructureMember` into the +fanned-out member `AbCipTagDefinition` so the read path takes the array branch (matching +discovery). Make `AbCipUdtMemberLayout.TryBuild` opt out of grouping (return `null`) when any +member is an array, sending array members to the per-tag read path that decodes them correctly +— mirroring the existing Bool/String/Structure opt-out. + +**Resolution:** Resolved 2026-06-19 — (1) the member fan-out in `AbCipDriver.InitializeAsync` +now passes `ElementCount: member.ElementCount` + `IsArray: member.IsArray` into the fanned-out +`AbCipTagDefinition`, so `IsArrayTag(def)` matches the discovery array-node decision exactly and +a declared UDT array member reads as a typed CLR array. (2) `AbCipUdtMemberLayout.TryBuild` now +returns `null` when any member has `IsArray` set or `ElementCount > 1`, opting the whole group +out of declaration-only grouping so array members always take the per-tag read path. Regression +tests: `AbCipArrayTests.Declared_udt_array_member_reads_as_typed_array` (a declared `REAL[4]` +member reads as `float[4]` + threads `elem_count`/`isArray` to libplctag) and +`AbCipUdtMemberLayoutTests.Returns_Null_When_A_Member_Is_An_Array` (both the explicit `IsArray` +flag and the legacy `ElementCount > 1` paths opt out). Full suite green (303 tests). + +### Driver.AbCip-017 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `AbCipAlarmProjection.cs:173-185` (`Tick`) | +| Status | Open | + +**Description:** `AbCipAlarmProjection.Tick` gates each node on the `InFaulted` snapshot's +`StatusCode` (`if (inFaultedDv.StatusCode != Good) continue;`) but reads the `Severity` +snapshot's *value* without checking *its* `StatusCode`. When a severity read returns Bad +(value `null`), `ToInt(null)` yields `0`, which `MapSeverity` buckets as `Low` — so a raise +event during a partial-read tick can be emitted with an artificially-`Low` severity rather +than the alarm's real severity (or a deliberately-chosen "unknown" sentinel). In practice +`InFaulted` and `Severity` are members of the same ALMD UDT read in one batch, so a Good +`InFaulted` almost always implies a Good `Severity`, which is why the impact is Low and this is +recorded as a documentation/robustness note rather than a behavioural bug. The current +"default-to-Low-on-unknown-severity" behaviour is also undocumented. + +**Recommendation:** Either check `severityDv.StatusCode` and carry a documented fallback (e.g. +retain the last-known severity, or map to `Medium`/an explicit "unknown" bucket) when the +severity read is Bad, or add an XML/inline comment on `Tick` stating that severity defaults to +`Low` when its read fails. Deferred (Open): the right fallback is a small alarm-semantics design +decision (what severity to surface when it is genuinely unknown) rather than a mechanical fix, +and the impact is negligible given the single-batch read shape. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 9ffd8b19..1e76a100 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -279,7 +279,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, TagPath: $"{tag.TagPath}.{member.Name}", DataType: member.DataType, Writable: member.Writable, - WriteIdempotent: member.WriteIdempotent); + WriteIdempotent: member.WriteIdempotent, + // Driver.AbCip-016 — carry the member's array shape into the fanned-out + // runtime definition. Discovery already emits an array node for an array + // member (member.IsArray || member.ElementCount > 1); without these the + // runtime def defaulted to scalar and the read returned a single element + // instead of the typed array, a declared-type-vs-runtime-value mismatch. + ElementCount: member.ElementCount, + IsArray: member.IsArray); // Member fan-out duplicate check: a member-path collision means two // configured structure tags produce the same member path, or a member // name collides with an independently-declared tag. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs index 543d96ba..daeb5d70 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs @@ -44,6 +44,14 @@ public static class AbCipUdtMemberLayout if (!TryGetSizeAlign(member.DataType, out var size, out var align)) return null; + // Driver.AbCip-016 — an array member can't be placed by declaration-only layout: the + // whole-UDT grouped read decodes one scalar per member at its offset and can't return + // an array, and advancing the cursor by the scalar size (not size * count) would + // mis-place every member after it. Opt the whole group out so array members fall back + // to the per-tag read path, which reads them as typed arrays. + if (member.IsArray || member.ElementCount > 1) + return null; + if (cursor % align != 0) cursor += align - (cursor % align); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs index f8047cd9..1df0b24f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs @@ -156,6 +156,37 @@ public sealed class AbCipArrayTests snapshots.Single().Value.ShouldNotBeOfType(); } + /// + /// Driver.AbCip-016 — a DECLARED UDT member that is a 1-D array (Setpoints : REAL[4]) + /// must READ as a typed CLR array, matching the array node it discovers as. Before the fix + /// the member fan-out in InitializeAsync dropped the member's ElementCount / + /// IsArray, so the fanned-out runtime definition defaulted to scalar and the read + /// returned a single element (or null) instead of the array — a declared-type-vs-runtime-value + /// mismatch. + /// + [Fact] + public async Task Declared_udt_array_member_reads_as_typed_array() + { + var (drv, factory) = NewDriver( + new AbCipTagDefinition("Motor", "ab://10.0.0.5/1,0", "Motor", AbCipDataType.Structure, + Members: + [ + new AbCipStructureMember("Setpoints", AbCipDataType.Real, ElementCount: 4), + new AbCipStructureMember("Speed", AbCipDataType.DInt), + ])); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = p => new ArrayFakeAbCipTag(p, new float[] { 1.5f, 2.5f, 3.5f, 4.5f }); + + var snapshots = await drv.ReadAsync(["Motor.Setpoints"], CancellationToken.None); + + snapshots.Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + var value = snapshots.Single().Value.ShouldBeOfType(); + value.ShouldBe([1.5f, 2.5f, 3.5f, 4.5f]); + // The fanned-out member runtime must thread the member's element count to libplctag. + factory.Tags["Motor.Setpoints"].CreationParams.ElementCount.ShouldBe(4); + factory.Tags["Motor.Setpoints"].CreationParams.IsArray.ShouldBeTrue(); + } + // ---- Resolver: arrayLength threading ---- /// The equipment-tag resolver threads arrayLength into the def's ElementCount. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs index df0a730f..b895b2cc 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs @@ -74,4 +74,29 @@ public sealed class AbCipUdtMemberLayoutTests { AbCipUdtMemberLayout.TryBuild(Array.Empty()).ShouldBeNull(); } + + /// + /// Driver.AbCip-016 — declaration-only layout returns null when any member is a 1-D array. + /// The whole-UDT grouped read path decodes one scalar per member at its offset + /// (DecodeValueAt) and cannot return an array, and the scalar-size cursor advance + /// would mis-place every member after the array. Opting the whole group out sends array + /// members through the per-tag read path, which reads them as typed arrays. + /// + [Fact] + public void Returns_Null_When_A_Member_Is_An_Array() + { + // Explicit IsArray flag (even a 1-element array). + AbCipUdtMemberLayout.TryBuild(new[] + { + new AbCipStructureMember("A", AbCipDataType.DInt), + new AbCipStructureMember("Buf", AbCipDataType.Real, IsArray: true, ElementCount: 1), + }).ShouldBeNull(); + + // Legacy ElementCount > 1 with the flag unset. + AbCipUdtMemberLayout.TryBuild(new[] + { + new AbCipStructureMember("A", AbCipDataType.DInt), + new AbCipStructureMember("Setpoints", AbCipDataType.Real, ElementCount: 4), + }).ShouldBeNull(); + } }