From 8b50b43048f2b6efebd42fe397642cb57be5ab4f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:20:17 -0400 Subject: [PATCH] fix(ablegacy): deterministic NotSupported refusals are BadNotSupported with no health write Review 07 H16: an array of String/Timer/Counter/Control passed the mapper, so every poll created a handle (one init read), the decode threw NotSupportedException, the generic catch evicted the handle and latched Degraded. FromTagConfig now rejects that combination (the skip carries the reason, Inspect/deploy gate and the AdminUI model state it), and a runtime NotSupportedException on read or write is BadNotSupported: handle kept, no health write, one Warning per tag and operation. Write refusals (native PLC-5 and status-bit writes, libplctag translated addresses) now warn once per tag too. Refs #738 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbLegacyTagDefinitionFactory.cs | 26 ++++ .../AbLegacyDriver.cs | 39 ++++- .../Uns/TagEditors/AbLegacyTagConfigModel.cs | 5 + .../AbLegacyDeterministicRefusalTests.cs | 136 ++++++++++++++++++ .../Uns/AbLegacyTagConfigModelTests.cs | 14 ++ 5 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDeterministicRefusalTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs index 8c0742f5..c6ab0a97 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs @@ -77,6 +77,10 @@ public static class AbLegacyTagDefinitionFactory if (rawLength >= 1) arrayLength = Math.Min(rawLength, AbLegacyArray.MaxElements); } + // A deterministic refusal (review 07 H16): String / Timer / Counter / Control elements are structured + // or variable-width and do not lay out as a flat array, so both runtimes refuse to decode them. Accepting + // the tag only bought a handle create + a NotSupportedException on every poll. + if (arrayLength is not null && !IsArrayElementTypeSupported(dataType)) return false; // "writable" defaults to true when absent (today's value) — makes read-only equipment tags // authorable. var writable = TagConfigJson.ReadWritable(root); @@ -224,8 +228,30 @@ public static class AbLegacyTagDefinitionFactory } if (TagConfigJson.DescribeInvalidEnum(root, "dataType") is { } w) warnings.Add(w); + + if (IsArrayFlag(root) && ReadInt(root, "arrayLength") >= 1 + && TagConfigJson.TryReadEnumStrict(root, "dataType", AbLegacyDataType.Int, out var dt) + && !IsArrayElementTypeSupported(dt)) + warnings.Add(ArrayElementTypeError(dt)); } + /// + /// Whether can be the element type of an array tag. Only the flat word/long/float/bit + /// files lay out as a contiguous array; String, Timer, Counter and Control elements are structured. + /// + /// The authored element data type. + /// for Bit, Int, AnalogInt, Long and Float. + public static bool IsArrayElementTypeSupported(AbLegacyDataType type) => type is + AbLegacyDataType.Bit or AbLegacyDataType.Int or AbLegacyDataType.AnalogInt + or AbLegacyDataType.Long or AbLegacyDataType.Float; + + /// The authoring / deploy-gate message for an array of an unsupported element type. + /// The authored element data type. + /// The message. + public static string ArrayElementTypeError(AbLegacyDataType type) => + $"AbLegacy {type} is not supported as an array element (String, Timer, Counter and Control elements are " + + "structured); author a scalar tag per element instead. The driver skips this tag and it reads BadNodeIdUnknown."; + private static int ReadInt(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number && e.TryGetInt32(out var v) ? v : 0; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index b5bc01e2..9adfd47c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -221,10 +221,13 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover { if (!AbLegacyTagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def)) { - _skippedTags.Skip(entry.RawPath, "TagConfig did not map to a definition"); + // The deploy gate's own explanation (same authority) — e.g. an array of a structured type. + var reason = AbLegacyTagDefinitionFactory.Inspect(entry.TagConfig).FirstOrDefault() + ?? "TagConfig did not map to a definition"; + _skippedTags.Skip(entry.RawPath, reason); _logger.LogWarning( - "AbLegacy tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}", - _driverInstanceId, entry.RawPath); + "AbLegacy tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath} Reason={Reason}", + _driverInstanceId, entry.RawPath, reason); continue; } tags[entry.RawPath] = def with @@ -523,6 +526,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover { // The runtime refused the operation before the wire (e.g. a native PLC-5 non-N read). // Not a comms fault: BadNotSupported, keep the handle, leave health alone. + WarnRefusalOnce(device, def, "read", ex.Message); + results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now); + } + catch (NotSupportedException nse) + { + // A deterministic decode refusal (review 07 H16) — the same answer every poll. BadNotSupported, keep + // the handle (re-creating it only re-paid the init read), warn once, and NO health write. + WarnRefusalOnce(device, def, "read", nse.Message); results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now); } catch (Exception ex) @@ -601,8 +612,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover // READ, and against a device lacking that file the read failure surfaced as a comms fault — // BadCommunicationError, evicted handle, Degraded health — instead of the refusal. // Ahead of the bit-RMW branch too, which would otherwise read the parent word first. - if (parsed is not null && _tagFactory.WriteRefusal(CreateParamsFor(device, def, parsed)) is not null) + if (parsed is not null && _tagFactory.WriteRefusal(CreateParamsFor(device, def, parsed)) is { } refusal) { + WarnRefusalOnce(device, def, "write", refusal); results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported); continue; } @@ -654,8 +666,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover catch (OperationCanceledException) { throw; } catch (NotSupportedException nse) { + // Deterministic (review 07 H16): nothing reached the wire, so it says nothing about the device. + WarnRefusalOnce(device, def, "write", nse.Message); results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported); - SetHealthIfCurrent(epoch, new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message)); } catch (Exception ex) when (ex is FormatException or InvalidCastException) { @@ -669,6 +682,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover { // The runtime refused the write before the wire — a native status-bit sub-element write or a // PLC-5 write. Not a comms fault: BadNotSupported, keep the handle, leave health alone. + WarnRefusalOnce(device, def, "write", ex.Message); results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported); } catch (Exception ex) @@ -1157,6 +1171,18 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover _driverInstanceId, def.DeviceHostAddress, def.Name, def.Address, device.Options.PlcFamily); } + /// + /// A deterministic refusal (BadNotSupported) is answered the same way every time, so it is logged ONCE per + /// tag and operation — a polled tag must not flood the log, and the refusal never touches driver health. + /// + private void WarnRefusalOnce(DeviceState device, AbLegacyTagDefinition def, string operation, string reason) + { + if (!device.RefusalLogged.TryAdd($"{operation}:{def.Name}", 0)) return; + _logger.LogWarning( + "AbLegacy {Operation} refused (BadNotSupported). Driver={DriverInstanceId} Device={DeviceHostAddress} Reference={Reference} Address={Address} Reason={Reason}", + operation, _driverInstanceId, def.DeviceHostAddress, def.Name, def.Address, reason); + } + /// The create parameters for a tag's own runtime — shared by creation and the write-refusal check, so /// the refusal is asked about exactly the runtime a write would otherwise create. private AbLegacyTagCreateParams CreateParamsFor(DeviceState device, AbLegacyTagDefinition def, AbLegacyAddress parsed) => @@ -1387,6 +1413,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover /// public bool FirstNonZeroStatusLogged { get; set; } + /// Tag operations already warned about for a deterministic refusal (once-per-tag-and-operation log). + public System.Collections.Concurrent.ConcurrentDictionary RefusalLogged { get; } = new(StringComparer.Ordinal); + /// Tags already warned about for a malformed address (once-per-tag log). public System.Collections.Concurrent.ConcurrentDictionary MalformedAddressLogged { get; } = new(); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs index ca742cff..dcb67114 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs @@ -87,6 +87,11 @@ public sealed class AbLegacyTagConfigModel if (isArray && arrayLength > AbLegacyArray.MaxElements) return $"AbLegacy array length cannot exceed {AbLegacyArray.MaxElements} (PCCC data-file element limit)."; + // Same rule the reader enforces (review 07 H16): structured elements never form an array. Stated here so + // the CSV-import / API paths get the specific reason instead of the parse gate's generic one. + if (isArray && arrayLength >= 1 && !AbLegacyTagDefinitionFactory.IsArrayElementTypeSupported(DataType)) + return AbLegacyTagDefinitionFactory.ArrayElementTypeError(DataType); + // Parse-gate backstop: the checks above mirror the reader's whole rejection surface today, so this is // expected to be silent for AbLegacy. It is here so a future reader rule cannot become a gap. return TagConfigParseGate.Describe( diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDeterministicRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDeterministicRefusalTests.cs new file mode 100644 index 00000000..6542df4c --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyDeterministicRefusalTests.cs @@ -0,0 +1,136 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Review 07 H16: an array of String/Timer/Counter/Control was accepted by the mapper, so every poll created a +/// handle (one init read), threw from the decode, disposed the handle and +/// latched Degraded. A deterministic refusal is now rejected at the mapper; a runtime +/// that still occurs is per-tag BadNotSupported, keeps its handle, +/// warns once and leaves driver health alone. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyDeterministicRefusalTests +{ + private const string Host = "ab://10.0.0.5/1,0"; + + [Theory] + [InlineData("String", "ST9:0")] + [InlineData("TimerElement", "T4:0")] + [InlineData("CounterElement", "C5:0")] + [InlineData("ControlElement", "R6:0")] + public void Mapper_rejects_arrays_of_structured_element_types(string dataType, string address) + { + var json = $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}"""; + + AbLegacyTagDefinitionFactory.FromTagConfig(json, "p", out _).ShouldBeFalse(); + AbLegacyTagDefinitionFactory.Inspect(json).ShouldContain(w => w.Contains("not supported as an array")); + // Positive control: the same type as a SCALAR is still accepted. + AbLegacyTagDefinitionFactory.FromTagConfig( + $$"""{"address":"{{address}}","dataType":"{{dataType}}"}""", "p", out _).ShouldBeTrue(); + } + + [Theory] + [InlineData("Int", "N7:0")] + [InlineData("Float", "F8:0")] + [InlineData("Long", "L9:0")] + [InlineData("Bit", "B3:0")] + public void Mapper_keeps_arrays_of_flat_element_types(string dataType, string address) + { + var json = $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}"""; + AbLegacyTagDefinitionFactory.FromTagConfig(json, "p", out var def).ShouldBeTrue(); + def.ArrayLength.ShouldBe(4); + AbLegacyTagDefinitionFactory.Inspect(json).ShouldBeEmpty(); + } + + [Fact] + public async Task Rejected_array_tag_is_skipped_with_its_reason_and_creates_no_runtime() + { + var factory = new FakeAbLegacyTagFactory { Customise = p => new FakeAbLegacyTag(p) { Value = 1 } }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(Host)], + RawTags = + [ + new RawTagEntry("Strs", """{"address":"ST9:0","dataType":"String","isArray":true,"arrayLength":3}""", false, Host), + new RawTagEntry("Good", """{"address":"N7:0","dataType":"Int"}""", false, Host), + ], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-h16", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var r = await drv.ReadAsync(["Strs", "Good"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNodeIdUnknown); + r[1].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); // positive control + factory.Tags.Keys.ShouldNotContain("ST9:0"); + var skipped = drv.GetSkippedTags().ShouldNotBeNull(); + skipped.Sample.ShouldContain(s => s.RawPath == "Strs" && s.Reason.Contains("not supported as an array")); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.ShutdownAsync(CancellationToken.None); + } + + private sealed class RefusingDecodeTag(AbLegacyTagCreateParams p) : FakeAbLegacyTag(p) + { + public override object? DecodeArray(AbLegacyDataType type, int count) => + throw new NotSupportedException("decode refused"); + + public override void EncodeValue(AbLegacyDataType type, int? bitIndex, object? value) => + throw new NotSupportedException("encode refused"); + } + + [Fact] + public async Task Runtime_NotSupported_on_read_is_BadNotSupported_keeps_handle_no_health_warns_once() + { + var logger = new AbLegacyTestLogger(); + var factory = new FakeAbLegacyTagFactory + { + Customise = p => p.TagName == "N7:0" ? new RefusingDecodeTag(p) : new FakeAbLegacyTag(p) { Value = 7 }, + }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(Host)], + RawTags = AbLegacyRawTags.Entries( + new AbLegacyTagDefinition("Arr", Host, "N7:0", AbLegacyDataType.Int, ArrayLength: 4), + new AbLegacyTagDefinition("Good", Host, "N9:0", AbLegacyDataType.Int)), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-h16", factory, logger); + await drv.InitializeAsync("{}", CancellationToken.None); + + var first = await drv.ReadAsync(["Arr", "Good"], CancellationToken.None); + var second = await drv.ReadAsync(["Arr", "Good"], CancellationToken.None); + + first[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); + second[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); + first[1].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); // positive control: the refusal is per tag + var tag = factory.Tags["N7:0"]; + tag.InitializeCount.ShouldBe(1); // handle kept, not churned + tag.Disposed.ShouldBeFalse(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + logger.Warnings.Count(w => w.Contains("Arr") && w.Contains("decode refused")).ShouldBe(1); + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task Runtime_NotSupported_on_write_is_BadNotSupported_with_no_health_write() + { + var factory = new FakeAbLegacyTagFactory { Customise = p => new RefusingDecodeTag(p) { Value = 1 } }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(Host)], + RawTags = AbLegacyRawTags.Entries(new AbLegacyTagDefinition("X", Host, "N7:0", AbLegacyDataType.Int)), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-h16", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + (await drv.ReadAsync(["X"], CancellationToken.None))[0].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + + var w = await drv.WriteAsync([new WriteRequest("X", 5)], CancellationToken.None); + + w[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + factory.Tags["N7:0"].Disposed.ShouldBeFalse(); + await drv.ShutdownAsync(CancellationToken.None); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs index 975bd68e..2bf866bb 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs @@ -122,6 +122,20 @@ public sealed class AbLegacyTagConfigModelTests m.Validate().ShouldBeNull(); } + /// Review 07 H16: an array of a structured element type is refused with the reader's own reason. + [Theory] + [InlineData("String", "ST9:0")] + [InlineData("TimerElement", "T4:0")] + public void Validate_refuses_an_array_of_a_structured_element_type(string dataType, string address) + { + var m = AbLegacyTagConfigModel.FromJson( + $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}"""); + + m.Validate().ShouldNotBeNull().ShouldContain("not supported as an array"); + // Positive control: the same type as a scalar validates. + AbLegacyTagConfigModel.FromJson($$"""{"address":"{{address}}","dataType":"{{dataType}}"}""").Validate().ShouldBeNull(); + } + [Fact] public void Validate_returns_null_for_scalar_tag_regardless_of_no_array_length() {