From e3cb2dd591d84c75e486e2ab8b8f8fd1cf5d8e21 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 06:59:29 -0400 Subject: [PATCH 1/3] fix(s7): thread ILoggerFactory into the S7 driver factory (#602 CONV-3) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../S7DriverFactoryExtensions.cs | 19 +++++++++++++++---- .../Drivers/DriverFactoryBootstrap.cs | 2 +- 2 files changed, 16 insertions(+), 5 deletions(-) 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 36d754c0..8fa880aa 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs @@ -1,5 +1,6 @@ using System.Text.Json; using System.Text.Json.Serialization; +using Microsoft.Extensions.Logging; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Hosting; @@ -16,22 +17,32 @@ public static class S7DriverFactoryExtensions public const string DriverTypeName = "S7"; /// Registers the S7 driver factory with the registry. + /// + /// The optional is captured at registration time and used to + /// build an per driver instance — without it the driver runs on + /// the null logger and none of its diagnostics (connect/reconnect, skipped tags, poll faults) + /// reach the node log (CONV-3). + /// /// The driver factory registry. - public static void Register(DriverFactoryRegistry registry) + /// Optional logger factory for driver instances. + public static void Register(DriverFactoryRegistry registry, ILoggerFactory? loggerFactory = null) { ArgumentNullException.ThrowIfNull(registry); - registry.Register(DriverTypeName, CreateInstance); + registry.Register(DriverTypeName, (id, json) => CreateInstance(id, json, loggerFactory)); } /// Creates a new S7 driver instance from configuration. /// The unique identifier for the driver instance. /// The JSON configuration for the driver. /// A newly created S7 driver instance. - internal static S7Driver CreateInstance(string driverInstanceId, string driverConfigJson) + /// Optional logger factory for the driver instance. + internal static S7Driver CreateInstance( + string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory = null) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); - return new S7Driver(ParseOptions(driverInstanceId, driverConfigJson), driverInstanceId); + return new S7Driver(ParseOptions(driverInstanceId, driverConfigJson), driverInstanceId, + logger: loggerFactory?.CreateLogger()); } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs index dc687cf8..bbab2d28 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs @@ -226,7 +226,7 @@ public static class DriverFactoryBootstrap // task, so the Tier A 5s Subscribe budget never covers the stream's lifetime. Driver.MTConnect.MTConnectDriverFactoryExtensions.Register(registry, loggerFactory); Driver.OpcUaClient.OpcUaClientDriverFactoryExtensions.Register(registry, loggerFactory, secretResolver); - Driver.S7.S7DriverFactoryExtensions.Register(registry); + Driver.S7.S7DriverFactoryExtensions.Register(registry, loggerFactory); Driver.Sql.SqlDriverFactoryExtensions.Register(registry, loggerFactory); Driver.TwinCAT.TwinCATDriverFactoryExtensions.Register(registry); } From 439752d3b8791d92f61ea855518674a4139a9ddb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 06:59:29 -0400 Subject: [PATCH 2/3] feat(adminui): S7 tag editor rejects wide-type and writable-Bool arrays at author time (#610) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../Uns/TagEditors/S7TagConfigModel.cs | 20 ++++++++++ .../Uns/S7TagConfigModelTests.cs | 40 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs index 1692f39a..3e944d4a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs @@ -112,6 +112,26 @@ public sealed class S7TagConfigModel + string.Join(", ", Enum.GetNames()) + "."; } + // S7-016: array shape is judged against the driver's INIT guards, which THROW (failing the whole + // driver's init, not just this tag) for a wide-type array and for a writable Bool array. The + // isArray / arrayLength keys themselves belong to the TagModal's generic array controls + // (TagArrayConfig) and are intentionally not fields of this model, so read them off the key bag. + if (TagConfigJson.GetBool(_bag, "isArray")) + { + var dt = DataType; + if (dt is S7DataType.Int64 or S7DataType.UInt64 or S7DataType.Float64 + or S7DataType.String or S7DataType.DateTime) + { + return $"S7 does not support {dt} arrays — use Bool (read-only), Byte, Int16, UInt16, Int32, " + + "UInt32 or Float32 for an array tag, or a scalar tag."; + } + if (dt == S7DataType.Bool && (Writable ?? true)) + { + return "A Bool array cannot be writable (a byte-granular write would clobber neighbouring bits) — " + + "uncheck Writable, or use scalar Bool tags."; + } + } + // Gitea #554: the reader's remaining, non-enum refusal — a String tag whose 'stringLength' is // negative or above 254 (S7 REJECTS it rather than clamping) — is not restated here. The verdict and // the wording both come from the driver's own reader, via TagConfigParseGate. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs index dbbccfa4..5152626c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs @@ -103,4 +103,44 @@ public sealed class S7TagConfigModelTests [InlineData("""{"address":"DB1.DBW0","dataType":7}""")] public void Validate_accepts_every_dataType_the_driver_accepts(string json) => S7TagConfigModel.FromJson(json).Validate().ShouldBeNull(); + + // ---- S7-016: array shape vs the driver's init guards (which fail the WHOLE driver init) ---- + + [Theory] + [InlineData("Int64")] + [InlineData("UInt64")] + [InlineData("Float64")] + [InlineData("String")] + [InlineData("DateTime")] + public void Validate_rejects_wide_type_arrays(string dataType) + { + var m = S7TagConfigModel.FromJson( + $$"""{"address":"DB1.DBB0","dataType":"{{dataType}}","isArray":true,"arrayLength":4,"writable":false}"""); + + m.Validate().ShouldNotBeNull().ShouldContain("arrays"); + } + + [Fact] + public void Validate_rejects_a_writable_bool_array_but_accepts_it_read_only() + { + var writable = S7TagConfigModel.FromJson( + """{"address":"DB1.DBX0.0","dataType":"Bool","isArray":true,"arrayLength":4}"""); + writable.Validate().ShouldNotBeNull().ShouldContain("Bool array"); + + var readOnly = S7TagConfigModel.FromJson( + """{"address":"DB1.DBX0.0","dataType":"Bool","isArray":true,"arrayLength":4,"writable":false}"""); + readOnly.Validate().ShouldBeNull(); + } + + [Theory] + [InlineData("Byte", "DB1.DBB0")] + [InlineData("Int16", "DB1.DBW0")] + [InlineData("Float32", "DB1.DBD0")] + public void Validate_accepts_writable_arrays_of_supported_element_types(string dataType, string address) + { + var m = S7TagConfigModel.FromJson( + $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}"""); + + m.Validate().ShouldBeNull(); + } } From 82372f167280c3b9db751ee87ae84346ea61fe1f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 06:59:29 -0400 Subject: [PATCH 3/3] fix(s7): connect backoff, worst-state write health, Volatile health, array writes (#602, #610) STAB-21 ConnectionBackoff in EnsureConnectedAsync; STAB-22 write-pass accumulator with a write-side Faulted that a clean read does not erase; _health on the Volatile idiom; S7-016 contiguous array write (Bool arrays stay refused). Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- code-reviews/Driver.S7/findings.md | 2 + docs/Uns.md | 2 +- docs/drivers/S7.md | 28 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 323 ++++++++++++--- .../S7_1500/S7_1500ArrayWriteTests.cs | 52 +++ .../S7ArrayWriteAndHealthTests.cs | 382 ++++++++++++++++++ .../S7DriverReconnectTests.cs | 6 +- .../S7DriverScaffoldTests.cs | 44 +- 8 files changed, 769 insertions(+), 70 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ArrayWriteTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs diff --git a/code-reviews/Driver.S7/findings.md b/code-reviews/Driver.S7/findings.md index c10bcb88..ed9d00d7 100644 --- a/code-reviews/Driver.S7/findings.md +++ b/code-reviews/Driver.S7/findings.md @@ -495,3 +495,5 @@ open-ended deferral. **Recommendation:** Add `int? ArrayCount` and `bool? IsArray` to `S7TagDto` and propagate them through `BuildTag`. Mirror the `S7EquipmentTagParser` semantics (honour `arrayLength` only when `isArray` is true and the value is positive). **Resolution:** Deferred — requires an AdminUI page/DTO change and a corresponding config-form field addition. Tracked as a follow-up to the array-read Phase 4d work. No code change in this review; the existing init guard-(e) correctly rejects any programmatically-constructed writable array reaching the driver until the DTO is wired. + +**Update (2026-10-08, #610 item 2):** the DTO half was already stale — `S7TagDto` is gone and `S7TagDefinitionFactory` reads `isArray`/`arrayLength` from TagConfig. Remaining pieces now done: the driver write path (`EncodeArrayBlock` + one contiguous `WriteBytesAsync`; guard-(e) narrowed to writable **Bool** arrays, which would clobber neighbouring bits), and AdminUI author-time validation in `S7TagConfigModel` (the isArray/arrayLength controls live in the TagModal's generic `TagArrayConfig`, not the S7 editor). Server-side exposure of array writes stays with #610 item 1. diff --git a/docs/Uns.md b/docs/Uns.md index b900f3d9..c053afab 100644 --- a/docs/Uns.md +++ b/docs/Uns.md @@ -314,7 +314,7 @@ Combined with historization (values are arrays — history of the whole array sn ### Out of scope (named deferrals) -- **Array writes** (inbound client→device write of an array value) — tagged for a follow-up phase. +- **Array writes** (inbound client→device write of an array value) — tagged for a follow-up phase. The S7 driver's array write path exists (`Byte`/`Int16`/`UInt16`/`Int32`/`UInt32`/`Float32`, not `Bool`; see [S7.md](drivers/S7.md)) but the applier still forces array nodes read-only (#610 item 1). - **Multi-dimensional arrays** (`ValueRank > 1`) — not supported; all arrays are 1-D. - **Array historization** — a historized array tag materialises with the correct `Historizing` flag, but the historian backend treats the value as an opaque blob; per-element history is out of scope. (Continuous historization is numeric-analog only — array / non-numeric values are not recorded.) diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 492f2faa..fea36579 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -195,6 +195,21 @@ Portal under *Protection & Security* for the CPU. | PUT/GET denied | `BadNotSupported` | `Faulted` (config alert) | | CPU / hardware fault | `BadDeviceFailure` | `Degraded` | | Socket / timeout | `BadCommunicationError` | `Degraded` | +| Reconnect suppressed (backoff window) | `BadCommunicationError` | `Degraded` | +| Array write of the wrong shape / length / element type | `BadTypeMismatch` | unchanged | +| Bool / wide-type array write | `BadNotSupported` | unchanged | + +**Health is decided once per pass, worst state wins.** A read pass and a write batch each +accumulate their outcomes and write health once (`Healthy < Degraded < Faulted`), so a PUT/GET-denied +`Faulted` item is not erased by a later `Degraded` or successful sibling. The write-side `Faulted` is +**sticky against reads**: a clean read pass (reads often work against a write-protected CPU) leaves +`Faulted` in place and only advances `LastSuccessfulRead`; the next write that succeeds clears it. A +transient `Degraded` write failure is still cleared by a clean read, as before. + +**Connect backoff.** A dead PLC costs one connect attempt per backoff window (1 s doubling to a 30 s +cap), not one per read / poll tick / probe tick: inside the window `EnsureConnectedAsync` fails fast +(`BadCommunicationError`, `Degraded`) without a connect. A successful connect clears the window, so a +fresh drop after a healthy period reconnects immediately; worst-case recovery lag is the 30 s cap. ## Testing @@ -228,7 +243,18 @@ normal dev (the S7 sim is on the shared Docker host but currently offline). **Live-verify** — integration-fixture-gated (not Mac-verifiable without the S7 sim up). -**Deferrals** — array *writes*, multi-dimensional arrays, per-element historization, +**Array writes (S7-016)** — a writable `Byte` / `Int16` / `UInt16` / `Int32` / `UInt32` / `Float32` array +is written as ONE contiguous `WriteBytesAsync` of `count x element-bytes` (the inverse of the read +decode), and the value must be an array of exactly `arrayLength` elements (else `BadTypeMismatch`, no +health impact). **Writable `Bool` arrays are refused at init** (`NotSupportedException`): the array +read is byte-granular, so a matching write would overwrite the 7 neighbouring bits of every byte it +touches, and a safe bit-packed write needs read-modify-write. Mark Bool arrays `writable: false`. +Note the *server* still presents every array node read-only to OPC UA clients ([Uns.md §Array +tags](../Uns.md#array-tags-1-d), #610 item 1) — the driver path is reachable today through the Client +CLI / driver-level callers and is proven by the unit suite plus the snap7 fixture test +(`S7_1500ArrayWriteTests`); the AdminUI tag editor rejects wide-type and writable-Bool arrays at author time. + +**Deferrals** — server-side exposure of array writes, multi-dimensional arrays, per-element historization, wide-type array elements (`Int64`, `UInt64`, `Float64`, `String`, `DateTime`), and Timer/Counter array elements. 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 bed5e1bc..8aa78144 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -115,6 +115,8 @@ public sealed class S7Driver private const uint StatusBadNodeIdUnknown = 0x80340000u; /// OPC UA StatusCode used when the tag's data type isn't implemented yet. private const uint StatusBadNotSupported = 0x803D0000u; + /// OPC UA StatusCode used when a written value has the wrong shape / length / element type. + private const uint StatusBadTypeMismatch = 0x80740000u; /// OPC UA StatusCode used when the tag is declared read-only. private const uint StatusBadNotWritable = 0x803B0000u; /// OPC UA StatusCode used when write fails validation (e.g. out-of-range value). @@ -179,6 +181,38 @@ public sealed class S7Driver private bool _plcDead; private DriverHealth _health = new(DriverState.Unknown, null, null); + + /// + /// The health snapshot, accessed with volatile semantics: it is written from the gate-holding + /// read/write passes, the poll engine's error sink and the lifecycle methods, and read lock-free + /// by (the health tick) — the same Volatile idiom ModbusDriver uses. + /// is an immutable record, so a reference swap is a complete update. + /// + private DriverHealth Health + { + get => Volatile.Read(ref _health); + set => Volatile.Write(ref _health, value); + } + + /// + /// Per-device connect-attempt throttle (STAB-21): a dead PLC costs one connect attempt per + /// backoff window (1 s doubling to 30 s) instead of one per data call / poll tick / probe tick. + /// Not internally synchronized — only touched from (under + /// ) and from init/shutdown, which run with every loop drained. + /// + private readonly ConnectionBackoff _connectBackoff = + new(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(30)); + + /// Clock the connect backoff is measured on. Internal so tests step it deterministically. + internal Func BackoffClock { get; set; } = static () => DateTime.UtcNow; + + /// + /// The sticky write-side config fault (access denied / PUT-GET disabled), or null. A read + /// pass that finds everything readable must NOT erase it (reads can succeed while writes are + /// protected); it is cleared by the next write pass that writes something successfully. + /// Guarded by . + /// + private (DriverState State, string Message)? _writeFault; private bool _disposed; /// @@ -189,7 +223,7 @@ public sealed class S7Driver /// public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { - _health = new DriverHealth(DriverState.Initializing, null, null); + Health = new DriverHealth(DriverState.Initializing, null, null); try { // Re-parse the supplied DriverConfig JSON so a config change delivered through the @@ -263,6 +297,7 @@ public sealed class S7Driver Plc = plc; _plcDead = false; + _connectBackoff.RecordSuccess(); // Parse every tag's address once at init so config typos fail fast here instead // of surfacing as BadInternalError on every Read against the bad tag. The parser @@ -274,7 +309,7 @@ public sealed class S7Driver } _initialized = true; - _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); + Health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _logger.LogInformation("S7Driver connected. Driver={DriverInstanceId} Host={Host} CPU={CpuType} Tags={TagCount}", _driverInstanceId, _options.Host, _options.CpuType, _tagsByRawPath.Count); @@ -299,7 +334,7 @@ public sealed class S7Driver Plc = null; _initialized = false; _plcDead = false; - _health = new DriverHealth(DriverState.Faulted, null, ex.Message); + Health = new DriverHealth(DriverState.Faulted, null, ex.Message); _logger.LogError(ex, "S7Driver connect failed. Driver={DriverInstanceId} Host={Host}", _driverInstanceId, _options.Host); throw; } @@ -346,13 +381,15 @@ public sealed class S7Driver _initialized = false; _plcDead = false; + _writeFault = null; + _connectBackoff.RecordSuccess(); try { Plc?.Dispose(); } catch { /* best-effort — tearing down anyway */ } Plc = null; - _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); } /// - public DriverHealth GetHealth() => _health; + public DriverHealth GetHealth() => Health; /// /// True when carries a real config body. The @@ -453,19 +490,18 @@ public sealed class S7Driver "Timer/Counter are read-only this phase; set Writable=false."); } - // (e) Array tag declared Writable — WriteOneAsync has no WriteArrayAsync path yet. - // Without this guard a writable array node is discovered and - // accepted, then every write returns BadCommunicationError (InvalidCastException from - // BoxValueForWrite receiving a typed array) instead of the informative BadNotSupported - // the caller could act on. Wide-type arrays are already rejected above (guard-a); this - // targets non-wide types (Int16[], Byte[], Float32[], etc.) that ARE supported for - // reading but not yet for writing. - if (t.ArrayCount is >= 1 && t.Writable) + // (e) Bool array declared Writable — element writes are byte-granular (one byte per + // element, mirroring the read), so a write would overwrite the 7 neighbouring bits of + // every byte it touches; a safe bit-packed write needs read-modify-write and is not + // implemented. Every other supported element type (Byte/Int16/UInt16/Int32/UInt32/ + // Float32) writes as ONE contiguous block (EncodeArrayBlock) — S7-016. Wide-type arrays + // are already rejected above (guard-a). + if (t.ArrayCount is >= 1 && t.Writable && t.DataType == S7DataType.Bool) { throw new NotSupportedException( - $"S7 tag '{t.Name}' is a {t.DataType} array (ArrayCount={t.ArrayCount}) declared Writable — " + - "array writes are not yet supported by the S7 driver. Set Writable=false for read-only array tags, " + - "or split into individual scalar tags until array-write support lands."); + $"S7 tag '{t.Name}' is a Bool array (ArrayCount={t.ArrayCount}) declared Writable — " + + "Bool array writes are not supported (a byte-granular write would clobber the neighbouring " + + "bits). Set Writable=false, or write the bits as individual scalar Bool tags."); } } } @@ -567,7 +603,7 @@ public sealed class S7Driver { for (var i = 0; i < fullReferences.Count; i++) results[i] = new DataValueSnapshot(null, StatusBadCommunicationError, null, now); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + Health = new DriverHealth(DriverState.Degraded, Health.LastSuccessfulRead, ex.Message); return results; } @@ -683,12 +719,23 @@ public sealed class S7Driver // // LastSuccessfulRead advances whenever ANYTHING read, even into a Degraded/Faulted // state — see ReadPassOutcome for why that is load-bearing. + // + // A sticky write-side Faulted (PUT/GET denied) outranks a read pass that found less + // wrong: reads can succeed against a CPU whose writes are protected, and a clean read + // must not erase the config alert. The write pass clears it on its next success. if (outcome.HasObservations) { - _health = new DriverHealth( - outcome.WorstFailure, - outcome.AnySuccess ? now : _health.LastSuccessfulRead, - outcome.WorstFailure == DriverState.Healthy ? null : outcome.WorstError); + var state = outcome.WorstFailure; + var error = state == DriverState.Healthy ? null : outcome.WorstError; + if (_writeFault is { } wf && ReadPassOutcome.Rank(wf.State) > ReadPassOutcome.Rank(state)) + { + state = wf.State; + error = wf.Message; + } + Health = new DriverHealth( + state, + outcome.AnySuccess ? now : Health.LastSuccessfulRead, + error); } _gate.Release(); @@ -698,7 +745,7 @@ public sealed class S7Driver /// /// The outcomes ONE pass observed, accumulated so the pass writes - /// exactly once — worst state wins. + /// exactly once — worst state wins. /// /// /// @@ -780,7 +827,7 @@ public sealed class S7Driver /// Severity order the pass ranks by: Healthy (none) < Degraded < Faulted. /// The state to rank. /// Its severity, higher being worse. - private static int Rank(DriverState state) => state switch + internal static int Rank(DriverState state) => state switch { DriverState.Faulted => 2, DriverState.Degraded => 1, @@ -1129,7 +1176,7 @@ public sealed class S7Driver /// /// /// The health state is recorded, not applied. It goes into - /// so the pass can write once at the + /// so the pass can write once at the /// end, worst-state-wins — writing it here is what let a later good tag erase this /// failure. The _plcDead / side effects /// stay IMMEDIATE: they govern whether the very next read reopens the handle, which @@ -1691,6 +1738,8 @@ public sealed class S7Driver ArgumentNullException.ThrowIfNull(writes); RequireInitialized(); var results = new WriteResult[writes.Count]; + // Health is decided ONCE, at the end of the batch, worst-state-wins — see WritePassOutcome. + var outcome = new WritePassOutcome(); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); try @@ -1709,7 +1758,7 @@ public sealed class S7Driver { for (var i = 0; i < writes.Count; i++) results[i] = new WriteResult(StatusBadCommunicationError); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + outcome.RecordFailure(DriverState.Degraded, ex.Message); return results; } @@ -1746,6 +1795,7 @@ public sealed class S7Driver { await WriteOneAsync(plc, tag, w.Value, cancellationToken).ConfigureAwait(false); results[i] = new WriteResult(0u); + outcome.RecordSuccess(); } catch (OperationCanceledException) { @@ -1761,13 +1811,18 @@ public sealed class S7Driver { results[i] = new WriteResult(StatusBadNotSupported); } + catch (S7WriteValueException) + { + // Client-supplied value of the wrong shape — not a device or comms fault. + results[i] = new WriteResult(StatusBadTypeMismatch); + } catch (PlcException pex) when (IsAccessDenied(pex)) { // PUT/GET-disabled / access-protection on write — same permanent // configuration fault as on read. BadNotSupported + // a config-alert health state, not a transient device failure. results[i] = new WriteResult(StatusBadNotSupported); - _health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead, + outcome.RecordFailure(DriverState.Faulted, "S7 access denied — enable PUT/GET communication in TIA Portal " + $"(Protection & Security) for this CPU. PLC reported: {pex.Message}"); } @@ -1779,7 +1834,7 @@ public sealed class S7Driver // dead so the next call reopens. MarkConnectionDeadIfFatal(pex); results[i] = new WriteResult(StatusBadDeviceFailure); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, pex.Message); + outcome.RecordFailure(DriverState.Degraded, pex.Message); } catch (Exception ex) { @@ -1788,25 +1843,84 @@ public sealed class S7Driver // dead when it's a connection loss. MarkConnectionDeadIfFatal(ex); results[i] = new WriteResult(StatusBadCommunicationError); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + outcome.RecordFailure(DriverState.Degraded, ex.Message); } } } - finally { _gate.Release(); } + finally + { + // THE single health write of the batch (a finally, so an OCE mid-batch still flushes the + // evidence gathered so far — a pass that observed nothing writes nothing). + ApplyWritePassHealth(outcome); + _gate.Release(); + } return results; } + /// + /// Publishes one write batch's verdict (STAB-22): worst state wins over every outcome the + /// batch recorded, so a Faulted access-denied item is not erased by a later Degraded or + /// successful sibling. Maintains , the sticky Faulted a read pass + /// must not erase. Caller holds . + /// + /// The batch's accumulated outcomes. + private void ApplyWritePassHealth(WritePassOutcome outcome) + { + if (!outcome.HasObservations) return; + + var worst = outcome.WorstFailure; + if (worst == DriverState.Faulted) + _writeFault = (DriverState.Faulted, outcome.WorstError!); + else if (outcome.AnySuccess) + _writeFault = null; // a write got through — the protection fault is gone + + if (worst == DriverState.Healthy) + { + // Success-only batch: no health write, except to retire a Faulted that WE (the write + // side) were holding — a clean write is the only evidence the alert has cleared. + if (outcome.AnySuccess && _writeFault is null && Health.State == DriverState.Faulted) + Health = new DriverHealth(DriverState.Healthy, Health.LastSuccessfulRead, null); + return; + } + + var state = worst; + var error = outcome.WorstError; + // A Degraded-only batch must not downgrade a still-sticky Faulted. + if (_writeFault is { } wf && ReadPassOutcome.Rank(wf.State) > ReadPassOutcome.Rank(state)) + { + state = wf.State; + error = wf.Message; + } + Health = new DriverHealth(state, Health.LastSuccessfulRead, error); + } + + /// + /// The outcomes ONE batch observed — the write-side sibling of + /// (same worst-state-wins ranking and severity-pinned message), + /// accumulated so the batch writes once instead of per item. + /// + private sealed class WritePassOutcome + { + private readonly ReadPassOutcome _inner = new(); + + /// True once any item in the batch wrote successfully. + public bool AnySuccess => _inner.AnySuccess; + /// The worst failure recorded; Healthy means none. + public DriverState WorstFailure => _inner.WorstFailure; + /// The message explaining . + public string? WorstError => _inner.WorstError; + /// True once the batch observed a success or a failure. + public bool HasObservations => _inner.HasObservations; + /// Records one successful write. + public void RecordSuccess() => _inner.RecordSuccess(); + /// Records one failed write. + /// The state the failure would have stamped alone. + /// The operator-facing message. + public void RecordFailure(DriverState state, string message) => _inner.RecordFailure(state, message); + } + private async Task WriteOneAsync(IS7Plc plc, S7TagDefinition tag, object? value, CancellationToken ct) { - // Defence-in-depth guard: authored array tags are rejected at init - // (RejectUnsupportedTagConfigs guard-e), but a transient equipment-tag ref resolved by - // _resolver bypasses that path. Both should fail with NotSupportedException → BadNotSupported, - // not InvalidCastException → BadCommunicationError from BoxValueForWrite receiving an array. - if (tag.ArrayCount is >= 1) - throw new NotSupportedException( - $"S7 array writes are not yet supported (tag '{tag.Name}'). " + - "Use individual scalar tags or set Writable=false for read-only arrays."); - // Parse the address the same way ReadOneAsync does: every tag pre-parses at init // (_parsedByRawPath), with an on-demand parse as a defensive fallback. Needed here so the // wide-type write can byte-address the block (the narrow path below addresses by the raw @@ -1815,6 +1929,17 @@ public sealed class S7Driver ? parsed : S7AddressParser.Parse(tag.Address); + // Array path (S7-016): ONE contiguous WriteBytesAsync of count × element-bytes — the inverse + // of ReadArrayAsync. Bool / wide-type arrays throw NotSupportedException (→ BadNotSupported); + // a wrong-shaped value throws S7WriteValueException (→ BadTypeMismatch, no health impact). + if (tag.ArrayCount is >= 1) + { + var arrayBytes = EncodeArrayBlock(tag, addr, value); + await plc.WriteBytesAsync(ToS7NetArea(addr.Area), addr.DbNumber, addr.ByteOffset, arrayBytes, ct) + .ConfigureAwait(false); + return; + } + // Wide/structured scalar path: encode the value to a big-endian byte block and write it // at the start byte via S7.Net's buffer-based WriteBytesAsync. Mirrors the read seam; // the narrow string-based write below stays unchanged for 1/2/4-byte types. @@ -1832,6 +1957,90 @@ public sealed class S7Driver await plc.WriteAsync(tag.Address, boxed, ct).ConfigureAwait(false); } + /// + /// Pure encode loop — the inverse of : turns the caller's array + /// into the contiguous big-endian block for a single WriteBytesAsync. No network I/O. + /// Accepts any non-string (OPC UA hands the server + /// a typed array, but a boxed object[] is equally fine); each element is coerced with + /// Convert.To* like the scalar path. + /// + /// Tag definition carrying the element type and declared array length. + /// Parsed address carrying the access . + /// The array to encode; must hold exactly ArrayCount elements. + /// The big-endian block, count × element-bytes long. + /// Bool or wide-type element arrays. + /// The value is not an array, has the wrong length, or an + /// element does not convert to the element type. + internal static byte[] EncodeArrayBlock(S7TagDefinition tag, S7ParsedAddress addr, object? value) + { + var count = tag.ArrayCount!.Value; + + switch (tag.DataType, addr.Size) + { + case (S7DataType.Bool, _): + throw new NotSupportedException( + $"S7 Bool array writes are not supported (tag '{tag.Name}'): a byte-granular write would clobber neighbouring bits."); + case (S7DataType.Int64, _): + case (S7DataType.UInt64, _): + case (S7DataType.Float64, _): + case (S7DataType.String, _): + case (S7DataType.DateTime, _): + throw new NotSupportedException($"S7 array writes of {tag.DataType} are not supported (tag '{tag.Name}')."); + case (S7DataType.Byte, S7Size.Byte): + case (S7DataType.UInt16, S7Size.Word): + case (S7DataType.Int16, S7Size.Word): + case (S7DataType.UInt32, S7Size.DWord): + case (S7DataType.Int32, S7Size.DWord): + case (S7DataType.Float32, S7Size.DWord): + break; + default: + throw new System.IO.InvalidDataException( + $"S7 array Write type-mismatch: tag '{tag.Name}' declared {tag.DataType} but address " + + $"'{tag.Address}' parsed as Size={addr.Size}"); + } + + if (value is string || value is not System.Collections.IEnumerable items) + throw new S7WriteValueException($"S7 array tag '{tag.Name}' requires an array value."); + + var elements = new List(count); + foreach (var e in items) elements.Add(e); + if (elements.Count != count) + throw new S7WriteValueException( + $"S7 array tag '{tag.Name}' has {count} elements but the write supplied {elements.Count}."); + + var elementBytes = ElementByteSize(addr.Size); + var block = new byte[count * elementBytes]; + try + { + for (var i = 0; i < count; i++) + { + var span = block.AsSpan(i * elementBytes, elementBytes); + var element = elements[i]; + switch (tag.DataType) + { + case S7DataType.Byte: span[0] = Convert.ToByte(element); break; + case S7DataType.UInt16: System.Buffers.Binary.BinaryPrimitives.WriteUInt16BigEndian(span, Convert.ToUInt16(element)); break; + case S7DataType.Int16: System.Buffers.Binary.BinaryPrimitives.WriteInt16BigEndian(span, Convert.ToInt16(element)); break; + case S7DataType.UInt32: System.Buffers.Binary.BinaryPrimitives.WriteUInt32BigEndian(span, Convert.ToUInt32(element)); break; + case S7DataType.Int32: System.Buffers.Binary.BinaryPrimitives.WriteInt32BigEndian(span, Convert.ToInt32(element)); break; + case S7DataType.Float32: System.Buffers.Binary.BinaryPrimitives.WriteSingleBigEndian(span, Convert.ToSingle(element)); break; + } + } + } + catch (Exception ex) when (ex is FormatException or InvalidCastException or OverflowException) + { + throw new S7WriteValueException( + $"S7 array tag '{tag.Name}': an element does not convert to {tag.DataType} ({ex.Message})."); + } + return block; + } + + /// + /// A caller-supplied write value the driver cannot encode (wrong shape / length / element + /// type). A client error — maps to BadTypeMismatch and does not touch driver health. + /// + internal sealed class S7WriteValueException(string message) : Exception(message); + /// /// Writes a wide/structured scalar as ONE contiguous byte block via S7.Net's /// buffer-based Plc.WriteBytesAsync(DataType, db, startByteAdr, byte[] value, ct). @@ -1893,18 +2102,15 @@ public sealed class S7Driver /// and reuses the single-connection-per-PLC invariant the gate /// enforces. The read / write / probe paths all take the gate before calling it. /// - /// While the PLC is down every data call and probe tick pays a full connect attempt + /// While the PLC is down every data call and probe tick would pay a full connect attempt /// bounded only by _options.Timeout (STAB-8). This method is the single /// choke-point all S7 reconnects flow through (already gate-serialized), so the - /// fleet-wide connect-attempt throttle plugs in here. R2-09 (sub-batch B3/B4) shipped the - /// shared primitive - /// (src/Core/…/Core.Abstractions/ConnectionBackoff.cs) — the S7 wiring is a - /// per-driver ConnectionBackoff field consulted with ShouldAttempt(now) at - /// the top of the slow path (fail fast + degrade inside the window) plus - /// RecordFailure/RecordSuccess around the connect, all under _gate - /// (which satisfies the primitive's caller-synchronized contract; TwinCAT/FOCAS/AbCip - /// already wire it this way). Deliberately NOT implemented driver-locally here — owned by - /// archreview/plans/R2-01-s7-fault-hardening-plan.md Finding 3. + /// connect-attempt throttle lives here (STAB-21): the shared + /// primitive is consulted with ShouldAttempt(now) at the top of the slow path — + /// inside the window the call fails fast with no connect attempt, which every caller + /// already degrades on — with RecordFailure/RecordSuccess around the connect. + /// A successful connect clears the window, so recovery is never delayed past the first + /// attempt that finds the PLC back; the 30 s cap bounds the worst case. /// /// /// Cancellation token for the (re)connect attempt. @@ -1922,6 +2128,12 @@ public sealed class S7Driver } _plcDead = false; + // STAB-21: inside the backoff window fail fast WITHOUT a connect attempt. A plain + // InvalidOperationException (not an OCE) so the read/write/probe catch ladders degrade on it. + if (!_connectBackoff.ShouldAttempt(BackoffClock())) + throw new InvalidOperationException( + $"S7 reconnect to {_options.Host}:{_options.Port} suppressed — backing off after a failed connect attempt."); + var plc = _plcFactory.Create( _options.CpuType, _options.Host, _options.Port, _options.Rack, _options.Slot, _options.Timeout); try @@ -1932,6 +2144,7 @@ public sealed class S7Driver } catch (OperationCanceledException) when (!ct.IsCancellationRequested) { + _connectBackoff.RecordFailure(BackoffClock()); // The linked CancelAfter fired — a connect TIMEOUT, not a caller cancellation. // Surface it as a connection FAILURE (TimeoutException) so callers route it through // their degrade paths: an escaping OCE reads as teardown and permanently killed the @@ -1941,12 +2154,16 @@ public sealed class S7Driver $"S7 connect to {_options.Host}:{_options.Port} timed out after " + $"{(int)_options.Timeout.TotalMilliseconds} ms."); } - catch + catch (Exception ex) { + // Caller cancellation is teardown, not a dead PLC — it must not open a window. + if (!(ex is OperationCanceledException && ct.IsCancellationRequested)) + _connectBackoff.RecordFailure(BackoffClock()); try { plc.Dispose(); } catch { /* best-effort */ } throw; } + _connectBackoff.RecordSuccess(); Plc = plc; _logger.LogInformation("S7Driver reconnected. Driver={DriverInstanceId} Host={Host} CPU={CpuType}", _driverInstanceId, _options.Host, _options.CpuType); @@ -2114,8 +2331,8 @@ public sealed class S7Driver private void HandlePollError(Exception ex) { _logger.LogWarning(ex, "S7 poll reader failed. Driver={DriverInstanceId}", _driverInstanceId); - if (_health.State != DriverState.Faulted) - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + if (Health.State != DriverState.Faulted) + Health = new DriverHealth(DriverState.Degraded, Health.LastSuccessfulRead, ex.Message); } // ---- IHostConnectivityProbe ---- @@ -2257,8 +2474,10 @@ public sealed class S7Driver _initialized = false; _plcDead = false; + _writeFault = null; + _connectBackoff.RecordSuccess(); try { Plc?.Dispose(); } catch { /* best-effort — tearing down anyway */ } Plc = null; - _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ArrayWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ArrayWriteTests.cs new file mode 100644 index 00000000..90d9e13f --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500ArrayWriteTests.cs @@ -0,0 +1,52 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// #610 item 2 (S7-016): array write round trip against the python-snap7 S7-1500 profile. Skips +/// cleanly when the sim is absent. Uses DB1 bytes 700+ (the profile's 1024-byte DB1 has no seed +/// there), so the writes cannot disturb another test's seeds. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500ArrayWriteTests(Snap7ServerFixture sim) +{ + private static S7DriverOptions Options(string host, int port) => new() + { + Host = host, + Port = port, + CpuType = S7CpuType.S71500, + Rack = 0, + Slot = 0, + Timeout = TimeSpan.FromSeconds(5), + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries( + new S7TagDefinition("I16Arr", "DB1.DBW700", S7DataType.Int16, Writable: true, ArrayCount: 4), + new S7TagDefinition("F32Arr", "DB1.DBD720", S7DataType.Float32, Writable: true, ArrayCount: 3)), + }; + + /// Writes a non-trivial Int16[4] and Float32[3], then reads each back element-for-element. + [Fact] + public async Task Array_write_then_read_round_trips_through_real_S7comm() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + + await using var drv = new S7Driver(Options(sim.Host, sim.Port), "s7-arr-write"); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var i16 = new short[] { 111, -222, 3333, -4444 }; + var f32 = new[] { 1.5f, -2.25f, 1234.5f }; + var res = await drv.WriteAsync( + [new WriteRequest("I16Arr", i16), new WriteRequest("F32Arr", f32)], + TestContext.Current.CancellationToken); + res.Select(r => r.StatusCode).ShouldAllBe(c => c == 0u); + + var read = await drv.ReadAsync(["I16Arr", "F32Arr"], TestContext.Current.CancellationToken); + read.Select(r => r.StatusCode).ShouldAllBe(c => c == 0u); + ((short[])read[0].Value!).ShouldBe(i16); + ((float[])read[1].Value!).ShouldBe(f32); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs new file mode 100644 index 00000000..5049ae7b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs @@ -0,0 +1,382 @@ +using Microsoft.Extensions.Logging; +using S7.Net; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Hosting; +using S7NetDataType = global::S7.Net.DataType; +using S7DataItem = global::S7.Net.Types.DataItem; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// #602 / #610 S7 items: array write path (S7-016), connect backoff (STAB-21), the write-pass +/// worst-state-wins accumulator (STAB-22) and the factory logger (CONV-3). All against an +/// in-memory — no live PLC, no fixture. +/// +[Trait("Category", "Unit")] +public sealed class S7ArrayWriteAndHealthTests +{ + private const uint BadTypeMismatch = 0x80740000u; + private const uint BadNotSupported = 0x803D0000u; + + private static S7TagDefinition Arr(S7DataType dt, string addr, int count, bool writable = true) => + new("Arr", addr, dt, Writable: writable, ArrayCount: count); + + private static S7DriverOptions Options(params S7TagDefinition[] tags) => new() + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(tags), + }; + + // ---- pure codec ---- + + /// Encode is the exact inverse of for every supported element type. + [Theory] + [InlineData(S7DataType.Byte, "DB1.DBB0")] + [InlineData(S7DataType.Int16, "DB1.DBW0")] + [InlineData(S7DataType.UInt16, "DB1.DBW0")] + [InlineData(S7DataType.Int32, "DB1.DBD0")] + [InlineData(S7DataType.UInt32, "DB1.DBD0")] + [InlineData(S7DataType.Float32, "DB1.DBD0")] + public void EncodeArrayBlock_round_trips_through_DecodeArrayBlock(S7DataType dt, string addrText) + { + var tag = Arr(dt, addrText, 3); + var addr = S7AddressParser.Parse(addrText); + object source = dt switch + { + S7DataType.Byte => new byte[] { 1, 200, 255 }, + S7DataType.Int16 => new short[] { -1, 0, 32000 }, + S7DataType.UInt16 => new ushort[] { 1, 0, 65535 }, + S7DataType.Int32 => new[] { -5, 0, int.MaxValue }, + S7DataType.UInt32 => new uint[] { 1, 0, uint.MaxValue }, + _ => new[] { -1.5f, 0f, 3.25f }, + }; + + var block = S7Driver.EncodeArrayBlock(tag, addr, source); + + block.Length.ShouldBe(3 * S7Driver.ElementByteSize(addr.Size)); + S7Driver.DecodeArrayBlock(tag, addr, block).ShouldBe(source); + } + + /// Big-endian layout is pinned byte-for-byte for Int16. + [Fact] + public void EncodeArrayBlock_is_big_endian() + { + var tag = Arr(S7DataType.Int16, "DB1.DBW0", 2); + S7Driver.EncodeArrayBlock(tag, S7AddressParser.Parse("DB1.DBW0"), new short[] { 0x0102, -2 }) + .ShouldBe(new byte[] { 0x01, 0x02, 0xFF, 0xFE }); + } + + /// A non-array, a string, a wrong length and an unconvertible element are client errors, not comms faults. + [Fact] + public void EncodeArrayBlock_rejects_wrongly_shaped_values() + { + var tag = Arr(S7DataType.Int16, "DB1.DBW0", 2); + var addr = S7AddressParser.Parse("DB1.DBW0"); + + Should.Throw(() => S7Driver.EncodeArrayBlock(tag, addr, (short)1)); + Should.Throw(() => S7Driver.EncodeArrayBlock(tag, addr, "ab")); + Should.Throw(() => S7Driver.EncodeArrayBlock(tag, addr, new short[] { 1 })); + Should.Throw(() => S7Driver.EncodeArrayBlock(tag, addr, new[] { 1, 2, 3 })); + Should.Throw(() => S7Driver.EncodeArrayBlock(tag, addr, new object[] { 1, 70000 })); // overflow + Should.Throw(() => S7Driver.EncodeArrayBlock(tag, addr, new object[] { 1, "x" })); // format + } + + /// Bool and wide-type arrays are refused with NotSupported (BadNotSupported on the wire). + [Theory] + [InlineData(S7DataType.Bool, "DB1.DBX0.0")] + [InlineData(S7DataType.Int64, "DB1.DBB0")] + [InlineData(S7DataType.String, "DB1.DBB0")] + public void EncodeArrayBlock_refuses_bool_and_wide_arrays(S7DataType dt, string addrText) + { + Should.Throw(() => S7Driver.EncodeArrayBlock( + Arr(dt, addrText, 2), S7AddressParser.Parse(addrText), new object[] { 1, 2 })); + } + + // ---- WriteAsync array path ---- + + /// + /// A valid array write issues exactly ONE contiguous WriteBytes at the tag's DB/offset + /// (S7-016) and reports Good. The bytes are the decode inverse, so a following read returns the value. + /// + [Fact] + public async Task Write_array_issues_one_contiguous_block_write() + { + var factory = new RecordingFactory(); + using var drv = new S7Driver(Options(Arr(S7DataType.Int16, "DB5.DBW10", 3)), "s7-arr-write", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var res = await drv.WriteAsync([new WriteRequest("Arr", new short[] { 1, 2, 3 })], TestContext.Current.CancellationToken); + + res[0].StatusCode.ShouldBe(0u); + var w = factory.Created[0].BlockWrites.ShouldHaveSingleItem(); + w.Area.ShouldBe(S7NetDataType.DataBlock); + w.Db.ShouldBe(5); + w.Start.ShouldBe(10); + w.Bytes.ShouldBe(new byte[] { 0, 1, 0, 2, 0, 3 }); + factory.Created[0].ScalarWrites.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// A wrong-length array is BadTypeMismatch, writes nothing and leaves health untouched; the + /// positive control is the valid write in the same batch landing. + /// + [Fact] + public async Task Write_array_with_wrong_length_is_BadTypeMismatch_and_not_a_health_event() + { + var factory = new RecordingFactory(); + using var drv = new S7Driver(Options(Arr(S7DataType.Int16, "DB1.DBW0", 3)), "s7-arr-len", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var res = await drv.WriteAsync( + [new WriteRequest("Arr", new short[] { 1, 2 }), new WriteRequest("Arr", new short[] { 7, 8, 9 })], + TestContext.Current.CancellationToken); + + res[0].StatusCode.ShouldBe(BadTypeMismatch); + res[1].StatusCode.ShouldBe(0u); + factory.Created[0].BlockWrites.Count.ShouldBe(1); // only the valid one reached the wire + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + // ---- STAB-21 connect backoff ---- + + /// + /// A dead PLC costs ONE connect attempt per backoff window, not one per read; the window grows + /// (1 s then 2 s), and a successful connect clears it so recovery is immediate. + /// + [Fact] + public async Task Dead_plc_costs_one_connect_attempt_per_backoff_window() + { + var factory = new RecordingFactory(); + using var drv = new S7Driver(Options(new S7TagDefinition("W0", "DB1.DBW0", S7DataType.UInt16)), "s7-backoff", factory); + var now = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + drv.BackoffClock = () => now; + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + factory.Created.Count.ShouldBe(1); + + // Drop the link; every subsequent open is refused. + factory.Created[0].NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped"); + factory.OpenThrows = new PlcException(ErrorCode.ConnectionError, "down"); + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); // marks dead + + // First attempt after the drop: really tries (and fails), opening a 1 s window. + (await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldNotBe(0u); + factory.Created.Count.ShouldBe(2); + + // Ten more reads inside the window: no further connect attempt, still a Bad result. + for (var i = 0; i < 10; i++) + (await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldNotBe(0u); + factory.Created.Count.ShouldBe(2); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + // Window elapses -> exactly one more attempt (fails -> 2 s window). + now += TimeSpan.FromSeconds(1.1); + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + factory.Created.Count.ShouldBe(3); + now += TimeSpan.FromSeconds(1.1); // inside the doubled window + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + factory.Created.Count.ShouldBe(3); + + // PLC returns; once the window passes the next read reconnects and the window is cleared. + factory.OpenThrows = null; + now += TimeSpan.FromSeconds(5); + (await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); + factory.Created.Count.ShouldBe(4); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + + // A fresh drop reconnects immediately (no residual window). + factory.Created[3].NextReadThrows = new PlcException(ErrorCode.ConnectionError, "dropped again"); + await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); + (await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); + factory.Created.Count.ShouldBe(5); + } + + // ---- STAB-22 write-pass worst-state-wins ---- + + private static async Task<(S7Driver Drv, RecordingFactory Factory)> TwoTagDriver() + { + var factory = new RecordingFactory(); + var drv = new S7Driver(Options( + new S7TagDefinition("A", "DB1.DBW0", S7DataType.UInt16, Writable: true), + new S7TagDefinition("B", "DB1.DBW2", S7DataType.UInt16, Writable: true), + new S7TagDefinition("C", "DB1.DBW4", S7DataType.UInt16, Writable: true)), "s7-writepass", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + return (drv, factory); + } + + /// + /// Access-denied (Faulted) followed by a generic failure (Degraded) and a success in ONE batch + /// must report Faulted with the access-denied message - not whichever item came last. + /// + [Fact] + public async Task Write_batch_reports_worst_state_not_last_item() + { + var (drv, factory) = await TwoTagDriver(); + using var _ = drv; + factory.Created[0].WriteFaults["DB1.DBW0"] = new PlcException(ErrorCode.WriteData, "Accessing object not allowed"); + factory.Created[0].WriteFaults["DB1.DBW2"] = new IOException("socket glitch"); + + var res = await drv.WriteAsync( + [new WriteRequest("A", (ushort)1), new WriteRequest("B", (ushort)2), new WriteRequest("C", (ushort)3)], + TestContext.Current.CancellationToken); + + res[2].StatusCode.ShouldBe(0u); + var h = drv.GetHealth(); + h.State.ShouldBe(DriverState.Faulted); + h.LastError.ShouldNotBeNull().ShouldContain("PUT/GET"); + } + + /// + /// A clean READ pass must not erase the write-side Faulted; the next write that succeeds clears + /// it, after which a read pass reports Healthy again. + /// + [Fact] + public async Task Read_pass_does_not_erase_write_fault_until_a_write_succeeds() + { + var (drv, factory) = await TwoTagDriver(); + using var _ = drv; + var plc = factory.Created[0]; + plc.WriteFaults["DB1.DBW0"] = new PlcException(ErrorCode.WriteData, "Accessing object not allowed"); + await drv.WriteAsync([new WriteRequest("A", (ushort)1)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Faulted); + + // Clean read of a different tag - reads work against a write-protected CPU. + var read = await drv.ReadAsync(["B"], TestContext.Current.CancellationToken); + read[0].StatusCode.ShouldBe(0u); + var h = drv.GetHealth(); + h.State.ShouldBe(DriverState.Faulted); // not erased + h.LastSuccessfulRead.ShouldNotBeNull(); // but the read still advanced the watchdog evidence + + // A write that gets through retires the fault ... + await drv.WriteAsync([new WriteRequest("B", (ushort)2)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + + // ... and the next read pass stays Healthy (the sticky state is really gone). + (await drv.ReadAsync(["B"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A transient (Degraded) write failure is still erased by a clean read, as before. + [Fact] + public async Task Degraded_write_failure_is_still_cleared_by_a_clean_read() + { + var (drv, factory) = await TwoTagDriver(); + using var _ = drv; + factory.Created[0].WriteFaults["DB1.DBW0"] = new IOException("glitch"); + await drv.WriteAsync([new WriteRequest("A", (ushort)1)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + (await drv.ReadAsync(["B"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + // ---- CONV-3 ---- + + /// + /// The registered factory builds drivers on the host logger: a failed connect reaches the + /// supplied . Before CONV-3 the factory took no logger and the driver ran on the null logger. + /// + [Fact] + public async Task Factory_threads_the_logger_factory_into_the_driver() + { + const string Json = """{"host":"192.0.2.1","timeoutMs":200}"""; + + var withLogger = new CapturingLoggerFactory(); + var registry = new DriverFactoryRegistry(); + S7DriverFactoryExtensions.Register(registry, withLogger); + var drv = registry.TryGet(S7DriverFactoryExtensions.DriverTypeName)!("s7-logs", Json); + try { await drv.InitializeAsync(Json, TestContext.Current.CancellationToken); } catch { /* unreachable host by design */ } + withLogger.Messages.ShouldContain(m => m.Contains("connect failed", StringComparison.OrdinalIgnoreCase)); + } + + private sealed class CapturingLoggerFactory : ILoggerFactory + { + public List Messages { get; } = []; + public void AddProvider(ILoggerProvider provider) { } + public ILogger CreateLogger(string categoryName) => new Capture(this); + public void Dispose() { } + + private sealed class Capture(CapturingLoggerFactory owner) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + lock (owner.Messages) owner.Messages.Add(formatter(state, exception)); + } + } + } + + // ---- in-memory PLC ---- + + private sealed class RecordingFactory : IS7PlcFactory + { + public List Created { get; } = []; + + /// When set, every connection created from now on throws this from OpenAsync. + public Exception? OpenThrows { get; set; } + + public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) + { + var plc = new RecordingPlc { OpenThrowsAlways = OpenThrows }; + Created.Add(plc); + return plc; + } + } + + private sealed class RecordingPlc : IS7Plc + { + public bool IsConnected { get; private set; } + public Exception? OpenThrowsAlways { get; init; } + public Exception? NextReadThrows { get; set; } + public Dictionary WriteFaults { get; } = new(StringComparer.Ordinal); + public List<(S7NetDataType Area, int Db, int Start, byte[] Bytes)> BlockWrites { get; } = []; + public int ScalarWrites { get; private set; } + + public Task OpenAsync(CancellationToken cancellationToken) + { + if (OpenThrowsAlways is { } ex) throw ex; + IsConnected = true; + return Task.CompletedTask; + } + + public void Close() => IsConnected = false; + + public Task ReadAsync(string address, CancellationToken cancellationToken) + { + if (NextReadThrows is { } ex) { NextReadThrows = null; throw ex; } + return Task.FromResult((ushort)42); + } + + public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) + { + if (NextReadThrows is { } ex) { NextReadThrows = null; throw ex; } + return Task.FromResult(new byte[count]); + } + + public Task> ReadMultipleVarsAsync(List items, CancellationToken cancellationToken) => + throw new NotSupportedException("not needed"); + + public Task WriteAsync(string address, object value, CancellationToken cancellationToken) + { + ScalarWrites++; + if (WriteFaults.TryGetValue(address, out var ex)) throw ex; + return Task.CompletedTask; + } + + public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) + { + BlockWrites.Add((area, db, startByteAdr, value)); + return Task.CompletedTask; + } + + public Task ReadStatusAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + public void Dispose() => IsConnected = false; + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs index 0faa1125..38eac33e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReconnectTests.cs @@ -149,6 +149,8 @@ public sealed class S7DriverReconnectTests { var factory = new FakeS7PlcFactory(); using var drv = new S7Driver(Options(), "s7-reopen-fail", factory); + var clock = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + drv.BackoffClock = () => clock; await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); // Drop the live socket, and make the NEXT-created connection fail to open. @@ -161,7 +163,9 @@ public sealed class S7DriverReconnectTests down[0].StatusCode.ShouldNotBe(0u); drv.GetHealth().State.ShouldBe(DriverState.Degraded); - // PLC comes back; the next call reopens cleanly and the tag reads Good. + // PLC comes back, and the 1 s STAB-21 backoff window the failed reopen opened has elapsed; + // the next call reopens cleanly and the tag reads Good. + clock += TimeSpan.FromSeconds(2); var up = await drv.ReadAsync(["W0"], TestContext.Current.CancellationToken); up[0].StatusCode.ShouldBe(0u); drv.GetHealth().State.ShouldBe(DriverState.Healthy); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs index 02a42a86..6c1c6561 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs @@ -226,24 +226,42 @@ public sealed class S7DriverScaffoldTests drv.GetHealth().State.ShouldBe(DriverState.Faulted); } - // ── Driver.S7-015 — writable array tag must fail fast at init ──────────────────────── + // ── Driver.S7-015 / S7-016 — writable Bool array must fail fast at init ───────────── // - // Array reads are implemented (ReadArrayAsync / DecodeArrayBlock), but WriteOneAsync has - // no WriteArrayAsync path. Without the init guard a writable array node is discovered and - // accepted, then every write returns BadCommunicationError (InvalidCastException from - // BoxValueForWrite receiving a typed array). Fail fast at init instead. + // Non-bool arrays now write as one contiguous block (S7-016, EncodeArrayBlock). A Bool array + // write would be byte-granular and clobber the neighbouring bits of every byte it touches, so + // a writable Bool array is still refused at init with an informative message. - /// Verifies that a writable non-wide array (e.g., Int16[4]) is rejected at init - /// with a clear "array writes not yet supported" message — Driver.S7-015. + /// Verifies that a writable Bool array is rejected at init. + [Fact] + public async Task Initialize_rejects_writable_bool_array_tag_with_NotSupportedException() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + RawTags = S7RawTags.Entries(new S7TagDefinition("ArrTag", "DB1.DBX0.0", S7DataType.Bool, Writable: true, ArrayCount: 4)), + }; + using var drv = new S7Driver(opts, "s7-writable-bool-arr"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.Message.ShouldContain("ArrTag"); + ex.Message.ShouldContain("Bool array", Case.Insensitive); + + drv.GetHealth().State.ShouldBe(DriverState.Faulted); + } + + /// Verifies that writable non-bool arrays pass the init guard — the failure + /// must be the TCP connect (unreachable host), not the array guard (S7-016). [Theory] - [InlineData(S7DataType.Bool, "DB1.DBX0.0", 4)] [InlineData(S7DataType.Byte, "DB1.DBB0", 8)] [InlineData(S7DataType.Int16, "DB1.DBW0", 4)] [InlineData(S7DataType.UInt16, "DB1.DBW0", 4)] [InlineData(S7DataType.Int32, "DB1.DBD0", 2)] [InlineData(S7DataType.UInt32, "DB1.DBD0", 2)] [InlineData(S7DataType.Float32,"DB1.DBD0", 2)] - public async Task Initialize_rejects_writable_array_tag_with_NotSupportedException( + public async Task Initialize_accepts_writable_non_bool_array_tag( S7DataType dt, string addr, int count) { var opts = new S7DriverOptions @@ -254,13 +272,9 @@ public sealed class S7DriverScaffoldTests }; using var drv = new S7Driver(opts, $"s7-writable-arr-{dt}"); - var ex = await Should.ThrowAsync(async () => + var ex = await Should.ThrowAsync(async () => await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("ArrTag"); - ex.Message.ShouldContain("array", Case.Insensitive); - ex.Message.ShouldContain("write", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + ex.ShouldNotBeOfType(); } /// Verifies that a read-only (Writable=false) array passes the init guard —