From 17432bb1a4a735baf0b29da96740a480b4c02475 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:30:54 -0400 Subject: [PATCH] fix(driver-abcip): correct Driver.AbCip-005 approach and fix 014 tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 005 revised approach: keep the parent Structure tag in `_tagsByName` so the whole-UDT grouping planner can find it (required for Driver.AbCip-003 opt-in path + alarm projection). Instead, detect a direct read of a Structure-with-Members in `ReadSingleAsync` and return `BadNotSupported` rather than Good/null — explicitly documenting the contract that callers must address member paths. Duplicate-key checks (scalar and member fan-out) remain. Finding 014 test corrections: `Structure_parent_tag_read_returns_BadNotSupported` now asserts the new contract. `Read_UDInt_tag_returns_uint_value_not_negative_wrapped_int` assertion fixed to use `ShouldBeOfType()` instead of `ShouldNotBe(-1)` (Shouldly overflows comparing uint.MaxValue with int). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../AbCipDriver.cs | 45 ++++++++++--------- .../AbCipDriverCodeReviewRegressionTests.cs | 18 ++++---- 2 files changed, 34 insertions(+), 29 deletions(-) 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 764ed56..b25ed40 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -146,10 +146,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } foreach (var tag in _options.Tags) { - // Driver.AbCip-005: Structure tags whose Members are declared fan out to per-member - // entries only. The bare parent name is deliberately NOT registered as a readable - // tag — reading it would return Good/null (DecodeValue for Structure returns null), - // which is misleading. Clients read the individual member paths instead. + // Duplicate-key check: a collision means two configured tags have the same name. + // Fail fast at init time with a diagnostic rather than silently clobbering. + // (Driver.AbCip-005) + if (_tagsByName.TryGetValue(tag.Name, out var existingTag)) + throw new InvalidOperationException( + $"AbCip tag name collision: '{tag.Name}' is declared more than once. " + + $"Existing entry DeviceHostAddress='{existingTag.DeviceHostAddress}', " + + $"TagPath='{existingTag.TagPath}'. Rename or remove the duplicate."); + _tagsByName[tag.Name] = tag; + if (tag.DataType == AbCipDataType.Structure && tag.Members is { Count: > 0 }) { foreach (var member in tag.Members) @@ -161,29 +167,17 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, DataType: member.DataType, Writable: member.Writable, WriteIdempotent: member.WriteIdempotent); - // Duplicate-key check: a collision means two configured tags produce the - // same readable path (e.g. two structure parents with the same member name, - // or a member name that matches an independently-declared tag). Fail fast - // at init time with a diagnostic rather than silently clobbering. - if (_tagsByName.TryGetValue(memberTag.Name, out var existing)) + // 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. + if (_tagsByName.TryGetValue(memberTag.Name, out var existingMember)) throw new InvalidOperationException( $"AbCip tag name collision: '{memberTag.Name}' is produced by both " + $"'{tag.Name}.{member.Name}' (member fan-out) and an existing tag " + - $"'{existing.Name}'. Rename one of the configured tags to resolve."); + $"'{existingMember.Name}'. Rename one of the configured tags to resolve."); _tagsByName[memberTag.Name] = memberTag; } } - else - { - // Non-structure tag (or Structure with no declared members — kept readable - // so the whole-UDT path or declaration-only grouping can serve it). - if (_tagsByName.TryGetValue(tag.Name, out var existing)) - throw new InvalidOperationException( - $"AbCip tag name collision: '{tag.Name}' is declared more than once. " + - $"Existing entry DeviceHostAddress='{existing.DeviceHostAddress}', " + - $"TagPath='{existing.TagPath}'. Rename or remove the duplicate."); - _tagsByName[tag.Name] = tag; - } } // Probe loops — one per device when enabled + a ProbeTagPath is configured. @@ -434,6 +428,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNodeIdUnknown, null, now); return; } + // Driver.AbCip-005: a Structure tag whose Members are declared is a container — + // its bare name is readable via the whole-UDT grouping path (ReadGroupAsync), not the + // per-tag path. Reading it here returns BadNotSupported rather than Good/null so the + // caller knows to address individual member paths (e.g. "Motor.Speed"). + if (def.DataType == AbCipDataType.Structure && def.Members is { Count: > 0 }) + { + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNotSupported, null, now); + return; + } if (!_devices.TryGetValue(def.DeviceHostAddress, out var device)) { results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNodeIdUnknown, null, now); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs index 1e1d1c5..6fc5e00 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverCodeReviewRegressionTests.cs @@ -223,19 +223,20 @@ public sealed class AbCipDriverCodeReviewRegressionTests var results = await drv.ReadAsync(["Counter"], CancellationToken.None); results.Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); - // The value must be the uint, not a negative int + // The value must be the uint — if it were (int)GetUInt32 it would be -1 (wrong type). results.Single().Value.ShouldBe(largeUDInt); - results.Single().Value.ShouldNotBe(-1); + results.Single().Value.ShouldBeOfType(); } // ---- Driver.AbCip-005 — Structure parent not registered; duplicate key check ---- [Fact] - public async Task Structure_parent_tag_is_not_readable_after_member_fan_out() + public async Task Structure_parent_tag_read_returns_BadNotSupported_not_Good_null() { - // Regression for Driver.AbCip-005: the bare parent "Motor" used to be added to - // _tagsByName before member fan-out, so ReadAsync("Motor") returned Good/null. - // After the fix, reading the parent name returns BadNodeIdUnknown. + // Regression for Driver.AbCip-005: reading the bare parent "Motor" used to return + // Good/null because DecodeValue(Structure, ...) returns null. After the fix, + // the per-tag read path detects a Structure-with-Members and returns BadNotSupported + // so callers know to address individual member paths instead. var factory = new FakeAbCipTagFactory(); var drv = new AbCipDriver(new AbCipDriverOptions { @@ -253,8 +254,9 @@ public sealed class AbCipDriverCodeReviewRegressionTests var results = await drv.ReadAsync(["Motor"], CancellationToken.None); - // Parent is not a readable tag — BadNodeIdUnknown, not Good/null. - results.Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + // Parent is a container, not a scalar — BadNotSupported, not Good/null. + results.Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + results.Single().Value.ShouldBeNull(); } [Fact]