From 3c75db7eb6fd7ae9848b847267a8b3cd987f0871 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 08:17:42 -0400 Subject: [PATCH] fix(driver-twincat): resolve Low code-review findings (Driver.TwinCAT-004,006,014,015,016) - Driver.TwinCAT-004: corrected the IEC time-type inline comments; documented that the driver currently surfaces them as raw UInt32 counters. - Driver.TwinCAT-006: ResolveHost returns a documented UnresolvedHost sentinel when no devices are configured instead of returning the logical DriverInstanceId (which never matches GetHostStatuses). - Driver.TwinCAT-014: wired Probe.Timeout into the probe-loop call and added a NotificationMaxDelayMs config knob threaded through AddNotificationAsync. - Driver.TwinCAT-015: Dispose() runs a genuinely synchronous teardown with bounded waits (no sync-over-async deadlock pattern). - Driver.TwinCAT-016: pinned the Structure-tag rejection and the probe-loop vs read disposal race with regression tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.TwinCAT/findings.md | 22 +- .../AdsTwinCATClient.cs | 7 +- .../ITwinCATClient.cs | 4 + .../TwinCATDataType.cs | 14 +- .../TwinCATDriver.cs | 69 ++++- .../TwinCATDriverFactoryExtensions.cs | 11 + .../TwinCATDriverOptions.cs | 10 + .../FakeTwinCATClient.cs | 5 +- .../TwinCATCapabilityTests.cs | 6 +- .../TwinCATLowFindingsRegressionTests.cs | 264 ++++++++++++++++++ .../TwinCATNativeNotificationTests.cs | 4 +- 11 files changed, 389 insertions(+), 27 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLowFindingsRegressionTests.cs diff --git a/code-reviews/Driver.TwinCAT/findings.md b/code-reviews/Driver.TwinCAT/findings.md index c528991..af595f1 100644 --- a/code-reviews/Driver.TwinCAT/findings.md +++ b/code-reviews/Driver.TwinCAT/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -112,7 +112,7 @@ does not support UDT tags, and `BrowseSymbolsAsync` already correctly yields | Severity | Low | | Category | Correctness & logic bugs | | Location | `TwinCATDataType.cs:24-27` | -| Status | Open | +| Status | Resolved | **Description:** The inline comments for the IEC time types are inaccurate. TwinCAT `TIME` is a duration (32-bit, milliseconds) — not "ms since epoch of day". `DATE` is stored as seconds @@ -125,7 +125,7 @@ implementer who tries to add proper conversion. date/time semantics are intended to be exposed properly, track a follow-up to decode them to `DriverDataType.DateTime`; otherwise document that they surface as raw counters. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — rewrote the inline comments to match the actual IEC 61131-3 / TwinCAT encoding (TIME = duration in ms, DATE = seconds since 1970-01-01 truncated to a day boundary, DT = seconds since 1970-01-01, TOD = ms since midnight) and added a block comment documenting that the driver surfaces them as raw UDINT counters via `DriverDataType.UInt32`. Test `Iec_time_types_map_to_uint32_raw_counter` pins the mapping. ### Driver.TwinCAT-005 @@ -156,7 +156,7 @@ catch), native-notification registration failures, and host state transitions | Severity | Low | | Category | OtOpcUa conventions | | Location | `TwinCATDriver.cs:406-411` | -| Status | Open | +| Status | Resolved | **Description:** `ResolveHost` falls back to `DriverInstanceId` when there are no configured devices and the reference is unknown. `DriverInstanceId` is a logical config-DB identifier, @@ -169,7 +169,7 @@ connectivity-status row. empty string or a documented unresolved marker), or document why the instance ID is the chosen fallback. Prefer the first device HostAddress only when one exists (already done). -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `ResolveHost` now returns `TwinCATDriver.UnresolvedHostSentinel` (empty string) when no devices are configured, replacing the `DriverInstanceId` collision with `GetHostStatuses()` rows. The sentinel is publicly documented on the driver type. Updated `ResolveHost_falls_back_to_unresolved_sentinel_when_no_devices` (was `_to_DriverInstanceId_`) and added `ResolveHost_returns_unresolved_sentinel_when_no_devices` + `ResolveHost_unresolved_sentinel_matches_no_GetHostStatuses_entry` regressions. ### Driver.TwinCAT-007 @@ -361,7 +361,7 @@ part of the documented driver contract, not optional. | Severity | Low | | Category | Design-document adherence | | Location | `TwinCATDriverOptions.cs:41-43`, `TwinCATDriverOptions.cs:57-62`, `AdsTwinCATClient.cs:145` | -| Status | Open | +| Status | Resolved | **Description:** Several drifts between the implemented config surface and `docs/v2/driver-specs.md` section 6. The spec connection-settings list has separate `Host` @@ -376,7 +376,7 @@ the probe path connects via `_options.Timeout` — a dead config field. The spec shape (the doc is DRAFT, so updating it is acceptable). Remove or wire up `TwinCATProbeOptions.Timeout`. Expose `NotificationMaxDelayMs` if batching control is wanted. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `TwinCATProbeOptions.Timeout` is now wired into `EnsureConnectedAsync` via an optional `timeoutOverride` parameter that the probe loop passes (reads / writes keep the driver-level `_options.Timeout`). Added a `TwinCATDriverOptions.NotificationMaxDelayMs` config knob (parsed from `driverConfigJson` via `TwinCATDriverConfigDto.NotificationMaxDelayMs`) and threaded it through `ITwinCATClient.AddNotificationAsync` so `NotificationSettings` carries the configured max-delay instead of the hard-coded 0. The `Host` / `AmsNetId` / `AmsPort` triple in the spec was already implemented as the single `HostAddress` (parsed `ads://{netId}:{port}` URI) — kept as-is to match the v2 driver convention; covered by `TwinCATAmsAddress`. Regression tests: `ProbeOptions_Timeout_is_applied_to_probe_calls`, `NotificationMaxDelayMs_is_exposed_on_driver_options`, `NotificationMaxDelayMs_parses_from_driver_config_json`. ### Driver.TwinCAT-015 @@ -385,7 +385,7 @@ shape (the doc is DRAFT, so updating it is acceptable). Remove or wire up | Severity | Low | | Category | Code organization & conventions | | Location | `TwinCATDriver.cs:431-432` | -| Status | Open | +| Status | Resolved | **Description:** `Dispose()` runs `DisposeAsync().AsTask().GetAwaiter().GetResult()` — sync-over-async. `docs/v2/driver-stability.md` section Galaxy explicitly lists "sync-over-async @@ -399,7 +399,7 @@ here — cancelling token sources, disposing clients, clearing dictionaries — synchronous, and `PollGroupEngine.DisposeAsync` completes synchronously, so factor the synchronous teardown out so `Dispose()` does not block on a `Task`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `Dispose()` now does an inline synchronous teardown with no `await` and no captured sync context: dispose native subscriptions, drive `PollGroupEngine.DisposeAsync` via `.AsTask().Wait(5s)` (no context capture), per-device `ProbeCts.Cancel()` + `ProbeTask.Wait(2s)`, `DisposeClient()` / `DisposeGate()`, then clear the dictionaries. `DisposeAsync` still routes through `ShutdownAsync` for genuinely async callers. Regression test `Dispose_does_not_block_on_async_in_default_synchronization_context` runs `Dispose()` inside a single-threaded `SynchronizationContext` that would deadlock a sync-over-async teardown and asserts it completes within 5s. ### Driver.TwinCAT-016 @@ -408,7 +408,7 @@ synchronous teardown out so `Dispose()` does not block on a `Task`. | Severity | Low | | Category | Testing coverage | | Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** Unit coverage exists for AMS-address parsing, symbol-path parsing, read/write, native notifications, symbol browse, and the capability surface. Gaps tied to the findings @@ -423,4 +423,4 @@ without truncation (Driver.TwinCAT-002). addressed, especially a concurrency stress test for `EnsureConnectedAsync` and a `ReinitializeAsync`-applies-new-config test. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — the previously-closed High findings each grew their regression coverage as they were resolved (see `TwinCATHighFindingsRegressionTests`: `ReinitializeAsync_applies_changed_device_config` for -001, `LInt_read_round_trips_value_above_int_MaxValue` + `DataType_mapping_preserves_width_and_signedness` for -002, `Concurrent_reads_on_one_device_create_a_single_client` + `Concurrent_reads_and_writes_share_one_client` for -007, `Symbol_version_changed_raises_OnRediscoveryNeeded` + `TwinCATDriver_implements_IRediscoverable` for -013). This pass added the two remaining gaps: `Structure_typed_pre_declared_tag_is_rejected_at_config_parse` (-003) and `Probe_loop_and_read_share_one_client_per_device` (-009 disposal-race coverage races 64 readers against the probe loop for 500ms and asserts a single client / single connect). All coverage lives in the test files `TwinCATHighFindingsRegressionTests.cs` and the new `TwinCATLowFindingsRegressionTests.cs`. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs index 6bec502..32d7856 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -167,6 +167,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient TwinCATDataType type, int? bitIndex, TimeSpan cycleTime, + int maxDelayMs, Action onChange, CancellationToken cancellationToken) { @@ -175,9 +176,11 @@ internal sealed class AdsTwinCATClient : ITwinCATClient // tcadsnetref/7313319051 — "The unit is 1ms"). AdsTransMode.OnChange fires when // the value differs; OnCycle fires every cycle. OnChange is the right default for // OPC UA data-change semantics — the PLC already has the best view of "has this - // changed" so we let it decide. + // changed" so we let it decide. maxDelayMs > 0 lets TwinCAT batch notifications up + // to that delay before pushing them — exposed via TwinCATDriverOptions + // (Driver.TwinCAT-014). var cycleMs = (int)Math.Max(1, cycleTime.TotalMilliseconds); - var settings = new NotificationSettings(AdsTransMode.OnChange, cycleMs, 0); + var settings = new NotificationSettings(AdsTransMode.OnChange, cycleMs, Math.Max(0, maxDelayMs)); // AddDeviceNotificationExAsync returns Task; AdsNotificationEx fires // with the handle as part of the event args so we use the handle as the correlation diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs index 29339b7..12d0949 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -67,6 +67,9 @@ public interface ITwinCATClient : IDisposable /// Declared type; drives the native layout + callback value boxing. /// For BOOL-within-word tags — the bit to extract from the parent word. /// Minimum interval between change notifications (native-floor depends on target). + /// Maximum batching delay in milliseconds — TwinCAT may coalesce + /// notifications up to this delay before pushing them. 0 = no batching, push + /// immediately (Driver.TwinCAT-014). /// Invoked with (symbolPath, boxedValue) per notification. /// Cancels the initial registration; does not tear down an established notification. Task AddNotificationAsync( @@ -74,6 +77,7 @@ public interface ITwinCATClient : IDisposable TwinCATDataType type, int? bitIndex, TimeSpan cycleTime, + int maxDelayMs, Action onChange, CancellationToken cancellationToken); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDataType.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDataType.cs index dead406..2512e84 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDataType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDataType.cs @@ -21,10 +21,16 @@ public enum TwinCATDataType LReal, // 64-bit IEEE-754 String, // ASCII string WString,// UTF-16 string - Time, // TIME — ms since epoch of day, stored as UDINT - Date, // DATE — days since 1970-01-01, stored as UDINT - DateTime, // DT — seconds since 1970-01-01, stored as UDINT - TimeOfDay,// TOD — ms since midnight, stored as UDINT + // IEC 61131-3 / TwinCAT temporal types — all stored on the wire as 32-bit unsigned (UDINT) + // raw counters. The driver surfaces them as DriverDataType.UInt32 (Driver.TwinCAT-002), so + // operators see the raw counter, not a decoded date/duration. Proper decoding to + // DriverDataType.DateTime is a future enhancement; until then comments accurately describe + // the on-wire encoding so the next implementer doesn't re-derive it wrong + // (Driver.TwinCAT-004). + Time, // TIME — duration in milliseconds, stored as UDINT (32-bit unsigned) + Date, // DATE — seconds since 1970-01-01 truncated to a day boundary, stored as UDINT + DateTime, // DT (DATE_AND_TIME) — seconds since 1970-01-01, stored as UDINT + TimeOfDay,// TOD — milliseconds since midnight, stored as UDINT /// UDT / FB instance. Resolved per member at discovery time. Structure, } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs index 9e9e605..fd7043f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -377,6 +377,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var reg = await client.AddNotificationAsync( symbolName, def.DataType, bitIndex, publishingInterval, + _options.NotificationMaxDelayMs, (_, value) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, new DataValueSnapshot( value, TwinCATStatusMapper.Good, DateTime.UtcNow, DateTime.UtcNow))), @@ -433,7 +434,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var success = false; try { - var client = await EnsureConnectedAsync(state, ct).ConfigureAwait(false); + // Probe-initiated connects honor TwinCATProbeOptions.Timeout — distinct from + // the driver-wide _options.Timeout used by reads/writes (Driver.TwinCAT-014). + var client = await EnsureConnectedAsync(state, ct, _options.Probe.Timeout) + .ConfigureAwait(false); success = await client.ProbeAsync(ct).ConfigureAwait(false); } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } @@ -469,11 +473,25 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // ---- IPerCallHostResolver ---- + /// + /// Documented sentinel returned by when neither the tag nor a + /// fallback device is configured. Empty-string never matches an + /// emitted by this driver (every real + /// host is an ads://… URI), so it cleanly signals "unresolved" without colliding + /// with a real host key. Used to be , which is a logical + /// config-DB identifier — that collided with consumers who expected the resolver and the + /// connectivity-status table to share keys (Driver.TwinCAT-006). + /// + public const string UnresolvedHostSentinel = ""; + public string ResolveHost(string fullReference) { if (_tagsByName.TryGetValue(fullReference, out var def)) return def.DeviceHostAddress; - return _options.Devices.FirstOrDefault()?.HostAddress ?? DriverInstanceId; + // First device's HostAddress when one exists; otherwise the unresolved sentinel — + // intentionally NOT DriverInstanceId, which is a config-DB key, not a host address + // (Driver.TwinCAT-006). + return _options.Devices.FirstOrDefault()?.HostAddress ?? UnresolvedHostSentinel; } /// @@ -484,7 +502,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// AB-CIP drivers serialize device access the same way; single-connection-per-PLC is /// also what docs/v2/driver-specs.md recommends. /// - private async Task EnsureConnectedAsync(DeviceState device, CancellationToken ct) + private async Task EnsureConnectedAsync( + DeviceState device, CancellationToken ct, TimeSpan? timeoutOverride = null) { // Fast path — already connected, no gate needed. if (device.Client is { IsConnected: true } fast) return fast; @@ -504,9 +523,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var client = _clientFactory.Create(); client.OnSymbolVersionChanged += HandleSymbolVersionChanged; + // timeoutOverride lets the probe loop use TwinCATProbeOptions.Timeout for probe- + // initiated connects rather than the driver-level _options.Timeout + // (Driver.TwinCAT-014). Reads / writes pass null and get the driver default. + var effectiveTimeout = timeoutOverride ?? _options.Timeout; try { - await client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct) + await client.ConnectAsync(device.ParsedAddress, effectiveTimeout, ct) .ConfigureAwait(false); _logger.LogInformation( "TwinCAT driver '{DriverInstanceId}' connected to {HostAddress}", @@ -542,7 +565,43 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery "TwinCAT symbol-version-changed (DeviceSymbolVersionInvalid 0x0711) — PLC program re-downloaded", ScopeHint: "TwinCAT")); - public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); + /// + /// Synchronous teardown — no await, no captured sync context. The OPC UA stack + /// thread can call ; routing through DisposeAsync().GetResult() + /// can deadlock on a single-threaded sync context (Driver.TwinCAT-015, + /// docs/v2/driver-stability.md). The operations here are all genuinely synchronous — + /// cancel tokens, wait on task handles with a hard timeout, dispose clients — so a + /// synchronous path does the right thing without re-entering the scheduler. + /// + public void Dispose() + { + // Dispose native subscriptions first — handle disposal is sync. + foreach (var sub in _nativeSubs.Values) + foreach (var r in sub.Registrations) { try { r.Dispose(); } catch { } } + _nativeSubs.Clear(); + + // PollGroupEngine.DisposeAsync awaits loop tasks; we drive that synchronously here + // (bounded wait — same 5s ceiling DisposeAsync uses internally) using Wait() on the + // returned ValueTask so no sync-context capture happens. + try { _poll.DisposeAsync().AsTask().Wait(TimeSpan.FromSeconds(5)); } catch { } + + foreach (var state in _devices.Values) + { + try { state.ProbeCts?.Cancel(); } catch { } + if (state.ProbeTask is Task pt) + { + try { pt.Wait(TimeSpan.FromSeconds(2)); } catch { /* probe-cancel races are expected */ } + } + state.ProbeCts?.Dispose(); + state.ProbeCts = null; + state.DisposeClient(); + state.DisposeGate(); + } + _devices.Clear(); + _tagsByName.Clear(); + _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + } + public async ValueTask DisposeAsync() => await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); internal sealed class DeviceState(TwinCATAmsAddress parsedAddress, TwinCATDeviceOptions options) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs index 2dde259..f38615b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs @@ -60,9 +60,19 @@ public static class TwinCATDriverFactoryExtensions Timeout = TimeSpan.FromMilliseconds(dto.TimeoutMs ?? 2_000), UseNativeNotifications = dto.UseNativeNotifications ?? true, EnableControllerBrowse = dto.EnableControllerBrowse ?? false, + NotificationMaxDelayMs = dto.NotificationMaxDelayMs ?? 0, }; } + /// + /// Test-visible wrapper around for the regression suite — + /// keeps the public driver surface unchanged while letting tests assert that JSON + /// fields like NotificationMaxDelayMs and Structure-tag rejection are + /// honored end-to-end. + /// + public static TwinCATDriverOptions ParseOptionsForTests(string driverConfigJson, string driverInstanceId) + => ParseOptions(driverConfigJson, driverInstanceId); + private static TwinCATTagDefinition BuildTag(TwinCATTagDto t, string driverInstanceId) { var dataType = ParseEnum(t.DataType, t.Name, driverInstanceId, "DataType"); @@ -117,6 +127,7 @@ public static class TwinCATDriverFactoryExtensions public int? TimeoutMs { get; init; } public bool? UseNativeNotifications { get; init; } public bool? EnableControllerBrowse { get; init; } + public int? NotificationMaxDelayMs { get; init; } public List? Devices { get; init; } public List? Tags { get; init; } public TwinCATProbeDto? Probe { get; init; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverOptions.cs index 173a3a0..316f7bd 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverOptions.cs @@ -32,6 +32,16 @@ public sealed class TwinCATDriverOptions /// the strict-config path for deployments where only declared tags should appear. /// public bool EnableControllerBrowse { get; init; } + + /// + /// Maximum batching delay in milliseconds for ADS device notifications. Passed to + /// NotificationSettings as the max-delay value: TwinCAT may coalesce notifications + /// up to this delay before pushing them. 0 (default) = no batching, push + /// immediately. Useful for high-churn signals where the OPC UA subscriber tolerates a + /// small delay in exchange for fewer wire round-trips. Listed in docs/v2/driver-specs.md + /// section 6 — was previously hard-coded to 0 (Driver.TwinCAT-014). + /// + public int NotificationMaxDelayMs { get; init; } } /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs index ae15fa0..8f9f741 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/FakeTwinCATClient.cs @@ -67,14 +67,17 @@ internal class FakeTwinCATClient : ITwinCATClient public List Notifications { get; } = new(); public bool ThrowOnAddNotification { get; set; } + /// Records the most recently-supplied maxDelayMs for Driver.TwinCAT-014 tests. + public int LastMaxDelayMs { get; private set; } public virtual Task AddNotificationAsync( string symbolPath, TwinCATDataType type, int? bitIndex, TimeSpan cycleTime, - Action onChange, CancellationToken cancellationToken) + int maxDelayMs, Action onChange, CancellationToken cancellationToken) { if (ThrowOnAddNotification) throw Exception ?? new InvalidOperationException("fake AddNotification failure"); + LastMaxDelayMs = maxDelayMs; var reg = new FakeNotification(symbolPath, type, bitIndex, onChange, this); Notifications.Add(reg); return Task.FromResult(reg); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs index 21d482a..ba07093 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapabilityTests.cs @@ -217,12 +217,14 @@ public sealed class TwinCATCapabilityTests } [Fact] - public async Task ResolveHost_falls_back_to_DriverInstanceId_when_no_devices() + public async Task ResolveHost_falls_back_to_unresolved_sentinel_when_no_devices() { + // Driver.TwinCAT-006: empty-string sentinel — DriverInstanceId is a config-DB key, not + // a host address, so it would collide with no GetHostStatuses() row. var drv = new TwinCATDriver(new TwinCATDriverOptions(), "drv-1"); await drv.InitializeAsync("{}", CancellationToken.None); - drv.ResolveHost("anything").ShouldBe("drv-1"); + drv.ResolveHost("anything").ShouldBe(TwinCATDriver.UnresolvedHostSentinel); } // ---- helpers ---- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLowFindingsRegressionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLowFindingsRegressionTests.cs new file mode 100644 index 0000000..dd41944 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLowFindingsRegressionTests.cs @@ -0,0 +1,264 @@ +using System.Text.Json; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// +/// Regression coverage for the remaining (Low/Medium) code-review findings: +/// Driver.TwinCAT-004 (IEC time-type doc-comment accuracy), -006 (ResolveHost +/// sentinel for no-devices fallback), -014 (NotificationMaxDelayMs config knob +/// and ProbeOptions.Timeout wiring), -015 (Dispose runs a true synchronous +/// teardown, no sync-over-async), -016 (gap-fill tests: Structure-tag rejection, +/// concurrent probe + read race). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATLowFindingsRegressionTests +{ + private const string DeviceA = "ads://5.23.91.23.1.1:851"; + + // ---- Driver.TwinCAT-004 — TIME/DATE/DT/TOD surface unchanged but comments corrected ---- + + [Fact] + public void Iec_time_types_map_to_uint32_raw_counter() + { + // Documents the contract called out in the corrected comments: TIME / DATE / DT / TOD + // surface as their raw UDINT counter (32-bit unsigned), not as decoded DateTime/TimeSpan. + // The next implementer who wants to decode them needs to see this mapping is intentional. + TwinCATDataType.Time.ToDriverDataType().ShouldBe(DriverDataType.UInt32); + TwinCATDataType.Date.ToDriverDataType().ShouldBe(DriverDataType.UInt32); + TwinCATDataType.DateTime.ToDriverDataType().ShouldBe(DriverDataType.UInt32); + TwinCATDataType.TimeOfDay.ToDriverDataType().ShouldBe(DriverDataType.UInt32); + } + + // ---- Driver.TwinCAT-006 — ResolveHost sentinel when no devices are configured ---- + + [Fact] + public async Task ResolveHost_returns_unresolved_sentinel_when_no_devices() + { + // DriverInstanceId is a logical config-DB key, not a host address; consumers expect a + // host key that correlates with GetHostStatuses(). When there are no devices and the + // reference is unknown, ResolveHost must return the documented unresolved sentinel + // (empty string), not the driver-instance ID. + var drv = new TwinCATDriver(new TwinCATDriverOptions(), "drv-1"); + await drv.InitializeAsync("{}", CancellationToken.None); + + drv.ResolveHost("anything").ShouldBe(string.Empty); + } + + [Fact] + public async Task ResolveHost_unresolved_sentinel_matches_no_GetHostStatuses_entry() + { + // Documents the contract: the sentinel should never match a real connectivity-status row. + var drv = new TwinCATDriver(new TwinCATDriverOptions(), "drv-1"); + await drv.InitializeAsync("{}", CancellationToken.None); + + var sentinel = drv.ResolveHost("anything"); + drv.GetHostStatuses().ShouldNotContain(s => s.HostName == sentinel); + } + + // ---- Driver.TwinCAT-014 — config surface knobs are honoured ---- + + [Fact] + public async Task ProbeOptions_Timeout_is_applied_to_probe_calls() + { + // The previous implementation declared a Timeout field but never read it — the probe + // path connected with _options.Timeout. The probe must use its own configured timeout. + var observed = new List(); + var factory = new FakeTwinCATClientFactory + { + Customise = () => new ProbeTimeoutCapturingFake(observed), + }; + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Probe = new TwinCATProbeOptions + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(100), + Timeout = TimeSpan.FromMilliseconds(750), // distinct from the connect timeout + }, + Timeout = TimeSpan.FromMilliseconds(2_000), + }, "drv-1", factory); + + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitForAsync(() => observed.Count >= 1, TimeSpan.FromSeconds(2)); + await drv.ShutdownAsync(CancellationToken.None); + + observed.ShouldContain(TimeSpan.FromMilliseconds(750)); + } + + [Fact] + public void NotificationMaxDelayMs_is_exposed_on_driver_options() + { + // The driver-spec lists NotificationMaxDelayMs as a per-device knob; the implementation + // previously hard-coded 0 in NotificationSettings. Expose a configurable field so + // operators can batch low-priority notifications. + var options = new TwinCATDriverOptions { NotificationMaxDelayMs = 200 }; + options.NotificationMaxDelayMs.ShouldBe(200); + } + + [Fact] + public void NotificationMaxDelayMs_parses_from_driver_config_json() + { + var json = JsonSerializer.Serialize(new + { + devices = new[] { new { hostAddress = DeviceA } }, + notificationMaxDelayMs = 150, + }); + var parsed = TwinCATDriverFactoryExtensions.ParseOptionsForTests(json, "drv-1"); + parsed.NotificationMaxDelayMs.ShouldBe(150); + } + + // ---- Driver.TwinCAT-015 — Dispose runs a true synchronous teardown ---- + + [Fact] + public void Dispose_does_not_block_on_async_in_default_synchronization_context() + { + // Sync-over-async on a single-threaded sync context (like the OPC UA stack thread) can + // deadlock. Dispose() must complete without scheduling continuations through a captured + // sync context. We verify by running Dispose() inside a SynchronizationContext that + // would deadlock a sync-over-async teardown. + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1"); + drv.InitializeAsync("{}", CancellationToken.None).GetAwaiter().GetResult(); + + var ctx = new SingleThreadedSyncContext(); + var prev = SynchronizationContext.Current; + Exception? captured = null; + try + { + SynchronizationContext.SetSynchronizationContext(ctx); + // If Dispose schedules its continuations through the captured context (sync-over- + // async pattern), and the context is single-threaded with nothing pumping, this + // will hang. We give it 5 seconds — well above any reasonable sync teardown. + var thread = new Thread(() => + { + try + { + SynchronizationContext.SetSynchronizationContext(ctx); + drv.Dispose(); + } + catch (Exception ex) { captured = ex; } + }) { IsBackground = true }; + thread.Start(); + thread.Join(TimeSpan.FromSeconds(5)).ShouldBeTrue( + "Dispose() did not complete within 5s — likely sync-over-async deadlock " + + "(Driver.TwinCAT-015)."); + } + finally + { + SynchronizationContext.SetSynchronizationContext(prev); + } + captured.ShouldBeNull(); + } + + /// + /// Single-threaded sync context that posts continuations to an internal queue but never + /// pumps them. Any sync-over-async code that captures this context and waits for a + /// continuation will deadlock — exactly the OPC UA stack thread scenario. + /// + private sealed class SingleThreadedSyncContext : SynchronizationContext + { + private readonly System.Collections.Concurrent.ConcurrentQueue<(SendOrPostCallback cb, object? state)> _queue = new(); + public override void Post(SendOrPostCallback d, object? state) => _queue.Enqueue((d, state)); + public override void Send(SendOrPostCallback d, object? state) => d(state); + } + + // ---- Driver.TwinCAT-016 — gap-fill tests for previously closed findings ---- + + [Fact] + public void Structure_typed_pre_declared_tag_is_rejected_at_config_parse() + { + // Driver.TwinCAT-003 — config-time rejection. A Structure tag must fail loudly with a + // clear error rather than reading as a garbage int blob or failing late on a write. + var json = JsonSerializer.Serialize(new + { + devices = new[] { new { hostAddress = DeviceA } }, + tags = new[] + { + new + { + name = "Udt1", + deviceHostAddress = DeviceA, + symbolPath = "MAIN.fbInstance", + dataType = "Structure", + }, + }, + }); + var ex = Should.Throw(() => + TwinCATDriverFactoryExtensions.ParseOptionsForTests(json, "drv-1")); + ex.Message.ShouldContain("Structure"); + ex.Message.ShouldContain("Udt1"); + } + + [Fact] + public async Task Probe_loop_and_read_share_one_client_per_device() + { + // Driver.TwinCAT-007 / -009 — gap-fill: race the probe loop against concurrent reads on + // the same device. The per-device gate must serialize connect; the probe-task await on + // ShutdownAsync must let the loop exit cleanly. Without these, the test trips a leaked + // client or a disposal race. + var factory = new FakeTwinCATClientFactory + { + Customise = () => new FakeTwinCATClient { Values = { ["GVL.X"] = 1 }, ProbeResult = true }, + }; + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Tags = [new TwinCATTagDefinition("X", DeviceA, "GVL.X", TwinCATDataType.DInt)], + Probe = new TwinCATProbeOptions + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(20), + Timeout = TimeSpan.FromMilliseconds(50), + }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + // Race 64 readers against the probe loop for ~500ms. + var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(500)); + var work = Enumerable.Range(0, 64).Select(_ => Task.Run(async () => + { + while (!cts.IsCancellationRequested) + { + try { await drv.ReadAsync(["X"], CancellationToken.None); } + catch { /* shutdown-races on the very last call are ok */ } + } + })).ToArray(); + await Task.WhenAll(work); + await drv.ShutdownAsync(CancellationToken.None); + + // One client total. If the gate is broken, concurrent connects leak additional clients. + factory.Clients.Count.ShouldBe(1); + factory.Clients[0].ConnectCount.ShouldBe(1); + } + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(20); + } + + /// Captures the timeout argument from ProbeAsync invocations (via the connect path). + private sealed class ProbeTimeoutCapturingFake : FakeTwinCATClient + { + private readonly List _observed; + public ProbeTimeoutCapturingFake(List observed) { _observed = observed; } + + public override Task ConnectAsync(TwinCATAmsAddress address, TimeSpan timeout, CancellationToken ct) + { + // The driver calls EnsureConnectedAsync with the probe timeout for probe-initiated + // connects. The probe timeout is distinct from the driver-level Timeout; we record + // it on the first connect. + lock (_observed) _observed.Add(timeout); + return base.ConnectAsync(address, timeout, ct); + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeNotificationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeNotificationTests.cs index 119fd5b..4c0caa3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeNotificationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeNotificationTests.cs @@ -137,12 +137,12 @@ public sealed class TwinCATNativeNotificationTests public override Task AddNotificationAsync( string symbolPath, TwinCATDataType type, int? bitIndex, TimeSpan cycleTime, - Action onChange, CancellationToken cancellationToken) + int maxDelayMs, Action onChange, CancellationToken cancellationToken) { AddCallCount++; if (AddCallCount > _succeedBefore) throw new InvalidOperationException($"fake fail on call #{AddCallCount}"); - return base.AddNotificationAsync(symbolPath, type, bitIndex, cycleTime, onChange, cancellationToken); + return base.AddNotificationAsync(symbolPath, type, bitIndex, cycleTime, maxDelayMs, onChange, cancellationToken); } }