From e3d6b76edee2ffe7865b8773f7f562ee70ac1e3b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:15:19 -0400 Subject: [PATCH] fix(abcip): a scalar read whose width disagrees with its authored type is BadTypeMismatch libplctag decodes whatever bytes the controller returns as the authored type and never checks the CIP type code. Live read-only check on a production press: WW_ActualVacuum is a REAL (L5X + the controller's symbol table agree) but the legacy Kepware export in the ignitionoee repo types it Short; read as INT it returned Good -17346 while the real value was 5.37. A scalar atomic read (not an array, not a bit-in-DINT) whose buffer size is not the authored type's width now reads BadTypeMismatch with a warning naming both. Widths observed live: SINT/BOOL 1, INT 2, DINT/REAL 4, LINT 8. IAbCipTagRuntime.GetSize() is a default-interface member (-1 = unknown, never checked). Same-width mismatches (DINT vs REAL) stay undetectable without the type code libplctag strips. --- .../AbCipDriver.cs | 30 ++++++++ .../IAbCipTagRuntime.cs | 7 ++ .../LibplctagTagRuntime.cs | 3 + .../AbCipReadWidthMismatchTests.cs | 72 +++++++++++++++++++ .../FakeAbCipTag.cs | 6 ++ 5 files changed, 118 insertions(+) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWidthMismatchTests.cs 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 36907a15..8afb0a38 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -799,6 +799,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var tagPath = AbCipTagPath.TryParse(def.TagPath); var bitIndex = tagPath?.BitIndex; + + // libplctag never checks the CIP type the controller returned; it decodes the bytes as + // the authored type. A scalar whose buffer is not that type's width is mis-typed config + // (seen live: a REAL authored as INT read Good -17346), so refuse it rather than publish + // garbage as Good. Same-width mismatches (DINT vs REAL) cannot be seen this way. + if (!IsArrayTag(def) && bitIndex is null + && ExpectedScalarWidth(def.DataType) is { } width + && runtime.GetSize() is var size && size >= 0 && size != width) + { + 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", + reference, def.DeviceHostAddress, size, def.DataType, width); + return ReadOutcome.Degraded($"type mismatch reading {reference}: {size} bytes for {def.DataType}"); + } // 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 // (ElementCount 1). Scalar tags keep the single-value path. @@ -1367,6 +1382,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, memberName.StartsWith("ZZZZZZZZZZ", StringComparison.Ordinal) || memberName.StartsWith("__", StringComparison.Ordinal); + /// + /// Wire width of a scalar atomic Logix type, or null where the read buffer's size is not + /// a reliable type check (BOOL, STRING, DT, structures). + /// + /// The authored type. + /// The width in bytes, or null. + internal static int? ExpectedScalarWidth(AbCipDataType type) => type switch + { + AbCipDataType.SInt or AbCipDataType.USInt => 1, + AbCipDataType.Int or AbCipDataType.UInt => 2, + AbCipDataType.DInt or AbCipDataType.UDInt or AbCipDataType.Real => 4, + AbCipDataType.LInt or AbCipDataType.ULInt or AbCipDataType.LReal => 8, + _ => null, + }; + /// /// Recursively fan a controller-discovered UDT shape out into Variable nodes — one per /// ATOMIC leaf — under the folder produced by . Mirrors the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs index 87a630be..d3cbafa8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs @@ -31,6 +31,13 @@ public interface IAbCipTagRuntime : IDisposable /// The raw libplctag status code (zero on success, negative on error). int GetStatus(); + /// + /// Bytes in the last read's buffer, or -1 when the runtime cannot say. Used to catch an + /// authored type whose width disagrees with what the controller actually returned. + /// + /// The buffer size in bytes, or -1 if unknown. + int GetSize() => -1; + /// /// Decode the local buffer into a boxed .NET value per the tag's configured type. /// is non-null only for BOOL-within-DINT tags captured in diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index b11cbd49..b503dc03 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -46,6 +46,9 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime /// public int GetStatus() => (int)_tag.GetStatus(); + /// + public int GetSize() => _tag.GetSize(); + /// public object? DecodeValue(AbCipDataType type, int? bitIndex) => DecodeValueAt(type, 0, bitIndex); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWidthMismatchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWidthMismatchTests.cs new file mode 100644 index 00000000..0e23496a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWidthMismatchTests.cs @@ -0,0 +1,72 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// libplctag hands back whatever bytes the controller sends and decodes them as the authored type +/// — it never checks the CIP type code. Seen live on a production press: WW_ActualVacuum is a +/// REAL (the L5X and the controller's symbol table agree), the legacy Kepware export types it as +/// Short, and reading it authored as INT returned Good -17346 (the low half of a float) +/// where the real value was 5.37. A scalar atomic read whose buffer is not the authored type's +/// width is now BadTypeMismatch. Same-width mismatches (DINT vs REAL) remain undetectable +/// without the type code. +/// +[Trait("Category", "Unit")] +public sealed class AbCipReadWidthMismatchTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private static async Task ReadStatusAsync(AbCipDataType authored, int bufferSize, string path = "WW_ActualVacuum") + { + var factory = new FakeAbCipTagFactory + { + Customise = p => new FakeAbCipTag(p) { Value = 1, Size = bufferSize }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", Device, path, authored)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-1", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var r = await drv.ReadAsync(["T"], TestContext.Current.CancellationToken); + return r[0].StatusCode; + } + + /// An INT read that comes back 4 bytes wide is a type mismatch, not a Good value. + [Fact] + public async Task INT_authored_over_a_4_byte_REAL_is_BadTypeMismatch() + { + (await ReadStatusAsync(AbCipDataType.Int, bufferSize: 4)).ShouldBe(AbCipStatusMapper.BadTypeMismatch); + } + + /// Matching widths still read Good — including the live-observed sizes per type. + /// Authored type. + /// Buffer size the controller returned for that type (observed live). + [Theory] + [InlineData(AbCipDataType.Int, 2)] + [InlineData(AbCipDataType.DInt, 4)] + [InlineData(AbCipDataType.Real, 4)] + [InlineData(AbCipDataType.LInt, 8)] + [InlineData(AbCipDataType.SInt, 1)] + public async Task Matching_width_reads_Good(AbCipDataType type, int size) + { + (await ReadStatusAsync(type, size)).ShouldBe(AbCipStatusMapper.Good); + } + + /// An unknown size (a runtime that cannot report it) never blocks a read. + [Fact] + public async Task Unknown_size_is_not_checked() + { + (await ReadStatusAsync(AbCipDataType.Int, bufferSize: -1)).ShouldBe(AbCipStatusMapper.Good); + } + + /// A bit-in-DINT read decodes from the parent's 4-byte buffer and is not width-checked. + [Fact] + public async Task Bit_index_read_is_not_width_checked() + { + (await ReadStatusAsync(AbCipDataType.Bool, bufferSize: 4, path: "Flags.3")).ShouldBe(AbCipStatusMapper.Good); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs index 7eb01fc5..3d8be6b3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs @@ -64,6 +64,12 @@ internal class FakeAbCipTag : IAbCipTagRuntime /// Returns the simulated status code. public virtual int GetStatus() => Status; + /// Gets or sets the simulated read-buffer size in bytes (-1 = unknown, the default). + public int Size { get; set; } = -1; + + /// Returns the simulated read-buffer size. + public virtual int GetSize() => Size; + /// Returns the mock tag value. /// The data type being decoded. /// The optional bit index for bit operations.