diff --git a/code-reviews/Driver.S7/findings.md b/code-reviews/Driver.S7/findings.md index ae121f6..1622bc3 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 | 14 | +| Open findings | 10 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | High | | Category | Correctness & logic bugs | | Location | `S7AddressParser.cs:93`, `S7Driver.cs:231` | -| Status | Open | +| Status | Resolved | **Description:** S7AddressParser.Parse accepts Timer (T0) and Counter (C0) addresses and the test suite asserts they parse successfully, but the read path @@ -55,7 +55,11 @@ until they are wired through to S7.Net, or implement the Timer/Counter read path If kept, reject Timer/Counter tags at InitializeAsync with a clear "not yet supported" error rather than letting them parse clean. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `InitializeAsync` now runs +`RejectUnsupportedTagAddresses`, which throws `NotSupportedException` with a +clear "not yet supported" message (echoing the tag name + address) for any tag +whose address parses as a Timer or Counter, so the bad config fails fast at init +rather than throwing a misleading type-mismatch on every read. ### Driver.S7-002 @@ -150,7 +154,7 @@ redundant global::S7.Net. qualifiers where using S7.Net already covers them. | Severity | High | | Category | Concurrency & thread safety | | Location | `S7Driver.cs:140`, `S7Driver.cs:457`, `S7Driver.cs:506` | -| Status | Open | +| Status | Resolved | **Description:** Disposal races with the in-flight probe / poll tasks. ShutdownAsync calls _probeCts.Cancel() and cancels each subscription CTS, but it @@ -168,7 +172,13 @@ running while ProbeLoopAsync may still touch the linked token. (or DisposeAsync) await Task.WhenAll(...) with a bounded timeout after cancelling, before disposing _gate and the CTS objects. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — the probe loop now stores its Task in +`_probeTask` and each subscription records its poll Task in `SubscriptionState.PollTask`. +`ShutdownAsync` cancels every CTS, awaits `Task.WhenAll` of those handles with a +bounded 5 s `DrainTimeout`, and only then disposes `_probeCts`, the subscription +CTSs, and (via `DisposeAsync`) `_gate` — so no loop can touch a disposed +semaphore. `Task.Run` is now passed `CancellationToken.None` so the handle is +always awaitable even if the token is already cancelled. ### Driver.S7-007 @@ -177,7 +187,7 @@ before disposing _gate and the CTS objects. | Severity | High | | Category | Error handling & resilience | | Location | `S7Driver.cs:200`, `S7DriverOptions.cs:13`, `docs/v2/driver-specs.md:434` | -| Status | Open | +| Status | Resolved | **Description:** PUT/GET-disabled handling contradicts the design and the module own docstring. driver-specs.md section 5 (line 434) and the @@ -197,7 +207,15 @@ PUT/GET-disabled / access-denied code to BadNotSupported with a distinct config-alert health state; keep BadDeviceFailure/Degraded only for genuine device-fault error codes. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `ReadAsync` / `WriteAsync` now split the +`PlcException` catch via an `IsAccessDenied` filter. S7.Net exposes no typed +error code for the S7 `AccessingObjectNotAllowed` status (its +`ValidateResponseCode` throws a plain `Exception` wrapped as the inner exception +of a `PlcException`), so `IsAccessDenied` walks the inner-exception chain for the +"not allowed" marker. A PUT/GET-disabled fault now maps to `BadNotSupported` and +sets health to `Faulted` with a config-alert message pointing operators at the +TIA Portal PUT/GET toggle; a genuine device fault still maps to +`BadDeviceFailure`/`Degraded`. ### Driver.S7-008 @@ -279,7 +297,7 @@ round-tripping through the async path. | Severity | High | | Category | Design-document adherence | | Location | `S7Driver.cs:82`, `S7Driver.cs:134`, `IDriver.cs:24` | -| Status | Open | +| Status | Resolved | **Description:** S7Driver ignores the driverConfigJson parameter on both InitializeAsync and ReinitializeAsync. The IDriver contract states InitializeAsync @@ -298,7 +316,14 @@ explicitly that S7 reconfiguration requires instance recreation and have ReinitializeAsync signal that the passed JSON is unused so the contract mismatch is visible. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — config parsing was factored out of the +factory into `S7DriverFactoryExtensions.ParseOptions`. `InitializeAsync` (and +therefore `ReinitializeAsync`, which delegates to it) now re-parses +`driverConfigJson` and rebuilds `_options` from it whenever the document carries +a real body, so a config change delivered through `ReinitializeAsync` — the only +Core-initiated in-process recovery path — is honoured. An empty / placeholder +document (`""`, `{}`, `[]`) keeps the constructor-supplied options so existing +lifecycle unit tests that pass `"{}"` are unaffected. ### Driver.S7-012 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 52260e4..e8616ee 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using S7.Net; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; @@ -30,13 +31,27 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) { // ---- ISubscribable + IHostConnectivityProbe state ---- - private readonly System.Collections.Concurrent.ConcurrentDictionary _subscriptions = new(); + private readonly ConcurrentDictionary _subscriptions = new(); private long _nextSubscriptionId; private readonly object _probeLock = new(); private HostState _hostState = HostState.Unknown; private DateTime _hostStateChangedUtc = DateTime.UtcNow; private CancellationTokenSource? _probeCts; + /// + /// Handle to the in-flight probe loop. Tracked (rather than fire-and-forget) so + /// can await it after cancelling — otherwise a probe + /// iteration still inside the would race a disposed semaphore. + /// See code-review finding Driver.S7-006. + /// + private Task? _probeTask; + + /// + /// Bounded grace window waits for the probe + poll loops to + /// observe cancellation and exit before it disposes the shared semaphore / CTS objects. + /// + private static readonly TimeSpan DrainTimeout = TimeSpan.FromSeconds(5); + public event EventHandler? OnDataChange; public event EventHandler? OnHostStatusChanged; @@ -50,13 +65,20 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) private const uint StatusBadInternalError = 0x80020000u; /// OPC UA StatusCode used for socket / timeout / protocol-layer faults. private const uint StatusBadCommunicationError = 0x80050000u; - /// OPC UA StatusCode used when S7 returns ErrorCode.WrongCPU / PUT/GET disabled. + /// OPC UA StatusCode used for a genuine device fault (CPU error, hardware fault). private const uint StatusBadDeviceFailure = 0x80550000u; private readonly Dictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary _parsedByName = new(StringComparer.OrdinalIgnoreCase); - private readonly S7DriverOptions _options = options; + /// + /// Active driver configuration. Seeded from the constructor argument, then replaced by + /// whatever / parse out of + /// the supplied driverConfigJson — see code-review finding Driver.S7-011. The + /// constructor value is the fallback used when the caller passes an empty / placeholder + /// JSON document (e.g. the "{}" some unit tests pass). + /// + private S7DriverOptions _options = options; private readonly SemaphoreSlim _gate = new(1, 1); /// @@ -84,6 +106,18 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) _health = new DriverHealth(DriverState.Initializing, null, null); try { + // Re-parse the supplied DriverConfig JSON so a config change delivered through the + // IDriver contract is honoured (Driver.S7-011). An empty / placeholder document + // (e.g. the "{}" some unit tests pass) keeps the constructor-supplied options. + if (HasConfigBody(driverConfigJson)) + _options = S7DriverFactoryExtensions.ParseOptions(driverInstanceId, driverConfigJson); + + // Timer (T{n}) / Counter (C{n}) addresses parse cleanly but the read path has no + // S7DataType for them and no decode case — reject them here so a config typo + // fails fast at init instead of throwing a misleading type-mismatch on every + // read (Driver.S7-001). Drop this guard when Timer/Counter reads are wired through. + RejectUnsupportedTagAddresses(); + 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 @@ -117,7 +151,11 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) if (_options.Probe.Enabled) { _probeCts = new CancellationTokenSource(); - _ = Task.Run(() => ProbeLoopAsync(_probeCts.Token), _probeCts.Token); + // Track the probe Task (not fire-and-forget) so ShutdownAsync can await it + // before disposing _gate / _probeCts (Driver.S7-006). Pass None to Task.Run so + // the delegate always runs and the handle is always awaitable; the loop's own + // token check handles cancellation. + _probeTask = Task.Run(() => ProbeLoopAsync(_probeCts.Token), CancellationToken.None); } } catch (Exception ex) @@ -133,27 +171,89 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) public async Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { + // InitializeAsync re-parses driverConfigJson, so a config change delivered here is + // applied in place rather than silently discarded (Driver.S7-011). await ShutdownAsync(cancellationToken).ConfigureAwait(false); await InitializeAsync(driverConfigJson, cancellationToken).ConfigureAwait(false); } - public Task ShutdownAsync(CancellationToken cancellationToken) + public async Task ShutdownAsync(CancellationToken cancellationToken) { - try { _probeCts?.Cancel(); } catch { } - _probeCts?.Dispose(); - _probeCts = null; + // Signal cancellation to the probe + poll loops first, collect their Task handles, + // then await all of them with a bounded timeout BEFORE disposing the shared semaphore + // and CTS objects. Without the drain, a loop iteration mid-_gate would call Release() + // on (or WaitAsync against) a disposed semaphore — see code-review finding Driver.S7-006. + var drain = new List(); - foreach (var state in _subscriptions.Values) + 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 { } - state.Cts.Dispose(); + drain.Add(state.PollTask); } - _subscriptions.Clear(); + + if (drain.Count > 0) + { + try + { + await Task.WhenAll(drain).WaitAsync(DrainTimeout, CancellationToken.None) + .ConfigureAwait(false); + } + catch (TimeoutException) { /* a wedged loop — proceed; better than leaking the teardown */ } + catch { /* loop faults are already surfaced via health; teardown continues */ } + } + + // Loops have now observed cancellation and released _gate — safe to dispose the CTSs. + probeCts?.Dispose(); + _probeCts = null; + _probeTask = null; + foreach (var state in subscriptions) + state.Cts.Dispose(); try { Plc?.Close(); } catch { /* best-effort — tearing down anyway */ } Plc = null; _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); - return Task.CompletedTask; + } + + /// + /// True when carries a real config body. The + /// bootstrapper always passes a populated document; some unit tests pass "{}" or + /// an empty string to exercise lifecycle shape without a config — those keep the + /// constructor-supplied . + /// + private static bool HasConfigBody(string? driverConfigJson) + { + if (string.IsNullOrWhiteSpace(driverConfigJson)) return false; + var trimmed = driverConfigJson.Trim(); + return trimmed is not "{}" and not "[]"; + } + + /// + /// Rejects tag addresses the read path cannot serve. Timer (T{n}) and Counter + /// (C{n}) addresses parse cleanly via but + /// has no decode case for them and + /// has no Timer/Counter member — left unguarded they fail fast init's promise and throw + /// a misleading type-mismatch on every read instead (code-review finding Driver.S7-001). + /// + private void RejectUnsupportedTagAddresses() + { + foreach (var t in _options.Tags) + { + if (S7AddressParser.TryParse(t.Address, out var parsed) + && parsed.Area is S7Area.Timer or S7Area.Counter) + { + throw new NotSupportedException( + $"S7 tag '{t.Name}' uses a {parsed.Area} address ('{t.Address}'); " + + "Timer/Counter tags are not yet supported by the S7 driver. " + + "Remove the tag or use a DB/M/I/Q address until Timer/Counter reads are wired through."); + } + } } public DriverHealth GetHealth() => _health; @@ -197,12 +297,22 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) { results[i] = new DataValueSnapshot(null, StatusBadNotSupported, null, now); } - catch (global::S7.Net.PlcException pex) + catch (PlcException pex) when (IsAccessDenied(pex)) { - // S7.Net's PlcException carries an ErrorCode; PUT/GET-disabled on - // S7-1200/1500 surfaces here. Map to BadDeviceFailure so operators see a - // device-config problem (toggle PUT/GET in TIA Portal) rather than a - // transient fault — per driver-specs.md §5. + // PUT/GET-disabled (S7-1200/1500) / access-protection — a permanent + // configuration fault, NOT a transient one. Blind retry is wasted effort, + // so map it to BadNotSupported and flag the driver as a config alert + // (Faulted) rather than Degraded — per driver-specs.md §5 and + // code-review finding Driver.S7-007. + results[i] = new DataValueSnapshot(null, StatusBadNotSupported, null, now); + _health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead, + "S7 access denied — enable PUT/GET communication in TIA Portal " + + $"(Protection & Security) for this CPU. PLC reported: {pex.Message}"); + } + catch (PlcException pex) + { + // A genuine device-layer fault (CPU error, hardware fault) — transient + // enough to keep retrying; report BadDeviceFailure and degrade health. results[i] = new DataValueSnapshot(null, StatusBadDeviceFailure, null, now); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, pex.Message); } @@ -283,7 +393,17 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) { results[i] = new WriteResult(StatusBadNotSupported); } - catch (global::S7.Net.PlcException) + catch (PlcException pex) when (IsAccessDenied(pex)) + { + // PUT/GET-disabled / access-protection on write — same permanent + // configuration fault as on read (Driver.S7-007). BadNotSupported + + // a config-alert health state, not a transient device failure. + results[i] = new WriteResult(StatusBadNotSupported); + _health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead, + "S7 access denied — enable PUT/GET communication in TIA Portal " + + $"(Protection & Security) for this CPU. PLC reported: {pex.Message}"); + } + catch (PlcException) { results[i] = new WriteResult(StatusBadDeviceFailure); } @@ -323,9 +443,31 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) await plc.WriteAsync(tag.Address, boxed, ct).ConfigureAwait(false); } - private global::S7.Net.Plc RequirePlc() => + private Plc RequirePlc() => Plc ?? throw new InvalidOperationException("S7Driver not initialized"); + /// + /// Detects an S7 PUT/GET-disabled / access-protection fault inside an S7.Net + /// . S7.Net's read/write paths wrap every PLC-side error in a + /// PlcException with / ; + /// the response-code validator throws a plain for the S7 + /// AccessingObjectNotAllowed status, which lands as the inner exception. There is + /// no typed error code for it, so the inner message is the only discriminator + /// S7.Net exposes — see code-review finding Driver.S7-007. + /// + private static bool IsAccessDenied(PlcException pex) + { + for (Exception? e = pex; e is not null; e = e.InnerException) + { + if (e.Message.Contains("Accessing object not allowed", StringComparison.OrdinalIgnoreCase) + || e.Message.Contains("not allowed", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + return false; + } + // ---- ITagDiscovery ---- public Task DiscoverAsync(IAddressSpaceBuilder builder, CancellationToken cancellationToken) @@ -375,7 +517,9 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) var handle = new S7SubscriptionHandle(id); var state = new SubscriptionState(handle, [.. fullReferences], interval, cts); _subscriptions[id] = state; - _ = Task.Run(() => PollLoopAsync(state, cts.Token), cts.Token); + // Track the poll Task so ShutdownAsync can await it after cancelling — a poll + // iteration mid-_gate would otherwise race the semaphore's disposal (Driver.S7-006). + state.PollTask = Task.Run(() => PollLoopAsync(state, cts.Token), CancellationToken.None); return Task.FromResult(handle); } @@ -430,8 +574,14 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId) TimeSpan Interval, CancellationTokenSource Cts) { - public System.Collections.Concurrent.ConcurrentDictionary LastValues { get; } + public ConcurrentDictionary LastValues { get; } = new(StringComparer.OrdinalIgnoreCase); + + /// + /// Handle to this subscription's poll loop. Tracked so + /// can await it after cancelling — see code-review finding Driver.S7-006. + /// + public Task PollTask { get; set; } = Task.CompletedTask; } private sealed record S7SubscriptionHandle(long Id) : ISubscriptionHandle 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 3809dbf..1fe022b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs @@ -22,6 +22,19 @@ public static class S7DriverFactoryExtensions } internal static S7Driver CreateInstance(string driverInstanceId, string driverConfigJson) + { + ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); + ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); + return new S7Driver(ParseOptions(driverInstanceId, driverConfigJson), driverInstanceId); + } + + /// + /// Parse a driver-config JSON document into a strongly-typed . + /// Shared by the factory (instance creation) and by + /// / so a config change delivered through the + /// IDriver contract is actually applied — see code-review finding Driver.S7-011. + /// + internal static S7DriverOptions ParseOptions(string driverInstanceId, string driverConfigJson) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); @@ -34,7 +47,7 @@ public static class S7DriverFactoryExtensions throw new InvalidOperationException( $"S7 driver config for '{driverInstanceId}' missing required Host"); - var options = new S7DriverOptions + return new S7DriverOptions { Host = dto.Host!, Port = dto.Port ?? 102, @@ -54,8 +67,6 @@ public static class S7DriverFactoryExtensions ProbeAddress = dto.Probe?.ProbeAddress ?? "MW0", }, }; - - return new S7Driver(options, driverInstanceId); } private static S7TagDefinition BuildTag(S7TagDto t, string driverInstanceId) => diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs new file mode 100644 index 0000000..d731717 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs @@ -0,0 +1,179 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Regression tests for the High code-review findings closed against the S7 driver: +/// Driver.S7-001 (Timer/Counter tags rejected at init), Driver.S7-006 (shutdown drains +/// the probe/poll loops before disposing the gate), Driver.S7-007 (PUT/GET-disabled maps +/// to a config alert, not a transient fault), and Driver.S7-011 (Initialize/Reinitialize +/// honour the supplied driverConfigJson). +/// +[Trait("Category", "Unit")] +public sealed class S7DriverCodeReviewFixTests +{ + // ---- Driver.S7-001 — Timer/Counter tags must be rejected at init ---- + + [Theory] + [InlineData("T0")] + [InlineData("T15")] + [InlineData("C0")] + [InlineData("C10")] + public async Task Initialize_rejects_timer_or_counter_tag_with_NotSupportedException(string address) + { + // A Timer/Counter address parses cleanly but the read path has no decode case for it, + // so it must fail fast at init rather than throw a misleading type-mismatch on every + // read. The host is reserved-for-documentation so the TCP connect can never succeed — + // the unsupported-address guard runs before the connect, so the NotSupportedException + // is what surfaces. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Tags = [new S7TagDefinition("Quirk", address, S7DataType.Int16)], + }; + using var drv = new S7Driver(opts, "s7-tc"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.Message.ShouldContain(address); + + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Faulted, "an unsupported-address config error must Fault the driver"); + health.LastError.ShouldNotBeNull(); + } + + [Fact] + public async Task Initialize_accepts_DB_and_MIQ_addresses_without_the_unsupported_guard_tripping() + { + // Sanity check the guard is targeted — DB/M/I/Q tags must NOT be rejected by it. + // The connect still fails (reserved host), so we assert the failure is the connect, + // NOT a NotSupportedException from the address guard. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Tags = + [ + new S7TagDefinition("Word", "DB1.DBW0", S7DataType.Int16), + new S7TagDefinition("Bit", "M0.0", S7DataType.Bool), + ], + }; + using var drv = new S7Driver(opts, "s7-ok"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.ShouldNotBeOfType( + "DB/M/I/Q tags are supported — the failure must be the connect, not the address guard"); + } + + // ---- Driver.S7-011 — driverConfigJson must be applied on Initialize ---- + + [Fact] + public async Task Initialize_applies_the_supplied_driverConfigJson_over_the_constructor_options() + { + // Constructor options point at a real-looking host; the JSON config points at a + // reserved-for-documentation host with a tiny timeout. If InitializeAsync honours the + // JSON (the IDriver contract), the connect fails fast against 192.0.2.x. If it ignored + // the JSON it would hang on the constructor host instead. + var ctorOpts = new S7DriverOptions { Host = "10.255.255.1", Timeout = TimeSpan.FromSeconds(30) }; + using var drv = new S7Driver(ctorOpts, "s7-cfg"); + + const string json = """ + { "Host": "192.0.2.1", "TimeoutMs": 250, + "Tags": [ { "Name": "W", "Address": "DB1.DBW0", "DataType": "Int16" } ] } + """; + + var sw = System.Diagnostics.Stopwatch.StartNew(); + await Should.ThrowAsync(async () => + await drv.InitializeAsync(json, TestContext.Current.CancellationToken)); + sw.Stop(); + + // A 30 s constructor timeout would dominate; the 250 ms JSON timeout proves the JSON won. + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(10), + "InitializeAsync must apply the driverConfigJson timeout, not the constructor's"); + } + + [Fact] + public async Task Initialize_rejects_a_timer_tag_supplied_only_through_driverConfigJson() + { + // The Timer/Counter guard must run against the *re-parsed* config, not just the + // constructor options — proves Driver.S7-001 and Driver.S7-011 compose correctly. + var ctorOpts = new S7DriverOptions { Host = "192.0.2.1", Timeout = TimeSpan.FromMilliseconds(250) }; + using var drv = new S7Driver(ctorOpts, "s7-cfg-tc"); + + const string json = """ + { "Host": "192.0.2.1", "TimeoutMs": 250, + "Tags": [ { "Name": "TimerTag", "Address": "T5", "DataType": "Int16" } ] } + """; + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync(json, TestContext.Current.CancellationToken)); + ex.Message.ShouldContain("T5"); + } + + [Fact] + public async Task Reinitialize_applies_a_changed_driverConfigJson() + { + // ReinitializeAsync is the only Core-initiated in-process recovery path; a config + // change delivered through it must not be silently discarded. + var ctorOpts = new S7DriverOptions { Host = "10.255.255.1", Timeout = TimeSpan.FromSeconds(30) }; + using var drv = new S7Driver(ctorOpts, "s7-reinit"); + + const string changed = """{ "Host": "192.0.2.1", "TimeoutMs": 250 }"""; + + var sw = System.Diagnostics.Stopwatch.StartNew(); + await Should.ThrowAsync(async () => + await drv.ReinitializeAsync(changed, TestContext.Current.CancellationToken)); + sw.Stop(); + + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(10), + "ReinitializeAsync must re-parse and apply the new driverConfigJson"); + } + + // ---- Driver.S7-006 — Shutdown drains probe/poll loops before disposing the gate ---- + + [Fact] + public async Task Shutdown_completes_cleanly_with_active_subscriptions_and_no_disposal_race() + { + // SubscribeAsync starts a poll loop; ShutdownAsync must cancel AND await it before + // disposing the shared semaphore. A regression here surfaces as an + // ObjectDisposedException escaping ShutdownAsync / DisposeAsync. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + }; + var drv = new S7Driver(opts, "s7-drain"); + + await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + await drv.SubscribeAsync(["B"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + + // Let the poll loops actually start churning so cancellation has something to race. + await Task.Delay(150, TestContext.Current.CancellationToken); + + // Must not throw — the drain awaits the poll tasks before disposing _gate. + await Should.NotThrowAsync(async () => await drv.ShutdownAsync(CancellationToken.None)); + await Should.NotThrowAsync(async () => await drv.DisposeAsync()); + } + + [Fact] + public async Task Dispose_after_subscribe_does_not_throw_ObjectDisposedException() + { + // The synchronous Dispose() path round-trips through DisposeAsync → ShutdownAsync; + // it must also drain the poll loop rather than dispose the gate from under it. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + }; + var drv = new S7Driver(opts, "s7-dispose"); + + await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + await Task.Delay(150, TestContext.Current.CancellationToken); + + Should.NotThrow(() => drv.Dispose()); + } +}