From 3b48e05e9a1cc19ce8612fcda3233717e9518944 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:04:11 -0400 Subject: [PATCH 1/7] fix(abcip): decode real @udt template buffers; browse STRINGs as String, hide BOOL hosts Live read-only check against production CompactLogix 5069-L320ERS2 rev 34.11 presses (Z-35162 / 10.209.70.160): every @udt/ template the controller returned decoded to null, so controller-discovered UDTs never fanned out. The decoder assumed a 12-byte header led by the member count and [info][array][offset] blocks with ';'-terminated names; libplctag actually returns a 14-byte header (u16 id, u32 def words, u32 instance size, u16 member count, u16 handle), [info][type][offset] blocks, and NUL-terminated names after a 'Name;' type name. All three test builders restated the old assumption, so the unit suite agreed with itself (the #468 trap again); they now share a TemplateBufferBuilder pinned by real captures (CipTemplateObjectWireCaptureTests). Discovery follow-ons that the working decoder exposed: - STRING (and user string types, LEN DINT + DATA SINT[n]) browse as one String variable, top-level and nested, not as a LEN/DATA folder. - Controller-generated members (ZZZZZZZZZZ* BOOL hosts, __* AOI backing) are not browsed. - A UDT array is not fanned out into element-free Tag.Member paths. --- .../AbCipDriver.cs | 94 +++++++---- .../AbCipTemplateCache.cs | 14 +- .../CipTemplateObjectDecoder.cs | 153 ++++++++---------- .../AbCipDriverDiscoveryTests.cs | 135 ++++++++++++---- .../AbCipFetchUdtShapeTests.cs | 32 +--- .../CipTemplateObjectDecoderTests.cs | 81 +++------- .../CipTemplateObjectWireCaptureTests.cs | 98 +++++++++++ .../TemplateBufferBuilder.cs | 45 ++++++ 8 files changed, 428 insertions(+), 224 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectWireCaptureTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/TemplateBufferBuilder.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 931a7069..5c780907 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1285,6 +1285,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // bogus single Structure → String placeholder Variable. When the shape can't be // resolved (or yields no emittable leaf) we fall through to the legacy single // Variable so nothing regresses. + // Review I-1 — a discovered array of length 1 is still an array; honour the + // explicit IsArray flag (legacy ElementCount > 1 still surfaces as an array). + var discoveredIsArray = discovered.IsArray || discovered.ElementCount > 1; + var discoveredType = discovered.DataType; + if (discovered.DataType == AbCipDataType.Structure) { // Pass the discovered tag's CIP template instance id (surfaced by the Symbol @@ -1294,7 +1299,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var shape = await ResolveDiscoveredUdtShapeAsync( device.HostAddress, discovered.Name, discovered.TemplateInstanceId, cancellationToken) .ConfigureAwait(false); - if (shape is not null) + if (shape is { IsStringFamily: true }) + { + // A controller reports every STRING (built-in or a user string type such as + // STR0016) as a LEN + DATA struct. It is one String value — the read path reads + // it as such — so it becomes one String Variable, never a LEN/DATA folder. + discoveredType = AbCipDataType.String; + } + else if (shape is not null && !discoveredIsArray) { // Materialise the member sub-folder LAZILY — only once a leaf is actually // about to be emitted into it. A shape that yields no usable atomic leaf @@ -1313,12 +1325,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } if (!emitted.Add(fullName)) continue; - // Review I-1 — a discovered array of length 1 is still an array; honour the - // explicit IsArray flag (legacy ElementCount > 1 still surfaces as an array). - var discoveredIsArray = discovered.IsArray || discovered.ElementCount > 1; discoveredFolder.Variable(fullName, discovered.Name, new DriverAttributeInfo( FullName: fullName, - DriverDataType: discovered.DataType.ToDriverDataType(), + DriverDataType: discoveredType.ToDriverDataType(), IsArray: discoveredIsArray, ArrayDim: discoveredIsArray ? (uint)Math.Max(1, discovered.ElementCount) : null, SecurityClass: discovered.ReadOnly @@ -1327,12 +1336,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, IsHistorized: false, IsAlarm: false, WriteIdempotent: false, - NativeDataType: discovered.DataType.ToString())); + NativeDataType: discoveredType.ToString())); } } } } + /// + /// true for a UDT member the controller generates rather than the user declares: the + /// hidden SINT/DINT hosts Logix packs BOOL members into (ZZZZZZZZZZAlarms0) and AOI + /// backing members (__BitHost00). A user-declared Logix name can never begin with two + /// underscores, so neither rule can hide a real member. + /// + /// The member name from the template. + /// true when the member is controller-generated. + internal static bool IsHiddenUdtMember(string memberName) => + memberName.StartsWith("ZZZZZZZZZZ", StringComparison.Ordinal) + || memberName.StartsWith("__", StringComparison.Ordinal); + /// /// Recursively fan a controller-discovered UDT shape out into Variable nodes — one per /// ATOMIC leaf — under the folder produced by . Mirrors the @@ -1381,37 +1402,50 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var anyEmitted = false; foreach (var member in shape.Members) { + if (IsHiddenUdtMember(member.Name)) continue; var memberFullName = $"{parentFullName}.{member.Name}"; + var leafType = member.DataType; if (member.DataType == AbCipDataType.Structure) { - // Nested struct: recurse into its sub-shape if it resolves, the depth cap allows it, - // and we're not re-entering a struct already on this path (cycle guard). The nested - // members live a level deeper, so the cap is checked against depth + 1. - if (depth + 1 > MaxUdtDepth) continue; - if (!visited.Add(member.Name)) continue; // cyclic UDT reference — stop here - // The decoded parent shape carries the nested UDT's template instance id (low 12 bits of the - // member info, captured by CipTemplateObjectDecoder) — thread it so the nested shape is fetched - // off the controller via FetchUdtShapeAsync, making nested-struct members addressable in - // production (name-keyed seeded shapes still win first, for the test seam). + // Nested struct: resolve its sub-shape first. The decoded parent shape carries the nested + // UDT's template instance id (low 12 bits of the member type word, captured by + // CipTemplateObjectDecoder) — thread it so the nested shape is fetched off the controller + // via FetchUdtShapeAsync (name-keyed seeded shapes still win first, for the test seam). var nested = await ResolveDiscoveredUdtShapeAsync( deviceHostAddress, member.Name, templateInstanceId: member.NestedTemplateId, cancellationToken) .ConfigureAwait(false); - if (nested is not null) + + if (nested is { IsStringFamily: true }) { - // Materialise the nested sub-folder lazily under the parent folder (itself lazy), - // so a nested struct that yields no leaf leaves no empty sub-folder behind. - var parentFolder = folder; - var nestedFolder = new Lazy( - () => parentFolder.Value.Folder(memberFullName, member.Name)); - var nestedEmitted = await FanOutDiscoveredUdtMembersAsync( - nestedFolder, deviceHostAddress, memberFullName, nested, parentReadOnly, - depth + 1, emitted, visited, cancellationToken) - .ConfigureAwait(false); - anyEmitted |= nestedEmitted; + // A nested STRING (or user string type) member is one String leaf, not LEN + DATA. + leafType = AbCipDataType.String; + } + else + { + // An array of a UDT has no element-free member path (Parent.Arr.X names no element), + // so it is not recursed into. Otherwise recurse if the depth cap allows it and we're + // not re-entering a struct already on this path (cycle guard). The nested members + // live a level deeper, so the cap is checked against depth + 1. + if (member.ArrayLength > 1) continue; + if (depth + 1 > MaxUdtDepth) continue; + if (!visited.Add(member.Name)) continue; // cyclic UDT reference — stop here + if (nested is not null) + { + // Materialise the nested sub-folder lazily under the parent folder (itself lazy), + // so a nested struct that yields no leaf leaves no empty sub-folder behind. + var parentFolder = folder; + var nestedFolder = new Lazy( + () => parentFolder.Value.Folder(memberFullName, member.Name)); + var nestedEmitted = await FanOutDiscoveredUdtMembersAsync( + nestedFolder, deviceHostAddress, memberFullName, nested, parentReadOnly, + depth + 1, emitted, visited, cancellationToken) + .ConfigureAwait(false); + anyEmitted |= nestedEmitted; + } + visited.Remove(member.Name); + continue; } - visited.Remove(member.Name); - continue; } // Atomic leaf — emit one Variable. Dedup by full-name within the Discovered folder. @@ -1420,7 +1454,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var memberIsArray = member.ArrayLength > 1; folder.Value.Variable(member.Name, member.Name, new DriverAttributeInfo( FullName: memberFullName, - DriverDataType: member.DataType.ToDriverDataType(), + DriverDataType: leafType.ToDriverDataType(), IsArray: memberIsArray, ArrayDim: memberIsArray ? (uint)Math.Max(1, member.ArrayLength) : null, // The Symbol Object carries no per-member ExternalAccess bits, so members inherit the @@ -1431,7 +1465,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, IsHistorized: false, IsAlarm: false, WriteIdempotent: false, - NativeDataType: member.DataType.ToString())); + NativeDataType: leafType.ToString())); anyEmitted = true; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs index 81cda88d..a6cb34ee 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTemplateCache.cs @@ -57,7 +57,19 @@ public sealed class AbCipTemplateCache public sealed record AbCipUdtShape( string TypeName, int TotalSize, - IReadOnlyList Members); + IReadOnlyList Members) +{ + /// + /// true when the shape is a Logix string-family type — the built-in STRING or + /// a user string type such as STR0016: exactly a LEN DINT at offset 0 and a + /// DATA SINT array at offset 4. A controller reports every string tag as a struct of + /// this shape, but it is read and presented as one String value, never as LEN + DATA leaves. + /// + public bool IsStringFamily => + Members.Count == 2 + && Members[0] is { Name: "LEN", DataType: AbCipDataType.DInt, Offset: 0 } + && Members[1] is { Name: "DATA", DataType: AbCipDataType.SInt, Offset: 4, ArrayLength: > 1 }; +} /// One member of a Logix UDT. /// Member name as reported by the Template Object. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipTemplateObjectDecoder.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipTemplateObjectDecoder.cs index 1bce8e91..7649ff48 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipTemplateObjectDecoder.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipTemplateObjectDecoder.cs @@ -4,103 +4,101 @@ using System.Text; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// -/// Decoder for the CIP Template Object (class 0x6C) blob returned by a Read Template -/// service. Produces an describing the UDT's name, total size, -/// + ordered member list with per-member offset + type + array length. +/// Decoder for the UDT template buffer libplctag returns for an @udt/<id> tag (CIP +/// Template Object, class 0x6C). Produces an describing the UDT's +/// name, total size, + ordered member list with per-member offset + type + array length. /// /// -/// Wire format per Rockwell CIP Vol 1 §5A + Logix 5000 CIP Programming Manual -/// 1756-PM019 §"Template Object", cross-checked against libplctag's ab/cip.c -/// handle_read_template_reply: +/// The layout below is what libplctag hands back from Tag.GetBuffer() — its own +/// header followed by the controller's Read Template reply — matching libplctag's +/// list_tags_logix example and pinned by real captures in +/// CipTemplateObjectWireCaptureTests (CompactLogix 5069-L320ERS2 rev 34.11). An earlier +/// version assumed a 12-byte header led by the member count and [info][array][offset] +/// blocks; every real buffer decoded to null under that assumption. /// -/// Header (fixed-size, little-endian): +/// Header (14 bytes, little-endian): /// -/// u16Member count. -/// u16Struct handle (opaque id). +/// u16Template instance id. +/// u32Member-definition size in 32-bit words — not used here. /// u32Instance size — bytes per structure instance. -/// u32Member-definition total size — not used here. +/// u16Member count. +/// u16Struct handle (opaque). /// /// /// Then member_count member blocks (8 bytes each): /// -/// u16Member info — type code + flags (same encoding -/// as Symbol Object: bit 15 = struct, lower 12 = CIP type code). -/// u16Array size — 0 for scalar members. -/// u32Struct offset — byte offset from struct start. +/// u16Info — the array length for an array member, the bit +/// number for a BOOL packed into a host integer, otherwise 0. +/// u16Type — bit 15 = struct, bit 13 = array, low 12 bits = +/// CIP atomic type code, or for a struct the nested template instance id. +/// u32Byte offset from the struct start. /// /// -/// Then strings: UDT name followed by each member name, each terminated by a -/// semicolon ; followed by a null \0. The UDT name may itself contain the -/// sequence UDTName;0\0 where 0 after the semicolon is an ASCII flag byte. -/// Decoder trims to the first semicolon. +/// Then the strings: the UDT name terminated by ; (followed by an encoded tail that +/// runs to a NUL — discarded), then each member name terminated by a NUL. Anything after the +/// last member name is ignored. /// public static class CipTemplateObjectDecoder { - private const int HeaderSize = 12; // u16 + u16 + u32 + u32 - private const int MemberBlockSize = 8; // u16 + u16 + u32 + private const int HeaderSize = 14; // u16 id + u32 def size + u32 instance size + u16 count + u16 handle + private const int MemberBlockSize = 8; // u16 info + u16 type + u32 offset - private const ushort MemberInfoStructFlag = 0x8000; - private const ushort MemberInfoTypeCodeMask = 0x0FFF; + private const ushort TypeStructFlag = 0x8000; + private const ushort TypeArrayFlag = 0x2000; + private const ushort TypeCodeMask = 0x0FFF; /// - /// Decode the raw Template Object blob. Returns null when the header indicates - /// zero members or the buffer is too short to hold the fixed header. + /// Decode the raw template buffer. Returns null when the header indicates zero + /// members or the buffer is too short to hold the header + member blocks + type name. /// - /// The raw Template Object buffer to decode. + /// The raw template buffer to decode. /// An AbCipUdtShape describing the structure, or null if decoding fails. public static AbCipUdtShape? Decode(byte[] buffer) { ArgumentNullException.ThrowIfNull(buffer); if (buffer.Length < HeaderSize) return null; - var memberCount = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(0)); - // bytes 2-3: struct handle — opaque, not needed for the shape record - var instanceSize = BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(4)); - // bytes 8-11: member-definition total size — inferred from names list instead + // bytes 0-1: template instance id; bytes 2-5: definition size in words — neither needed. + var instanceSize = BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(6)); + var memberCount = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(10)); + // bytes 12-13: struct handle — opaque. if (memberCount == 0) return null; - var memberBlocksOffset = HeaderSize; - var namesOffset = memberBlocksOffset + MemberBlockSize * memberCount; + var namesOffset = HeaderSize + (MemberBlockSize * memberCount); if (namesOffset > buffer.Length) return null; - var stringsSpan = buffer.AsSpan(namesOffset); - var names = ParseSemicolonTerminatedStrings(stringsSpan); - if (names.Count == 0) return null; - - // Strings layout: UDT name first, then one per member (in the same order as the - // member-info blocks). Always consume the first entry as the UDT name; missing - // trailing member names get placeholders below. - var udtName = names[0]; - var memberNames = names.Skip(1).ToArray(); + var strings = ParseTemplateStrings(buffer.AsSpan(namesOffset), memberCount); + if (strings.Count == 0) return null; + var udtName = strings[0]; var members = new List(memberCount); for (var i = 0; i < memberCount; i++) { - var blockOffset = memberBlocksOffset + (i * MemberBlockSize); + var blockOffset = HeaderSize + (i * MemberBlockSize); var info = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(blockOffset)); - var arraySize = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(blockOffset + 2)); + var type = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(blockOffset + 2)); var offset = (int)BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(blockOffset + 4)); - var isStruct = (info & MemberInfoStructFlag) != 0; - // Scalar path only — Rockwell primitive type codes (0xC1-0xD0) fit a byte. Do NOT use for a - // struct member: a nested template id can exceed a byte (see nestedTemplateId below). - var typeCode = (byte)(info & MemberInfoTypeCodeMask); + var isStruct = (type & TypeStructFlag) != 0; + var isArray = (type & TypeArrayFlag) != 0; + // Atomic CIP type codes (0xC1-0xD0) fit a byte; for a struct the low 12 bits are the nested + // template id, which can exceed a byte, so it is taken from the full 12-bit mask below. var dataType = isStruct ? AbCipDataType.Structure - : (CipSymbolObjectDecoder.MapTypeCode(typeCode) ?? AbCipDataType.Structure); + : (CipSymbolObjectDecoder.MapTypeCode((byte)(type & TypeCodeMask)) ?? AbCipDataType.Structure); + var nestedTemplateId = isStruct ? (uint?)(type & TypeCodeMask) : null; - // For a struct member the low 12 bits are the nested UDT's template instance id (same encoding as the - // Symbol Object), NOT a primitive type code — capture it so the nested shape can be fetched. Use the - // FULL 12-bit mask (not the byte-cast typeCode, which truncates a 12-bit id). - var nestedTemplateId = isStruct ? (uint?)(info & MemberInfoTypeCodeMask) : null; + // info is the element count only for an array member; for a BOOL it is the bit number, + // which must never be read as a length. + var arrayLength = isArray && info > 0 ? info : 1; - var memberName = i < memberNames.Length ? memberNames[i] : $""; + var memberName = i + 1 < strings.Count ? strings[i + 1] : $""; members.Add(new AbCipUdtMember( Name: memberName, Offset: offset, DataType: dataType, - ArrayLength: arraySize == 0 ? 1 : arraySize, + ArrayLength: arrayLength, NestedTemplateId: nestedTemplateId)); } @@ -111,41 +109,30 @@ public static class CipTemplateObjectDecoder } /// - /// Walk a span of NAME;\0NAME;\0… byte sequences. Splits at each semicolon — - /// the null byte after each semicolon is optional padding per Rockwell's string - /// encoding convention. Stops at a trailing null / end of buffer. + /// Split the template string section into the UDT name (cut at its first ;) followed + /// by up to NUL-terminated member names. Stops at the end of + /// the buffer; trailing bytes past the last member name are ignored. /// - /// The byte span containing semicolon-terminated strings. - /// List of parsed strings, one per name in the span. - internal static List ParseSemicolonTerminatedStrings(ReadOnlySpan span) + /// The string section, starting right after the last member block. + /// Member names to read after the UDT name. + /// The UDT name followed by the member names found. + internal static List ParseTemplateStrings(ReadOnlySpan span, int memberCount) { - var result = new List(); - var start = 0; - for (var i = 0; i < span.Length; i++) + var result = new List(memberCount + 1); + var pos = 0; + while (pos < span.Length && result.Count < memberCount + 1) { - var b = span[i]; - if (b == ';') + var nul = span[pos..].IndexOf((byte)0); + var end = nul < 0 ? span.Length : pos + nul; + var raw = span[pos..end]; + if (result.Count == 0) { - if (i > start) - result.Add(Encoding.ASCII.GetString(span[start..i])); - // Skip the optional null/space padding following the semicolon. - while (i + 1 < span.Length && (span[i + 1] == '\0' || span[i + 1] == ' ')) - i++; - start = i + 1; + // The type name carries an encoded tail after ';' ("STRING;nEAIA"). + var semi = raw.IndexOf((byte)';'); + if (semi >= 0) raw = raw[..semi]; } - else if (b == 0 && start == i) - { - // Trailing null at a string boundary — done. - break; - } - } - // Trailing name without a semicolon (unlikely but observed on some firmwares). - if (start < span.Length) - { - var zeroAt = span[start..].IndexOf((byte)0); - var end = zeroAt < 0 ? span.Length : start + zeroAt; - if (end > start) - result.Add(Encoding.ASCII.GetString(span[start..end])); + result.Add(Encoding.ASCII.GetString(raw)); + pos = end + 1; } return result; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs index 439a5bda..404ef00c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDriverDiscoveryTests.cs @@ -603,6 +603,110 @@ public sealed class AbCipDriverDiscoveryTests cache.Count.ShouldBe(0); } + // ---- real-controller shapes (captures from a CompactLogix 5069-L320ERS2 rev 34.11) ---- + + private static async Task DiscoverWithTemplatesAsync( + Dictionary templates, params AbCipDiscoveredTag[] tags) + { + var builder = new RecordingBuilder(); + var readers = new IdKeyedTemplateReaderFactory(); + foreach (var (id, blob) in templates) readers.Templates[id] = blob; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions("ab://10.0.0.5/1,0")], + EnableControllerBrowse = true, + }, "drv-1", enumeratorFactory: new FakeEnumeratorFactory(tags), templateReaderFactory: readers); + await drv.InitializeAsync("{}", CancellationToken.None); + await drv.DiscoverAsync(builder, CancellationToken.None); + return builder; + } + + /// + /// A controller reports every STRING tag as a struct (symbol type 0x8FCE → template 0x0FCE, + /// LEN + DATA). Browse must present it as the one String value it is — the read path already + /// reads it as a String — not as a folder holding a DINT and an SINT[82]. + /// + [Fact] + public async Task Discovered_STRING_struct_emits_one_String_variable_not_LEN_and_DATA() + { + var builder = await DiscoverWithTemplatesAsync( + new() { [0x0FCE] = Convert.FromHexString(CipTemplateObjectWireCaptureTests.StringTemplate) }, + new AbCipDiscoveredTag("WW_WorkOrderNum", null, AbCipDataType.Structure, ReadOnly: false, + TemplateInstanceId: 0x0FCE)); + + var v = builder.Variables.ShouldHaveSingleItem(); + v.Info.FullName.ShouldBe("WW_WorkOrderNum"); + v.Info.DriverDataType.ShouldBe(DriverDataType.String); + v.Info.IsArray.ShouldBeFalse(); + builder.Folders.ShouldNotContain(f => f.BrowseName == "WW_WorkOrderNum"); + } + + /// A nested STRING member of a UDT is one String leaf, addressable as Parent.Member. + [Fact] + public async Task Nested_STRING_member_emits_a_String_leaf() + { + var builder = await DiscoverWithTemplatesAsync( + new() + { + [3386] = Convert.FromHexString(CipTemplateObjectWireCaptureTests.LookupMemberTemplate), + [0x0FCE] = Convert.FromHexString(CipTemplateObjectWireCaptureTests.StringTemplate), + }, + new AbCipDiscoveredTag("Lookup", null, AbCipDataType.Structure, ReadOnly: false, + TemplateInstanceId: 3386)); + + var byName = builder.Variables.ToDictionary(v => v.Info.FullName, v => v.Info); + byName.Keys.ShouldBe(["Lookup.Code", "Lookup.Desc", "Lookup.DataType", "Lookup.Lock"], ignoreOrder: true); + byName["Lookup.Desc"].DriverDataType.ShouldBe(DriverDataType.String); + byName["Lookup.Code"].DriverDataType.ShouldBe(DriverDataType.Int32); + } + + /// + /// Logix packs BOOL members into hidden host integers (ZZZZZZZZZZAlarms0) and AOIs carry + /// __BitHost00-style backing members. Neither is user data, and a Logix name can never + /// contain a double underscore, so both are dropped from the browse. + /// + [Fact] + public async Task Hidden_BOOL_host_members_are_not_browsed() + { + var alarms = TemplateBufferBuilder.Build("Alarms", 4, + ("ZZZZZZZZZZAlarms0", 0xC2, 0, 0), + ("_00", 0xC1, 0, 0), + ("__BitHost00", 0xC4, 0, 0)); + + var builder = await DiscoverWithTemplatesAsync( + new() { [3425] = alarms }, + new AbCipDiscoveredTag("Alarms", null, AbCipDataType.Structure, ReadOnly: false, + TemplateInstanceId: 3425)); + + builder.Variables.Select(v => v.Info.FullName).ShouldBe(["Alarms._00"]); + } + + /// + /// An ARRAY of a UDT is not fanned out into Tag.Member leaves — those paths name no + /// element and every read of them would fail. An array of STRING is a String array. + /// + [Fact] + public async Task Discovered_struct_arrays_do_not_fan_out_and_STRING_arrays_are_String_arrays() + { + var udt = TemplateBufferBuilder.Build("Rec", 8, ("A", 0xC4, 0, 0), ("B", 0xCA, 0, 4)); + var builder = await DiscoverWithTemplatesAsync( + new() + { + [0x0FCE] = Convert.FromHexString(CipTemplateObjectWireCaptureTests.StringTemplate), + [0x55] = udt, + }, + new AbCipDiscoveredTag("arrMessages", null, AbCipDataType.Structure, ReadOnly: false, + ElementCount: 50, IsArray: true, TemplateInstanceId: 0x0FCE), + new AbCipDiscoveredTag("Stored", null, AbCipDataType.Structure, ReadOnly: false, + ElementCount: 255, IsArray: true, TemplateInstanceId: 0x55)); + + builder.Variables.ShouldNotContain(v => v.Info.FullName.StartsWith("Stored.")); + var messages = builder.Variables.Single(v => v.Info.FullName == "arrMessages").Info; + messages.DriverDataType.ShouldBe(DriverDataType.String); + messages.IsArray.ShouldBeTrue(); + messages.ArrayDim.ShouldBe(50u); + } + // ---- helpers ---- /// Test implementation of IAddressSpaceBuilder that records calls. @@ -714,34 +818,11 @@ public sealed class AbCipDriverDiscoveryTests } /// - /// Build a minimal CIP Template Object blob can decode - /// into an . Mirrors the helper in - /// AbCipFetchUdtShapeTests: 12-byte header + 8-byte member blocks + semicolon/NUL - /// terminated UDT-then-member name strings. + /// Build a template buffer in libplctag's real @udt/id layout via + /// . info is the member's CIP type word; arr + /// > 0 marks an array member. /// private static byte[] BuildSimpleTemplate( string name, uint instanceSize, params (string n, ushort info, ushort arr, uint off)[] members) - { - const int headerSize = 12; - const int blockSize = 8; - var strings = new MemoryStream(); - void Add(string s) { var b = Encoding.ASCII.GetBytes(s + ";\0"); strings.Write(b, 0, b.Length); } - Add(name); - foreach (var m in members) Add(m.n); - var stringsArr = strings.ToArray(); - - var buf = new byte[headerSize + blockSize * members.Length + stringsArr.Length]; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(0), (ushort)members.Length); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(2), 0x1234); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(4), instanceSize); - for (var i = 0; i < members.Length; i++) - { - var o = headerSize + i * blockSize; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o), members[i].info); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o + 2), members[i].arr); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(o + 4), members[i].off); - } - Buffer.BlockCopy(stringsArr, 0, buf, headerSize + blockSize * members.Length, stringsArr.Length); - return buf; - } + => TemplateBufferBuilder.Build(name, instanceSize, [.. members.Select(m => (m.n, m.info, m.arr, m.off))]); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFetchUdtShapeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFetchUdtShapeTests.cs index f56e5893..c2a2c642 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFetchUdtShapeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipFetchUdtShapeTests.cs @@ -60,30 +60,14 @@ public sealed class AbCipFetchUdtShapeTests } } - private static byte[] BuildSimpleTemplate(string name, uint instanceSize, params (string n, ushort info, ushort arr, uint off)[] members) - { - var headerSize = 12; - var blockSize = 8; - var strings = new MemoryStream(); - void Add(string s) { var b = Encoding.ASCII.GetBytes(s + ";\0"); strings.Write(b, 0, b.Length); } - Add(name); - foreach (var m in members) Add(m.n); - var stringsArr = strings.ToArray(); - - var buf = new byte[headerSize + blockSize * members.Length + stringsArr.Length]; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(0), (ushort)members.Length); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(2), 0x1234); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(4), instanceSize); - for (var i = 0; i < members.Length; i++) - { - var o = headerSize + i * blockSize; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o), members[i].info); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o + 2), members[i].arr); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(o + 4), members[i].off); - } - Buffer.BlockCopy(stringsArr, 0, buf, headerSize + blockSize * members.Length, stringsArr.Length); - return buf; - } + /// + /// Build a template buffer in libplctag's real @udt/id layout via + /// . info is the member's CIP type word; arr + /// > 0 marks an array member. + /// + private static byte[] BuildSimpleTemplate( + string name, uint instanceSize, params (string n, ushort info, ushort arr, uint off)[] members) + => TemplateBufferBuilder.Build(name, instanceSize, [.. members.Select(m => (m.n, m.info, m.arr, m.off))]); private static Task InvokeFetch(AbCipDriver drv, string deviceHostAddress, uint templateId) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectDecoderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectDecoderTests.cs index aeb5a987..39600b54 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectDecoderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectDecoderTests.cs @@ -10,45 +10,15 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; public sealed class CipTemplateObjectDecoderTests { /// - /// Construct a Template Object blob — header + member blocks + semicolon-delimited - /// strings (UDT name first, then member names). + /// Construct a template buffer in libplctag's real layout (see ). + /// info here is the member's CIP type word; arraySize > 0 marks an array member. /// private static byte[] BuildTemplate( string udtName, uint instanceSize, params (string name, ushort info, ushort arraySize, uint offset)[] members) - { - var memberCount = (ushort)members.Length; - var headerSize = 12; - var memberBlockSize = 8; - var blocksSize = memberBlockSize * members.Length; - - var stringsBuf = new MemoryStream(); - void AppendString(string s) - { - var bytes = Encoding.ASCII.GetBytes(s + ";\0"); - stringsBuf.Write(bytes, 0, bytes.Length); - } - AppendString(udtName); - foreach (var m in members) AppendString(m.name); - var strings = stringsBuf.ToArray(); - - var buf = new byte[headerSize + blocksSize + strings.Length]; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(0), memberCount); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(2), 0x1234); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(4), instanceSize); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(8), 0); - - for (var i = 0; i < members.Length; i++) - { - var o = headerSize + (i * memberBlockSize); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o), members[i].info); - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o + 2), members[i].arraySize); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(o + 4), members[i].offset); - } - Buffer.BlockCopy(strings, 0, buf, headerSize + blocksSize, strings.Length); - return buf; - } + => TemplateBufferBuilder.Build(udtName, instanceSize, + [.. members.Select(m => (m.name, m.info, m.arraySize, m.offset))]); /// Verifies that a simple UDT with two members decodes correctly. [Fact] @@ -161,8 +131,9 @@ public sealed class CipTemplateObjectDecoderTests [Fact] public void Zero_member_count_returns_null() { - var buf = new byte[12]; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(0), 0); + var buf = new byte[14]; + BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(6), 8); // instance size + BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(10), 0); // member count CipTemplateObjectDecoder.Decode(buf).ShouldBeNull(); } @@ -178,19 +149,10 @@ public sealed class CipTemplateObjectDecoderTests public void Missing_member_name_surfaces_placeholder() { // Header says 3 members but strings list has only UDT name + 2 member names. - var memberCount = (ushort)3; - var buf = new byte[12 + 8 * 3]; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(0), memberCount); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(4), 12); - for (var i = 0; i < 3; i++) - { - var o = 12 + i * 8; - BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(o), 0xC4); - BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(o + 4), (uint)(i * 4)); - } - // strings: only UDT + 2 members, missing the third. - var strings = Encoding.ASCII.GetBytes("MyUdt;\0A;\0B;\0"); - var combined = buf.Concat(strings).ToArray(); + var full = TemplateBufferBuilder.Build("MyUdt", 12, + ("A", 0xC4, 0, 0), ("B", 0xC4, 0, 4), ("C", 0xC4, 0, 8)); + // Drop the trailing "C\0" so only two member names remain. + var combined = full[..^2]; var shape = CipTemplateObjectDecoder.Decode(combined); shape.ShouldNotBeNull(); @@ -198,19 +160,20 @@ public sealed class CipTemplateObjectDecoderTests shape.Members[2].Name.ShouldBe(""); } - /// Verifies that semicolon-terminated string parsing handles various input shapes. - /// The raw semicolon-terminated string input. - /// The expected array of parsed strings. + /// Verifies the template string section splits into the type name + NUL-terminated member names. + /// The raw string section. + /// Member names to read after the type name. + /// The expected type name followed by member names. [Theory] - [InlineData("Foo;\0Bar;\0", new[] { "Foo", "Bar" })] - [InlineData("Foo;Bar;", new[] { "Foo", "Bar" })] // no nulls - [InlineData("Only;\0", new[] { "Only" })] - [InlineData(";\0", new string[] { })] // empty - [InlineData("", new string[] { })] - public void ParseSemicolonTerminatedStrings_handles_shapes(string input, string[] expected) + [InlineData("STRING;nEAIA\0LEN\0DATA\0", 2, new[] { "STRING", "LEN", "DATA" })] + [InlineData("Udt;n\0A\0B\0\0\u0001junk", 2, new[] { "Udt", "A", "B" })] // trailing bytes ignored + [InlineData("Udt\0A\0", 1, new[] { "Udt", "A" })] // no ';' tail + [InlineData("Udt;nEA\0A\0", 3, new[] { "Udt", "A" })] // fewer names than members + [InlineData("", 1, new string[] { })] + public void ParseTemplateStrings_handles_shapes(string input, int memberCount, string[] expected) { var bytes = Encoding.ASCII.GetBytes(input); - var result = CipTemplateObjectDecoder.ParseSemicolonTerminatedStrings(bytes); + var result = CipTemplateObjectDecoder.ParseTemplateStrings(bytes, memberCount); result.ShouldBe(expected); } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectWireCaptureTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectWireCaptureTests.cs new file mode 100644 index 00000000..8ebd336b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipTemplateObjectWireCaptureTests.cs @@ -0,0 +1,98 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Decodes @udt/<id> template buffers captured byte for byte off a real controller — +/// a CompactLogix 5069-L320ERS2 rev 34.11, read through libplctag 1.5.2's Tag.GetBuffer() +/// on 2026-10-07 (read-only; one template read per id). +/// +/// +/// +/// Every pre-existing template test built its own input in the layout the decoder assumed +/// (u16 member count first, a 12-byte header, [info][array][offset] blocks, names +/// each ending ;\0). A real buffer agrees with none of that, so the decoder returned +/// null for every UDT a controller has and discovered UDTs never fanned out. The +/// same builder-shares-the-parser's-assumption trap as #468. +/// +/// +/// What libplctag actually hands back for @udt/id (cross-checked against its own +/// list_tags_logix example): a 14-byte header — u16 template id, u32 member-definition +/// size in 32-bit words, u32 instance size in bytes, u16 member count, u16 struct handle — +/// then one 8-byte block per member: u16 info (array length, or the bit number of a +/// BOOL), u16 type (bit 15 = struct, bit 13 = array, low 12 bits = CIP type code or +/// the nested template id), u32 byte offset. Then the strings: the type name ending at +/// ; (followed by an encoded tail up to a NUL), then each member name NUL-terminated. +/// +/// +[Trait("Category", "Unit")] +public sealed class CipTemplateObjectWireCaptureTests +{ + /// The built-in STRING template (id 0x0FCE): LEN DINT + DATA SINT[82], 88 bytes. + internal const string StringTemplate = + "CE0F10000000580000000200CE0F0000C400000000005200C22004000000" + + "535452494E473B6E45414941004C454E004441544100000001000200"; + + /// + /// Rockwell library UDT raC_UDT_LookupMember_STR0082 (id 3386): DINT, a nested + /// STRING member (struct type 0x8FCE), SINT, BOOL. Trailing bytes after the last name are + /// part of the capture and must be ignored. + /// + internal const string LookupMemberTemplate = + "3A0D1F000000600000000400B3810000C400000000000000CE8F040000000000C2005C0000000000C1005C000000" + + "7261435F5544545F4C6F6F6B75704D656D6265725F535452303038323B6E45414141454245430043" + + "6F64650044657363004461746154797065004C6F636B00008F92361DA1314013"; + + private static AbCipUdtShape DecodeHex(string hex) + { + var shape = CipTemplateObjectDecoder.Decode(Convert.FromHexString(hex)); + shape.ShouldNotBeNull(); + return shape; + } + + /// The STRING template decodes to its two members with the real array length. + [Fact] + public void Captured_STRING_template_decodes_LEN_and_DATA() + { + var shape = DecodeHex(StringTemplate); + + shape.TypeName.ShouldBe("STRING"); + shape.TotalSize.ShouldBe(88); + shape.Members.Select(m => m.Name).ShouldBe(["LEN", "DATA"]); + shape.Members[0].DataType.ShouldBe(AbCipDataType.DInt); + shape.Members[0].Offset.ShouldBe(0); + shape.Members[0].ArrayLength.ShouldBe(1); + shape.Members[1].DataType.ShouldBe(AbCipDataType.SInt); + shape.Members[1].Offset.ShouldBe(4); + shape.Members[1].ArrayLength.ShouldBe(82); + } + + /// A nested struct member carries its template id; trailing bytes are ignored. + [Fact] + public void Captured_library_UDT_decodes_nested_STRING_member_and_ignores_trailing_bytes() + { + var shape = DecodeHex(LookupMemberTemplate); + + shape.TypeName.ShouldBe("raC_UDT_LookupMember_STR0082"); + shape.TotalSize.ShouldBe(96); + shape.Members.Select(m => m.Name).ShouldBe(["Code", "Desc", "DataType", "Lock"]); + shape.Members[0].DataType.ShouldBe(AbCipDataType.DInt); + shape.Members[1].DataType.ShouldBe(AbCipDataType.Structure); + shape.Members[1].NestedTemplateId.ShouldBe(0x0FCEu); + shape.Members[1].Offset.ShouldBe(4); + shape.Members[2].DataType.ShouldBe(AbCipDataType.SInt); + shape.Members[2].Offset.ShouldBe(92); + shape.Members[3].DataType.ShouldBe(AbCipDataType.Bool); + shape.Members.ShouldAllBe(m => m.ArrayLength == 1); + } + + /// A shape is recognised as a Logix string family (LEN DINT + DATA SINT[n]). + [Fact] + public void Captured_STRING_template_is_a_string_family_shape_and_a_plain_UDT_is_not() + { + DecodeHex(StringTemplate).IsStringFamily.ShouldBeTrue(); + DecodeHex(LookupMemberTemplate).IsStringFamily.ShouldBeFalse(); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/TemplateBufferBuilder.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/TemplateBufferBuilder.cs new file mode 100644 index 00000000..f699267e --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/TemplateBufferBuilder.cs @@ -0,0 +1,45 @@ +using System.Buffers.Binary; +using System.Text; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Builds an @udt/<id> template buffer in the layout libplctag really returns — +/// the layout pinned by the real-controller captures in CipTemplateObjectWireCaptureTests. +/// Keep it in step with those captures, never with the decoder: a builder that restates the +/// decoder's assumption proves nothing (#468, and the template decoder after it). +/// +internal static class TemplateBufferBuilder +{ + /// Build a template buffer. + /// UDT type name (written as name;n + an encoded tail, as a controller does). + /// Bytes per instance. + /// Per member: name, CIP type word (atomic code, or 0x8000 | nestedTemplateId), + /// array length (0 = scalar; > 0 sets the array flag and the info word), byte offset. + /// The buffer. + public static byte[] Build( + string udtName, uint instanceSize, params (string name, ushort type, ushort arraySize, uint offset)[] members) + { + const int header = 14; + var buf = new MemoryStream(); + var head = new byte[header + (8 * members.Length)]; + BinaryPrimitives.WriteUInt16LittleEndian(head.AsSpan(0), 0x0123); // template id + BinaryPrimitives.WriteUInt32LittleEndian(head.AsSpan(2), 0); // def size (words) + BinaryPrimitives.WriteUInt32LittleEndian(head.AsSpan(6), instanceSize); + BinaryPrimitives.WriteUInt16LittleEndian(head.AsSpan(10), (ushort)members.Length); + BinaryPrimitives.WriteUInt16LittleEndian(head.AsSpan(12), 0xABCD); // struct handle + for (var i = 0; i < members.Length; i++) + { + var o = header + (i * 8); + var (_, type, arraySize, offset) = members[i]; + var typeWord = arraySize > 0 ? (ushort)(type | 0x2000) : type; + BinaryPrimitives.WriteUInt16LittleEndian(head.AsSpan(o), arraySize); + BinaryPrimitives.WriteUInt16LittleEndian(head.AsSpan(o + 2), typeWord); + BinaryPrimitives.WriteUInt32LittleEndian(head.AsSpan(o + 4), offset); + } + buf.Write(head); + buf.Write(Encoding.ASCII.GetBytes(udtName + ";nEAIA\0")); + foreach (var m in members) buf.Write(Encoding.ASCII.GetBytes(m.name + "\0")); + return buf.ToArray(); + } +} From 28bbb5e5e2821cb3979f2a792235a8242ae84226 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:06:48 -0400 Subject: [PATCH 2/7] fix(abcip): browse program-scoped tags and surface symbol array dimensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live read-only check against production CompactLogix 5069-L320ERS2 presses: - The controller @tags listing names each program (Program:UN01_Press, a system symbol) but none of its tags; those are listed only by Program:.@tags. The enumerator read @tags alone, so every program-scoped tag was unbrowseable — including the PackML state (Program:UN01_Press.Out_eState) and all 51 alarm bits the Ignition project reads. The enumerator now reads each program's listing and decodes it with that program as scope; an unreadable program listing is skipped, not fatal. - The Symbol Object decoder ignored the dimension bits + dim words, so every discovered array (DINT[32], REAL[32], STRING[50], UDT[23] on the two controllers checked) browsed as a scalar. IsArray/ElementCount are now populated (multi-dim flattened). A 1-D single element entry stays scalar: ab_server reports every scalar that way, a real controller sets no dimension bits on scalars. Pinned by entries captured off the controllers (CipSymbolObjectRealControllerCaptureTests, AbCipProgramScopeEnumerationTests). --- .../CipSymbolObjectDecoder.cs | 69 +++++++++++++-- .../LibplctagTagEnumerator.cs | 71 +++++++++++++--- .../AbCipProgramScopeEnumerationTests.cs | 69 +++++++++++++++ ...pSymbolObjectRealControllerCaptureTests.cs | 85 +++++++++++++++++++ .../CipSymbolObjectWireCaptureTests.cs | 18 ++++ 5 files changed, 294 insertions(+), 18 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProgramScopeEnumerationTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectRealControllerCaptureTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs index aad6a2b1..2bf7f5d0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/CipSymbolObjectDecoder.cs @@ -42,6 +42,10 @@ public static class CipSymbolObjectDecoder private const ushort SymbolTypeSystemFlag = 0x1000; private const ushort SymbolTypeStructFlag = 0x8000; private const ushort SymbolTypeTypeCodeMask = 0x0FFF; + private const int SymbolTypeDimsShift = 13; + private const int SymbolTypeDimsMask = 0x3; + + private const string ProgramPrefix = "Program:"; /// /// Decode the raw @tags blob into an enumerable sequence. Malformed entries at @@ -49,22 +53,44 @@ public static class CipSymbolObjectDecoder /// cleanly before the corruption. /// /// The raw byte buffer from the Symbol Object response. + /// The program this listing belongs to when it is a + /// Program:<name>.@tags read — whose entries carry bare names — or null + /// for the controller-scope @tags listing. /// An enumerable sequence of discovered CIP tags. - public static IEnumerable Decode(byte[] buffer) + public static IEnumerable Decode(byte[] buffer, string? programScope = null) { ArgumentNullException.ThrowIfNull(buffer); - return DecodeImpl(buffer); + return DecodeImpl(buffer, programScope); } - private static IEnumerable DecodeImpl(byte[] buffer) + /// + /// The program name when is a controller-scope program entry + /// (Program:UN01_Press, a system symbol) — the name to read Program:<name>.@tags + /// for, since a controller lists program-scope tags only under their own program. null + /// for anything else. + /// + /// A decoded controller-scope symbol. + /// The program name, or null. + public static string? ProgramNameOf(AbCipDiscoveredTag tag) + { + ArgumentNullException.ThrowIfNull(tag); + if (!tag.IsSystemTag || tag.ProgramScope is not null) return null; + if (!tag.Name.StartsWith(ProgramPrefix, StringComparison.Ordinal)) return null; + var name = tag.Name[ProgramPrefix.Length..]; + return name.Length == 0 || name.Contains('.') ? null : name; + } + + private static IEnumerable DecodeImpl(byte[] buffer, string? listingScope) { var pos = 0; while (pos + FixedHeaderSize <= buffer.Length) { var instanceId = BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(pos)); var symbolType = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(pos + 4)); - // element_length at pos+6 (u16) — useful for array sizing but not surfaced here - // array_dims at pos+8, pos+12, pos+16 — same (scalar-tag case has all zeros) + // element_length at pos+6 (u16) — not surfaced. + var dim0 = BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(pos + 8)); + var dim1 = BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(pos + 12)); + var dim2 = BinaryPrimitives.ReadUInt32LittleEndian(buffer.AsSpan(pos + 16)); var nameLength = BinaryPrimitives.ReadUInt16LittleEndian(buffer.AsSpan(pos + 20)); pos += FixedHeaderSize; @@ -83,7 +109,10 @@ public static class CipSymbolObjectDecoder var isStruct = (symbolType & SymbolTypeStructFlag) != 0; var typeCode = symbolType & SymbolTypeTypeCodeMask; - var (programScope, simpleName) = SplitProgramScope(name); + // A program listing (Program:X.@tags) carries bare names; the scope is the program read. + var (programScope, simpleName) = listingScope is null + ? SplitProgramScope(name) + : (listingScope, name); var dataType = isStruct ? AbCipDataType.Structure : MapTypeCode((byte)typeCode); // For a struct symbol the lower 12 bits (typeCode) are NOT a primitive CIP type code — @@ -92,18 +121,46 @@ public static class CipSymbolObjectDecoder // carry no template id. var templateInstanceId = isStruct ? (uint?)typeCode : null; + var (isArray, elementCount) = ArrayShape((symbolType >> SymbolTypeDimsShift) & SymbolTypeDimsMask, + dim0, dim1, dim2); + yield return new AbCipDiscoveredTag( Name: simpleName, ProgramScope: programScope, DataType: dataType ?? AbCipDataType.Structure, // unknown type code → treat as opaque ReadOnly: false, // Symbol Object doesn't carry write-protection bits; lift via AccessControl Object later IsSystemTag: isSystem, + ElementCount: elementCount, + IsArray: isArray, TemplateInstanceId: templateInstanceId); _ = instanceId; // retained in the wire format for diagnostics; not surfaced to the driver today } } + /// + /// Array shape from the symbol type's dimension count (bits 13-14) and the three dimension + /// words. A real controller sets no dimension bits on a scalar; ab_server reports every + /// scalar as a one-dimensional array of one, so a 1-D single-element entry is taken as a scalar + /// too. A multi-dimensional array is surfaced flattened (element count = product of dims) — + /// the driver addresses 1-D arrays only. + /// + /// Dimension count from bits 13-14 of the symbol type. + /// First dimension. + /// Second dimension. + /// Third dimension. + /// Whether the symbol is an array, and its flattened element count. + internal static (bool IsArray, int ElementCount) ArrayShape(int dimCount, uint dim0, uint dim1, uint dim2) + { + if (dimCount == 0) return (false, 1); + long count = 1; + if (dimCount >= 1) count *= Math.Max(1u, dim0); + if (dimCount >= 2) count *= Math.Max(1u, dim1); + if (dimCount >= 3) count *= Math.Max(1u, dim2); + if (dimCount == 1 && count == 1) return (false, 1); + return (true, (int)Math.Min(count, int.MaxValue)); + } + /// /// Split a Program:MainProgram.StepIndex-style name into its scope + local /// parts. Names without the Program: prefix pass through unchanged. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs index b7ed45f9..e2858385 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagEnumerator.cs @@ -19,29 +19,73 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// internal sealed class LibplctagTagEnumerator : IAbCipTagEnumerator { - private Tag? _tag; + private readonly Func> _readListing; + + /// Production enumerator: reads each listing through libplctag. + public LibplctagTagEnumerator() : this(ReadListingAsync) { } + + /// Enumerator over an injected listing reader (unit-test seam). + /// Reads one symbol listing (@tags or Program:X.@tags) and + /// returns its raw Symbol Object bytes. + internal LibplctagTagEnumerator(Func> readListing) + => _readListing = readListing; /// + /// + /// A controller's @tags listing holds controller-scope tags plus one system entry per + /// program (Program:UN01_Press); the program's own tags are listed only by + /// Program:<name>.@tags. So the controller listing is read first, then each program's + /// listing, decoded with that program as the scope. A program whose listing cannot be read is + /// skipped — browse is best-effort and one unreadable program must not cost the others. + /// public async IAsyncEnumerable EnumerateAsync( AbCipTagCreateParams deviceParams, [EnumeratorCancellation] CancellationToken cancellationToken) { - _tag = BuildSymbolListTag(deviceParams); + var buffer = await _readListing(deviceParams, "@tags", cancellationToken).ConfigureAwait(false); - await _tag.InitializeAsync(cancellationToken).ConfigureAwait(false); - await _tag.ReadAsync(cancellationToken).ConfigureAwait(false); - - var buffer = _tag.GetBuffer(); // The decoder already surfaces each struct tag's CIP template instance id on // AbCipDiscoveredTag.TemplateInstanceId (from the lower 12 bits of the symbol type), so // re-yielding the decoded records as-is carries it through to DiscoverAsync's UDT fan-out. + var programs = new List(); foreach (var tag in CipSymbolObjectDecoder.Decode(buffer)) + { + if (CipSymbolObjectDecoder.ProgramNameOf(tag) is { } program) programs.Add(program); yield return tag; + } + + foreach (var program in programs) + { + byte[] programBuffer; + try + { + programBuffer = await _readListing(deviceParams, $"Program:{program}.@tags", cancellationToken) + .ConfigureAwait(false); + } + catch (OperationCanceledException) { throw; } + catch (Exception) + { + continue; + } + + foreach (var tag in CipSymbolObjectDecoder.Decode(programBuffer, programScope: program)) + yield return tag; + } + } + + private static async Task ReadListingAsync( + AbCipTagCreateParams deviceParams, string listingName, CancellationToken cancellationToken) + { + using var tag = BuildSymbolListTag(deviceParams, listingName); + await tag.InitializeAsync(cancellationToken).ConfigureAwait(false); + await tag.ReadAsync(cancellationToken).ConfigureAwait(false); + return tag.GetBuffer(); } /// - /// Builds the (uninitialized) libplctag tag for the @tags pseudo — same gateway + - /// path as the device, distinguished by the name alone. + /// Builds the (uninitialized) libplctag tag for a symbol listing pseudo-tag — @tags or + /// Program:<name>.@tags — same gateway + path as the device, distinguished by the + /// name alone. /// /// /// Gateway must be (host:port), never the @@ -49,18 +93,21 @@ internal sealed class LibplctagTagEnumerator : IAbCipTagEnumerator /// host, so a browse against a device authored on another port silently returns whatever /// answers on 44818 (Gitea #539, the discovery half of #534). /// - internal static Tag BuildSymbolListTag(AbCipTagCreateParams deviceParams) => new() + /// The device's create params. + /// The listing pseudo-tag name (default @tags). + /// The uninitialized tag. + internal static Tag BuildSymbolListTag(AbCipTagCreateParams deviceParams, string listingName = "@tags") => new() { Gateway = deviceParams.LibplctagGateway, Path = deviceParams.CipPath, PlcType = MapPlcType(deviceParams.LibplctagPlcAttribute), Protocol = Protocol.ab_eip, - Name = "@tags", + Name = listingName, Timeout = deviceParams.Timeout, }; - /// Disposes the enumerator and releases the underlying libplctag tag. - public void Dispose() => _tag?.Dispose(); + /// Nothing to release — each listing's libplctag tag is disposed after its read. + public void Dispose() { } private static PlcType MapPlcType(string attribute) => attribute switch { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProgramScopeEnumerationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProgramScopeEnumerationTests.cs new file mode 100644 index 00000000..53061245 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProgramScopeEnumerationTests.cs @@ -0,0 +1,69 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// A controller's @tags listing names each program (Program:UN01_Press, a system +/// symbol) but none of its tags — those are listed only by Program:<name>.@tags. +/// Seen live on a production press: the PackML state (Program:UN01_Press.Out_eState) and +/// every alarm bit the Ignition project reads are program-scoped, and the browse showed none. +/// +[Trait("Category", "Unit")] +public sealed class AbCipProgramScopeEnumerationTests +{ + // Captured entries (CompactLogix 5069-L320ERS2 rev 34.11): WW_InCycle + the Program:UN01_Press entry + // from @tags, and Out_eState from Program:UN01_Press.@tags. + private const string ControllerListing = + "AF010000C10001000000000000000000000000000A0057575F496E4379636C65" + + "7900000068100000000000000000000000000000120050726F6772616D3A554E30315F5072657373"; + + private const string Un01Listing = "1A000000C40004000000000000000000000000000A004F75745F655374617465"; + + private static readonly AbCipTagCreateParams Params = new( + Gateway: "10.0.0.5", Port: 44818, CipPath: "1,0", LibplctagPlcAttribute: "controllogix", + TagName: "@tags", Timeout: TimeSpan.FromSeconds(2)); + + /// The enumerator reads each program's own listing and yields its tags program-scoped. + [Fact] + public async Task Program_scoped_tags_are_enumerated_from_each_program_listing() + { + var reads = new List(); + using var enumerator = new LibplctagTagEnumerator((_, name, _) => + { + reads.Add(name); + return Task.FromResult(name switch + { + "@tags" => Convert.FromHexString(ControllerListing), + "Program:UN01_Press.@tags" => Convert.FromHexString(Un01Listing), + _ => throw new InvalidOperationException(name), + }); + }); + + var tags = new List(); + await foreach (var t in enumerator.EnumerateAsync(Params, TestContext.Current.CancellationToken)) + tags.Add(t); + + reads.ShouldBe(["@tags", "Program:UN01_Press.@tags"]); + var outState = tags.Single(t => t.Name == "Out_eState"); + outState.ProgramScope.ShouldBe("UN01_Press"); + outState.DataType.ShouldBe(AbCipDataType.DInt); + tags.ShouldContain(t => t.Name == "WW_InCycle" && t.ProgramScope == null); + } + + /// One unreadable program listing does not cost the controller tags or other programs. + [Fact] + public async Task A_failing_program_listing_is_skipped() + { + using var enumerator = new LibplctagTagEnumerator((_, name, _) => name == "@tags" + ? Task.FromResult(Convert.FromHexString(ControllerListing)) + : Task.FromException(new InvalidOperationException("ErrorNotFound"))); + + var tags = new List(); + await foreach (var t in enumerator.EnumerateAsync(Params, TestContext.Current.CancellationToken)) + tags.Add(t); + + tags.ShouldContain(t => t.Name == "WW_InCycle"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectRealControllerCaptureTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectRealControllerCaptureTests.cs new file mode 100644 index 00000000..934ddcbc --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectRealControllerCaptureTests.cs @@ -0,0 +1,85 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Symbol Object (@tags) entries captured byte for byte off two real CompactLogix +/// controllers on 2026-10-07 (read-only, one listing read each): a 5069-L320ERS2 rev 34.11 press +/// and a second CompactLogix. Unlike the ab_server capture in +/// — which reports every scalar as a 1-element +/// array — a real controller sets the dimension bits only on genuine arrays. +/// +[Trait("Category", "Unit")] +public sealed class CipSymbolObjectRealControllerCaptureTests +{ + // Controller scope, press: WW_InCycle (BOOL), Inf_LookupFwkB (UDT[23], template 3386), + // Program:UN01_Press (the program entry itself, type 0x1068). + private const string WwInCycle = "AF010000C10001000000000000000000000000000A0057575F496E4379636C65"; + private const string InfLookupFwkB = "6D0000003AAD60001700000000000000000000000E00496E665F4C6F6F6B757046776B42"; + private const string ProgramUn01 = "7900000068100000000000000000000000000000120050726F6772616D3A554E30315F5072657373"; + + // Controller scope, second controller: arrSysTime (DINT[7]), arrMessages (STRING[50]). + private const string ArrSysTime = "11000000C42004000700000000000000000000000A0061727253797354696D65"; + private const string ArrMessages = "61000000CEAF58003200000000000000000000000B006172724D65737361676573"; + + // Program:UN01_Press.@tags: names arrive WITHOUT the Program: prefix. + private const string OutEState = "1A000000C40004000000000000000000000000000A004F75745F655374617465"; + private const string DisableStates = "4C000000C42004002000000000000000000000000E005F44697361626C65537461746573"; + + private static List Decode(params string[] entries) => + CipSymbolObjectDecoder.Decode(Convert.FromHexString(string.Concat(entries))).ToList(); + + /// Array dimensions in the entry surface as IsArray + ElementCount; scalars stay scalar. + [Fact] + public void Real_controller_arrays_surface_their_element_count() + { + var tags = Decode(WwInCycle, InfLookupFwkB, ArrSysTime, ArrMessages); + + var inCycle = tags.Single(t => t.Name == "WW_InCycle"); + inCycle.IsArray.ShouldBeFalse(); + inCycle.ElementCount.ShouldBe(1); + + var sysTime = tags.Single(t => t.Name == "arrSysTime"); + sysTime.DataType.ShouldBe(AbCipDataType.DInt); + sysTime.IsArray.ShouldBeTrue(); + sysTime.ElementCount.ShouldBe(7); + + var lookup = tags.Single(t => t.Name == "Inf_LookupFwkB"); + lookup.DataType.ShouldBe(AbCipDataType.Structure); + lookup.TemplateInstanceId.ShouldBe(3386u); + lookup.IsArray.ShouldBeTrue(); + lookup.ElementCount.ShouldBe(23); + + var messages = tags.Single(t => t.Name == "arrMessages"); + messages.TemplateInstanceId.ShouldBe(0x0FCEu); + messages.IsArray.ShouldBeTrue(); + messages.ElementCount.ShouldBe(50); + } + + /// A program entry is a system tag whose program name the enumerator can walk. + [Fact] + public void Program_entry_is_system_and_names_its_program() + { + var program = Decode(ProgramUn01).ShouldHaveSingleItem(); + + program.IsSystemTag.ShouldBeTrue(); + CipSymbolObjectDecoder.ProgramNameOf(program).ShouldBe("UN01_Press"); + CipSymbolObjectDecoder.ProgramNameOf(Decode(WwInCycle).Single()).ShouldBeNull(); + } + + /// A program listing decodes with the program as each tag's scope. + [Fact] + public void Program_listing_decodes_with_program_scope() + { + var tags = CipSymbolObjectDecoder.Decode( + Convert.FromHexString(OutEState + DisableStates), programScope: "UN01_Press").ToList(); + + tags.Select(t => t.Name).ShouldBe(["Out_eState", "_DisableStates"]); + tags.ShouldAllBe(t => t.ProgramScope == "UN01_Press"); + tags[0].DataType.ShouldBe(AbCipDataType.DInt); + tags[1].IsArray.ShouldBeTrue(); + tags[1].ElementCount.ShouldBe(32); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs index b8fdea96..e536f7c4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/CipSymbolObjectWireCaptureTests.cs @@ -100,6 +100,24 @@ public sealed class CipSymbolObjectWireCaptureTests tags.Single(t => t.Name == "TestSINT").DataType.ShouldBe(AbCipDataType.SInt); } + /// + /// ab_server marks every symbol as a one-dimensional array (a scalar is + /// [1]); only TestArray has more than one element. A 1-element, 1-D entry is + /// treated as a scalar, so the fixture's scalars do not browse as 1-element arrays — a + /// real controller sets no dimension bits on a scalar at all + /// (). + /// + [Fact] + public void Only_the_multi_element_capture_entry_is_an_array() + { + var tags = CipSymbolObjectDecoder.Decode(Capture()).ToList(); + + var array = tags.Single(t => t.Name == "TestArray"); + array.IsArray.ShouldBeTrue(); + array.ElementCount.ShouldBe(16); + tags.Where(t => t.Name != "TestArray").ShouldAllBe(t => !t.IsArray && t.ElementCount == 1); + } + /// None of the captured symbols is misread as a system tag or a structure. /// /// The dimension bits sit next to the flags the decoder tests for, so a mask that is one From a0a024c80b5b430727d53c1239619550ed865b45 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:07:49 -0400 Subject: [PATCH 3/7] fix(abcip): accept module-defined I/O tag paths (Local:1:I.Data, ABB_OmniCore:I.X) Live read-only check: a production CompactLogix lists the module tag ABB_OmniCore:I in @tags and libplctag reads ABB_OmniCore:I.ConnectionFaulted without complaint, but the driver refused the path as malformed (identifier rule had no colon) and reported the tag BadCommunicationError. Colon-separated identifier/slot parts are now legal in the controller-scope base segment only (never past it, never under Program: scope). --- .../AbCipTagPath.cs | 24 ++++++++++++- .../AbCipTagPathTests.cs | 35 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs index d97231a2..5084e201 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagPath.cs @@ -100,7 +100,10 @@ public sealed record AbCipTagPath( var bracketIdx = part.IndexOf('['); if (bracketIdx < 0) { - if (!IsValidIdent(part)) return null; + // The controller-scope base name of a module-defined I/O tag carries colons + // (Local:1:I, ABB_OmniCore:I) — legal in the first segment only. + var isModuleBase = segments.Count == 0 && programScope is null && IsValidModuleBase(part); + if (!isModuleBase && !IsValidIdent(part)) return null; segments.Add(new AbCipTagPathSegment(part, [])); continue; } @@ -122,6 +125,25 @@ public sealed record AbCipTagPath( return new AbCipTagPath(programScope, segments, bitIndex); } + /// + /// true for a module-defined I/O tag base name: an identifier followed by one or more + /// :-separated identifier or slot-number parts (Local:1:I, ABB_OmniCore:I). + /// + /// The candidate base name. + /// true when is a module tag base name. + private static bool IsValidModuleBase(string s) + { + var parts = s.Split(':'); + if (parts.Length < 2 || !IsValidIdent(parts[0])) return false; + for (var i = 1; i < parts.Length; i++) + { + var p = parts[i]; + if (p.Length == 0) return false; + if (!IsValidIdent(p) && !p.All(char.IsAsciiDigit)) return false; + } + return true; + } + private static bool IsValidIdent(string s) { if (string.IsNullOrEmpty(s)) return false; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTagPathTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTagPathTests.cs index 3ec53e34..03af71ee 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTagPathTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTagPathTests.cs @@ -128,6 +128,41 @@ public sealed class AbCipTagPathTests AbCipTagPath.TryParse(input).ShouldBeNull(); } + /// + /// Module-defined I/O tags carry colons in their base name (Local:1:I, + /// ABB_OmniCore:I). Seen live: a CompactLogix lists ABB_OmniCore:I in + /// @tags, libplctag reads ABB_OmniCore:I.ConnectionFaulted fine, and the driver + /// refused it as a malformed path. + /// + /// A module tag path. + /// The expected base (first segment) name. + [Theory] + [InlineData("ABB_OmniCore:I.ConnectionFaulted", "ABB_OmniCore:I")] + [InlineData("Local:1:I.Data", "Local:1:I")] + [InlineData("Local:2:O.Data.3", "Local:2:O")] + [InlineData("Rack1:3:C.Ch0Config.RangeType", "Rack1:3:C")] + public void Module_IO_tag_base_names_with_colons_parse_and_round_trip(string input, string baseName) + { + var parsed = AbCipTagPath.TryParse(input); + parsed.ShouldNotBeNull(); + parsed.ProgramScope.ShouldBeNull(); + parsed.Segments[0].Name.ShouldBe(baseName); + parsed.ToLibplctagName().ShouldBe(input); + } + + /// A colon is only legal in the base name, and never as a dangling or doubled separator. + /// A malformed path. + [Theory] + [InlineData("Motor.Sub:I")] // colon past the base segment + [InlineData("Local::I.Data")] // empty colon part + [InlineData("Local:1:.Data")] // trailing colon + [InlineData(":Local.Data")] // leading colon + [InlineData("Program:P.Local:1:I")] // module tags are controller-scope only + public void Colons_outside_a_module_base_name_are_rejected(string input) + { + AbCipTagPath.TryParse(input).ShouldBeNull(); + } + /// Verifies that identifiers with underscores are accepted. [Fact] public void Ident_with_underscore_accepted() From 0ca56cb1f61fabcf456d00ac7ec1b556efe44afc Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:09:43 -0400 Subject: [PATCH 4/7] fix(abcip): an unknown tag no longer throttles its whole device or reads as a comms error Live read-only check against a production CompactLogix press: with one nonexistent tag first in the read list, libplctag's create threw LibPlcTagException("ErrorNotFound"); the driver counted it as a failed Forward-Open, opened the device's 05/STAB-8 connect-backoff window, and every other cold tag on the device - all real (WW_InCycle, WW_ActualPress, WW_WorkOrderNum) - read BadCommunicationError. In steady polling the bad tag re-opens the window each attempt, so siblings ordered after it may never connect. - A create that fails with a "controller answered" status (NotFound/NoMatch/BadDevice, the probe's existing reachable classification) records backoff SUCCESS, not failure. Genuine connection failures still open the window (falsification control test). - Read exceptions map through the libplctag Status they carry: unknown tag -> BadNodeIdUnknown, decode past the buffer -> BadOutOfRange, timeout -> BadTimeout, instead of a blanket BadCommunicationError. One status-name parser (AbCipStatusMapper.TryGetStatus) now serves both the read path and the probe. --- .../AbCipDriver.cs | 23 ++++-- .../AbCipDriverProbe.cs | 2 +- .../AbCipStatusMapper.cs | 28 +++++++ .../AbCipUnknownTagReadTests.cs | 73 +++++++++++++++++++ 4 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUnknownTagReadTests.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 5c780907..36907a15 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1,3 +1,4 @@ +using libplctag; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.OtOpcUa.Commons.Observability; @@ -818,10 +819,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (Exception ex) { - // Transport exception — evict so the next read creates a fresh handle. + // Transport exception — evict so the next read creates a fresh handle. A libplctag + // exception carries its Status, so a tag the controller does not have reads + // BadNodeIdUnknown and a decode past the buffer BadOutOfRange, not a comms error. EvictRuntime(device, def.Name); results[fb.OriginalIndex] = new DataValueSnapshot(null, - AbCipStatusMapper.BadCommunicationError, null, now); + AbCipStatusMapper.MapException(ex), null, now); _logger.LogWarning(ex, "AbCip read transport exception for tag {Tag} on device {Device}", reference, def.DeviceHostAddress); @@ -891,7 +894,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, catch (Exception ex) { EvictRuntime(device, parent.Name); - StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError); + StampGroupStatus(group, results, now, AbCipStatusMapper.MapException(ex)); _logger.LogWarning(ex, "AbCip whole-UDT read transport exception for parent {Parent} on device {Device}", group.ParentName, parent.DeviceHostAddress); @@ -1158,10 +1161,20 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { await runtime.InitializeAsync(ct).ConfigureAwait(false); } - catch + catch (Exception ex) { runtime.Dispose(); - device.Backoff.RecordFailure(DateTime.UtcNow); // 05/STAB-8: open the backoff window + // 05/STAB-8: a failed create opens the backoff window — but only when the device did + // not answer. "Tag not found" means the controller replied over a working session; it + // is a config error on this one tag, and counting it as a failed Forward-Open throttled + // every sibling tag on the device (seen live: one typo blacked out the whole device). + if (ex is LibPlcTagException lpe + && AbCipStatusMapper.TryGetStatus(lpe, out var status) + && status != Status.Ok + && AbCipDriverProbe.IsReachableStatus(status)) + device.Backoff.RecordSuccess(); + else + device.Backoff.RecordFailure(DateTime.UtcNow); throw; } device.Backoff.RecordSuccess(); // 05/STAB-8: Forward-Open succeeded — reset the window diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs index 9ad6abae..ccfa04f4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs @@ -200,7 +200,7 @@ public sealed class AbCipDriverProbe : IDriverProbe internal static bool IsReachableException(LibPlcTagException ex) { var msg = ex.Message ?? string.Empty; - if (Enum.TryParse(msg.Trim(), ignoreCase: true, out var status) && Enum.IsDefined(status)) + if (AbCipStatusMapper.TryGetStatus(ex, out var status)) return IsReachableStatus(status); return msg.Contains("NOT_FOUND", StringComparison.OrdinalIgnoreCase) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs index 4f50252b..bbe48a20 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs @@ -76,6 +76,34 @@ public static class AbCipStatusMapper /// The mapped OPC UA StatusCode value. public static uint MapLibplctagStatus(int status) => MapLibplctagStatus((Status)status); + /// + /// Recover the libplctag a was thrown on. + /// The wrapper builds the exception from Status.ToString(), so the message is the enum + /// name ("ErrorNotFound"). + /// + /// The exception. + /// The recovered status. + /// true when the message names a defined . + public static bool TryGetStatus(LibPlcTagException ex, out Status status) + { + ArgumentNullException.ThrowIfNull(ex); + return Enum.TryParse((ex.Message ?? string.Empty).Trim(), ignoreCase: true, out status) + && Enum.IsDefined(status); + } + + /// + /// Status for an exception thrown while creating / reading a tag: a libplctag exception maps + /// through its (so a tag the controller does not have reads + /// , not a comms error); anything else is + /// . + /// + /// The exception. + /// The OPC UA status code. + public static uint MapException(Exception ex) => + ex is LibPlcTagException lpe && TryGetStatus(lpe, out var status) + ? MapLibplctagStatus(status) + : BadCommunicationError; + /// /// Map a libplctag.NET enum value to an OPC UA StatusCode. This is /// the strongly-typed core of the mapper; the int overload exists only for the diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUnknownTagReadTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUnknownTagReadTests.cs new file mode 100644 index 00000000..15643010 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUnknownTagReadTests.cs @@ -0,0 +1,73 @@ +using libplctag; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// A tag the controller does not have is a configuration error, not a lost connection. Seen live +/// against a production CompactLogix: with one misspelled tag first in the read list, libplctag's +/// create threw LibPlcTagException("ErrorNotFound"), the driver counted that as a failed +/// Forward-Open, opened the device's connect-backoff window, and every other not-yet-connected tag +/// on the device — all of them real — came back BadCommunicationError (throttled). The bad +/// tag itself also read BadCommunicationError instead of BadNodeIdUnknown. +/// +[Trait("Category", "Unit")] +public sealed class AbCipUnknownTagReadTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private static (AbCipDriver Driver, FakeAbCipTagFactory Factory) Build(Func initFault) + { + var factory = new FakeAbCipTagFactory + { + Customise = p => initFault(p.TagName) is { } ex + ? new FakeAbCipTag(p) { ThrowOnInitialize = true, Exception = ex } + : new FakeAbCipTag(p) { Value = 42 }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Typo", Device, "NoSuchTag_xyz", AbCipDataType.DInt), + new AbCipTagDefinition("Real1", Device, "WW_InCycle", AbCipDataType.DInt), + new AbCipTagDefinition("Real2", Device, "WW_ActualPress", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + MaxConcurrentReads = 1, + }, "abcip-1", factory); + return (drv, factory); + } + + /// The unknown tag reads BadNodeIdUnknown and its siblings still connect and read Good. + [Fact] + public async Task Unknown_tag_does_not_throttle_sibling_tags_and_reads_BadNodeIdUnknown() + { + var (drv, _) = Build(name => name == "NoSuchTag_xyz" ? new LibPlcTagException("ErrorNotFound") : null); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var r = await drv.ReadAsync(["Typo", "Real1", "Real2"], TestContext.Current.CancellationToken); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); + r[1].Value.ShouldBe(42); + r[2].StatusCode.ShouldBe(AbCipStatusMapper.Good); + } + + /// + /// Falsification control: a genuine connection failure still opens the backoff window, so a + /// sibling cold tag is throttled exactly as before (05/STAB-8 is unchanged). + /// + [Fact] + public async Task A_connection_failure_still_opens_the_backoff_window() + { + var (drv, factory) = Build(name => name == "NoSuchTag_xyz" ? new LibPlcTagException("ErrorTimeout") : null); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var r = await drv.ReadAsync(["Typo", "Real1"], TestContext.Current.CancellationToken); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTimeout); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError); + factory.Tags.ContainsKey("WW_InCycle").ShouldBeFalse(); // throttled before any create + } +} From 68cf3ae8b7ee29c7bc9da5fbca4cf711d7927aa8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:11:08 -0400 Subject: [PATCH 5/7] fix(abcip): stride user-string arrays by the read buffer, not the built-in STRING size Live read-only check against a production CompactLogix press: Program:UN01_Press._ModeNames (STR0016[32], 4 elements requested) returned an 80-byte buffer with libplctag.NET 1.5.2's ElementSize unset. The driver fell back to the static 88-byte STRING stride, read element 1 past the buffer, and the whole read failed (ErrorOutOfBounds -> BadCommunicationError). When the buffer divides evenly by the element count, that quotient is now the stride; the read returns [undefined, Production, Maintenance, Manual] as the controller holds. --- .../LibplctagTagRuntime.cs | 23 +++++++++- .../AbCipArrayStrideTests.cs | 45 +++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayStrideTests.cs 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 11ef9b26..b11cbd49 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -79,8 +79,7 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime // accounts for STRING capacity + atomic widths. ElementSize is int? on the wrapper — fall // back to a static size table when it is null/<=0 (e.g. a zero-length read) so the stride // is never 0. - var elementSize = _tag.ElementSize ?? 0; - var stride = elementSize > 0 ? elementSize : ElementByteSize(type); + var stride = ResolveArrayStride(_tag.ElementSize, _tag.GetSize(), count, type); return type switch { @@ -99,6 +98,26 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime }; } + /// + /// Byte stride between array elements. libplctag's ElementSize wins when populated, but + /// after a read it is null (observed live, libplctag.NET 1.5.2) — and the static table + /// assumes the built-in 88-byte STRING, which is wrong for a user string type: a STR0016 + /// element is 20 bytes, so element 1 was read at offset 88 of an 80-byte buffer and the whole + /// read failed. When the read buffer divides evenly by the element count, that quotient IS the + /// element size, so it is used before the static table. + /// + /// libplctag's reported element size, if any. + /// Bytes in the read buffer. + /// Element count requested. + /// Element type. + /// The stride in bytes. + internal static int ResolveArrayStride(int? elementSize, int bufferSize, int count, AbCipDataType type) + { + if (elementSize is > 0) return elementSize.Value; + if (count > 0 && bufferSize > 0 && bufferSize % count == 0) return bufferSize / count; + return ElementByteSize(type); + } + private bool[] BuildBoolArray(int count) { var arr = new bool[count]; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayStrideTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayStrideTests.cs new file mode 100644 index 00000000..86111824 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayStrideTests.cs @@ -0,0 +1,45 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Array element stride. Seen live on a production CompactLogix: reading +/// Program:UN01_Press._ModeNames (STR0016[32], 4 elements requested) returned an +/// 80-byte buffer with libplctag's ElementSize unset; the driver strode 88 bytes (built-in +/// STRING) and the read failed ErrorOutOfBounds. The real values were +/// "undefined" / "Production" / "Maintenance" / "Manual" at a 20-byte stride. +/// +[Trait("Category", "Unit")] +public sealed class AbCipArrayStrideTests +{ + /// A user string type's stride comes from the buffer, not the built-in STRING size. + [Fact] + public void User_string_array_strides_by_buffer_size_over_count() + { + LibplctagTagRuntime.ResolveArrayStride(null, bufferSize: 80, count: 4, AbCipDataType.String).ShouldBe(20); + } + + /// The built-in STRING still strides 88. + [Fact] + public void Builtin_STRING_array_strides_88() + { + LibplctagTagRuntime.ResolveArrayStride(null, bufferSize: 3 * 88, count: 3, AbCipDataType.String).ShouldBe(88); + } + + /// libplctag's own element size wins when it is populated. + [Fact] + public void Reported_element_size_wins() + { + LibplctagTagRuntime.ResolveArrayStride(4, bufferSize: 80, count: 4, AbCipDataType.DInt).ShouldBe(4); + } + + /// An empty or uneven buffer falls back to the static per-type size. + [Fact] + public void Empty_or_uneven_buffer_falls_back_to_the_type_size() + { + LibplctagTagRuntime.ResolveArrayStride(null, bufferSize: 0, count: 4, AbCipDataType.Real).ShouldBe(4); + LibplctagTagRuntime.ResolveArrayStride(null, bufferSize: 81, count: 4, AbCipDataType.String).ShouldBe(88); + } +} From e3d6b76edee2ffe7865b8773f7f562ee70ac1e3b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:15:19 -0400 Subject: [PATCH 6/7] 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. From cc876bdf4340980fe9d2c6d1f66a14bbfeb2cb94 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 14:17:19 -0400 Subject: [PATCH 7/7] docs(abcip): record the 2026-10-07 real-controller verification and the browse/read behaviour it changed --- docs/drivers/AbCip.md | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 2f9594a2..37db1507 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -37,7 +37,7 @@ AB Legacy surface. | Capability | Implementation entry point | Notes | |------------|---------------------------|-------| -| `ITagDiscovery` | `DiscoverAsync` | Emits pre-declared tags under per-device folders; UDT tags with declared `Members` fan out into a sub-folder + one variable per member. With `EnableControllerBrowse` the `@tags` symbol table is walked into a `Discovered/` folder (system/module/routine tags filtered out). | +| `ITagDiscovery` | `DiscoverAsync` | Emits pre-declared tags under per-device folders; UDT tags with declared `Members` fan out into a sub-folder + one variable per member. With `EnableControllerBrowse` the `@tags` symbol table — plus each `Program:.@tags` listing — is walked into a `Discovered/` folder (system/module/routine tags filtered out). | | `IReadable` | `ReadAsync` → `ReadGroupAsync` / `ReadSingleAsync` | Per-tag reads; opt-in whole-UDT grouping (`EnableDeclarationOnlyUdtGrouping`) collapses N member reads into one. | | `IWritable` | `WriteAsync` | BOOL-within-DINT writes do a per-parent read-modify-write under a lock; `SafetyTag` and non-writable tags return `BadNotWritable`. | | `ISubscribable` | `SubscribeAsync` driven by the shared `PollGroupEngine` | CIP has no push model — subscriptions become polling groups. | @@ -142,13 +142,32 @@ Variables — in addition to the UDT container tag itself. Nested struct leaves that resolve to no emittable atomic leaf produce no sub-folder (lazy materialisation). - **Live verification is Emulate-tier only** (`AB_SERVER_PROFILE=emulate` + a Logix Emulate - instance): `ab_server` (the default Docker simulator) does not implement the CIP Template - Object service (class 0x6C), so the nested-id fetch cannot be exercised against it. - See `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Emulate/AbCipEmulateNestedUdtTests.cs` - for the skip-gated live-gate smoke (backlog #6). Offline unit coverage lives in - `AbCipDriverDiscoveryTests.Controller_discovered_UDT_nested_struct_expands_via_nested_template_id_fetch_no_seam` - and `CipTemplateObjectDecoderTests`. + `ab_server` (the default Docker simulator) does not implement the CIP Template Object service + (class 0x6C), so the fixture cannot exercise it; see + `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Emulate/AbCipEmulateNestedUdtTests.cs` + for the skip-gated Emulate smoke (backlog #6). +- **STRING tags and members** — a controller reports every STRING (built-in, or a user string + type such as `STR0016`) as a `LEN` DINT + `DATA` SINT[n] struct. Discovery recognises that + shape and emits **one String variable**, top-level or nested, instead of a LEN/DATA folder. +- **Hidden members are skipped** — the `ZZZZZZZZZZ…` hosts Logix packs BOOL members into and + `__…` AOI backing members are not browsed (a user name can never start with `__`). +- **Arrays** — the Symbol Object's dimension bits populate `IsArray`/`ElementCount` (multi-dim + flattened). An array of a UDT is emitted as one variable, not fanned out (`Tag.Member` names no + element); an array of STRING is a String array. +- **Program scope** — `@tags` lists each program (`Program:Main`) but none of its tags; the + enumerator reads every `Program:.@tags` listing too, so program-scoped tags appear as + `Program:.`. One unreadable program listing is skipped, not fatal. + +### Real-controller verification (2026-10-07) + +Run read-only against production CompactLogix 5069-L320ERS2 rev 34.11 presses (and a second +CompactLogix) from the Mac over VPN. Before the fixes above, every real `@udt/` buffer +decoded to `null` (the decoder assumed a layout no controller sends — the test builders shared +the assumption, the #468 trap), program-scoped tags were absent, and arrays browsed as scalars. +After them, one browse of a press took ~30 s and produced 23,412 variables covering all 80 +tags the Ignition project reads on it, with matching types. The template and symbol layouts are +pinned by bytes captured off those controllers: `CipTemplateObjectWireCaptureTests`, +`CipSymbolObjectRealControllerCaptureTests`, `AbCipProgramScopeEnumerationTests`. ### Bare-container reads @@ -170,4 +189,8 @@ UDT-typed tags found in the walk. - **Native heap is invisible to the GC.** libplctag's native `Tag` heap does not show up in CLR allocation figures. Watch whole-process RSS, and use `ReinitializeAsync` (tears down + re-creates every device's libplctag handles) as the remediation for native-heap growth. (There is no per-driver footprint number to consult: `IDriver.GetMemoryFootprint()` was deleted as unconsumed in Gitea #525.) - **Handle eviction on failure** — a non-zero libplctag status or a transport exception evicts the cached tag runtime so the next read/write re-creates a fresh handle, mirroring the probe loop's recreate-on-failure behaviour. +- **Read statuses carry the libplctag cause** — a tag the controller does not have reads `BadNodeIdUnknown` (not `BadCommunicationError`), and it does **not** open the device's connect-backoff window (the controller answered; before this, one typo throttled every sibling tag on the device). A genuine connection failure still opens it. +- **Width check on scalar reads** — libplctag never checks the CIP type the controller returns. A scalar atomic read whose buffer is not the authored type's width (e.g. a REAL authored as INT — the legacy Kepware export does exactly this for some press tags) reads `BadTypeMismatch` instead of Good garbage. Same-width mistakes (DINT vs REAL) are still undetectable. +- **Module-defined I/O tags** — `Local:1:I.Data`, `ABB_OmniCore:I.ConnectionFaulted` and similar colon-bearing base names are valid tag paths (controller scope only). +- **User string arrays** — `STR0016[N]` and similar arrays stride by the actual element size (read buffer ÷ count); libplctag.NET 1.5.2 leaves `ElementSize` unset after a read. - **Declaration-only UDT grouping is a footgun unless verified** — only enable `EnableDeclarationOnlyUdtGrouping` when every UDT's member declaration order has been hand-verified against the controller's compiled layout.