fix(abcip): explicit IsArray flag so 1-element arrays read as arrays (review I-1)

2026-06-16 22:14:41 -04:00
parent ce5d46be08
commit 94e8c55b5c
6 changed files with 172 additions and 42 deletions
@@ -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.</param>
-/// <param name="ElementCount">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
-///     (<c>IsArray</c> + <c>ArrayDim</c>) and reads via libplctag's <c>elem_count</c> into an
-///     element-typed CLR array. Ignored for <see cref="AbCipDataType.Structure"/>.</param>
+/// <param name="ElementCount">Phase 4c — number of array elements for a 1-D array tag. Reads via
+///     libplctag's <c>elem_count</c> into an element-typed CLR array when <paramref name="IsArray"/>
+///     is set; <c>1</c> for a scalar. Ignored for <see cref="AbCipDataType.Structure"/>.</param>
+/// <param name="IsArray">Review I-1 — the EXPLICIT array signal. <c>true</c> ⟺ the source TagConfig
+///     had <c>isArray:true</c> (with <c>arrayLength &gt;= 1</c>); the tag discovers as an OPC UA
+///     array node (<c>IsArray</c> + <c>ArrayDim</c>) and reads as a typed CLR array — even when
+///     <paramref name="ElementCount"/> is 1 (a valid 1-element array). <c>ElementCount</c> alone
+///     cannot carry this because a scalar and a 1-element array both have a count of 1.</param>
 public sealed record AbCipTagDefinition(
    string Name,
    string DeviceHostAddress,
@@ -148,7 +152,8 @@ public sealed record AbCipTagDefinition(
    bool WriteIdempotent = false,
    IReadOnlyList<AbCipStructureMember>? Members = null,
    bool SafetyTag = false,
-    int ElementCount = 1);
+    int ElementCount = 1,
+    bool IsArray = false);

 /// <summary>
 ///     One declared member of a UDT tag. Name is the member identifier on the PLC (e.g. <c>Speed</c>,
@@ -156,12 +161,20 @@ public sealed record AbCipTagDefinition(
 ///     <see cref="AbCipTagDefinition"/>. Declaration-driven — the real CIP Template Object reader
 ///     (class 0x6C) that would auto-discover member layouts lands as a follow-up PR.
 /// </summary>
+/// <param name="Name">The member identifier on the PLC.</param>
+/// <param name="DataType">The atomic Logix type of the member.</param>
+/// <param name="Writable">Whether the member is writable.</param>
+/// <param name="WriteIdempotent">Whether writes to the member are idempotent.</param>
+/// <param name="ElementCount">Number of array elements for a 1-D array member; <c>1</c> for scalar.</param>
+/// <param name="IsArray">Review I-1 — the EXPLICIT array signal for a member: <c>true</c> ⟺ the
+///     member is a 1-D array (even of length 1). Discovers as an OPC UA array node when set.</param>
 public sealed record AbCipStructureMember(
    string Name,
    AbCipDataType DataType,
    bool Writable = true,
    bool WriteIdempotent = false,
-    int ElementCount = 1);
+    int ElementCount = 1,
+    bool IsArray = false);

 /// <summary>Which AB PLC family the device is — selects the profile applied to connection params.</summary>
 public enum AbCipPlcFamily
@@ -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
    }

    /// <summary>
-    ///     Resolve the 1-D array element count from an <c>isArray</c> / <c>arrayLength</c> pair.
-    ///     Returns 1 (scalar) unless <c>isArray</c> is truthy AND <c>arrayLength</c> is a number
-    ///     greater than 1; matches the sink's "isArray + arrayLength" carrier.
+    ///     Resolve the 1-D array shape from an <c>isArray</c> / <c>arrayLength</c> pair (the foundation
+    ///     contract carrier). The tag is an ARRAY ⟺ <c>isArray</c> is truthy AND <c>arrayLength</c>
+    ///     is a number <c>&gt;= 1</c> (a 1-element array is valid). Returns
+    ///     <c>(IsArray: false, ElementCount: 1)</c> for a scalar.
    /// </summary>
-    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<TEnum>(JsonElement o, string name, TEnum fallback) where TEnum : struct, Enum
@@ -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,
        /// <param name="timeout">The timeout for tag operations.</param>
        /// <param name="elementCount">libplctag <c>elem_count</c> — 1 for a scalar tag, the array
        ///     length for a 1-D array tag (Phase 4c). Coerced to a minimum of 1.</param>
+        /// <param name="isArray">Review I-1 — the EXPLICIT array signal threaded through so a
+        ///     1-element array (<paramref name="elementCount"/> 1) is still read as an array.</param>
        /// <returns>The computed tag creation parameters.</returns>
-        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);

        /// <summary>Disposes all runtime tag handles and clears the caches.</summary>
        public void DisposeHandles()
@@ -40,15 +40,19 @@ public interface IAbCipTagEnumeratorFactory
 ///     the driver applies <see cref="AbCipSystemTagFilter"/> on top so the enumerator is not the
 ///     single source of truth.</param>
 /// <param name="ElementCount">Phase 4c — libplctag <c>elem_count</c> 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.</param>
+///     Object's array-dimension fields. Defaults to 1 (scalar).</param>
+/// <param name="IsArray">Review I-1 — the EXPLICIT array signal: <c>true</c> ⟺ the Symbol Object
+///     reported a 1-D array (even of length 1). Surfaces the tag as an OPC UA array node at
+///     discovery; <see cref="ElementCount"/> alone can't distinguish a scalar from a 1-element
+///     array.</param>
 public sealed record AbCipDiscoveredTag(
    string Name,
    string? ProgramScope,
    AbCipDataType DataType,
    bool ReadOnly,
    bool IsSystemTag = false,
-    int ElementCount = 1);
+    int ElementCount = 1,
+    bool IsArray = false);

 /// <summary>
 ///     No-op enumerator returning an empty sequence. Useful for tests + strict-config
@@ -101,7 +101,9 @@ public interface IAbCipTagFactory
 /// <param name="ElementCount">Phase 4c — libplctag <c>elem_count</c>. Forwarded to the
 ///     libplctag <c>Tag.ElementCount</c> 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 <c>isArray</c> tag.</param>
+///     element count for an <c>isArray</c> tag — including a 1-element array.</param>
+/// <param name="IsArray">Review I-1 — the EXPLICIT array signal threaded from the tag definition so
+///     a 1-element array (<see cref="ElementCount"/> 1) is still read as an array, not a scalar.</param>
 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);
@@ -181,22 +181,111 @@ public sealed class AbCipArrayTests
        factory.Tags["Recipe"].CreationParams.ElementCount.ShouldBe(4);
    }

-    /// <summary>The parser threads arrayLength into the transient definition's ElementCount.</summary>
+    /// <summary>The parser threads arrayLength into the transient definition's ElementCount and sets IsArray.</summary>
    [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();
    }

-    /// <summary>A non-array equipment ref defaults ElementCount to 1 (scalar).</summary>
+    /// <summary>A non-array equipment ref defaults ElementCount to 1 (scalar) and IsArray false.</summary>
    [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();
+    }
+
+    /// <summary>
+    ///     Review finding I-1 — a 1-element array (<c>isArray:true, arrayLength:1</c>) is a valid
+    ///     1-element array, NOT a scalar: the parser sets <see cref="AbCipTagDefinition.IsArray"/>
+    ///     true and <see cref="AbCipTagDefinition.ElementCount"/> 1.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    ///     Review finding I-1 — <c>isArray:true, arrayLength:1</c> must DISCOVER as a [1] array node
+    ///     (IsArray + ArrayDim 1), matching the foundation's materialisation, not as a scalar.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    ///     Review finding I-1 — the I-1 case: an <c>isArray:true, arrayLength:1</c> equipment tag
+    ///     reads a 1-ELEMENT typed array, NOT a scalar. On current code (gate <c>ElementCount &gt; 1</c>)
+    ///     this reads a scalar; the explicit IsArray flag fixes it.
+    /// </summary>
+    [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<int[]>();
+        value.ShouldBe([99]);
+        // A 1-element array still threads elem_count 1 to libplctag.
+        factory.Tags["Recipe"].CreationParams.ElementCount.ShouldBe(1);
+    }
+
+    /// <summary>
+    ///     Regression — a genuinely scalar equipment ref (<c>isArray:false</c>) reads a boxed
+    ///     scalar via <see cref="IAbCipTagRuntime.DecodeValue"/>, never an array.
+    /// </summary>
+    [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<int[]>();
    }

    // ---- helpers ----