fix(modbus): review r0 - FC23 opt-in, explicit maxReadGap 0 = off, legacy coil types load, one health writer

Coordinator rulings and Important findings from review r0: 1. FC23 is opt-in (new UseFC23ForMultiRegisterWrites, default false); a config without it - including the persisted "disableFC23": false every form-saved config carries - keeps the FC16 wire. DisableFC23 stays the hard kill-switch. 2. An explicit maxReadGap 0 means coalescing off again (its pre-lane meaning); the gap-8 default applies only to an absent key. 3. Coils/DiscreteInputs tags persisted with a register dataType (the old editor saved Int16) are coerced to Bool, never refused, with no deploy warning (a warning can be promoted to a blocking error). The editor loads them as Bool. Register region + Bool is still refused. 4. One comms-fault slot for reads and writes: a good read clears a write's transport fault and vice versa. Minors: Dispose writes its own Unknown health (m1); persisted region/address are authoritative over addressString so a Family change cannot remap a live tag (m2); a malformed-but-delivered reply no longer fails the unit fast (m3); a fresh socket published after a concurrent dispose is torn down (m4); the editor resets address text per tag (m5); master merged (m6); the FC23 latch log says re-init restores it (m7). Back-compat tests use the old persisted JSON shapes and fail without the fixes. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 15:55:32 -04:00
parent c12269c221
commit fd6ecf8293
15 changed files with 314 additions and 117 deletions
@@ -89,16 +89,24 @@ public sealed class ModbusDriverOptions
    public bool UseFC16ForSingleRegisterWrites { get; init; } = false;

    /// <summary>
-    ///     Kill-switch for FC23 (Read/Write Multiple Registers). By default a multi-register holding
-    ///     write (2–121 registers: 32/64-bit values, arrays, strings) is sent as ONE FC23 that writes
-    ///     the block and reads the same block back, so the device confirms what it now holds. A unit
-    ///     that answers FC23 with exception 01 (MELSEC QJ71MT91, S7 Modbus servers) is moved to FC16
-    ///     automatically, for that write and every later one; one that fails FC23 at the transport is
-    ///     moved too (that one write is reported as a comms failure). Set <c>true</c> to always use
-    ///     FC16. Default <c>false</c>.
+    ///     Hard kill-switch for FC23 (Read/Write Multiple Registers): when <c>true</c> the driver never
+    ///     emits FC23, even with <see cref="UseFC23ForMultiRegisterWrites"/> set. Default <c>false</c>.
+    ///     FC23 is OPT-IN — a config without <see cref="UseFC23ForMultiRegisterWrites"/> keeps the FC16 wire
+    ///     behaviour, so the persisted <c>"disableFC23": false</c> every form-saved config carries changes
+    ///     nothing on the wire.
    /// </summary>
    public bool DisableFC23 { get; init; } = false;

+    /// <summary>
+    ///     Opt-in: send a multi-register holding write (2–121 registers: 32/64-bit values, arrays, strings)
+    ///     as ONE FC23 that writes the block and reads the same block back, so the device confirms what it
+    ///     now holds. A unit that answers FC23 with exception 01 (MELSEC QJ71MT91, S7 Modbus servers) moves
+    ///     to FC16 for that write and every later one until the next initialize; one that fails FC23 at the
+    ///     transport moves too (that one write is reported as a comms failure). Ignored when
+    ///     <see cref="DisableFC23"/> is set. Default <c>false</c> = FC16, the historical behaviour.
+    /// </summary>
+    public bool UseFC23ForMultiRegisterWrites { get; init; } = false;
+
    /// <summary>
    ///     Interval for the background re-probe loop that retries auto-prohibited
    ///     coalesced ranges. When non-null, every <c>AutoProhibitReprobeInterval</c>
@@ -110,14 +118,13 @@ public sealed class ModbusDriverOptions
    public TimeSpan? AutoProhibitReprobeInterval { get; init; } = null;

    /// <summary>
-    ///     Block-read coalescing budget. The read planner combines tags in the same (UnitId, Region)
-    ///     group whose addresses are at most this many registers (Coils / DiscreteInputs: bits) apart
-    ///     into a single FC03/FC04/FC01/FC02 read, then slices the response back into per-tag values.
-    ///     <c>0</c> coalesces only adjacent or overlapping tags (several <c>BitInRegister</c> tags on one
-    ///     status word still fold into one read). Default <c>8</c>. A range the device refuses is
-    ///     auto-prohibited and read per tag, so a protected hole costs one extra round trip, not data. To
-    ///     turn coalescing off entirely set <see cref="DisableReadCoalescing"/>, or exempt one tag with
-    ///     its <c>coalesceProhibited</c> key.
+    ///     Block-read coalescing budget. When non-zero, the read planner combines tags in the same
+    ///     (UnitId, Region) group whose addresses are at most this many registers (Coils / DiscreteInputs:
+    ///     bits) apart into a single FC03/FC04/FC01/FC02 read, then slices the response back into per-tag
+    ///     values (several <c>BitInRegister</c> tags on one status word fold into one read). <c>0</c> = no
+    ///     coalescing — an explicitly authored 0 keeps that historical meaning. Default <c>8</c> applies
+    ///     only when the key is ABSENT. A range the device refuses is auto-prohibited and read per tag.
+    ///     <see cref="DisableReadCoalescing"/> also turns it off; <c>coalesceProhibited</c> exempts one tag.
    /// </summary>
    public ushort MaxReadGap { get; init; } = DefaultMaxReadGap;

@@ -35,12 +35,14 @@ public static class ModbusTagDefinitionFactory
        => FromTagConfig(tagConfig, rawPath, ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out def);

    /// <summary>
-    ///     Family-aware overload: an <c>addressString</c> key (the Modicon / mnemonic / family-native grammar
-    ///     of <see cref="ModbusAddressParser"/> — <c>40001:F:CDAB</c>, DL205 <c>V2000</c>, MELSEC <c>D100</c>)
-    ///     is resolved with the DRIVER's <paramref name="family"/> / <paramref name="melsecSubFamily"/> and is
-    ///     authoritative for region, offset, type, bit, string length, byte order and array count; the
-    ///     editor also persists the resolved <c>region</c> / <c>address</c> keys beside it for display and for
-    ///     readers that predate it. Without an <c>addressString</c> this is exactly the 3-argument overload.
+    ///     Family-aware overload. The persisted <c>region</c> + <c>address</c> keys are AUTHORITATIVE whenever
+    ///     present — the editor resolves an <c>addressString</c> with the driver's family and persists the
+    ///     result, so a later Family / MelsecSubFamily change can never silently remap a live tag. A blob that
+    ///     carries ONLY an <c>addressString</c> (CSV / API authoring, the Modicon / mnemonic / family-native
+    ///     grammar of <see cref="ModbusAddressParser"/> — <c>40001:F:CDAB</c>, DL205 <c>V2000</c>, MELSEC
+    ///     <c>D100</c>) is resolved here with the DRIVER's <paramref name="family"/> /
+    ///     <paramref name="melsecSubFamily"/>. Without an <c>addressString</c> this is exactly the 3-argument
+    ///     overload.
    /// </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>
@@ -67,8 +69,9 @@ public static class ModbusTagDefinitionFactory
            ushort stringLength;
            int? arrayCount;
            var addressText = ReadAddressString(root, out var addressStringInvalid);
-            if (addressStringInvalid) return false;
-            if (addressText is not null)
+            var hasResolvedKeys = root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _);
+            if (!hasResolvedKeys && addressStringInvalid) return false;
+            if (!hasResolvedKeys && addressText is not null)
            {
                if (!ModbusAddressParser.TryParse(addressText, family, melsecSubFamily, out var parsed, out _) || parsed is null)
                    return false;
@@ -106,9 +109,12 @@ public static class ModbusTagDefinitionFactory
                var arrayLength = ReadInt(root, "arrayLength");
                arrayCount = (isArray && arrayLength >= 1) ? arrayLength : null;
            }
-            // Region/type compatibility: a bit region carries only Bool, a register region never Bool.
-            // A mismatch used to throw out of the coalescing planner (killing every tag's poll) or
-            // silently return a bool into an Int32 node.
+            // Region/type compatibility. A bit region (Coils / DiscreteInputs) carries only Bool: a non-Bool
+            // dataType there is COERCED to Bool, never refused — the pre-#741 editor defaulted dataType to
+            // Int16 and persisted it for coil tags, and the old driver ignored dataType for bit regions, so
+            // those tags read and wrote correctly and must keep doing so. A register region with Bool is
+            // refused: that pair never read (it threw out of the planner and killed every tag's poll).
+            if (region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs) dataType = ModbusDataType.Bool;
            if (!IsRegionTypeCompatible(region, dataType)) return false;
            if (!TagConfigJson.TryReadEnumStrict(root, "stringByteOrder", ModbusStringByteOrder.HighByteFirst, out var stringByteOrder)) return false;
            // Guard: String tags require StringLength >= 1. RegisterCount = (StringLength+1)/2,
@@ -314,14 +320,15 @@ public static class ModbusTagDefinitionFactory
    private static void DescribeRejection(JsonElement root, List<string> warnings)
    {
        var addressText = ReadAddressString(root, out var addressStringInvalid);
-        if (addressStringInvalid)
+        var hasResolvedKeys = root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _);
+        if (!hasResolvedKeys && addressStringInvalid)
        {
            warnings.Add("Modbus TagConfig 'addressString' is not a string — the tag will not resolve (BadNodeIdUnknown).");
            return;
        }
-        if (addressText is not null)
+        if (!hasResolvedKeys && addressText is not null)
        {
-            // The addressString is authoritative; the other address keys are not read at all.
+            // String-only blob: the addressString is what the driver resolves.
            if (!ModbusAddressParser.TryParse(addressText, out _, out var error))
                warnings.Add($"Modbus 'addressString' '{addressText}' does not parse under any PLC family ({error}) — "
                             + "the tag will not resolve (BadNodeIdUnknown).");
@@ -372,12 +379,12 @@ public static class ModbusTagDefinitionFactory
            ? r : ModbusRegion.HoldingRegisters;
        if (!TagConfigJson.TryReadEnumStrict(root, "dataType", DefaultDataType(region), out var dataType)) return;

-        if (root.TryGetProperty("region", out _) && !IsRegionTypeCompatible(region, dataType))
+        // Only a register region with Bool is refused (a bit region with any type is coerced to Bool).
+        if (root.TryGetProperty("region", out _) && region is not (ModbusRegion.Coils or ModbusRegion.DiscreteInputs)
+            && !IsRegionTypeCompatible(region, dataType))
        {
-            warnings.Add(region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs
-                ? $"Modbus region {region} only carries Bool tags; got {dataType} — the tag is rejected (BadNodeIdUnknown)."
-                : $"Modbus Bool tags only live in Coils / DiscreteInputs; a {region} tag must use a register type "
-                  + "(use BitInRegister for a bit of a register) — the tag is rejected (BadNodeIdUnknown).");
+            warnings.Add($"Modbus Bool tags only live in Coils / DiscreteInputs; a {region} tag must use a register type "
+                         + "(use BitInRegister for a bit of a register) — the tag is rejected (BadNodeIdUnknown).");
        }

        if (dataType == ModbusDataType.String && (ushort)ReadInt(root, "stringLength") < 1)