diff --git a/code-reviews/Driver.S7/findings.md b/code-reviews/Driver.S7/findings.md index c211046..312b2e6 100644 --- a/code-reviews/Driver.S7/findings.md +++ b/code-reviews/Driver.S7/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -89,7 +89,7 @@ correct the comment so the lossiness of UInt32 is documented. | Severity | Low | | Category | Correctness & logic bugs | | Location | `S7Driver.cs:172`, `S7Driver.cs:255` | -| Status | Open | +| Status | Resolved | **Description:** ReadAsync and WriteAsync dereference fullReferences.Count / writes.Count with no null guard. A null argument throws NullReferenceException @@ -101,7 +101,13 @@ inconsistent with it. **Recommendation:** Add ArgumentNullException.ThrowIfNull for the list parameters at the top of ReadAsync and WriteAsync. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added `ArgumentNullException.ThrowIfNull` +at the top of both `ReadAsync` and `WriteAsync`, placed BEFORE `RequirePlc()` so +a null argument produces a typed `ArgumentNullException` (consistent with +`DiscoverAsync`) rather than either an NRE on `.Count` or the "not initialized" +`InvalidOperationException` from `RequirePlc`. Regression tests +`ReadAsync_with_null_fullReferences_throws_ArgumentNullException` and +`WriteAsync_with_null_writes_throws_ArgumentNullException`. ### Driver.S7-004 @@ -133,7 +139,7 @@ and swallowed poll-loop / shutdown exceptions. | Severity | Low | | Category | OtOpcUa conventions | | Location | `S7Driver.cs:33`, `S7Driver.cs:433` | -| Status | Open | +| Status | Resolved | **Description:** System.Collections.Concurrent.ConcurrentDictionary is written out with a fully-qualified namespace at the field declarations instead of a @@ -145,7 +151,11 @@ S7Driver.cs despite the file-top using S7.Net. **Recommendation:** Add using System.Collections.Concurrent and drop the redundant global::S7.Net. qualifiers where using S7.Net already covers them. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `using System.Collections.Concurrent` was +already added by an earlier finding fix; this resolution removes the remaining +`global::S7.Net.Plc` qualifiers from the `ReadOneAsync` and `WriteOneAsync` +signatures, now using the unqualified `Plc` type (the file-top `using S7.Net` +already covers it). House style restored. ### Driver.S7-006 @@ -250,7 +260,7 @@ status, and update _health to Degraded on transport failures. | Severity | Low | | Category | Error handling & resilience | | Location | `S7Driver.cs:392` | -| Status | Open | +| Status | Resolved | **Description:** The subscription poll loop never reflects sustained polling failure anywhere an operator can see it. PollLoopAsync swallows every @@ -266,7 +276,19 @@ Interval indefinitely on a hard failure. apply a capped backoff after consecutive errors; at minimum log the swallowed exception (see Driver.S7-004). -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `PollLoopAsync` now tracks +`consecutiveFailures`, calls new `HandlePollFailure` which both logs (with the +failure count) AND degrades `_health` to `Degraded` once +`PollFailureHealthThreshold` (1) consecutive failures have accumulated, and +applies a capped exponential backoff via new `ComputeBackoffDelay` (doubles the +wait each consecutive failure up to a 30 s `PollBackoffCap`). A healthy tick +resets the counter so the cadence snaps back to the configured Interval. +`HandlePollFailure` refuses to downgrade a `Faulted` state (reserved for +permanent config faults like PUT/GET-denied). Regression test +`PollLoop_against_uninitialized_driver_degrades_health` proves the health +surface now reflects sustained failure; `PollLoop_applies_capped_backoff_after_consecutive_failures` +proves shutdown still completes inside the drain window even under a fault +storm. ### Driver.S7-010 @@ -275,7 +297,7 @@ exception (see Driver.S7-004). | Severity | Low | | Category | Performance & resource management | | Location | `S7Driver.cs:504` | -| Status | Open | +| Status | Resolved | **Description:** Dispose() is implemented as DisposeAsync().AsTask().GetAwaiter().GetResult() - sync-over-async. Inside the @@ -288,7 +310,16 @@ blocking wrap is unnecessary risk. perform the teardown directly (cancel CTSs, close Plc, dispose _gate) without round-tripping through the async path. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `Dispose()` now performs teardown +directly via a new private `SynchronousTeardown` method that mirrors +`ShutdownAsync` but uses `Task.WhenAll(...).Wait(DrainTimeout)` instead of +`await Task.WhenAll(...).WaitAsync(...)`. Probe + poll Tasks are still drained +with the bounded 5 s timeout (so a wedged loop cannot hang `Dispose` indefinitely), +but the sync path no longer round-trips through `DisposeAsync().AsTask().GetAwaiter().GetResult()`. +`DisposeAsync` keeps its existing implementation for callers that opt into the +async dispose pattern. Regression tests +`Dispose_completes_synchronously_without_sync_over_async_round_trip` and +`Dispose_is_idempotent`. ### Driver.S7-011 @@ -358,7 +389,7 @@ ReadStatusAsync-based probe. | Severity | Low | | Category | Code organization & conventions | | Location | `S7DriverOptions.cs:90`, `S7Driver.cs:300` | -| Status | Open | +| Status | Resolved | **Description:** S7TagDefinition.StringLength is a public configured/JSON-bound parameter (default 254) but is dead: S7DataType.String reads and writes both @@ -376,7 +407,20 @@ StringLength) at InitializeAsync / factory validation with a clear "not yet supported" error, so a partially-implemented type cannot be configured into a live address space. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `InitializeAsync` now runs new +`RejectUnsupportedTagDataTypes`, which throws `NotSupportedException` for any +tag whose `DataType` is in the `UnimplementedDataTypes` set (`Int64`, `UInt64`, +`Float64`, `String`, `DateTime`). The half-implemented types can no longer leak +into the live address space — a site that configures one fails fast at init +rather than seeing a node that returns `BadNotSupported` on every access. +Entries should be removed from `UnimplementedDataTypes` as each type is wired +through; the comment on `RejectUnsupportedTagDataTypes` makes it a single grep +target for that follow-up. `StringLength` remains in `S7TagDefinition` because +removing it would be a breaking change to existing config JSON; once `String` +is implemented it will be consumed without further config changes. Regression +tests `Initialize_rejects_not_yet_implemented_data_type_with_NotSupportedException` +(Theory, 5 types) and `Initialize_accepts_implemented_data_types` (Theory, 7 +types) prove the guard is targeted. ### Driver.S7-014 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 eaab8ad..1ff4a5d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -121,6 +121,15 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I // read (Driver.S7-001). Drop this guard when Timer/Counter reads are wired through. RejectUnsupportedTagAddresses(); + // S7DataType values that ReadOneAsync / WriteOneAsync currently throw + // NotSupportedException for (Int64, UInt64, Float64, String, DateTime) must also + // be rejected at init — without this guard a site can configure e.g. a Float64 + // tag, see the node appear in the address space via DiscoverAsync, and get + // BadNotSupported on every access. Half-implemented types must not leak into the + // configurable surface (Driver.S7-013). Drop entries from the set as each data + // type is wired through. + RejectUnsupportedTagDataTypes(); + var plc = new Plc(_options.CpuType, _options.Host, _options.Port, _options.Rack, _options.Slot); // S7netplus writes timeouts into the underlying TcpClient via Plc.WriteTimeout / // Plc.ReadTimeout (milliseconds). Set before OpenAsync so the handshake itself @@ -262,6 +271,44 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I } } + /// + /// Rejects tags configured with an that + /// / still throw + /// for. Without this guard those tags create live + /// OPC UA nodes via but every Read/Write returns + /// BadNotSupported — code-review finding Driver.S7-013. Drop entries from + /// as each type is wired through. + /// + private void RejectUnsupportedTagDataTypes() + { + foreach (var t in _options.Tags) + { + if (UnimplementedDataTypes.Contains(t.DataType)) + { + throw new NotSupportedException( + $"S7 tag '{t.Name}' uses data type '{t.DataType}' which is not yet " + + "supported by the S7 driver — Read/Write would return BadNotSupported. " + + "Remove the tag or use Bool/Byte/Int16/UInt16/Int32/UInt32/Float32 until " + + $"{t.DataType} is wired through."); + } + } + } + + /// + /// S7DataType members that the read/write helpers throw NotSupportedException for. + /// Kept here (rather than reflecting over ) so + /// is a single grep target for the + /// follow-up PR that wires each through. + /// + private static readonly HashSet UnimplementedDataTypes = new() + { + S7DataType.Int64, + S7DataType.UInt64, + S7DataType.Float64, + S7DataType.String, + S7DataType.DateTime, + }; + public DriverHealth GetHealth() => _health; /// @@ -278,6 +325,10 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I public async Task> ReadAsync( IReadOnlyList fullReferences, CancellationToken cancellationToken) { + // Validate the list before RequirePlc() so a null argument produces an + // ArgumentNullException (consistent with DiscoverAsync) rather than an + // InvalidOperationException from the not-initialized check — Driver.S7-003. + ArgumentNullException.ThrowIfNull(fullReferences); var plc = RequirePlc(); var now = DateTime.UtcNow; var results = new DataValueSnapshot[fullReferences.Count]; @@ -333,7 +384,7 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I return results; } - private async Task ReadOneAsync(global::S7.Net.Plc plc, S7TagDefinition tag, CancellationToken ct) + private async Task ReadOneAsync(Plc plc, S7TagDefinition tag, CancellationToken ct) { var addr = _parsedByName[tag.Name]; // S7.Net's string-based ReadAsync returns object where the boxed .NET type depends on @@ -381,6 +432,9 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I public async Task> WriteAsync( IReadOnlyList writes, CancellationToken cancellationToken) { + // Same as ReadAsync — validate before RequirePlc() so a null argument is a + // typed argument error, not the "not initialized" surface (Driver.S7-003). + ArgumentNullException.ThrowIfNull(writes); var plc = RequirePlc(); var results = new WriteResult[writes.Count]; @@ -446,7 +500,7 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I return results; } - private async Task WriteOneAsync(global::S7.Net.Plc plc, S7TagDefinition tag, object? value, CancellationToken ct) + private async Task WriteOneAsync(Plc plc, S7TagDefinition tag, object? value, CancellationToken ct) { // S7.Net's Plc.WriteAsync(string address, object value) expects the boxed value to // match the address's size-suffix type: DBX=bool, DBB=byte, DBW=ushort, DBD=uint. @@ -574,32 +628,103 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I return Task.CompletedTask; } + /// + /// Upper bound on the poll-loop backoff window. After enough consecutive failures the + /// loop waits this long between retries instead of , + /// so a subscription against a dropped / uninitialised driver doesn't spin (Driver.S7-009). + /// + private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30); + + /// + /// Number of consecutive poll failures before the loop transitions the driver's + /// health to . One stray failure can be transient; + /// a sustained run indicates the operator should see it. Threshold of 1 because the + /// first failure already lives in the LastError surface — see Driver.S7-009. + /// + private const int PollFailureHealthThreshold = 1; + private async Task PollLoopAsync(SubscriptionState state, CancellationToken ct) { + var consecutiveFailures = 0; + // Initial-data push per OPC UA Part 4 convention. - try { await PollOnceAsync(state, forceRaise: true, ct).ConfigureAwait(false); } + try + { + await PollOnceAsync(state, forceRaise: true, ct).ConfigureAwait(false); + consecutiveFailures = 0; + } catch (OperationCanceledException) { return; } catch (Exception ex) { // First-read error — polling continues; log so the operator has an event trail. - _logger.LogWarning(ex, "S7 poll initial-read failed. Driver={DriverInstanceId}", driverInstanceId); + consecutiveFailures++; + HandlePollFailure(ex, consecutiveFailures, initial: true); } while (!ct.IsCancellationRequested) { - try { await Task.Delay(state.Interval, ct).ConfigureAwait(false); } + // Capped exponential backoff: Interval, 2×, 4×, ... up to PollBackoffCap. Healthy + // ticks reset consecutiveFailures back to 0 so the cadence snaps back to Interval. + var delay = ComputeBackoffDelay(state.Interval, consecutiveFailures); + try { await Task.Delay(delay, ct).ConfigureAwait(false); } catch (OperationCanceledException) { return; } - try { await PollOnceAsync(state, forceRaise: false, ct).ConfigureAwait(false); } + try + { + await PollOnceAsync(state, forceRaise: false, ct).ConfigureAwait(false); + consecutiveFailures = 0; + } catch (OperationCanceledException) { return; } catch (Exception ex) { - // Transient polling error — loop continues; log so the operator has an event trail. - _logger.LogWarning(ex, "S7 poll tick failed. Driver={DriverInstanceId}", driverInstanceId); + // Sustained polling error — loop continues with backoff; log + update health. + consecutiveFailures++; + HandlePollFailure(ex, consecutiveFailures, initial: false); } } } + /// + /// Logs the swallowed poll exception and, once + /// consecutive failures have accumulated, degrades the driver health so the failure + /// surfaces on the dashboard — see Driver.S7-009. The probe loop owns Running/Stopped + /// transitions for the host-connectivity surface, so we touch + /// rather than the probe state. + /// + private void HandlePollFailure(Exception ex, int consecutiveFailures, bool initial) + { + if (initial) + _logger.LogWarning(ex, "S7 poll initial-read failed. Driver={DriverInstanceId} ConsecutiveFailures={Count}", + driverInstanceId, consecutiveFailures); + else + _logger.LogWarning(ex, "S7 poll tick failed. Driver={DriverInstanceId} ConsecutiveFailures={Count}", + driverInstanceId, consecutiveFailures); + + if (consecutiveFailures >= PollFailureHealthThreshold) + { + // Don't downgrade a Faulted state (e.g. PUT/GET-denied set by ReadAsync) — Faulted + // is a stronger signal than Degraded and is reserved for permanent config faults. + if (_health.State != DriverState.Faulted) + _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + } + } + + /// + /// Capped exponential backoff. consecutiveFailures == 0 returns the configured + /// ; each subsequent failure doubles the wait up to + /// . Computed in ticks to avoid overflow at large counts. + /// + internal static TimeSpan ComputeBackoffDelay(TimeSpan interval, int consecutiveFailures) + { + if (consecutiveFailures <= 0) return interval; + // Cap the shift to avoid overflow — at 30 the result already saturates PollBackoffCap + // for any reasonable Interval. + var shift = Math.Min(consecutiveFailures - 1, 30); + var ticks = interval.Ticks << shift; + if (ticks <= 0 || ticks > PollBackoffCap.Ticks) return PollBackoffCap; + return TimeSpan.FromTicks(ticks); + } + private async Task PollOnceAsync(SubscriptionState state, bool forceRaise, CancellationToken ct) { var snapshots = await ReadAsync(state.TagReferences, ct).ConfigureAwait(false); @@ -702,7 +827,18 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I OnHostStatusChanged?.Invoke(this, new HostStatusChangedEventArgs(HostName, old, newState)); } - public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); + public void Dispose() + { + // Driver.S7-010: avoid the sync-over-async DisposeAsync().AsTask().GetAwaiter().GetResult() + // pattern (a known deadlock surface even when currently safe here). ShutdownAsync's + // body is effectively synchronous apart from waiting on probe/poll Tasks; do the same + // teardown directly, blocking only on the drain — and only with a bounded timeout so + // a wedged loop can't hang Dispose() indefinitely. + if (_disposed) return; + _disposed = true; + SynchronousTeardown(); + _gate.Dispose(); + } public async ValueTask DisposeAsync() { @@ -712,4 +848,46 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I catch { /* disposal is best-effort */ } _gate.Dispose(); } + + /// + /// Synchronous teardown — mirrors but blocks (with a bounded + /// timeout) on the probe + poll Tasks instead of awaiting them. Used by the sync + /// path so we don't sync-over-async + /// (Driver.S7-010). + /// + private void SynchronousTeardown() + { + var drain = new List(); + + var probeCts = _probeCts; + var probeTask = _probeTask; + try { probeCts?.Cancel(); } catch { } + if (probeTask is not null) drain.Add(probeTask); + + var subscriptions = _subscriptions.Values.ToArray(); + _subscriptions.Clear(); + foreach (var state in subscriptions) + { + try { state.Cts.Cancel(); } catch { } + drain.Add(state.PollTask); + } + + if (drain.Count > 0) + { + try { Task.WhenAll(drain).Wait(DrainTimeout); } + catch { /* timeouts/loop faults are tolerated — teardown continues */ } + } + + probeCts?.Dispose(); + _probeCts = null; + _probeTask = null; + foreach (var state in subscriptions) + { + try { state.Cts.Dispose(); } catch { } + } + + try { Plc?.Close(); } catch { /* best-effort — tearing down anyway */ } + Plc = null; + _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs new file mode 100644 index 0000000..9277e6e --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs @@ -0,0 +1,191 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Regression tests for the remaining code-review findings closed against the S7 driver: +/// Driver.S7-003 (Read/WriteAsync null-arg validation), Driver.S7-009 (poll-loop health +/// update + backoff), Driver.S7-010 (Dispose without sync-over-async), and Driver.S7-013 +/// (reject not-yet-implemented S7DataType values at init). +/// +[Trait("Category", "Unit")] +public sealed class S7DriverCodeReviewFixTests2 +{ + // ── Driver.S7-003 — Read/WriteAsync must throw ArgumentNullException, not NRE ───────── + + [Fact] + public async Task ReadAsync_with_null_fullReferences_throws_ArgumentNullException() + { + // The driver must validate its inputs consistently with DiscoverAsync (which already + // uses ArgumentNullException.ThrowIfNull). A NullReferenceException escaping the entry + // point bypasses the gate and gives the caller a non-actionable stack. + using var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-null-read"); + await Should.ThrowAsync(async () => + await drv.ReadAsync(null!, TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task WriteAsync_with_null_writes_throws_ArgumentNullException() + { + using var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-null-write"); + await Should.ThrowAsync(async () => + await drv.WriteAsync(null!, TestContext.Current.CancellationToken)); + } + + // ── Driver.S7-009 — Poll loop must update health on sustained failure ──────────────── + + [Fact] + public async Task PollLoop_against_uninitialized_driver_degrades_health() + { + // Subscribing without InitializeAsync means RequirePlc() throws on every poll tick. + // Previously the empty catch swallowed everything and the dashboard reported Healthy + // / Unknown indefinitely. With the fix the poll loop must surface the failure on the + // health surface so an operator can see it (driver-stability convention). + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + }; + var drv = new S7Driver(opts, "s7-poll-health"); + + await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken); + + // Wait long enough for several poll ticks. With backoff the second tick should arrive + // within ~100-200 ms; allow generous slack. + for (var i = 0; i < 40 && drv.GetHealth().State is DriverState.Unknown or DriverState.Initializing; i++) + await Task.Delay(50, TestContext.Current.CancellationToken); + + drv.GetHealth().State.ShouldBe(DriverState.Degraded, + "sustained poll failure must surface on the health state — see Driver.S7-009"); + + await drv.ShutdownAsync(CancellationToken.None); + await drv.DisposeAsync(); + } + + [Fact] + public async Task PollLoop_applies_capped_backoff_after_consecutive_failures() + { + // After repeated poll errors the loop must back off rather than burn CPU re-polling + // every Interval — Driver.S7-009 calls for a capped backoff. Inspect the ratio of + // observed tick count to the floor count we would have seen WITHOUT backoff over a + // short window. Without backoff, an Interval=50 ms loop with sub-ms ReadAsync + // RequirePlc-throw would tick ~20 times in 1 s. With capped backoff it ticks far + // fewer; we use a generous upper bound that still proves "something is throttling". + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + }; + var drv = new S7Driver(opts, "s7-poll-backoff"); + + var ticks = 0; + drv.OnDataChange += (_, _) => Interlocked.Increment(ref ticks); + + await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken); + + // OnDataChange is never raised (every poll fails) so we can't count via the event. + // Instead, time-bound the test and rely on the health-degradation test above to prove + // the catch ran; here we just confirm the driver doesn't deadlock or spin so hot it + // refuses to shut down. ShutdownAsync must complete within the drain window. + await Task.Delay(500, TestContext.Current.CancellationToken); + + var sw = System.Diagnostics.Stopwatch.StartNew(); + await drv.ShutdownAsync(CancellationToken.None); + sw.Stop(); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(6), + "shutdown must complete inside the drain timeout — a runaway backoff would block it"); + + await drv.DisposeAsync(); + _ = ticks; // silence unused + } + + // ── Driver.S7-010 — Dispose() must not deadlock via sync-over-async ────────────────── + + [Fact] + public void Dispose_completes_synchronously_without_sync_over_async_round_trip() + { + // The sync Dispose() path must perform the teardown directly rather than blocking on + // DisposeAsync().AsTask().GetAwaiter().GetResult(). The current sync-over-async pattern + // is a known deadlock risk even when the wrapped Task.Run paths happen to be safe. + // We assert by measuring Dispose() runtime on a no-subscription, no-init driver: it + // should complete in microseconds, not the ~5 s drain window that a hung sync-over-async + // would burn waiting on a never-completing continuation. + var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-dispose-sync"); + var sw = System.Diagnostics.Stopwatch.StartNew(); + drv.Dispose(); + sw.Stop(); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(1), + "Dispose() must teardown directly — see Driver.S7-010"); + } + + [Fact] + public void Dispose_is_idempotent() + { + // After the rewrite Dispose() must remain safe to call twice — disposal is + // best-effort and the second call must not throw. + var drv = new S7Driver(new S7DriverOptions { Host = "192.0.2.1" }, "s7-dispose-twice"); + drv.Dispose(); + Should.NotThrow(() => drv.Dispose()); + } + + // ── Driver.S7-013 — Reject not-yet-implemented S7DataType values at init ───────────── + + [Theory] + [InlineData(S7DataType.Int64)] + [InlineData(S7DataType.UInt64)] + [InlineData(S7DataType.Float64)] + [InlineData(S7DataType.String)] + [InlineData(S7DataType.DateTime)] + public async Task Initialize_rejects_not_yet_implemented_data_type_with_NotSupportedException(S7DataType dt) + { + // A tag declared with one of the not-yet-wired data types parses cleanly and creates + // an OPC UA node via DiscoverAsync — then every Read/Write of it returns BadNotSupported. + // The half-implemented type must be rejected at init so a site can't deploy a config + // that produces dead nodes (Driver.S7-013). + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + // Use a DB.DBD address — the parser accepts it for every data type. The init guard + // must fault on the data-type rather than on the address. + Tags = [new S7TagDefinition("X", "DB1.DBD0", dt)], + }; + using var drv = new S7Driver(opts, $"s7-bad-dt-{dt}"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.Message.ShouldContain(dt.ToString()); + + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Faulted); + } + + [Theory] + [InlineData(S7DataType.Bool, "DB1.DBX0.0")] + [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 async Task Initialize_accepts_implemented_data_types(S7DataType dt, string addr) + { + // Sanity check the guard is targeted — implemented data types must still pass. The + // TCP connect still fails (reserved host); the failure must NOT be a NotSupportedException + // from the data-type guard. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Tags = [new S7TagDefinition("X", addr, dt)], + }; + using var drv = new S7Driver(opts, $"s7-good-dt-{dt}"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.ShouldNotBeOfType( + "implemented data types must pass the init guard — the failure must be the connect"); + } +}