From ff055ebd79bdb8c841f1285e90549d65a5c951ed Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 14:38:49 -0400 Subject: [PATCH] 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 | 9 ++++ .../S7DriverOptions.cs | 32 ++++++++++++++ .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 2 +- .../S7DriverFactoryExtensions.cs | 3 +- .../S7DriverProbe.cs | 40 ++++++++++++++++++ .../S7ValueCodecs.cs | 2 + .../Shared/Drivers/Forms/S7DriverForm.razor | 10 +++++ .../S7TsapAndPduTests.cs | 42 +++++++++++++++++++ .../S7ValueCodecTests.cs | 10 +++++ .../S7DriverPageFormSerializationTests.cs | 24 +++++++++++ 10 files changed, 171 insertions(+), 3 deletions(-) diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 711ae322..23ce7de8 100644 --- a/docs/drivers/S7.md +++ b/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 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs index fdba6eb3..d94968c3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs @@ -56,6 +56,7 @@ public sealed class S7DriverOptions /// "0100", "0x0100"). Null/blank keeps S7.Net's per-CPU default (01.00). /// Needed for CPUs / gateways that reserve connection resources by TSAP (S7-400H, CP343, LOGO!). /// + [System.Text.Json.Serialization.JsonConverter(typeof(S7TsapJsonConverter))] public string? LocalTsap { get; init; } /// @@ -63,6 +64,7 @@ public sealed class S7DriverOptions /// (e.g. "03.01" = S7-basic resource, rack 0 slot 1). Null/blank keeps the slot-derived default. /// Mutually exclusive with . /// + [System.Text.Json.Serialization.JsonConverter(typeof(S7TsapJsonConverter))] public string? RemoteTsap { get; init; } /// @@ -72,6 +74,11 @@ public sealed class S7DriverOptions /// public S7ConnectionType? ConnectionType { get; init; } + /// The authoring/load error for setting both and ; shared by the factory and the AdminUI form so they never disagree. + public const string TsapConflictDetail = + "sets both RemoteTsap and ConnectionType; ConnectionType only fills the first byte of the " + + "slot-derived remote TSAP, so choose one."; + /// Connect + per-operation timeout. public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(5); @@ -160,6 +167,31 @@ public sealed class S7DriverOptions public const int DefaultSpanReadMaxSpanBytes = 220; } +/// +/// Reads a TSAP as either hex text ("03.01") or a JSON number (769, 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. +/// +public sealed class S7TsapJsonConverter : System.Text.Json.Serialization.JsonConverter +{ + /// + 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."); + } + + /// + 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 { /// Gets or sets a value indicating whether probing is enabled. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index b0fdeece..7c2a0ef1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/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. /// - private static bool IsAccessDenied(PlcException pex) + internal static bool IsAccessDenied(PlcException pex) { for (Exception? e = pex; e is not null; e = e.InnerException) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs index d93cafb6..e769dba8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs @@ -96,8 +96,7 @@ public static class S7DriverFactoryExtensions : ParseEnum(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 { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs index 2bde0697..d936b98a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs +++ b/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 } } + /// + /// 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. + /// + /// The minimal read. + /// Cancellation token. + /// A failure result, or null on success. + internal static async Task CheckDataAccessAsync(Func> 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); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ValueCodecs.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ValueCodecs.cs index 9b4a23a6..8017987b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ValueCodecs.cs +++ b/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); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/S7DriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/S7DriverForm.razor index 830903a4..b68cf28d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/S7DriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/S7DriverForm.razor @@ -56,6 +56,10 @@
Optional hex override, e.g. 03.01. Not combinable with Connection type.
+ @if (_form.TsapConflictError() is { } tsapError) + { + + } @@ -183,6 +187,12 @@ public string? ResilienceConfig { get; set; } public byte[] RowVersion { get; set; } = []; + /// The same error the driver factory raises for Remote TSAP + Connection type together; null when fine. + 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, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs index 956ba2b1..f1dd5dde 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs +++ b/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(() => 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"); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs index 2d182d34..bb285f52 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs +++ b/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(() => Encode(S7DataType.S5Time, 100.7)); // rounds to 101: off the grid + } + // ---- TIME ---- [Theory] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/S7DriverPageFormSerializationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/S7DriverPageFormSerializationTests.cs index 09a98e61..e4ab368a 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/S7DriverPageFormSerializationTests.cs +++ b/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( + """{"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(); + } }