Merge lane S7: S7 driver remediation (#742)

Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs

@@ -48,8 +48,23 @@ public static class S7TagDefinitionFactory
     /// <param name="def">The mapped definition when this returns <see langword="true"/>.</param>
     /// <returns><see langword="true"/> when <paramref name="tagConfig"/> is a valid S7 address object.</returns>
     public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def)
+        => FromTagConfig(tagConfig, rawPath, out def, out _);
+
+    /// <summary>
+    ///     <see cref="FromTagConfig(string, string, out S7TagDefinition)"/>, also reporting WHY a structurally
+    ///     valid blob was refused by <see cref="S7TagShapeRules"/> (an unparseable address, or a shape the
+    ///     driver cannot serve). <paramref name="shapeRejection"/> stays <see langword="null"/> for a JSON-level
+    ///     refusal (missing keys, bad enum), which <see cref="Inspect"/> explains on its own.
+    /// </summary>
+    /// <param name="tagConfig">The authored equipment-tag TagConfig JSON.</param>
+    /// <param name="rawPath">The tag's RawPath — becomes the definition's identity (<c>Name</c>).</param>
+    /// <param name="def">The mapped definition when this returns <see langword="true"/>.</param>
+    /// <param name="shapeRejection">The shape/address rejection reason, when that is why it refused.</param>
+    /// <returns><see langword="true"/> when the driver can serve the tag.</returns>
+    public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def, out string? shapeRejection)
     {
         def = null!;
+        shapeRejection = null;
         if (string.IsNullOrWhiteSpace(tagConfig)) return false;
         try
         {
@@ -78,7 +93,7 @@ public static class S7TagDefinitionFactory
             // "writable" defaults to true when absent (today's value); node-level authz still governs
             // writes. Honouring the key makes read-only equipment tags authorable.
             var writable = TagConfigJson.ReadWritable(root);
-            def = new S7TagDefinition(
+            var mapped = new S7TagDefinition(
                 Name: rawPath,
                 Address: address,
                 DataType: dataType,
@@ -86,6 +101,15 @@ public static class S7TagDefinitionFactory
                 StringLength: stringLength == 0 ? MaxStringLength : stringLength,
                 WriteIdempotent: false,
                 ArrayCount: arrayCount);
+            // The address must parse and the shape must be one the driver can serve. These used to be
+            // init-time guards that THREW and faulted every tag on the PLC; refusing here skips only
+            // this tag, and the editor + deploy gate (which ask this method) refuse it too.
+            if (S7TagShapeRules.Validate(mapped, out _) is { } reason)
+            {
+                shapeRejection = reason;
+                return false;
+            }
+            def = mapped;
             return true;
         }
         catch (JsonException) { return false; }
@@ -162,7 +186,9 @@ public static class S7TagDefinitionFactory
     ///     <b>reject</b> — a structurally unparseable or non-object document, an absent / non-string /
     ///     blank <c>address</c>, an <b>absent</b> <c>dataType</c> (see <see cref="DeclaresDataType"/>), a
     ///     present-but-invalid <c>dataType</c> (the runtime enum read is STRICT, so a
-    ///     typo rejects the tag), or an out-of-range <c>stringLength</c> on a <c>String</c> tag. Such a tag
+    ///     typo rejects the tag), an out-of-range <c>stringLength</c> on a <c>String</c> tag, or a shape
+    ///     <see cref="S7TagShapeRules"/> refuses (an unparseable address, a wrong access width, a mis-typed
+    ///     Timer/Counter, an unsupported array). Such a tag
     ///     is skipped by the driver and reads <c>BadNodeIdUnknown</c> forever. Empty when the mapper accepts
     ///     the blob, when it is blank, or when it is not an equipment-tag object. Never throws.
     /// </summary>
@@ -184,7 +210,13 @@ public static class S7TagDefinitionFactory
 
         // The accept gate: whatever the driver reads draws nothing. The RAW reference is handed to the
         // mapper — the driver reads the stored blob, not a trimmed copy of it.
-        if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings;
+        if (FromTagConfig(reference, InspectionRawPath, out _, out var shapeRejection)) return warnings;
+
+        if (shapeRejection is not null)
+        {
+            warnings.Add(shapeRejection + " The driver will skip this tag and it will read BadNodeIdUnknown.");
+            return warnings;
+        }
 
         try
         {

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs

@@ -0,0 +1,149 @@
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7;
+
+/// <summary>
+///     <b>The single authority on which S7 tag shapes the driver can serve.</b> Consulted by
+///     <see cref="S7TagDefinitionFactory.FromTagConfig"/>, so the AdminUI editor, the deploy gate
+///     (<see cref="S7TagDefinitionFactory.Inspect"/>) and the driver's own init all refuse the same
+///     tags with the same words.
+/// </summary>
+/// <remarks>
+///     <para>
+///         These rules used to be init-time guards in <c>S7Driver</c> that THREW. One mis-authored tag
+///         (<c>DB1.DBZ0</c>, <c>DB1.DBW0</c> typed <c>Float64</c>, <c>T5</c> typed <c>Int32</c>) faulted the
+///         whole driver, and the actor retried that same init every 10 s forever, darkening every tag on
+///         the PLC. The editor and the deploy gate checked none of it, and a CSV import or API blob
+///         bypasses the editor entirely. A rejected tag is now skipped on its own (it reads
+///         <c>BadNodeIdUnknown</c>) and the deploy gate warns about it.
+///     </para>
+///     <para>
+///         Every rule rejects a tag that could never be served correctly. None of them can newly break
+///         a tag that used to work: each one either faulted init, or failed (or silently mis-decoded)
+///         every read of that tag.
+///     </para>
+/// </remarks>
+public static class S7TagShapeRules
+{
+    /// <summary>
+    ///     S7 data types addressed by START BYTE (the <c>B</c> suffix) rather than by an access-width
+    ///     suffix. The driver decodes them from a contiguous byte buffer, so a wide tag authored against a
+    ///     non-byte suffix (DBW/DBD/...) would mis-frame its value.
+    /// </summary>
+    private static readonly HashSet<S7DataType> ByteAnchoredDataTypes =
+    [
+        S7DataType.Int64,
+        S7DataType.UInt64,
+        S7DataType.Float64,
+        S7DataType.String,
+        S7DataType.DateTime,
+        S7DataType.WString,
+        S7DataType.Char,
+        S7DataType.WChar,
+        S7DataType.Dtl,
+        S7DataType.Date,
+        S7DataType.Tod,
+        S7DataType.Time,
+        S7DataType.S5Time,
+    ];
+
+    /// <summary>True when <paramref name="dataType"/> is a wide/structured type read from a byte-anchored block.</summary>
+    /// <param name="dataType">The declared data type.</param>
+    /// <returns><see langword="true"/> for the byte-anchored types.</returns>
+    public static bool IsByteAnchored(S7DataType dataType) => ByteAnchoredDataTypes.Contains(dataType);
+
+    /// <summary>
+    ///     Judges one tag definition. Returns <see langword="null"/> when the driver can serve it, or
+    ///     the operator-facing reason it cannot.
+    /// </summary>
+    /// <param name="def">The mapped tag definition.</param>
+    /// <param name="parsed">The parsed address when this returns <see langword="null"/>.</param>
+    /// <returns>The rejection reason, or <see langword="null"/> when the tag is servable.</returns>
+    public static string? Validate(S7TagDefinition def, out S7ParsedAddress parsed)
+    {
+        ArgumentNullException.ThrowIfNull(def);
+        parsed = default;
+
+        S7ParsedAddress addr;
+        try
+        {
+            addr = S7AddressParser.Parse(def.Address);
+        }
+        catch (FormatException ex)
+        {
+            return $"S7 address '{def.Address}' does not parse: {ex.Message}";
+        }
+
+        var isWide = IsByteAnchored(def.DataType);
+        var isArray = def.ArrayCount is >= 1;
+
+        // Wide-type arrays have no decoder.
+        if (isWide && isArray)
+            return $"S7 {def.DataType} arrays are not supported. Use a scalar {def.DataType} tag, or an " +
+                   "Int16/UInt16/Int32/UInt32/Float32/Byte/Bool element type for an array.";
+
+        // Timer / Counter type constraints: a timer decodes to Float64 seconds, a counter to an Int32 count.
+        if (addr.Area is S7Area.Timer && def.DataType != S7DataType.Float64)
+            return $"S7 Timer address '{def.Address}' is typed {def.DataType}. Timer tags must be " +
+                   "DataType=Float64 (decoded to seconds, read-only).";
+        if (addr.Area is S7Area.Counter && def.DataType != S7DataType.Int32)
+            return $"S7 Counter address '{def.Address}' is typed {def.DataType}. Counter tags must be " +
+                   "DataType=Int32 (decoded to a count, read-only).";
+        if (addr.Area is S7Area.Timer or S7Area.Counter)
+        {
+            if (isArray)
+                return $"S7 Timer/Counter address '{def.Address}' cannot be an array.";
+            if (def.Writable)
+                return $"S7 Timer/Counter address '{def.Address}' is declared writable. Timer/Counter tags " +
+                       "are read-only; set Writable=false.";
+            parsed = addr;
+            return null;
+        }
+
+        // Wide/structured scalars on DB/M/I/Q must be byte-anchored (DBB/MB/IB/QB).
+        if (isWide)
+        {
+            if (addr.Size != S7Size.Byte)
+                return $"S7 {def.DataType} at '{def.Address}' is not byte-addressed. Wide/structured types " +
+                       "must be byte-addressed (DBB/MB/IB/QB), e.g. 'DB1.DBB0', 'MB4'.";
+            parsed = addr;
+            return null;
+        }
+
+        // Narrow types: the declared type must match the address's access width, or every read fails
+        // (or decodes garbage) with a type mismatch.
+        var required = def.DataType switch
+        {
+            S7DataType.Bool => S7Size.Bit,
+            S7DataType.Byte => S7Size.Byte,
+            S7DataType.Int16 or S7DataType.UInt16 => S7Size.Word,
+            _ => S7Size.DWord, // Int32 / UInt32 / Float32
+        };
+        if (addr.Size != required)
+            return $"S7 {def.DataType} at '{def.Address}' has the wrong access width ({addr.Size}). " +
+                   $"A {def.DataType} tag needs a {SuffixHint(required)} address.";
+
+        if (isArray && def.DataType == S7DataType.Bool)
+        {
+            // Element i of a Bool array is bit 0 of byte (start + i): the array is byte-granular. A
+            // non-zero bit offset used to be silently ignored (DB1.DBX0.3 returned bit 0 of bytes 0..n).
+            if (addr.BitOffset != 0)
+                return $"S7 Bool array at '{def.Address}' starts at bit {addr.BitOffset}. A Bool array reads " +
+                       "one byte per element (bit 0 of each byte), so it must start at bit 0 (e.g. 'DB1.DBX0.0').";
+            // A byte-granular write would overwrite the 7 neighbouring bits of every byte it touches.
+            if (def.Writable)
+                return $"S7 Bool array at '{def.Address}' is declared writable. Bool array writes are not " +
+                       "supported (a byte-granular write would clobber neighbouring bits); set Writable=false " +
+                       "or write the bits as individual scalar Bool tags.";
+        }
+
+        parsed = addr;
+        return null;
+    }
+
+    private static string SuffixHint(S7Size size) => size switch
+    {
+        S7Size.Bit => "bit (DBX/M/I/Q n.b)",
+        S7Size.Byte => "byte (DBB/MB/IB/QB)",
+        S7Size.Word => "word (DBW/MW/IW/QW)",
+        _ => "double-word (DBD/MD/ID/QD)",
+    };
+}

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs

@@ -42,6 +42,14 @@ public sealed class S7DriverProbe : IDriverProbe
         if (string.IsNullOrWhiteSpace(host) || port <= 0)
             return new(false, "Config has no host/port to probe.", null);
 
+        // The S7 handshake + test read are bounded by the driver's own per-operation timeout
+        // (config "timeoutMs", what the deployed driver will wait), capped by the caller's overall
+        // Test Connect timeout. The options type carries it as a TimeSpan the JSON never names, so it
+        // used to be ignored and every handshake waited the full Test Connect timeout (R07:H7).
+        var opTimeout = ReadTimeoutMs(configJson) is { } ms && TimeSpan.FromMilliseconds(ms) < timeout
+            ? TimeSpan.FromMilliseconds(ms)
+            : timeout;
+
         // Phase 1: bare TCP preflight — fast rejection for unreachable hosts.
         var sw = Stopwatch.StartNew();
         try
@@ -71,9 +79,9 @@ public sealed class S7DriverProbe : IDriverProbe
         var plc = S7Tsap.Resolve(opts) is { } tsap
             ? new Plc(host, port, tsap)
             : new Plc(S7CpuTypeMap.ToS7Net(opts.CpuType), host, port, opts.Rack, opts.Slot);
-        plc.ReadTimeout = (int)timeout.TotalMilliseconds;
+        plc.ReadTimeout = (int)opTimeout.TotalMilliseconds;
         using var handshakeCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
-        handshakeCts.CancelAfter(timeout);
+        handshakeCts.CancelAfter(opTimeout);
         try
         {
             await plc.OpenAsync(handshakeCts.Token);
@@ -128,15 +136,17 @@ public sealed class S7DriverProbe : IDriverProbe
             return null;
         }
         catch (OperationCanceledException) { throw; }
-        catch (PlcException pex) when (S7Driver.IsAccessDenied(pex))
+        // S7.Net's read paths raise the CPU's item error as a BARE Exception ("Received error from
+        // PLC: …"), not a PlcException, so both classifiers walk the whole chain (the driver's own).
+        catch (Exception ex) when (S7Driver.IsAccessDenied(ex))
         {
             return new(false,
                 "S7 connected but data access is denied — enable PUT/GET communication in TIA Portal " +
-                $"(Protection & Security) for this CPU. PLC reported: {pex.Message}", null);
+                $"(Protection & Security) for this CPU. PLC reported: {ex.Message}", null);
         }
-        catch (PlcException pex) when (pex.ErrorCode == ErrorCode.ReadData)
+        catch (Exception ex) when (S7Driver.IsRefusal(ex))
         {
-            return null; // address not available on this CPU — not a connection problem
+            return null; // address not available on this CPU — the CPU answered; not a connection problem
         }
         catch (Exception ex)
         {
@@ -146,4 +156,31 @@ public sealed class S7DriverProbe : IDriverProbe
 
     private static (string host, int port) ExtractTarget(S7DriverOptions opts)
         => (opts.Host, opts.Port);
+
+    /// <summary>
+    ///     The config's <c>timeoutMs</c> (any casing, as the driver factory binds it), or null when absent
+    ///     or not a positive integer.
+    /// </summary>
+    /// <param name="configJson">The driver config.</param>
+    /// <returns>The configured per-operation timeout in milliseconds, or null.</returns>
+    internal static int? ReadTimeoutMs(string configJson)
+    {
+        try
+        {
+            using var doc = JsonDocument.Parse(configJson);
+            if (doc.RootElement.ValueKind != JsonValueKind.Object) return null;
+            foreach (var prop in doc.RootElement.EnumerateObject())
+            {
+                if (!string.Equals(prop.Name, "timeoutMs", StringComparison.OrdinalIgnoreCase)) continue;
+                return prop.Value.ValueKind == JsonValueKind.Number && prop.Value.TryGetInt32(out var ms) && ms > 0
+                    ? ms
+                    : null;
+            }
+            return null;
+        }
+        catch (JsonException)
+        {
+            return null;
+        }
+    }
 }

src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs

@@ -11,8 +11,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.S7;
 ///         <b>The safety invariant is that a span never extends past its last member's end.</b> A
 ///         span may deliberately read bytes nobody asked for BETWEEN its members (that is what the
 ///         gap budget buys), but every byte it requests lies inside a range some member was going
-///         to read anyway. A grouped read therefore cannot fault (area overrun, DB overrun) where
-///         the individual per-tag reads would have succeeded.
+///         to read anyway.
+///     </para>
+///     <para>
+///         <b>That does NOT make a grouped read as safe as the per-tag reads</b> (R07:P2): the block is
+///         the union of its members' ranges, so ONE member the CPU refuses (running past the DB's real
+///         end, an over-declared STRING) makes it refuse the whole block, neighbours included.
+///         <see cref="S7Driver.ReadAsync"/> owns that case: a refused multi-member span is re-read one
+///         member at a time, and the members that must not be grouped are left out of later plans.
 ///     </para>
 ///     <para>
 ///         <b>Timer and Counter tags are never grouped.</b> Their