From 352ea449e9a3cd26c9ddee7f8adf306340c0e044 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 21:40:04 -0400 Subject: [PATCH] =?UTF-8?q?fix(abcip):=20final=20review=20I4=20=E2=80=94?= =?UTF-8?q?=20a=20type=20mismatch=20is=20a=20refusal,=20not=20a=20comms=20?= =?UTF-8?q?fault?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A native TypeMismatch (read status, write gate, or CIP 0xFF/0x2107) used to evict the runtime and degrade health on every poll; for a sole lease holder that meant Forward Close + Forward Open per poll, and an all- mismatched driver tripped the comms-loss watchdog. It now reports BadTypeMismatch, keeps the handle, leaves health alone and warns once per tag per instance — reads, whole-UDT reads and writes, plus the libplctag width check. Tests (red against the pre-fix driver): single create over N polls, unchanged health, one warning, and one Forward Open / zero Forward Close over the real native runtime against FakeCipTarget. AbLegacy has no TypeMismatch path (its native runtime has no type check). Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/AbCip.md | 8 +- .../AbCipDriver.cs | 79 ++++++++- .../NativeAbCipTagFactory.cs | 7 +- .../AbCipTypeMismatchTests.cs | 159 ++++++++++++++++++ ...B.MOM.WW.OtOpcUa.Driver.AbCip.Tests.csproj | 2 + 5 files changed, 247 insertions(+), 8 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 12044105..6d7b7193 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -123,7 +123,8 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi `Native`; libplctag remains available as the fallback. - ⚠️ **Client-visible:** native reports **`BadTypeMismatch`** for a mis-authored same-width type (for example a tag authored `DINT` where the controller holds a `REAL`), where libplctag returned Good garbage. Correct the - authored type; see [Native transport behaviour](#native-transport-behaviour). + authored type; see [Native transport behaviour](#native-transport-behaviour). It does not degrade driver + health or churn the connection; one Warning per tag names it. - **To fall back** one driver, set `"transport": "Libplctag"` in its driver config (or pick "libplctag (fallback)" in the AdminUI dropdown). To fall back a whole node, set `Drivers:AbCip:DefaultTransport` to `Libplctag` (below). @@ -176,7 +177,10 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi holds a `REAL` returns the REAL's bits as an integer with a Good status under libplctag, and `BadTypeMismatch` natively. That is the fix working, but a consumer will see Bad quality on tags that previously looked healthy: correct the authored type. Same-width signed/unsigned pairs are exempt, and - writes are gated on a mismatch too. + writes are gated on a mismatch too. A mismatch is a **deterministic configuration refusal**, handled like + an Unsupported refusal (final review I4): the tag's handle is kept (no re-create, no reconnect), driver + health is unchanged (no `Degraded`, nothing fed to the comms-loss watchdog), and the Warning is logged + **once per tag** per driver instance, not every poll. The same applies to the libplctag-path width check. - Malformed tag names put the device into backoff, as under libplctag (#570). STRING capacity includes the alignment pad (84, not 82) (#570). - **A refused operation is not a comms fault.** Any operation a runtime refuses as Unsupported (in AbCip, e.g. 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 1d7af2ac..f8f03953 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -195,6 +195,18 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Configured-vs-effective rendering of _transport for the init log, e.g. "Native (default)". private readonly string _transportDescription; + // Tags whose type-mismatch Warning has already been logged (final review I4): a mis-authored tag is + // polled forever, so the Warning is once per tag per driver instance, not once per poll. + private readonly System.Collections.Concurrent.ConcurrentDictionary _typeMismatchWarned = + new(StringComparer.OrdinalIgnoreCase); + + /// Logs a type-mismatch Warning the first time reports one on this instance. + private void WarnTypeMismatchOnce(string tagName, string message, params object?[] args) + { + if (_typeMismatchWarned.TryAdd(tagName, 0)) + _logger.LogWarning(message, args); + } + /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30); @@ -807,6 +819,19 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, await runtime.ReadAsync(ct).ConfigureAwait(false); var status = runtime.GetStatus(); + if (status == (int)AbStatus.TypeMismatch) + { + // Final review I4: the controller ANSWERED with a type that disagrees with the authored + // one — a deterministic config error, not a comms fault. Keep the handle (evicting would + // re-create it — and, for a sole lease holder, re-Forward-Open — on every poll), leave + // health alone, warn once per tag. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); + WarnTypeMismatchOnce(def.Name, + "AbCip read of {Tag} on device {Device}: the controller's type does not match the authored " + + "{DataType}; reporting BadTypeMismatch until the tag's dataType is corrected (logged once per tag)", + reference, def.DeviceHostAddress, def.DataType); + return ReadOutcome.None; + } if (status != 0) { // Evict the stale handle so the next call re-creates it. @@ -834,11 +859,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, && ExpectedScalarWidth(def.DataType) is { } width && runtime.GetSize() is var size && size >= 0 && size != width) { + // Same class as the native TypeMismatch above (final review I4): deterministic, device + // answered — no Degraded, warn once per tag. results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); - _logger.LogWarning( - "AbCip read of {Tag} on device {Device} returned {Size} bytes but its authored type {DataType} is {Width} bytes wide; check the tag's dataType", + WarnTypeMismatchOnce(def.Name, + "AbCip read of {Tag} on device {Device} returned {Size} bytes but its authored type {DataType} is {Width} bytes wide; check the tag's dataType (logged once per tag)", reference, def.DeviceHostAddress, size, def.DataType, width); - return ReadOutcome.Degraded($"type mismatch reading {reference}: {size} bytes for {def.DataType}"); + return ReadOutcome.None; } // Review I-1 — an array tag (the EXPLICIT IsArray flag) decodes the whole buffer into an // element-typed CLR array (int[]/float[]/bool[]/string[]…), INCLUDING a 1-element array @@ -865,6 +892,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNotSupported, null, now); return ReadOutcome.None; } + catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) + { + // Final review I4: deterministic, keep the handle, no health signal, warn once per tag. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); + WarnTypeMismatchOnce(def.Name, "AbCip read of {Tag} on device {Device} refused: {Reason} (logged once per tag)", + reference, def.DeviceHostAddress, ate.Message); + return ReadOutcome.None; + } catch (Exception ex) { // Transport exception — evict so the next read creates a fresh handle. An AbTransportException @@ -911,6 +946,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, await runtime.ReadAsync(ct).ConfigureAwait(false); var status = runtime.GetStatus(); + if (status == (int)AbStatus.TypeMismatch) + { + // Final review I4: deterministic — keep the handle, no health signal, warn once. + StampGroupStatus(group, results, now, AbCipStatusMapper.BadTypeMismatch); + WarnTypeMismatchOnce(parent.Name, + "AbCip whole-UDT read of {Parent} on device {Device}: the controller's type does not match " + + "the authored type; {MemberCount} members report BadTypeMismatch (logged once per tag)", + group.ParentName, parent.DeviceHostAddress, group.Members.Count); + return ReadOutcome.None; + } if (status != 0) { EvictRuntime(device, parent.Name); @@ -944,6 +989,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, StampGroupStatus(group, results, now, AbCipStatusMapper.BadNotSupported); return ReadOutcome.None; } + catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) + { + StampGroupStatus(group, results, now, AbCipStatusMapper.BadTypeMismatch); + WarnTypeMismatchOnce(parent.Name, + "AbCip whole-UDT read of {Parent} on device {Device} refused: {Reason} (logged once per tag)", + group.ParentName, parent.DeviceHostAddress, ate.Message); + return ReadOutcome.None; + } catch (Exception ex) { EvictRuntime(device, parent.Name); @@ -1027,7 +1080,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, opLock.Release(); } - if (status != 0) + if (status == (int)AbStatus.TypeMismatch) + { + // Final review I4: deterministic — keep the handle, leave health, warn once per tag. + results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); + WarnTypeMismatchOnce(def.Name, + "AbCip write to {Tag} on device {Device} refused: the controller's type does not match the authored {DataType} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType); + } + else if (status != 0) { EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.MapStatus(status)); @@ -1089,6 +1150,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write refused (not supported) for tag {Tag} on device {Device}", w.FullReference, def.DeviceHostAddress); } + catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) + { + // Final review I4: the controller's type disagrees with the authored one (native type gate, or + // CIP 0xFF/0x2107 from the controller) — deterministic: BadTypeMismatch, keep the handle, leave + // health alone, warn once per tag. Retrying cannot succeed until the config is corrected. + results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); + WarnTypeMismatchOnce(def.Name, + "AbCip write to {Tag} on device {Device} refused: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, ate.Message); + } catch (Exception ex) { // Transport / wire error — evict so the next write creates a fresh handle. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagFactory.cs index 5c435bdc..9e84daaa 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagFactory.cs @@ -7,9 +7,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// connection from , so all tags on one device share one CIP /// connection and one request scheduler. Stateless; safe to share across devices. /// -internal sealed class NativeAbCipTagFactory : IAbCipTagFactory +internal sealed class NativeAbCipTagFactory(DeviceSessionPool? pool = null) : IAbCipTagFactory { + // Tests pass a private pool (and a fake target); production uses the process-wide one. + private readonly DeviceSessionPool _pool = pool ?? DeviceSessionPool.Shared; + /// public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => - new NativeAbCipTagRuntime(createParams, DeviceSessionPool.Shared); + new NativeAbCipTagRuntime(createParams, _pool); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs new file mode 100644 index 00000000..d44485c9 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs @@ -0,0 +1,159 @@ +using Microsoft.Extensions.Logging; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; +using CapturingLogger = ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.AbCipLoggingTests.CapturingLogger; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Final review I4: a type mismatch (the controller answered with a type that disagrees with the authored +/// one) is a deterministic configuration refusal, handled like : +/// BadTypeMismatch, the handle is KEPT (one create across N polls), health is unchanged, and the +/// Warning is logged once per tag — not per poll. Before the fix each poll evicted the runtime, so a sole +/// lease holder cost a Forward Close + Forward Open on every poll and the driver sat Degraded. The +/// control tests (a genuine transport failure still evicts and degrades) live in +/// and below. +/// +[Trait("Category", "Unit")] +public sealed class AbCipTypeMismatchTests +{ + private const string Host = "ab://10.0.0.5/1,0"; + private const int Polls = 4; + + /// A fake whose read completes but reports (the native + /// runtime's same-width check), and whose write throws it (the native write gate / CIP 0xFF 0x2107). + private sealed class MismatchTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override async Task ReadAsync(CancellationToken cancellationToken) + { + await base.ReadAsync(cancellationToken); + Status = (int)AbStatus.TypeMismatch; + } + + public override Task WriteAsync(CancellationToken cancellationToken) => + Task.FromException(new AbTransportException(AbStatus.TypeMismatch, "REAL on the controller, DINT authored")); + } + + private static async Task<(AbCipDriver Driver, FakeAbCipTagFactory Factory, Func Creates, CapturingLogger Log)> + NewAsync() + { + var creates = 0; + var factory = new FakeAbCipTagFactory + { + Customise = p => { Interlocked.Increment(ref creates); return new MismatchTag(p) { Value = 1 }; }, + }; + var log = new CapturingLogger(); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", Host, "T", AbCipDataType.DInt, Writable: true)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-i4", factory, logger: log); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory, () => Volatile.Read(ref creates), log); + } + + private static int MismatchWarnings(CapturingLogger log) => + log.Entries.Count(e => e.Level == LogLevel.Warning && e.Message.Contains("logged once per tag")); + +#pragma warning disable OTOPCUA0001 // the driver's own status handling is under test, not resilience wrapping + [Fact] + public async Task A_type_mismatched_read_keeps_the_handle_leaves_health_alone_and_warns_once() + { + var (drv, factory, creates, log) = await NewAsync(); + var health = drv.GetHealth(); + + for (var n = 0; n < Polls; n++) + (await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + creates().ShouldBe(1); + factory.Tags["T"].Disposed.ShouldBeFalse(); + factory.Tags["T"].ReadCount.ShouldBe(Polls); // positive control: every poll did reach the runtime + drv.GetHealth().ShouldBe(health); + MismatchWarnings(log).ShouldBe(1); + } + + [Fact] + public async Task A_type_mismatched_write_keeps_the_handle_leaves_health_alone_and_warns_once() + { + var (drv, factory, creates, log) = await NewAsync(); + var health = drv.GetHealth(); + + for (var n = 0; n < Polls; n++) + (await drv.WriteAsync([new WriteRequest("T", 5)], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + creates().ShouldBe(1); + factory.Tags["T"].Disposed.ShouldBeFalse(); + drv.GetHealth().ShouldBe(health); + MismatchWarnings(log).ShouldBe(1); + } + + /// Control: a genuine transport failure on a READ still evicts and degrades. + [Fact] + public async Task A_transport_failure_read_still_evicts_and_degrades() + { + var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Status = (int)AbStatus.BadConnection } }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", Host, "T", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-i4", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + (await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode + .ShouldNotBe(AbCipStatusMapper.BadTypeMismatch); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + factory.Tags["T"].Disposed.ShouldBeTrue(); + } + + /// + /// End to end over the REAL native runtime + a private against a fake + /// CIP target that answers every read with a REAL for a DINT-authored tag. The tag is the only lease + /// holder (probe off), so an evict would be the LAST release: Forward Close, then a throttled Forward + /// Open on the next poll. One Forward Open across N polls and no Forward Close proves there is no + /// connection churn. + /// + [Fact] + public async Task A_native_type_mismatch_causes_no_connection_churn() + { + await using var target = new FakeCipTarget(); + // Read Tag Fragmented (0x52) → success reply typed REAL (0x00CA) with 4 data bytes. + target.ConnectedHandler = req => Task.FromResult( + req[0] == 0x52 ? [0xD2, 0x00, 0x00, 0x00, 0xCA, 0x00, 0x00, 0x00, 0x80, 0x3F] : target.ServiceReply(req)); + var pool = new DeviceSessionPool(); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var log = new CapturingLogger(); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", host, "T", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "drv-i4-native", new NativeAbCipTagFactory(pool), logger: log); + await drv.InitializeAsync("{}", CancellationToken.None); + var health = drv.GetHealth(); + try + { + for (var n = 0; n < Polls; n++) + (await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + target.ForwardOpens.ShouldBe(1); + target.ForwardCloses.ShouldBe(0); + target.ConnectedRequests.ShouldBeGreaterThanOrEqualTo(Polls); // positive control: polls hit the wire + drv.GetHealth().ShouldBe(health); + MismatchWarnings(log).ShouldBe(1); + } + finally + { + await drv.ShutdownAsync(CancellationToken.None); + } + } +#pragma warning restore OTOPCUA0001 +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.csproj b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.csproj index 1b4c8152..38c3d151 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.csproj +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.csproj @@ -34,6 +34,8 @@ + +