diff --git a/code-reviews/Driver.TwinCAT/findings.md b/code-reviews/Driver.TwinCAT/findings.md index af595f18..70f20ff2 100644 --- a/code-reviews/Driver.TwinCAT/findings.md +++ b/code-reviews/Driver.TwinCAT/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `04e0877b` | | Status | Reviewed | | Open findings | 0 | @@ -27,6 +27,25 @@ a category produced nothing rather than leaving it blank. | 9 | Testing coverage | Driver.TwinCAT-016 | | 10 | Documentation & comments | Driver.TwinCAT-004 (data-type comment), Driver.TwinCAT-014 | +## Re-review 2026-06-19 (commit 04e0877b) + +All 16 prior findings remain Resolved. One new finding (Driver.TwinCAT-017) identified and fixed in this session. + +#### Re-review checklist + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.TwinCAT-017 | +| 2 | OtOpcUa conventions | No new issues | +| 3 | Concurrency & thread safety | No new issues | +| 4 | Error handling & resilience | No new issues | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No new issues | +| 7 | Design-document adherence | No new issues | +| 8 | Code organization & conventions | No new issues | +| 9 | Testing coverage | No new issues (2 regression tests added for -017) | +| 10 | Documentation & comments | No new issues | + ## Findings ### Driver.TwinCAT-001 @@ -424,3 +443,36 @@ addressed, especially a concurrency stress test for `EnsureConnectedAsync` and a `ReinitializeAsync`-applies-new-config test. **Resolution:** Resolved 2026-05-23 — the previously-closed High findings each grew their regression coverage as they were resolved (see `TwinCATHighFindingsRegressionTests`: `ReinitializeAsync_applies_changed_device_config` for -001, `LInt_read_round_trips_value_above_int_MaxValue` + `DataType_mapping_preserves_width_and_signedness` for -002, `Concurrent_reads_on_one_device_create_a_single_client` + `Concurrent_reads_and_writes_share_one_client` for -007, `Symbol_version_changed_raises_OnRediscoveryNeeded` + `TwinCATDriver_implements_IRediscoverable` for -013). This pass added the two remaining gaps: `Structure_typed_pre_declared_tag_is_rejected_at_config_parse` (-003) and `Probe_loop_and_read_share_one_client_per_device` (-009 disposal-race coverage races 64 readers against the probe loop for 500ms and asserts a single client / single connect). All coverage lives in the test files `TwinCATHighFindingsRegressionTests.cs` and the new `TwinCATLowFindingsRegressionTests.cs`. + +### Driver.TwinCAT-017 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `TwinCATDriverFactoryExtensions.cs:168-188` (`TwinCATTagDto`) | +| Status | Resolved | + +**Description:** `TwinCATTagDto` (the JSON-deserialization DTO for pre-declared tags in the +driver config) lacked an `ArrayLength` field. `BuildTag` therefore always constructed +`TwinCATTagDefinition` with `ArrayLength: null`, making it impossible to declare a 1-D array +pre-declared tag via the JSON driver config. The Phase 4c array-read path (`ReadValueAsync` +with `arrayCount != null`) and the array-discovery path (`IsArray` / `ArrayDim` in +`DiscoverAsync`) were both silently unavailable to operators who author pre-declared tags in +the driver config JSON — only equipment-tag JSON blobs (`TwinCATEquipmentTagParser`) and the +in-memory `TwinCATTagDefinition` constructor (used in tests) supported arrays. + +This is a correctness gap introduced alongside Phase 4c (the two authoring surfaces were +inconsistent) but not caught because the Phase 4c tests use the in-memory constructor directly +rather than round-tripping through the JSON parser. + +**Recommendation:** Add `int? ArrayLength` to `TwinCATTagDto` and thread it through `BuildTag` +(only positive values are honoured; null / non-positive keeps the scalar default, mirroring the +equipment-tag parser's `ReadArrayLength` guard). + +**Resolution:** Resolved 2026-06-19 — added `int? ArrayLength` to `TwinCATTagDto`; `BuildTag` +now passes `ArrayLength: t.ArrayLength is > 0 ? t.ArrayLength : null` to the +`TwinCATTagDefinition` constructor. Two regression tests in `TwinCATArraySupportTests`: +`Predeclared_array_tag_arrayLength_parses_from_driver_config_json` (parse) and +`Predeclared_array_tag_from_json_reports_IsArray_in_discovery` (end-to-end discovery). All 170 +unit tests pass. (SHA blank — fix committed in this review session.) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs index 68dbe38b..53c9ef48 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs @@ -109,7 +109,11 @@ public static class TwinCATDriverFactoryExtensions $"TwinCAT tag '{t.Name}' in '{driverInstanceId}' missing SymbolPath"), DataType: dataType, Writable: t.Writable ?? true, - WriteIdempotent: t.WriteIdempotent ?? false); + WriteIdempotent: t.WriteIdempotent ?? false, + // Driver.TwinCAT-017: thread arrayLength through so JSON-authored pre-declared tags can + // declare 1-D arrays (Phase 4c). Only positive values are honoured; null / non-positive + // keeps the scalar default (ArrayLength: null on TwinCATTagDefinition). + ArrayLength: t.ArrayLength is > 0 ? t.ArrayLength : null); } private static T ParseEnum(string? raw, string? tagName, string driverInstanceId, string field) @@ -184,6 +188,14 @@ public static class TwinCATDriverFactoryExtensions /// Gets or sets a value indicating whether writes are idempotent. public bool? WriteIdempotent { get; init; } + + /// + /// Optional 1-D array element count. When positive, the tag is a 1-D array of this + /// many elements — drives IsArray/ArrayDim at + /// discovery and a native ADS array read at runtime (Phase 4c, Driver.TwinCAT-017). + /// null or non-positive = scalar (the default). + /// + public int? ArrayLength { get; init; } } internal sealed class TwinCATProbeDto diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArraySupportTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArraySupportTests.cs index fd5ec364..bb63b7ef 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArraySupportTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArraySupportTests.cs @@ -186,6 +186,69 @@ public sealed class TwinCATArraySupportTests def!.ArrayLength.ShouldBeNull(); } + // ---- (4) Driver.TwinCAT-017 — pre-declared array tag parseable via driver config JSON ---- + + /// + /// A pre-declared tag in the driver config JSON with arrayLength must produce a + /// with the correct , + /// which in turn drives IsArray/ArrayDim at discovery and a native array read + /// at runtime (Driver.TwinCAT-017). + /// + [Fact] + public void Predeclared_array_tag_arrayLength_parses_from_driver_config_json() + { + var json = System.Text.Json.JsonSerializer.Serialize(new + { + devices = new[] { new { hostAddress = Host } }, + tags = new[] + { + new + { + name = "Speeds", + deviceHostAddress = Host, + symbolPath = "MAIN.Speeds", + dataType = "DInt", + arrayLength = 8, + }, + }, + }); + var parsed = TwinCATDriverFactoryExtensions.ParseOptionsForTests(json, "drv-1"); + parsed.Tags.Single().ArrayLength.ShouldBe(8); + } + + /// + /// A pre-declared array tag parsed from JSON must surface as IsArray=true / + /// ArrayDim=8 in the discovery address space. + /// + [Fact] + public async Task Predeclared_array_tag_from_json_reports_IsArray_in_discovery() + { + var configJson = System.Text.Json.JsonSerializer.Serialize(new + { + devices = new[] { new { hostAddress = Host } }, + tags = new[] + { + new + { + name = "Speeds", + deviceHostAddress = Host, + symbolPath = "MAIN.Speeds", + dataType = "DInt", + arrayLength = 8, + }, + }, + }); + var builder = new RecordingBuilder(); + var drv = new TwinCATDriver(new TwinCATDriverOptions(), "drv-1", new FakeTwinCATClientFactory()); + await drv.InitializeAsync(configJson, CancellationToken.None); + + await drv.DiscoverAsync(builder, CancellationToken.None); + + var v = builder.Variables.Single(x => x.BrowseName == "Speeds").Info; + v.IsArray.ShouldBeTrue(); + v.ArrayDim.ShouldBe(8u); + } + // ---- helpers ---- private sealed class RecordingBuilder : IAddressSpaceBuilder