fix(s7): numeric TSAP loads in form, shared TSAP conflict error, Test Connect read detects PUT/GET disabled, pin Int32 ms rounding (#632 #638)

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

docs/drivers/S7.md

@@ -82,6 +82,13 @@ malformed TSAP fails to load rather than silently falling back to the slot-deriv
 the three set the connection is built exactly as before. The override applies to the driver, the AdminUI
 Test Connect probe and the AdminUI S7 driver form (Connection type / Local TSAP / Remote TSAP fields).
 
+### Test Connect and PUT/GET
+
+After the handshake, Test Connect reads 2 bytes from `MB0`. A CPU with PUT/GET disabled (the S7-1200/1500
+default) completes the handshake but denies every read, so the probe fails with an actionable "enable
+PUT/GET communication" message instead of a false green tick. A CPU that merely rejects that address (no
+`MB0`) still passes — the connection is fine; any other read fault fails the probe with its message.
+
 ## Address forms
 
 Addresses use Siemens TIA-Portal / STEP 7 Classic syntax, parsed by
@@ -151,6 +158,8 @@ These add no new OPC UA data types — they map onto existing ones:
 | `Time` | **Int32 ms** (signed duration) | Int32 ms | outside the Int32 range |
 | `S5Time` | **Int32 ms** | 2-bit time base (10 ms / 100 ms / 1 s / 10 s) + 3 BCD digits | above 9 990 000 ms, negative, or not exactly representable (resolution is 10 ms up to 9 990 ms, then coarser) — **never rounded** |
 
+A fractional millisecond `Double` is rounded to the nearest whole millisecond (banker's rounding), exactly
+as the driver does for every other Int32 tag write; only values off the S5TIME time-base grid are refused.
 A write of a `TimeSpan` is also accepted for `Tod` / `Time` / `S5Time`. A PLC buffer the codec cannot
 decode — an uninitialised (all-zero) `Dtl`, a non-BCD `S5Time` digit, a `WString` whose current length
 exceeds its reserved length — reads `BadOutOfRange` for that tag only, without degrading driver health

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

@@ -56,6 +56,7 @@ public sealed class S7DriverOptions
     ///     <c>"0100"</c>, <c>"0x0100"</c>). Null/blank keeps S7.Net's per-CPU default (<c>01.00</c>).
     ///     Needed for CPUs / gateways that reserve connection resources by TSAP (S7-400H, CP343, LOGO!).
     /// </summary>
+    [System.Text.Json.Serialization.JsonConverter(typeof(S7TsapJsonConverter))]
     public string? LocalTsap { get; init; }
 
     /// <summary>
@@ -63,6 +64,7 @@ public sealed class S7DriverOptions
     ///     (e.g. <c>"03.01"</c> = S7-basic resource, rack 0 slot 1). Null/blank keeps the slot-derived default.
     ///     Mutually exclusive with <see cref="ConnectionType"/>.
     /// </summary>
+    [System.Text.Json.Serialization.JsonConverter(typeof(S7TsapJsonConverter))]
     public string? RemoteTsap { get; init; }
 
     /// <summary>
@@ -72,6 +74,11 @@ public sealed class S7DriverOptions
     /// </summary>
     public S7ConnectionType? ConnectionType { get; init; }
 
+    /// <summary>The authoring/load error for setting both <see cref="RemoteTsap"/> and <see cref="ConnectionType"/>; shared by the factory and the AdminUI form so they never disagree.</summary>
+    public const string TsapConflictDetail =
+        "sets both RemoteTsap and ConnectionType; ConnectionType only fills the first byte of the " +
+        "slot-derived remote TSAP, so choose one.";
+
     /// <summary>Connect + per-operation timeout.</summary>
     public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(5);
 
@@ -160,6 +167,31 @@ public sealed class S7DriverOptions
     public const int DefaultSpanReadMaxSpanBytes = 220;
 }
 
+/// <summary>
+///     Reads a TSAP as either hex text (<c>"03.01"</c>) or a JSON number (<c>769</c>, the form the driver
+///     factory also accepts) and always writes the hex text. Without it a numeric TSAP would fail to bind to
+///     the string property, and a typed consumer (the AdminUI form, the Test Connect probe) would fall back
+///     to defaults — a form Save would then silently erase the authored config.
+/// </summary>
+public sealed class S7TsapJsonConverter : System.Text.Json.Serialization.JsonConverter<string?>
+{
+    /// <inheritdoc />
+    public override string? Read(ref System.Text.Json.Utf8JsonReader reader, Type typeToConvert, System.Text.Json.JsonSerializerOptions options)
+    {
+        if (reader.TokenType == System.Text.Json.JsonTokenType.Null) return null;
+        if (reader.TokenType == System.Text.Json.JsonTokenType.Number && reader.TryGetUInt16(out var n))
+            return $"{n >> 8:X2}.{n & 0xFF:X2}";
+        if (reader.TokenType == System.Text.Json.JsonTokenType.String) return reader.GetString();
+        throw new System.Text.Json.JsonException("S7 TSAP must be hex text such as \"03.01\" or a 16-bit number.");
+    }
+
+    /// <inheritdoc />
+    public override void Write(System.Text.Json.Utf8JsonWriter writer, string? value, System.Text.Json.JsonSerializerOptions options)
+    {
+        if (value is null) writer.WriteNullValue(); else writer.WriteStringValue(value);
+    }
+}
+
 public sealed class S7ProbeOptions
 {
     /// <summary>Gets or sets a value indicating whether probing is enabled.</summary>

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

@@ -2335,7 +2335,7 @@ public sealed class S7Driver
     ///     no typed error code for it, so the inner message is the only discriminator
     ///     S7.Net exposes.
     /// </summary>
-    private static bool IsAccessDenied(PlcException pex)
+    internal static bool IsAccessDenied(PlcException pex)
     {
         for (Exception? e = pex; e is not null; e = e.InnerException)
         {

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

@@ -96,8 +96,7 @@ public static class S7DriverFactoryExtensions
             : ParseEnum<S7ConnectionType>(dto.ConnectionType, driverInstanceId, "ConnectionType");
         if (remoteTsap is not null && connectionType is not null)
             throw new InvalidOperationException(
-                $"S7 driver config for '{driverInstanceId}' sets both RemoteTsap and ConnectionType; " +
-                "ConnectionType only fills the first byte of the slot-derived remote TSAP, so choose one.");
+                $"S7 driver config for '{driverInstanceId}' {S7DriverOptions.TsapConflictDetail}");
 
         return new S7DriverOptions
         {

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

@@ -80,7 +80,15 @@ public sealed class S7DriverProbe : IDriverProbe
             sw.Stop();
 
             if (plc.IsConnected)
+            {
+                // A handshake alone passes on a CPU with PUT/GET disabled (S7-1200/1500 default) — every
+                // later read would fail. One minimal read proves data access.
+                if (await CheckDataAccessAsync(
+                        t => plc.ReadBytesAsync(global::S7.Net.DataType.Memory, 0, 0, 2, t), handshakeCts.Token)
+                    is { } denied)
+                    return denied;
                 return new(true, $"S7 connected (CPU {opts.CpuType}, PDU {plc.MaxPDUSize} bytes)", sw.Elapsed);
+            }
 
             return new(false, $"Reachable at {host}:{port} but S7 handshake failed: not connected", null);
         }
@@ -104,6 +112,38 @@ public sealed class S7DriverProbe : IDriverProbe
         }
     }
 
+    /// <summary>
+    ///     Runs one minimal read after the handshake. Returns null when data access works — including when the
+    ///     CPU merely rejects that particular address (MB0 not present / out of range: the connection is fine).
+    ///     PUT/GET-disabled yields the actionable message; any other fault is a probe failure with its message.
+    /// </summary>
+    /// <param name="read">The minimal read.</param>
+    /// <param name="ct">Cancellation token.</param>
+    /// <returns>A failure result, or null on success.</returns>
+    internal static async Task<DriverProbeResult?> CheckDataAccessAsync(Func<CancellationToken, Task<byte[]>> read, CancellationToken ct)
+    {
+        try
+        {
+            await read(ct).ConfigureAwait(false);
+            return null;
+        }
+        catch (OperationCanceledException) { throw; }
+        catch (PlcException pex) when (S7Driver.IsAccessDenied(pex))
+        {
+            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);
+        }
+        catch (PlcException pex) when (pex.ErrorCode == ErrorCode.ReadData)
+        {
+            return null; // address not available on this CPU — not a connection problem
+        }
+        catch (Exception ex)
+        {
+            return new(false, $"S7 connected but a test read failed: {ex.Message}", null);
+        }
+    }
+
     private static (string host, int port) ExtractTarget(S7DriverOptions opts)
         => (opts.Host, opts.Port);
 }

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

@@ -287,6 +287,8 @@ internal static class S7ValueCodecs
         }
     }
 
+    // Fractional input is rounded (Convert.ToInt64, banker's), matching the driver's existing Int32 write
+    // path (BoxValueForWrite → Convert.ToInt32). "Refused, not rounded" applies to S5TIME's time-base grid.
     private static long ToMilliseconds(object? value, string type)
     {
         try { return value is TimeSpan ts ? (long)ts.TotalMilliseconds : Convert.ToInt64(value, System.Globalization.CultureInfo.InvariantCulture); }

src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/S7DriverForm.razor

@@ -56,6 +56,10 @@
                 <div class="form-text">Optional hex override, e.g. <code>03.01</code>. Not combinable with Connection type.</div>
             </div>
         </div>
+        @if (_form.TsapConflictError() is { } tsapError)
+        {
+            <div class="text-danger small" role="alert">@tsapError</div>
+        }
     </div>
 </section>
 
@@ -183,6 +187,12 @@
         public string? ResilienceConfig { get; set; }
         public byte[] RowVersion { get; set; } = [];
 
+        /// <summary>The same error the driver factory raises for Remote TSAP + Connection type together; null when fine.</summary>
+        public string? TsapConflictError() =>
+            !string.IsNullOrWhiteSpace(RemoteTsap) && !string.IsNullOrWhiteSpace(ConnectionTypeText)
+                ? "S7 driver config " + S7DriverOptions.TsapConflictDetail
+                : null;
+
         public static FormModel FromOptions(S7DriverOptions o) => new()
         {
             Host = o.Host,

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs

@@ -188,4 +188,46 @@ public sealed class S7TsapAndPduTests
             public void Dispose() => IsConnected = false;
         }
     }
+
+    [Fact]
+    public void Factory_conflict_message_matches_the_shared_detail()
+    {
+        var ex = Should.Throw<InvalidOperationException>(() => S7DriverFactoryExtensions.ParseOptions("d",
+            """{"host":"h","remoteTsap":"03.01","connectionType":"OP"}"""));
+        ex.Message.ShouldContain(S7DriverOptions.TsapConflictDetail);
+    }
+
+    // ---- Test Connect data-access check (PUT/GET) ----
+
+    [Fact]
+    public async Task Probe_data_access_succeeds_when_the_read_works() =>
+        (await S7DriverProbe.CheckDataAccessAsync(_ => Task.FromResult(new byte[2]), TestContext.Current.CancellationToken))
+            .ShouldBeNull();
+
+    [Fact]
+    public async Task Probe_fails_with_actionable_message_when_put_get_is_disabled()
+    {
+        var denied = new global::S7.Net.PlcException(global::S7.Net.ErrorCode.ReadData,
+            new Exception("Accessing object not allowed"));
+        var r = await S7DriverProbe.CheckDataAccessAsync(_ => throw denied, TestContext.Current.CancellationToken);
+        r.ShouldNotBeNull().Ok.ShouldBeFalse();
+        r.Message!.ShouldContain("PUT/GET");
+    }
+
+    [Fact]
+    public async Task Probe_tolerates_an_address_not_available_read_error()
+    {
+        var missing = new global::S7.Net.PlcException(global::S7.Net.ErrorCode.ReadData, new Exception("Object does not exist"));
+        (await S7DriverProbe.CheckDataAccessAsync(_ => throw missing, TestContext.Current.CancellationToken)).ShouldBeNull();
+    }
+
+    [Fact]
+    public async Task Probe_reports_any_other_read_fault_as_a_failure()
+    {
+        var r = await S7DriverProbe.CheckDataAccessAsync(
+            _ => throw new global::S7.Net.PlcException(global::S7.Net.ErrorCode.ConnectionError, "socket dropped"),
+            TestContext.Current.CancellationToken);
+        r.ShouldNotBeNull().Ok.ShouldBeFalse();
+        r.Message!.ShouldContain("socket dropped");
+    }
 }

tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs

@@ -221,6 +221,16 @@ public sealed class S7ValueCodecTests
         Encode(S7DataType.Tod, TimeSpan.FromHours(1)).ShouldBe([0, 0x36, 0xEE, 0x80]);
     }
 
+    [Fact]
+    public void Fractional_millisecond_double_rounds_like_the_other_int32_writes()
+    {
+        // Pinned: matches BoxValueForWrite's Convert.ToInt32 behaviour (banker's), see docs.
+        Encode(S7DataType.Tod, 100.7).ShouldBe(Encode(S7DataType.Tod, 101));
+        Encode(S7DataType.Time, 100.7).ShouldBe(Encode(S7DataType.Time, 101));
+        Encode(S7DataType.S5Time, 99.7).ShouldBe(Encode(S7DataType.S5Time, 100));
+        Should.Throw<S7ValueRangeException>(() => Encode(S7DataType.S5Time, 100.7)); // rounds to 101: off the grid
+    }
+
     // ---- TIME ----
 
     [Theory]

tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/S7DriverPageFormSerializationTests.cs

@@ -138,4 +138,28 @@ public sealed class S7DriverPageFormSerializationTests
             new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase, Converters = { new JsonStringEnumConverter() } });
         json.ShouldContain("\"connectionType\":\"PG\"");
     }
+
+    [Fact]
+    public void Numeric_tsap_loads_into_the_form_model_instead_of_falling_back_to_defaults()
+    {
+        // The factory accepts a JSON number; the form binds the same document the same way, so a Save
+        // cannot silently replace the authored config with defaults.
+        var opts = JsonSerializer.Deserialize<S7DriverOptions>(
+            """{"remoteTsap":769,"localTsap":"01.00","cpuType":"S7300"}""",
+            new JsonSerializerOptions { PropertyNameCaseInsensitive = true, Converters = { new JsonStringEnumConverter() } });
+        opts.ShouldNotBeNull();
+        var back = S7DriverForm.FormModel.FromOptions(opts).ToOptions();
+        back.RemoteTsap.ShouldBe("03.01");
+        back.LocalTsap.ShouldBe("01.00");
+        back.CpuType.ShouldBe(S7CpuType.S7300);
+    }
+
+    [Fact]
+    public void Form_reports_the_factory_conflict_error_for_remote_tsap_plus_connection_type()
+    {
+        var form = new S7DriverForm.FormModel { RemoteTsap = "03.01", ConnectionTypeText = "PG" };
+        form.TsapConflictError().ShouldNotBeNull().ShouldContain(S7DriverOptions.TsapConflictDetail);
+        new S7DriverForm.FormModel { RemoteTsap = "03.01" }.TsapConflictError().ShouldBeNull();
+        new S7DriverForm.FormModel { ConnectionTypeText = "PG" }.TsapConflictError().ShouldBeNull();
+    }
 }