diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index 03f1c0f1..350d585c 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -269,7 +269,7 @@ were spot-checked as still in force. | # | Category | Result | |---|---|---| -| 1 | Correctness & logic bugs | Driver.AbCip-016 | +| 1 | Correctness & logic bugs | Driver.AbCip-016, Driver.AbCip-018 | | 2 | OtOpcUa conventions | No issues found | | 3 | Concurrency & thread safety | No issues found | | 4 | Error handling & resilience | No issues found | @@ -356,3 +356,39 @@ severity read is Bad, or add an XML/inline comment on `Tick` stating that severi `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. + +### Driver.AbCip-018 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `AbCipDriverFactoryExtensions.cs:89-109` (`BuildTag`), `AbCipDriverFactoryExtensions.cs:205-246` (`AbCipTagDto`), `AbCipDriverFactoryExtensions.cs:248-269` (`AbCipMemberDto`) | +| Status | Resolved | + +**Description:** The driver-config factory path dropped the array shape of a pre-declared tag. +`AbCipTagDto` and `AbCipMemberDto` had no `ElementCount` / `IsArray` fields, so a `tags[]` entry +in the driver-config JSON authored with an explicit `"isArray": true, "elementCount": 4` +deserialised into a **scalar** `AbCipTagDefinition` (both fields silently dropped → defaulting to +`IsArray=false` / `ElementCount=1`). At read time `IsArrayTag(def)` was therefore `false`, so the +tag decoded via `DecodeValue` (single scalar) instead of the `DecodeArray` path — a declared +`REAL[4]` array tag read as one element. This is the factory/driver-config path; the +equipment-tag path (`AbCipEquipmentTagParser` in Driver.AbCip.Contracts) already threads array +shape correctly, and the in-driver UDT member fan-out was fixed under Driver.AbCip-016 — but the +factory `tags[]` array still lost the signal. + +#### Recommendation + +Add `ElementCount` (int?) and `IsArray` (bool) to `AbCipTagDto` and `AbCipMemberDto` with the +correct camelCase `[JsonPropertyName]` (`elementCount` / `isArray`), and thread them into the +`AbCipTagDefinition` / `AbCipStructureMember` construction in `BuildTag`, applying the +positive-value guard (`elementCount is > 0` ⇒ value, else `1`). Additive/optional — a config +without the fields keeps the legacy scalar default. + +**Resolution:** Resolved 2026-06-19 — `AbCipTagDto` + `AbCipMemberDto` gained `IsArray` (bool, +`[JsonPropertyName("isArray")]`) and `ElementCount` (int?, `[JsonPropertyName("elementCount")]`); +`BuildTag` threads them into the `AbCipTagDefinition` and each fanned-out `AbCipStructureMember` +with the `m.ElementCount is > 0 ? m.ElementCount.Value : 1` guard (mirroring the equipment-tag +parser), so a driver-config array tag now reads as a typed array. Additive — missing fields still +produce a scalar. Regression: `AbCipFactoryArrayTagTests` (4 cases — top-level tag, UDT member, +absent-fields-stay-scalar, non-positive-elementCount-clamps-to-1). Full suite green (307 tests). diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs index 3ec8d9eb..3eaad920 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverFactoryExtensions.cs @@ -104,9 +104,13 @@ public static class AbCipDriverFactoryExtensions DataType: ParseEnum(m.DataType, t.Name, driverInstanceId, $"Members[{m.Name}].DataType"), Writable: m.Writable ?? true, - WriteIdempotent: m.WriteIdempotent ?? false))] + WriteIdempotent: m.WriteIdempotent ?? false, + ElementCount: m.ElementCount is > 0 ? m.ElementCount.Value : 1, + IsArray: m.IsArray))] : null, - SafetyTag: t.SafetyTag ?? false); + SafetyTag: t.SafetyTag ?? false, + ElementCount: t.ElementCount is > 0 ? t.ElementCount.Value : 1, + IsArray: t.IsArray); private static T ParseEnum(string? raw, string? tagName, string driverInstanceId, string field, T? fallback = null) where T : struct, Enum @@ -243,6 +247,20 @@ public static class AbCipDriverFactoryExtensions /// Gets or sets whether this is a safety tag. /// public bool? SafetyTag { get; init; } + + /// + /// Gets or sets the explicit 1-D array signal — true ⟺ the tag is an array (even a + /// 1-element one). Mirrors AbCipTagDefinition.IsArray; optional (absent ⇒ scalar). + /// + [JsonPropertyName("isArray")] + public bool IsArray { get; init; } + + /// + /// Gets or sets the number of elements for a 1-D array tag; 1 (or absent) for a scalar. + /// Mirrors AbCipTagDefinition.ElementCount; only positive values are honoured. + /// + [JsonPropertyName("elementCount")] + public int? ElementCount { get; init; } } internal sealed class AbCipMemberDto @@ -266,6 +284,20 @@ public static class AbCipDriverFactoryExtensions /// Gets or sets whether write is idempotent. /// public bool? WriteIdempotent { get; init; } + + /// + /// Gets or sets the explicit 1-D array signal for the member — true ⟺ the member is + /// an array (even a 1-element one). Mirrors AbCipStructureMember.IsArray; optional. + /// + [JsonPropertyName("isArray")] + public bool IsArray { get; init; } + + /// + /// Gets or sets the number of elements for a 1-D array member; 1 (or absent) for a + /// scalar. Mirrors AbCipStructureMember.ElementCount; only positive values are honoured. + /// + [JsonPropertyName("elementCount")] + public int? ElementCount { get; init; } } internal sealed class AbCipProbeDto diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFactoryArrayTagTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFactoryArrayTagTests.cs new file mode 100644 index 00000000..92f0ca7a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFactoryArrayTagTests.cs @@ -0,0 +1,119 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Regression coverage for Driver.AbCip-018 — the driver-config factory path +/// () dropped the array shape of a +/// pre-declared tag. A tags[] entry with "isArray": true, "elementCount": 4 +/// deserialised into a scalar (because AbCipTagDto / +/// AbCipMemberDto had no ElementCount / IsArray fields), so the tag read +/// as a single scalar despite the explicit array declaration. These tests assert the array +/// shape now survives factory deserialization for both top-level tags and UDT members, and +/// that omitting the fields keeps the legacy scalar default (additive change — no break). +/// +[Trait("Category", "Unit")] +public sealed class AbCipFactoryArrayTagTests +{ + /// A driver-config tag declaring isArray/elementCount produces an array definition. + [Fact] + public void ParseOptions_threads_isArray_and_elementCount_into_tag_definition() + { + const string json = """ + { + "Tags": [ { + "Name": "Setpoints", + "DeviceHostAddress": "ab://10.0.0.5/1,0", + "TagPath": "Setpoints", + "DataType": "Real", + "isArray": true, + "elementCount": 4 + } ] + } + """; + + var opts = AbCipDriverFactoryExtensions.ParseOptions("drv-1", json); + + var tag = opts.Tags.Single(); + tag.IsArray.ShouldBeTrue(); + tag.ElementCount.ShouldBe(4); + } + + /// A driver-config UDT member declaring isArray/elementCount produces an array member. + [Fact] + public void ParseOptions_threads_isArray_and_elementCount_into_structure_member() + { + const string json = """ + { + "Tags": [ { + "Name": "Motor", + "DeviceHostAddress": "ab://10.0.0.5/1,0", + "TagPath": "Motor", + "DataType": "Structure", + "Members": [ { + "Name": "Setpoints", + "DataType": "Real", + "isArray": true, + "elementCount": 4 + } ] + } ] + } + """; + + var opts = AbCipDriverFactoryExtensions.ParseOptions("drv-1", json); + + var member = opts.Tags.Single().Members!.Single(); + member.IsArray.ShouldBeTrue(); + member.ElementCount.ShouldBe(4); + } + + /// A driver-config tag without array fields stays scalar (additive change — no break). + [Fact] + public void ParseOptions_defaults_to_scalar_when_array_fields_absent() + { + const string json = """ + { + "Tags": [ { + "Name": "Speed", + "DeviceHostAddress": "ab://10.0.0.5/1,0", + "TagPath": "Speed", + "DataType": "DInt" + } ] + } + """; + + var opts = AbCipDriverFactoryExtensions.ParseOptions("drv-1", json); + + var tag = opts.Tags.Single(); + tag.IsArray.ShouldBeFalse(); + tag.ElementCount.ShouldBe(1); + } + + /// A non-positive elementCount falls back to the scalar count of 1 (positive-value guard). + [Fact] + public void ParseOptions_guards_against_non_positive_elementCount() + { + const string json = """ + { + "Tags": [ { + "Name": "Speed", + "DeviceHostAddress": "ab://10.0.0.5/1,0", + "TagPath": "Speed", + "DataType": "DInt", + "isArray": true, + "elementCount": 0 + } ] + } + """; + + var opts = AbCipDriverFactoryExtensions.ParseOptions("drv-1", json); + + var tag = opts.Tags.Single(); + // elementCount <= 0 is degenerate — count clamps to 1; the explicit IsArray flag still + // carries (a 1-element array is valid), mirroring the equipment-tag parser's contract. + tag.ElementCount.ShouldBe(1); + tag.IsArray.ShouldBeTrue(); + } +}