review(Driver.AbCip): fix declared UDT array members read as scalar (Medium)
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.
This commit is contained in:
Joseph Doherty
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -156,6 +156,37 @@ public sealed class AbCipArrayTests
|
||||
snapshots.Single().Value.ShouldNotBeOfType<int[]>();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Driver.AbCip-016 — a DECLARED UDT member that is a 1-D array (<c>Setpoints : REAL[4]</c>)
|
||||
/// must READ as a typed CLR array, matching the array node it discovers as. Before the fix
|
||||
/// the member fan-out in <c>InitializeAsync</c> dropped the member's <c>ElementCount</c> /
|
||||
/// <c>IsArray</c>, 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.
|
||||
/// </summary>
|
||||
[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<float[]>();
|
||||
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 ----
|
||||
|
||||
/// <summary>The equipment-tag resolver threads arrayLength into the def's ElementCount.</summary>
|
||||
|
||||
@@ -74,4 +74,29 @@ public sealed class AbCipUdtMemberLayoutTests
|
||||
{
|
||||
AbCipUdtMemberLayout.TryBuild(Array.Empty<AbCipStructureMember>()).ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// (<c>DecodeValueAt</c>) 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.
|
||||
/// </summary>
|
||||
[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();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user