diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs index 66417010..573268a2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs @@ -135,10 +135,14 @@ public sealed record AbCipDeviceOptions( /// GuardLogix controller; non-safety writes violate the safety-partition isolation and are /// rejected by the PLC anyway. Surfaces the intent explicitly instead of relying on the /// write attempt failing at runtime. -/// Phase 4c — number of array elements for a 1-D array tag. Defaults -/// to 1 (scalar). When greater than 1 the tag discovers as an OPC UA array node -/// (IsArray + ArrayDim) and reads via libplctag's elem_count into an -/// element-typed CLR array. Ignored for . +/// Phase 4c — number of array elements for a 1-D array tag. Reads via +/// libplctag's elem_count into an element-typed CLR array when +/// is set; 1 for a scalar. Ignored for . +/// Review I-1 — the EXPLICIT array signal. true ⟺ the source TagConfig +/// had isArray:true (with arrayLength >= 1); the tag discovers as an OPC UA +/// array node (IsArray + ArrayDim) and reads as a typed CLR array — even when +/// is 1 (a valid 1-element array). ElementCount alone +/// cannot carry this because a scalar and a 1-element array both have a count of 1. public sealed record AbCipTagDefinition( string Name, string DeviceHostAddress, @@ -148,7 +152,8 @@ public sealed record AbCipTagDefinition( bool WriteIdempotent = false, IReadOnlyList? Members = null, bool SafetyTag = false, - int ElementCount = 1); + int ElementCount = 1, + bool IsArray = false); /// /// One declared member of a UDT tag. Name is the member identifier on the PLC (e.g. Speed, @@ -156,12 +161,20 @@ public sealed record AbCipTagDefinition( /// . Declaration-driven — the real CIP Template Object reader /// (class 0x6C) that would auto-discover member layouts lands as a follow-up PR. /// +/// The member identifier on the PLC. +/// The atomic Logix type of the member. +/// Whether the member is writable. +/// Whether writes to the member are idempotent. +/// Number of array elements for a 1-D array member; 1 for scalar. +/// Review I-1 — the EXPLICIT array signal for a member: true ⟺ the +/// member is a 1-D array (even of length 1). Discovers as an OPC UA array node when set. public sealed record AbCipStructureMember( string Name, AbCipDataType DataType, bool Writable = true, bool WriteIdempotent = false, - int ElementCount = 1); + int ElementCount = 1, + bool IsArray = false); /// Which AB PLC family the device is — selects the profile applied to connection params. public enum AbCipPlcFamily diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs index 8d590622..80e1746a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs @@ -31,13 +31,15 @@ public static class AbCipEquipmentTagParser var deviceHostAddress = ReadString(root, "deviceHostAddress"); var dataType = ReadEnum(root, "dataType", AbCipDataType.DInt); - // Phase 4c — an isArray equipment tag carries arrayLength; thread it into the def's - // ElementCount so the read pulls the whole array via libplctag elem_count. When - // isArray is absent/false (or arrayLength is missing/<=1) the tag stays scalar. - var elementCount = ReadArrayElementCount(root); + // Review I-1 — an equipment tag is an ARRAY ⟺ isArray:true AND arrayLength >= 1. A + // 1-element array (isArray:true, arrayLength:1) is a VALID 1-element array — the + // foundation materialises a [1] OPC UA array node — so it must read as an array, not a + // scalar. ElementCount can't carry the signal (a scalar and a 1-element array both + // have a count of 1), so the explicit IsArray flag does. + var (isArray, elementCount) = ReadArrayShape(root); def = new AbCipTagDefinition( Name: reference, DeviceHostAddress: deviceHostAddress, TagPath: tagPath, - DataType: dataType, Writable: true, ElementCount: elementCount); + DataType: dataType, Writable: true, ElementCount: elementCount, IsArray: isArray); return true; } catch (JsonException) { return false; } @@ -46,20 +48,22 @@ public static class AbCipEquipmentTagParser } /// - /// Resolve the 1-D array element count from an isArray / arrayLength pair. - /// Returns 1 (scalar) unless isArray is truthy AND arrayLength is a number - /// greater than 1; matches the sink's "isArray + arrayLength" carrier. + /// Resolve the 1-D array shape from an isArray / arrayLength pair (the foundation + /// contract carrier). The tag is an ARRAY ⟺ isArray is truthy AND arrayLength + /// is a number >= 1 (a 1-element array is valid). Returns + /// (IsArray: false, ElementCount: 1) for a scalar. /// - private static int ReadArrayElementCount(JsonElement o) + private static (bool IsArray, int ElementCount) ReadArrayShape(JsonElement o) { var isArray = o.TryGetProperty("isArray", out var a) && a.ValueKind == JsonValueKind.True; - if (!isArray) return 1; + if (!isArray) return (false, 1); if (o.TryGetProperty("arrayLength", out var len) && len.ValueKind == JsonValueKind.Number && len.TryGetInt32(out var n) - && n > 1) - return n; - return 1; + && n >= 1) + return (true, n); + // isArray:true but arrayLength missing/invalid — treat as a 1-element array (count 1). + return (true, 1); } private static TEnum ReadEnum(JsonElement o, string name, TEnum fallback) where TEnum : struct, Enum 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 b69a2a3c..d8b7be01 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -553,10 +553,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var tagPath = AbCipTagPath.TryParse(def.TagPath); var bitIndex = tagPath?.BitIndex; - // Phase 4c — a 1-D array tag decodes the whole buffer into an element-typed CLR - // array (int[]/float[]/bool[]/string[]…); scalar tags keep the single-value path. - var value = def.ElementCount > 1 - ? runtime.DecodeArray(def.DataType, def.ElementCount) + // 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. + var value = IsArrayTag(def) + ? runtime.DecodeArray(def.DataType, Math.Max(1, def.ElementCount)) : runtime.DecodeValue(def.DataType, bitIndex); results[fb.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); _health = new DriverHealth(DriverState.Healthy, now, null); @@ -854,11 +855,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, ?? throw new InvalidOperationException( $"AbCip tag '{def.Name}' has malformed TagPath '{def.TagPath}'."); - // Phase 4c — a 1-D array tag (ElementCount > 1) sets libplctag's elem_count so the read - // pulls every element in one CIP transaction; the read path then boxes them into a - // typed CLR array. Scalar tags pass the default count of 1, unchanged. + // Review I-1 — an array tag (the EXPLICIT IsArray flag, incl. a 1-element array) sets + // libplctag's elem_count so the read pulls every element in one CIP transaction; the read + // path then boxes them into a typed CLR array. Scalar tags pass count 1 + IsArray false. var runtime = _tagFactory.Create( - device.BuildCreateParams(parsed.ToLibplctagName(), _options.Timeout, def.ElementCount)); + device.BuildCreateParams( + parsed.ToLibplctagName(), _options.Timeout, def.ElementCount, IsArrayTag(def))); try { await runtime.InitializeAsync(ct).ConfigureAwait(false); @@ -950,11 +952,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, foreach (var member in tag.Members) { var memberFullName = $"{tag.Name}.{member.Name}"; + // Review I-1 — array-ness is the EXPLICIT IsArray flag (a 1-element array is + // still an array); a legacy member with ElementCount > 1 but the flag unset + // remains an array for back-compat. + var memberIsArray = member.IsArray || member.ElementCount > 1; udtFolder.Variable(member.Name, member.Name, new DriverAttributeInfo( FullName: memberFullName, DriverDataType: member.DataType.ToDriverDataType(), - IsArray: member.ElementCount > 1, - ArrayDim: member.ElementCount > 1 ? (uint)member.ElementCount : null, + IsArray: memberIsArray, + ArrayDim: memberIsArray ? (uint)Math.Max(1, member.ElementCount) : null, SecurityClass: member.Writable ? SecurityClassification.Operate : SecurityClassification.ViewOnly, @@ -988,11 +994,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var fullName = discovered.ProgramScope is null ? discovered.Name : $"Program:{discovered.ProgramScope}.{discovered.Name}"; + // 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(), - IsArray: discovered.ElementCount > 1, - ArrayDim: discovered.ElementCount > 1 ? (uint)discovered.ElementCount : null, + IsArray: discoveredIsArray, + ArrayDim: discoveredIsArray ? (uint)Math.Max(1, discovered.ElementCount) : null, SecurityClass: discovered.ReadOnly ? SecurityClassification.ViewOnly : SecurityClassification.Operate, @@ -1004,11 +1013,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } + // Review I-1 — array-ness is the EXPLICIT IsArray flag (a 1-element array is still an array); + // a legacy definition carrying only ElementCount > 1 stays an array for back-compat. + private static bool IsArrayTag(AbCipTagDefinition tag) => tag.IsArray || tag.ElementCount > 1; + private static DriverAttributeInfo ToAttributeInfo(AbCipTagDefinition tag) => new( FullName: tag.Name, DriverDataType: tag.DataType.ToDriverDataType(), - IsArray: tag.ElementCount > 1, - ArrayDim: tag.ElementCount > 1 ? (uint)tag.ElementCount : null, + IsArray: IsArrayTag(tag), + ArrayDim: IsArrayTag(tag) ? (uint)Math.Max(1, tag.ElementCount) : null, SecurityClass: (tag.Writable && !tag.SafetyTag) ? SecurityClassification.Operate : SecurityClassification.ViewOnly, @@ -1112,8 +1125,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// The timeout for tag operations. /// libplctag elem_count — 1 for a scalar tag, the array /// length for a 1-D array tag (Phase 4c). Coerced to a minimum of 1. + /// Review I-1 — the EXPLICIT array signal threaded through so a + /// 1-element array ( 1) is still read as an array. /// The computed tag creation parameters. - public AbCipTagCreateParams BuildCreateParams(string tagName, TimeSpan timeout, int elementCount = 1) => new( + public AbCipTagCreateParams BuildCreateParams( + string tagName, TimeSpan timeout, int elementCount = 1, bool isArray = false) => new( Gateway: ParsedAddress.Gateway, Port: ParsedAddress.Port, CipPath: ParsedAddress.CipPath, @@ -1122,7 +1138,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, Timeout: timeout, AllowPacking: Options.AllowPacking ?? Profile.SupportsRequestPacking, ConnectionSize: Options.ConnectionSize ?? Profile.DefaultConnectionSize, - ElementCount: elementCount < 1 ? 1 : elementCount); + ElementCount: elementCount < 1 ? 1 : elementCount, + IsArray: isArray); /// Disposes all runtime tag handles and clears the caches. public void DisposeHandles() diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs index 6bf42a06..8163ab96 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagEnumerator.cs @@ -40,15 +40,19 @@ public interface IAbCipTagEnumeratorFactory /// the driver applies on top so the enumerator is not the /// single source of truth. /// Phase 4c — libplctag elem_count reported by the Symbol -/// Object's array-dimension fields. Defaults to 1 (scalar); greater than 1 surfaces the tag -/// as an OPC UA array node at discovery. +/// Object's array-dimension fields. Defaults to 1 (scalar). +/// Review I-1 — the EXPLICIT array signal: true ⟺ the Symbol Object +/// reported a 1-D array (even of length 1). Surfaces the tag as an OPC UA array node at +/// discovery; alone can't distinguish a scalar from a 1-element +/// array. public sealed record AbCipDiscoveredTag( string Name, string? ProgramScope, AbCipDataType DataType, bool ReadOnly, bool IsSystemTag = false, - int ElementCount = 1); + int ElementCount = 1, + bool IsArray = false); /// /// No-op enumerator returning an empty sequence. Useful for tests + strict-config 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 68d5f211..b0520177 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs @@ -101,7 +101,9 @@ public interface IAbCipTagFactory /// Phase 4c — libplctag elem_count. Forwarded to the /// libplctag Tag.ElementCount property so a 1-D array tag pulls all elements in one /// CIP transaction. Defaults to 1 (scalar); the driver sets it from the tag definition's -/// element count for an isArray tag. +/// element count for an isArray tag — including a 1-element array. +/// Review I-1 — the EXPLICIT array signal threaded from the tag definition so +/// a 1-element array ( 1) is still read as an array, not a scalar. public sealed record AbCipTagCreateParams( string Gateway, int Port, @@ -111,4 +113,5 @@ public sealed record AbCipTagCreateParams( TimeSpan Timeout, bool AllowPacking = true, int ConnectionSize = 4002, - int ElementCount = 1); + int ElementCount = 1, + bool IsArray = false); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs index 4ea40fc6..3ea4fb0e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayTests.cs @@ -181,22 +181,111 @@ public sealed class AbCipArrayTests factory.Tags["Recipe"].CreationParams.ElementCount.ShouldBe(4); } - /// The parser threads arrayLength into the transient definition's ElementCount. + /// The parser threads arrayLength into the transient definition's ElementCount and sets IsArray. [Fact] public void Parser_threads_arrayLength_into_ElementCount() { var json = """{"tagPath":"Recipe","dataType":"DInt","isArray":true,"arrayLength":8}"""; AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); def!.ElementCount.ShouldBe(8); + def.IsArray.ShouldBeTrue(); } - /// A non-array equipment ref defaults ElementCount to 1 (scalar). + /// A non-array equipment ref defaults ElementCount to 1 (scalar) and IsArray false. [Fact] public void Parser_defaults_ElementCount_to_one_when_not_an_array() { var json = """{"tagPath":"Recipe","dataType":"DInt"}"""; AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); def!.ElementCount.ShouldBe(1); + def.IsArray.ShouldBeFalse(); + } + + /// + /// Review finding I-1 — a 1-element array (isArray:true, arrayLength:1) is a valid + /// 1-element array, NOT a scalar: the parser sets + /// true and 1. + /// + [Fact] + public void Parser_treats_isArray_with_arrayLength_one_as_a_one_element_array() + { + var json = """{"tagPath":"Recipe","dataType":"DInt","isArray":true,"arrayLength":1}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + def!.IsArray.ShouldBeTrue(); + def.ElementCount.ShouldBe(1); + } + + /// + /// Review finding I-1 — isArray:true, arrayLength:1 must DISCOVER as a [1] array node + /// (IsArray + ArrayDim 1), matching the foundation's materialisation, not as a scalar. + /// + [Fact] + public async Task Equipment_ref_isArray_arrayLength_one_discovers_as_one_element_array() + { + var json = """{"deviceHostAddress":"ab://10.0.0.5/1,0","tagPath":"Recipe","dataType":"DInt","isArray":true,"arrayLength":1}"""; + AbCipEquipmentTagParser.TryParse(json, out var def).ShouldBeTrue(); + + var builder = new RecordingBuilder(); + var (drv, _) = NewDriver(def!); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.DiscoverAsync(builder, CancellationToken.None); + + var arr = builder.Variables.Single().Info; + arr.IsArray.ShouldBeTrue(); + arr.ArrayDim.ShouldBe(1u); + } + + /// + /// Review finding I-1 — the I-1 case: an isArray:true, arrayLength:1 equipment tag + /// reads a 1-ELEMENT typed array, NOT a scalar. On current code (gate ElementCount > 1) + /// this reads a scalar; the explicit IsArray flag fixes it. + /// + [Fact] + public async Task Equipment_ref_isArray_arrayLength_one_reads_as_one_element_array() + { + var json = """{"deviceHostAddress":"ab://10.0.0.5/1,0","tagPath":"Recipe","dataType":"DInt","isArray":true,"arrayLength":1}"""; + var factory = new FakeAbCipTagFactory(); + var opts = new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [], + }; + var drv = new AbCipDriver(opts, "abcip-eq-array1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = p => new ArrayFakeAbCipTag(p, new int[] { 99 }); + + var snapshots = await drv.ReadAsync([json], CancellationToken.None); + + snapshots.Single().StatusCode.ShouldBe(AbCipStatusMapper.Good); + var value = snapshots.Single().Value.ShouldBeOfType(); + value.ShouldBe([99]); + // A 1-element array still threads elem_count 1 to libplctag. + factory.Tags["Recipe"].CreationParams.ElementCount.ShouldBe(1); + } + + /// + /// Regression — a genuinely scalar equipment ref (isArray:false) reads a boxed + /// scalar via , never an array. + /// + [Fact] + public async Task Equipment_ref_isArray_false_reads_as_scalar() + { + var json = """{"deviceHostAddress":"ab://10.0.0.5/1,0","tagPath":"Speed","dataType":"DInt","isArray":false}"""; + var factory = new FakeAbCipTagFactory(); + var opts = new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [], + }; + var drv = new AbCipDriver(opts, "abcip-eq-scalar", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = p => new FakeAbCipTag(p) { Value = 4200 }; + + var snapshots = await drv.ReadAsync([json], CancellationToken.None); + + snapshots.Single().Value.ShouldBe(4200); + snapshots.Single().Value.ShouldNotBeOfType(); } // ---- helpers ----