fix(twincat): size strings by the resolved type and refuse VAR_IN_OUT references (live TC2 gate)

Found by the read-only live gate against the TwinCAT 2.11 Elma cleaners Z33856 / Z33733: - The Beckhoff library reports a TC2 dotted MEMBER symbol's ByteSize as if the PLC's byte count were a bit count (STRING(80) -> 11, INT -> 1, UDINT -> 1) while DataType.ByteSize is right (81/2/4, raw SYM_INFOBYNAMEEX says 81). The previous commit's typeof(byte[]) read sliced by the symbol size, so .OPC_MACHINEDATA.PROCESSSTATUS read "ready for o" at Good; the notification width check would also have refused every numeric member. The shape now takes the resolved type's size, and string reads go by index group/offset (or a fresh handle for 0xF005) at that width. - P_INITINPUT_ACTION.SLOTID is "STRING(20) (VAR_IN_OUT)" - a 4-byte reference - but resolves to a plain STRING(20); a String tag read 21 bytes at the pointer as text at Good. The symbol's own VAR_IN_OUT marker now classifies it NonScalar -> BadTypeMismatch.
2026-10-07 14:27:22 -04:00
parent 75754478ac
commit 8060cfd2a5
5 changed files with 136 additions and 15 deletions
@@ -135,6 +135,32 @@ found against a real TwinCAT 2 PLC:
  registers, and the probe-tick retry re-checks it after a PLC program change.
  Reads go through the Beckhoff symbol cache, which converts numeric values by
  the PLC's own type, so only the string-kind rule applies there.
+- **Sizes come from the resolved data type, not the symbol.** On a TwinCAT 2
+  target the Beckhoff library reports a dotted *member* symbol's `ByteSize` as if
+  the PLC's byte count were a bit count (`.OPC_MACHINEDATA.PROCESSSTATUS`
+  `STRING(80)` → 11 B, an `INT` member → 1, a `UDINT` → 1) while its resolved
+  `DataType.ByteSize` is correct (81 / 2 / 4, matching raw `SYM_INFOBYNAMEEX`).
+  String reads therefore address the symbol's index group/offset explicitly at
+  the type's width — sizing them by the symbol truncated `"ready for operation"`
+  to `"ready for o"` at Good.
+- **A `VAR_IN_OUT` member is a reference, not a value.** TC2 names it
+  `STRING(20) (VAR_IN_OUT)` (4 bytes) but the library resolves its type to the
+  plain `STRING(20)`; the symbol's own name wins and such a tag is refused
+  (`BadTypeMismatch`) instead of publishing the memory at the pointer as text.
+
+### Live validation (2026-10-07, TwinCAT 2.11 Elma cleaners)
+
+Read-only, against Z33856 (`10.201.45.236.1.1:801`) and Z33733
+(`5.25.53.46.1.1:801`) over the VPN, through an in-process
+`Beckhoff.TwinCAT.Ads.TcpRouter` in a scratch harness: ReadState RUN; browse
+discovered 45,083 / 42,666 variables incl. the `INT16`/`UINT16` globals
+(`.ACTUALOPERATIONMODE`, `.ADSSTATE_RUN`) and 3,663 / 3,564 leading-dot names, no
+empty names; `.ADSSTATE_RUN` = 5 with bits `.0`/`.1`/`.2` = true/false/true;
+work-order and part-number strings whose PLC buffers carry a stale tail after
+the NUL (`68261005\0` + `01`) read as `68261005` through both poll and native
+notifications; `DInt` over a `STRING(80)` → `BadTypeMismatch` in both paths with
+the rest of the subscription Good. No PLC value held a genuine non-ASCII string
+to exercise Windows-1252 live — that rule is unit-tested only.

 ## Capability surface

@@ -129,7 +129,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
            // Scalar read: check the declared type against the PLC symbol first. The symbol is
            // resolved through the Beckhoff symbol cache (the same lookup ReadValueAsync makes
            // internally), so this costs no extra round trip after the first read.
-            var (shape, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken)
+            var (shape, symbol, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken)
                .ConfigureAwait(false);
            if (symbolStatus != TwinCATStatusMapper.Good) return (null, symbolStatus);
            if (TwinCATTypeGuard.CheckRead(type, shape) is not null)
@@ -137,14 +137,17 @@ internal sealed class AdsTwinCATClient : ITwinCATClient

            // A STRING / WSTRING is read as its raw bytes at the PLC's own declared width and
            // decoded here: it ends at the first NUL, and a STRING is Windows-1252 rather than the
-            // host culture's ANSI code page the Beckhoff marshaller would fall back to.
+            // host culture's ANSI code page the Beckhoff marshaller would fall back to. The width is
+            // the RESOLVED type's (shape.ByteSize) and the read is addressed explicitly: asking the
+            // library for typeof(byte[]) slices by the symbol's own ByteSize, which a TwinCAT 2
+            // member symbol under-reports (STRING(80) → 11 B, live on a TC 2.11 CX).
            if (type is TwinCATDataType.String or TwinCATDataType.WString)
            {
-                var raw = await _client.ReadValueAsync(symbolPath, typeof(byte[]), cancellationToken)
+                var (bytes, readStatus) = await ReadRawAsync(symbolPath, symbol!, shape.ByteSize, cancellationToken)
                    .ConfigureAwait(false);
-                if (raw.ErrorCode != AdsErrorCode.NoError)
-                    return (null, MapAndSignal((uint)raw.ErrorCode));
-                return (DecodeString(type, raw.Value as byte[] ?? [], StringsAreUtf8()), TwinCATStatusMapper.Good);
+                return readStatus == TwinCATStatusMapper.Good
+                    ? (DecodeString(type, bytes, StringsAreUtf8()), TwinCATStatusMapper.Good)
+                    : (null, readStatus);
            }

            var clrType = MapToClrType(type);
@@ -167,19 +170,55 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
    ///     <see cref="TwinCATTypeGuard"/> compares a declared type against. Uses the Beckhoff symbol
    ///     cache (lookup on), so repeat calls are local.
    /// </summary>
-    private async Task<(TwinCATPlcSymbolShape shape, uint status)> DescribeSymbolAsync(
+    private async Task<(TwinCATPlcSymbolShape shape, IAdsSymbol? symbol, uint status)> DescribeSymbolAsync(
        string symbolPath, CancellationToken cancellationToken)
    {
        var result = await _client.ReadSymbolAsync(symbolPath, cancellationToken).ConfigureAwait(false);
        if (result.ErrorCode != AdsErrorCode.NoError)
-            return (default, MapAndSignal((uint)result.ErrorCode));
+            return (default, null, MapAndSignal((uint)result.ErrorCode));
        if (result.Value is not { } symbol)
-            return (default, TwinCATStatusMapper.BadNodeIdUnknown);
+            return (default, null, TwinCATStatusMapper.BadNodeIdUnknown);

-        // Resolve aliases (T_MaxString → STRING(255)) so the guard sees the base type.
+        // Resolve aliases (T_MaxString → STRING(255)) so the guard sees the base type, and take the
+        // resolved type's byte size — a TwinCAT 2 member symbol's own ByteSize is under-reported.
        var dataType = symbol.DataType?.Resolve();
-        return (TwinCATPlcSymbolShape.Classify(dataType?.Name ?? symbol.TypeName, dataType?.Category, symbol.ByteSize),
-            TwinCATStatusMapper.Good);
+        var shape = TwinCATPlcSymbolShape.Classify(
+            dataType?.Name ?? symbol.TypeName, dataType?.Category, symbol.ByteSize, dataType?.ByteSize, symbol.TypeName);
+        return (shape, symbol, TwinCATStatusMapper.Good);
+    }
+
+    /// <summary>
+    ///     Reads exactly <paramref name="length"/> raw bytes of a resolved symbol: by its
+    ///     index group/offset, or — for a handle-addressed symbol (0xF005, whose offset is a handle
+    ///     that may be stale) — through a fresh variable handle.
+    /// </summary>
+    private async Task<(byte[] bytes, uint status)> ReadRawAsync(
+        string symbolPath, IAdsSymbol symbol, int length, CancellationToken cancellationToken)
+    {
+        const uint SymbolValueByHandle = 0xF005;
+        if (symbol.IndexGroup != SymbolValueByHandle)
+        {
+            var direct = await _client.ReadAsync(symbol.IndexGroup, symbol.IndexOffset, length, cancellationToken)
+                .ConfigureAwait(false);
+            return direct.ErrorCode == AdsErrorCode.NoError
+                ? (direct.Data.ToArray(), TwinCATStatusMapper.Good)
+                : ([], MapAndSignal((uint)direct.ErrorCode));
+        }
+
+        var handle = await _client.CreateVariableHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        if (handle.ErrorCode != AdsErrorCode.NoError) return ([], MapAndSignal((uint)handle.ErrorCode));
+        try
+        {
+            var read = await _client.ReadAsync(handle.Handle, length, cancellationToken).ConfigureAwait(false);
+            return read.ErrorCode == AdsErrorCode.NoError
+                ? (read.Data.ToArray(), TwinCATStatusMapper.Good)
+                : ([], MapAndSignal((uint)read.ErrorCode));
+        }
+        finally
+        {
+            try { await _client.DeleteVariableHandleAsync(handle.Handle, CancellationToken.None).ConfigureAwait(false); }
+            catch { /* best-effort release */ }
+        }
    }

    /// <summary>
@@ -271,7 +310,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
            // A notification carries raw bytes decoded as the DECLARED type, so a declared type that
            // cannot decode the PLC symbol (width, float/integer or string kind differ) would publish
            // wrong values at Good. Refuse it here; the driver isolates the one tag to Bad.
-            var (shape, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken)
+            var (shape, _, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken)
                .ConfigureAwait(false);
            if (symbolStatus != TwinCATStatusMapper.Good)
                throw new InvalidOperationException(
@@ -279,7 +279,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
                    if (status == TwinCATStatusMapper.BadTypeMismatch)
                        _logger.LogWarning(
                            "TwinCAT driver '{DriverInstanceId}' refused '{Reference}': declared {DataType} " +
-                            "does not match the PLC symbol '{Symbol}' (string vs numeric)",
+                            "cannot decode the PLC symbol '{Symbol}' (string/numeric kind mismatch or a reference/container)",
                            _driverInstanceId, reference, def.DataType, symbolName);
                    else
                        _logger.LogWarning(
@@ -36,9 +36,24 @@ internal readonly record struct TwinCATPlcSymbolShape(TwinCATPlcKind Kind, int B
    /// <param name="typeName">The PLC type name (<c>STRING(80)</c>, <c>INT16</c>, <c>ST_LOGDATA</c>, …).</param>
    /// <param name="category">The resolved type category, or null when the type system has none.</param>
    /// <param name="byteSize">The symbol's byte size.</param>
+    /// <param name="dataTypeByteSize">The resolved data type's byte size, when the type system has one.
+    ///     It wins over <paramref name="byteSize"/>: on a TwinCAT 2 target the Beckhoff library reports a
+    ///     dotted MEMBER symbol's size as if the PLC's byte count were a bit count (STRING(80) → 11,
+    ///     INT → 1, UDINT → 1; live on a TC 2.11 CX), while the resolved type carries the true size.</param>
+    /// <param name="symbolTypeName">The symbol's OWN type name, when it differs from the resolved one. A
+    ///     TwinCAT 2 <c>VAR_IN_OUT</c> member is named <c>STRING(20) (VAR_IN_OUT)</c> — a 4-byte reference —
+    ///     while the library resolves its data type to the plain <c>STRING(20)</c>; the reference marker
+    ///     wins, so the symbol is never read as the value it points at.</param>
    /// <returns>The classified shape.</returns>
-    public static TwinCATPlcSymbolShape Classify(string? typeName, DataTypeCategory? category, int byteSize)
+    public static TwinCATPlcSymbolShape Classify(
+        string? typeName, DataTypeCategory? category, int byteSize, int? dataTypeByteSize = null,
+        string? symbolTypeName = null)
    {
+        if (symbolTypeName?.Contains("VAR_IN_OUT", StringComparison.OrdinalIgnoreCase) == true
+            || typeName?.Contains("VAR_IN_OUT", StringComparison.OrdinalIgnoreCase) == true)
+            return new(TwinCATPlcKind.NonScalar, byteSize, false, (symbolTypeName ?? typeName)!.Trim());
+
+        if (dataTypeByteSize is > 0) byteSize = dataTypeByteSize.Value;
        var name = (typeName ?? string.Empty).Trim();
        var match = StringTypeName.Match(name);
        if (match.Success || category == DataTypeCategory.String)
@@ -37,6 +37,47 @@ public sealed class TwinCATTypeGuardTests
        Plc("INT16", 2).IsFloat.ShouldBeFalse();
    }

+    /// <summary>
+    ///     Live on the TwinCAT 2.11 Elma cleaner Z33856 (2026-10-07): for a dotted MEMBER path the
+    ///     Beckhoff library reports the symbol's <c>ByteSize</c> as if the PLC's byte count were a bit
+    ///     count — <c>.OPC_MACHINEDATA.PROCESSSTATUS</c> (STRING(80)) came back as 11 bytes, an
+    ///     <c>INT</c> member as 1, a <c>UDINT</c> as 1 — while its resolved <c>DataType.ByteSize</c> was
+    ///     the true 81 / 2 / 4 (raw <c>SYM_INFOBYNAMEEX</c> answers 81). Sizing a string read by the
+    ///     symbol truncated <c>"ready for operation"</c> to <c>"ready for o"</c> at Good, and the
+    ///     notification width check refused every numeric member. The resolved type's size wins.
+    /// </summary>
+    [Fact]
+    public void Classify_prefers_the_resolved_data_type_size_over_the_symbol_size()
+    {
+        TwinCATPlcSymbolShape.Classify("STRING(80)", DataTypeCategory.String, byteSize: 11, dataTypeByteSize: 81)
+            .ByteSize.ShouldBe(81);
+        var member = TwinCATPlcSymbolShape.Classify("INT", DataTypeCategory.Primitive, byteSize: 1, dataTypeByteSize: 2);
+        member.ByteSize.ShouldBe(2);
+        TwinCATTypeGuard.CheckNotification(TwinCATDataType.Int, member).ShouldBeNull();
+        TwinCATPlcSymbolShape.Classify("UDINT", DataTypeCategory.Primitive, byteSize: 1, dataTypeByteSize: 4).ByteSize.ShouldBe(4);
+
+        // No resolved type (or a zero size) falls back to the symbol's own size.
+        TwinCATPlcSymbolShape.Classify("STRING(30)", null, byteSize: 31, dataTypeByteSize: null).ByteSize.ShouldBe(31);
+        TwinCATPlcSymbolShape.Classify("STRING(30)", null, byteSize: 31, dataTypeByteSize: 0).ByteSize.ShouldBe(31);
+    }
+
+    /// <summary>
+    ///     Live on Z33856 (2026-10-07): <c>P_INITINPUT_ACTION.SLOTID</c> is a TwinCAT 2
+    ///     <c>STRING(20) (VAR_IN_OUT)</c> — a 4-byte reference — but the Beckhoff library resolves its
+    ///     <c>DataType</c> to the plain <c>STRING(20)</c> (category String). Classifying by the resolved
+    ///     type alone let a String tag read 21 bytes starting at the pointer and publish them as text at
+    ///     Good (<c>"n±\u008dâa»‰â…"</c>). The symbol's own type name marks it a reference, and that wins.
+    /// </summary>
+    [Fact]
+    public void Classify_treats_a_var_in_out_symbol_as_non_scalar_even_when_its_resolved_type_is_a_string()
+    {
+        var shape = TwinCATPlcSymbolShape.Classify("STRING(20)", DataTypeCategory.String, byteSize: 4,
+            dataTypeByteSize: 21, symbolTypeName: "STRING(20) (VAR_IN_OUT)");
+        shape.Kind.ShouldBe(TwinCATPlcKind.NonScalar);
+        TwinCATTypeGuard.CheckRead(TwinCATDataType.String, shape).ShouldNotBeNull();
+        TwinCATTypeGuard.CheckNotification(TwinCATDataType.String, shape).ShouldNotBeNull();
+    }
+
    // ---- read path (symbol-aware: only the string kind can go wrong) ----

    /// <summary>