From 5197b6c23760a59430649277b543d43f8787c044 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 06:37:05 -0400 Subject: [PATCH] fix(driver-twincat): resolve High code-review findings (Driver.TwinCAT-001, -002, -007, -008, -013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Driver.TwinCAT-001 — InitializeAsync/ReinitializeAsync ignored driverConfigJson. Extracted the DTO-to-options parse into a shared TwinCATDriverFactoryExtensions.ParseOptions; InitializeAsync now re-parses driverConfigJson into a mutable _options field, so a config generation pushed via ReinitializeAsync (added/removed devices, tags, probe settings) is actually applied at runtime. Driver.TwinCAT-002 — LInt/ULInt narrowed to Int32. ToDriverDataType now maps LInt to Int64, ULInt to UInt64, UDInt to UInt32, UInt/USInt to UInt16, Int/SInt to Int16, and the IEC TIME/DATE/DT/TOD types to UInt32 (their raw UDINT counter). Removed the stale "Int64 gap" comment — no truncation or sign flips at the OPC UA encode layer. Driver.TwinCAT-007 — EnsureConnectedAsync was not thread-safe. Connect/reconnect is now serialized per device by a SemaphoreSlim (DeviceState.ConnectGate) with a double-checked connect, mirroring the S7 driver. Concurrent read/write/probe callers can no longer leak a client or race a create-vs-dispose. Driver.TwinCAT-008 — native ADS notification callbacks ran driver logic on the AMS router thread. AdsTwinCATClient now enqueues AdsNotificationEx callbacks onto a bounded Channel drained by a dedicated managed task; the router-thread callback only does a non-blocking TryWrite, so a slow consumer cannot stall ADS notification delivery process-wide. Driver.TwinCAT-013 — TwinCATDriver did not implement IRediscoverable. The driver now implements IRediscoverable; AdsTwinCATClient detects ADS 0x0702 (symbol-version-changed) on read/write paths and raises OnSymbolVersionChanged, which the driver forwards as OnRediscoveryNeeded so Core rebuilds the address space after a PLC program re-download. Adds TwinCATHighFindingsRegressionTests covering all five fixes; updates the data-type mapping assertion in TwinCATDriverTests. TwinCAT driver builds clean; 119 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.TwinCAT/findings.md | 22 +- .../AdsTwinCATClient.cs | 78 ++++++- .../ITwinCATClient.cs | 10 + .../TwinCATDataType.cs | 14 +- .../TwinCATDriver.cs | 82 +++++++- .../TwinCATDriverFactoryExtensions.cs | 16 +- .../TwinCATStatusMapper.cs | 13 ++ .../FakeTwinCATClient.cs | 5 + .../TwinCATDriverTests.cs | 3 +- .../TwinCATHighFindingsRegressionTests.cs | 194 ++++++++++++++++++ 10 files changed, 400 insertions(+), 37 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHighFindingsRegressionTests.cs diff --git a/code-reviews/Driver.TwinCAT/findings.md b/code-reviews/Driver.TwinCAT/findings.md index a4ad550..91a418e 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 | 16 | +| Open findings | 11 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | High | | Category | Correctness & logic bugs | | Location | `TwinCATDriver.cs:41-78` | -| Status | Open | +| Status | Resolved | **Description:** `InitializeAsync` and `ReinitializeAsync` both ignore their `driverConfigJson` parameter entirely. `InitializeAsync` builds device/tag state exclusively from `_options`, @@ -55,7 +55,7 @@ parser) and assign the resulting options to a mutable field, rather than relying constructor-captured `_options`. Alternatively, document explicitly that the constructor is the sole config source and have the Core recreate the driver instance on config change. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — extracted the DTO→options parse into a shared TwinCATDriverFactoryExtensions.ParseOptions; InitializeAsync re-parses driverConfigJson into a now-mutable _options field, so ReinitializeAsync applies a changed config generation. ### Driver.TwinCAT-002 @@ -64,7 +64,7 @@ the sole config source and have the Core recreate the driver instance on config | Severity | High | | Category | Correctness & logic bugs | | Location | `TwinCATDataType.cs:34-48`, `AdsTwinCATClient.cs:264-281` | -| Status | Open | +| Status | Resolved | **Description:** `TwinCATDataTypeExtensions.ToDriverDataType` maps `LInt` and `ULInt` (signed/ unsigned 64-bit) to `DriverDataType.Int32` (comment: "matches Int64 gap"). The address-space @@ -79,7 +79,7 @@ the range 0x80000000 to 0xFFFFFFFF surface as negative. **Recommendation:** Map `LInt` to `Int64`, `ULInt` to `UInt64`, `UDInt` to `UInt32`, `UInt` to `UInt16`, and `USInt`/`SInt` to their natural widths. Remove the stale "Int64 gap" comment. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — ToDriverDataType now maps LInt→Int64, ULInt→UInt64, UDInt→UInt32, UInt/USInt→UInt16, Int/SInt→Int16, and the IEC time types→UInt32; removed the stale Int64-gap comment. ### Driver.TwinCAT-003 @@ -178,7 +178,7 @@ fallback. Prefer the first device HostAddress only when one exists (already done | Severity | High | | Category | Concurrency & thread safety | | Location | `TwinCATDriver.cs:413-429` | -| Status | Open | +| Status | Resolved | **Description:** `EnsureConnectedAsync` is not thread-safe. `ReadAsync`, `WriteAsync`, `SubscribeAsync`, and the per-device `ProbeLoopAsync` background task can all call it @@ -196,7 +196,7 @@ continuously, so this race is not hypothetical under any concurrent read/write l serialize device access with a `SemaphoreSlim` — follow that pattern. Note this also serializes the wire, which `docs/v2/driver-specs.md` recommends for single-connection-per-PLC. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — EnsureConnectedAsync is now serialized per device by a SemaphoreSlim (DeviceState.ConnectGate) with a double-checked connect, mirroring the S7 driver; no client is leaked and no disposal race remains. ### Driver.TwinCAT-008 @@ -205,7 +205,7 @@ serializes the wire, which `docs/v2/driver-specs.md` recommends for single-conne | Severity | High | | Category | Concurrency & thread safety | | Location | `AdsTwinCATClient.cs:162-169`, `TwinCATDriver.cs:319-324` | -| Status | Open | +| Status | Resolved | **Description:** Native ADS notification callbacks (`OnAdsNotificationEx`) run on the `AdsClient` AMS router thread. `docs/v2/driver-specs.md` section 6 explicitly calls this out @@ -221,7 +221,7 @@ device in the process. a dedicated managed task before invoking `OnChange`/`OnDataChange`, exactly as the Galaxy `EventPump` does. Keep the router-thread callback to a non-blocking enqueue only. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — AdsTwinCATClient now enqueues native AdsNotificationEx callbacks onto a bounded Channel drained by a dedicated managed task; the AMS router thread only does a non-blocking TryWrite, so a slow consumer cannot stall ADS delivery. ### Driver.TwinCAT-009 @@ -334,7 +334,7 @@ stream-and-discard design is intentional, report the real footprint of `_nativeS | Severity | High | | Category | Design-document adherence | | Location | `TwinCATDriver.cs:11-12` (capability list), whole file | -| Status | Open | +| Status | Resolved | **Description:** `TwinCATDriver` does not implement `IRediscoverable`. Both `docs/v2/driver-specs.md` section 6 and `docs/v2/driver-stability.md` section "TwinCAT — Deep @@ -352,7 +352,7 @@ on read/write/notification paths, raise `OnRediscoveryNeeded` with a scoped reas re-establish native notifications after the Core re-runs `DiscoverAsync`. This is explicitly part of the documented driver contract, not optional. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — TwinCATDriver implements IRediscoverable; AdsTwinCATClient detects ADS 0x0702 on read/write paths and raises OnSymbolVersionChanged, which the driver forwards as OnRediscoveryNeeded so Core rebuilds the address space. ### Driver.TwinCAT-014 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 364cee9..cc5bf4c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -1,5 +1,6 @@ using System.Collections.Concurrent; using System.Runtime.CompilerServices; +using System.Threading.Channels; using TwinCAT; using TwinCAT.Ads; using TwinCAT.Ads.TypeSystem; @@ -21,16 +22,50 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; /// internal sealed class AdsTwinCATClient : ITwinCATClient { + // Bounded so a slow downstream consumer cannot back the AMS router thread up — the + // router thread enqueues and returns immediately (Driver.TwinCAT-008). 50k matches the + // Galaxy EventPump default; ~500 notifications/connection is the ADS ceiling so this is + // generous headroom against bursty change storms. + private const int NotificationQueueCapacity = 50_000; + private readonly AdsClient _client = new(); private readonly ConcurrentDictionary _notifications = new(); + // Marshals native ADS notifications off the AMS router thread onto a dedicated managed + // task. The router-thread callback (OnAdsNotificationEx) only enqueues; DispatchLoopAsync + // drains and invokes the per-tag OnChange delegates. Blocking the router thread would + // stall every ADS notification across the whole process (docs/v2/driver-specs.md §6). + private readonly Channel _notificationQueue = + Channel.CreateBounded(new BoundedChannelOptions(NotificationQueueCapacity) + { + FullMode = BoundedChannelFullMode.DropWrite, + SingleReader = true, + SingleWriter = false, + }); + private readonly CancellationTokenSource _dispatchCts = new(); + private readonly Task _dispatchLoop; + private int _disposed; + public AdsTwinCATClient() { _client.AdsNotificationEx += OnAdsNotificationEx; + _dispatchLoop = Task.Run(() => DispatchLoopAsync(_dispatchCts.Token)); } + private readonly record struct PendingNotification(NotificationRegistration Registration, object? Value); + public bool IsConnected => _client.IsConnected; + public event EventHandler? OnSymbolVersionChanged; + + /// Raise when is 0x0702. + private uint MapAndSignal(uint adsError) + { + if (TwinCATStatusMapper.IsSymbolVersionChanged(adsError)) + OnSymbolVersionChanged?.Invoke(this, EventArgs.Empty); + return TwinCATStatusMapper.MapAdsError(adsError); + } + public Task ConnectAsync(TwinCATAmsAddress address, TimeSpan timeout, CancellationToken cancellationToken) { if (_client.IsConnected) return Task.CompletedTask; @@ -60,7 +95,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient var parentResult = await _client.ReadValueAsync(parent, typeof(uint), cancellationToken) .ConfigureAwait(false); if (parentResult.ErrorCode != AdsErrorCode.NoError) - return (null, TwinCATStatusMapper.MapAdsError((uint)parentResult.ErrorCode)); + return (null, MapAndSignal((uint)parentResult.ErrorCode)); return (ExtractBit(parentResult.Value, bit), TwinCATStatusMapper.Good); } @@ -69,13 +104,13 @@ internal sealed class AdsTwinCATClient : ITwinCATClient .ConfigureAwait(false); if (result.ErrorCode != AdsErrorCode.NoError) - return (null, TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode)); + return (null, MapAndSignal((uint)result.ErrorCode)); return (result.Value, TwinCATStatusMapper.Good); } catch (AdsErrorException ex) { - return (null, TwinCATStatusMapper.MapAdsError((uint)ex.ErrorCode)); + return (null, MapAndSignal((uint)ex.ErrorCode)); } } @@ -106,11 +141,11 @@ internal sealed class AdsTwinCATClient : ITwinCATClient .ConfigureAwait(false); return result.ErrorCode == AdsErrorCode.NoError ? TwinCATStatusMapper.Good - : TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode); + : MapAndSignal((uint)result.ErrorCode); } catch (AdsErrorException ex) { - return TwinCATStatusMapper.MapAdsError((uint)ex.ErrorCode); + return MapAndSignal((uint)ex.ErrorCode); } } @@ -159,13 +194,35 @@ internal sealed class AdsTwinCATClient : ITwinCATClient return reg; } + /// + /// Runs on the AMS router thread. Does the cheap bit-extraction + /// decode then enqueues — no driver logic, no consumer callbacks — so a slow consumer + /// can never stall ADS notification delivery for the rest of the process + /// (Driver.TwinCAT-008). Drops the notification (DropWrite) if the queue is saturated. + /// private void OnAdsNotificationEx(object? sender, AdsNotificationExEventArgs args) { if (!_notifications.TryGetValue(args.Handle, out var reg)) return; var value = args.Value; if (reg.BitIndex is int bit && reg.Type == TwinCATDataType.Bool && value is not bool) value = ExtractBit(value, bit); - try { reg.OnChange(reg.SymbolPath, value); } catch { /* consumer-side errors don't crash the ADS thread */ } + _notificationQueue.Writer.TryWrite(new PendingNotification(reg, value)); + } + + private async Task DispatchLoopAsync(CancellationToken ct) + { + try + { + await foreach (var pending in _notificationQueue.Reader.ReadAllAsync(ct).ConfigureAwait(false)) + { + try { pending.Registration.OnChange(pending.Registration.SymbolPath, pending.Value); } + catch { /* consumer-side errors stay on this managed task, not the ADS thread */ } + } + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Clean shutdown. + } } internal async Task DeleteNotificationAsync(uint handle, CancellationToken cancellationToken) @@ -236,8 +293,17 @@ internal sealed class AdsTwinCATClient : ITwinCATClient public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) return; _client.AdsNotificationEx -= OnAdsNotificationEx; _notifications.Clear(); + + // Stop the dispatch loop: complete the queue so the reader drains + exits, then + // cancel as a backstop. Best-effort wait so a wedged consumer can't hang teardown. + _notificationQueue.Writer.TryComplete(); + _dispatchCts.Cancel(); + try { _dispatchLoop.Wait(TimeSpan.FromSeconds(2)); } catch { /* shutdown */ } + _dispatchCts.Dispose(); + _client.Dispose(); } 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 9ee748a..39b2f78 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -19,6 +19,16 @@ public interface ITwinCATClient : IDisposable /// True when the AMS router + target both accept commands. bool IsConnected { get; } + /// + /// Raised when the client observes the ADS symbol-version-changed code + /// () on any read / write / + /// notification — the signal that a PLC program re-download has invalidated every + /// symbol + notification handle. The driver forwards this to + /// so Core rebuilds + /// the address space subtree (docs/v2/driver-specs.md §6, Driver.TwinCAT-013). + /// + event EventHandler? OnSymbolVersionChanged; + /// /// Read a symbolic value. Returns a boxed .NET value matching the requested /// , or null when the read produced no data; the 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 a11fc56..dead406 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDataType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDataType.cs @@ -34,15 +34,19 @@ public static class TwinCATDataTypeExtensions public static DriverDataType ToDriverDataType(this TwinCATDataType t) => t switch { TwinCATDataType.Bool => DriverDataType.Boolean, - TwinCATDataType.SInt or TwinCATDataType.USInt - or TwinCATDataType.Int or TwinCATDataType.UInt - or TwinCATDataType.DInt or TwinCATDataType.UDInt => DriverDataType.Int32, - TwinCATDataType.LInt or TwinCATDataType.ULInt => DriverDataType.Int32, // matches Int64 gap + TwinCATDataType.SInt => DriverDataType.Int16, // signed 8-bit — no narrower OPC UA atom + TwinCATDataType.USInt => DriverDataType.UInt16, // unsigned 8-bit — no narrower OPC UA atom + TwinCATDataType.Int => DriverDataType.Int16, + TwinCATDataType.UInt => DriverDataType.UInt16, + TwinCATDataType.DInt => DriverDataType.Int32, + TwinCATDataType.UDInt => DriverDataType.UInt32, + TwinCATDataType.LInt => DriverDataType.Int64, + TwinCATDataType.ULInt => DriverDataType.UInt64, TwinCATDataType.Real => DriverDataType.Float32, TwinCATDataType.LReal => DriverDataType.Float64, TwinCATDataType.String or TwinCATDataType.WString => DriverDataType.String, TwinCATDataType.Time or TwinCATDataType.Date - or TwinCATDataType.DateTime or TwinCATDataType.TimeOfDay => DriverDataType.Int32, + or TwinCATDataType.DateTime or TwinCATDataType.TimeOfDay => DriverDataType.UInt32, TwinCATDataType.Structure => DriverDataType.String, _ => DriverDataType.Int32, }; 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 c265b35..ba02e32 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -9,9 +9,11 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; /// resolver land in PRs 2 and 3. /// public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery, ISubscribable, - IHostConnectivityProbe, IPerCallHostResolver, IDisposable, IAsyncDisposable + IHostConnectivityProbe, IPerCallHostResolver, IRediscoverable, IDisposable, IAsyncDisposable { - private readonly TwinCATDriverOptions _options; + // Mutable so ReinitializeAsync can apply a new config generation (Driver.TwinCAT-001). + // The constructor seeds it; InitializeAsync re-parses driverConfigJson over the top of it. + private TwinCATDriverOptions _options; private readonly string _driverInstanceId; private readonly ITwinCATClientFactory _clientFactory; private readonly PollGroupEngine _poll; @@ -21,6 +23,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public event EventHandler? OnDataChange; public event EventHandler? OnHostStatusChanged; + public event EventHandler? OnRediscoveryNeeded; public TwinCATDriver(TwinCATDriverOptions options, string driverInstanceId, ITwinCATClientFactory? clientFactory = null) @@ -43,6 +46,16 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _health = new DriverHealth(DriverState.Initializing, null, null); try { + // Apply the supplied config generation (Driver.TwinCAT-001). A blank or content-free + // document keeps the constructor-seeded options — that path covers callers that have + // already materialised options up front (the factory passes both, in agreement). + if (!string.IsNullOrWhiteSpace(driverConfigJson)) + { + var parsed = TwinCATDriverFactoryExtensions.ParseOptions(driverConfigJson, _driverInstanceId); + if (parsed.Devices.Count > 0 || parsed.Tags.Count > 0) + _options = parsed; + } + foreach (var device in _options.Devices) { var addr = TwinCATAmsAddress.TryParse(device.HostAddress) @@ -92,6 +105,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery state.ProbeCts?.Dispose(); state.ProbeCts = null; state.DisposeClient(); + state.DisposeGate(); } _devices.Clear(); _tagsByName.Clear(); @@ -410,24 +424,64 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery return _options.Devices.FirstOrDefault()?.HostAddress ?? DriverInstanceId; } + /// + /// Lazily connect a device's client, serialized per device by + /// (Driver.TwinCAT-007). Without the gate, a + /// concurrent read / write / probe could each create + connect a separate client and + /// leak all-but-one, or dispose a client another thread is mid-connect on. The S7 and + /// 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) { - if (device.Client is { IsConnected: true } c) return c; - device.Client ??= _clientFactory.Create(); + // Fast path — already connected, no gate needed. + if (device.Client is { IsConnected: true } fast) return fast; + + await device.ConnectGate.WaitAsync(ct).ConfigureAwait(false); try { - await device.Client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct) - .ConfigureAwait(false); + // Re-check under the gate: another caller may have connected while we waited. + if (device.Client is { IsConnected: true } c) return c; + + // Discard a stale (created-but-disconnected) client before making a fresh one. + if (device.Client is { IsConnected: false } stale) + { + try { stale.Dispose(); } catch { /* best-effort */ } + device.Client = null; + } + + var client = _clientFactory.Create(); + client.OnSymbolVersionChanged += HandleSymbolVersionChanged; + try + { + await client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct) + .ConfigureAwait(false); + } + catch + { + client.OnSymbolVersionChanged -= HandleSymbolVersionChanged; + client.Dispose(); + throw; + } + device.Client = client; + return client; } - catch + finally { - device.Client.Dispose(); - device.Client = null; - throw; + device.ConnectGate.Release(); } - return device.Client; } + /// + /// Routes a wire-detected ADS symbol-version-changed (0x0702) to Core as an + /// invocation (Driver.TwinCAT-013). A PLC re-download + /// invalidates every symbol + notification handle, so the address space must be rebuilt + /// — this is the documented TwinCAT failure mode, not a transient connection error. + /// + private void HandleSymbolVersionChanged(object? sender, EventArgs e) => + OnRediscoveryNeeded?.Invoke(this, new RediscoveryEventArgs( + "TwinCAT symbol-version-changed 0x0702 — PLC program re-downloaded", ScopeHint: "TwinCAT")); + public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); public async ValueTask DisposeAsync() => await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); @@ -437,6 +491,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public TwinCATDeviceOptions Options { get; } = options; public ITwinCATClient? Client { get; set; } + /// Serializes connect / reconnect so concurrent callers never race a client + /// create-or-dispose for this device (Driver.TwinCAT-007). + public SemaphoreSlim ConnectGate { get; } = new(1, 1); + public object ProbeLock { get; } = new(); public HostState HostState { get; set; } = HostState.Unknown; public DateTime HostStateChangedUtc { get; set; } = DateTime.UtcNow; @@ -447,5 +505,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery Client?.Dispose(); Client = null; } + + public void DisposeGate() => ConnectGate.Dispose(); } } 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 7ddfd10..fe2f710 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriverFactoryExtensions.cs @@ -22,13 +22,25 @@ public static class TwinCATDriverFactoryExtensions internal static TwinCATDriver CreateInstance(string driverInstanceId, string driverConfigJson) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); + var options = ParseOptions(driverConfigJson, driverInstanceId); + return new TwinCATDriver(options, driverInstanceId); + } + + /// + /// Parse a TwinCAT driver-config JSON document into a . + /// Shared by (constructor-time) and + /// / + /// so a config generation pushed via Reinitialize is actually applied (Driver.TwinCAT-001). + /// + internal static TwinCATDriverOptions ParseOptions(string driverConfigJson, string driverInstanceId) + { ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); var dto = JsonSerializer.Deserialize(driverConfigJson, JsonOptions) ?? throw new InvalidOperationException( $"TwinCAT driver config for '{driverInstanceId}' deserialised to null"); - var options = new TwinCATDriverOptions + return new TwinCATDriverOptions { Devices = dto.Devices is { Count: > 0 } ? [.. dto.Devices.Select(d => new TwinCATDeviceOptions( @@ -49,8 +61,6 @@ public static class TwinCATDriverFactoryExtensions UseNativeNotifications = dto.UseNativeNotifications ?? true, EnableControllerBrowse = dto.EnableControllerBrowse ?? false, }; - - return new TwinCATDriver(options, driverInstanceId); } private static TwinCATTagDefinition BuildTag(TwinCATTagDto t, string driverInstanceId) => diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs index a82831d..78a9b5a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs @@ -9,6 +9,19 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; public static class TwinCATStatusMapper { public const uint Good = 0u; + + /// + /// ADS ADSERR_DEVICE_SYMBOLVERSIONINVALID — error 0x0702 (1794 decimal). + /// Raised by the runtime after a PLC program re-download: every symbol handle and + /// notification handle the driver holds is now stale. The driver treats this as an + /// trigger, not a connection error + /// (docs/v2/driver-specs.md §6, Driver.TwinCAT-013). + /// + public const uint AdsSymbolVersionChanged = 0x0702u; + + /// True when is the symbol-version-changed code. + public static bool IsSymbolVersionChanged(uint adsError) => adsError == AdsSymbolVersionChanged; + public const uint BadInternalError = 0x80020000u; public const uint BadNodeIdUnknown = 0x80340000u; public const uint BadNotWritable = 0x803B0000u; 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 9ff7b01..ae15fa0 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 @@ -19,6 +19,11 @@ internal class FakeTwinCATClient : ITwinCATClient public List<(string symbol, TwinCATDataType type, int? bit, object? value)> WriteLog { get; } = new(); public bool ProbeResult { get; set; } = true; + public event EventHandler? OnSymbolVersionChanged; + + /// Test hook — fire the symbol-version-changed signal as the real client would. + public void FireSymbolVersionChanged() => OnSymbolVersionChanged?.Invoke(this, EventArgs.Empty); + public virtual Task ConnectAsync(TwinCATAmsAddress address, TimeSpan timeout, CancellationToken ct) { ConnectCount++; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATDriverTests.cs index 0b264fc..6f30bde 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATDriverTests.cs @@ -87,7 +87,8 @@ public sealed class TwinCATDriverTests TwinCATDataType.LReal.ToDriverDataType().ShouldBe(DriverDataType.Float64); TwinCATDataType.String.ToDriverDataType().ShouldBe(DriverDataType.String); TwinCATDataType.WString.ToDriverDataType().ShouldBe(DriverDataType.String); - TwinCATDataType.Time.ToDriverDataType().ShouldBe(DriverDataType.Int32); + // IEC TIME/DATE/DT/TOD surface as their raw UDINT counter (Driver.TwinCAT-002). + TwinCATDataType.Time.ToDriverDataType().ShouldBe(DriverDataType.UInt32); } [Theory] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHighFindingsRegressionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHighFindingsRegressionTests.cs new file mode 100644 index 0000000..723bf27 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHighFindingsRegressionTests.cs @@ -0,0 +1,194 @@ +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 five High code-review findings closed 2026-05-22: +/// Driver.TwinCAT-001 (driverConfigJson ignored), -002 (LInt/ULInt narrowed to Int32), +/// -007 (EnsureConnectedAsync not thread-safe), -008 (ADS callbacks on the router thread), +/// -013 (no IRediscoverable / symbol-version-changed handling). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATHighFindingsRegressionTests +{ + private const string DeviceA = "ads://5.23.91.23.1.1:851"; + + // ---- Driver.TwinCAT-001 — Reinitialize applies the new config generation ---- + + [Fact] + public async Task ReinitializeAsync_applies_changed_device_config() + { + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1"); + await drv.InitializeAsync("{}", CancellationToken.None); + drv.DeviceCount.ShouldBe(1); + + // A config generation that adds a second device must be picked up at runtime. + var newConfig = JsonSerializer.Serialize(new + { + probe = new { enabled = false }, + devices = new[] + { + new { hostAddress = DeviceA, deviceName = "Machine1" }, + new { hostAddress = "ads://10.0.0.1.1.1:852", deviceName = "Machine2" }, + }, + }); + + await drv.ReinitializeAsync(newConfig, CancellationToken.None); + + drv.DeviceCount.ShouldBe(2); + drv.GetDeviceState("ads://10.0.0.1.1.1:852")!.Options.DeviceName.ShouldBe("Machine2"); + } + + [Fact] + public async Task InitializeAsync_applies_supplied_config_over_constructor_options() + { + // Constructor seeds an empty option set; the JSON document is the authoritative config. + var drv = new TwinCATDriver(new TwinCATDriverOptions(), "drv-1"); + var config = JsonSerializer.Serialize(new + { + probe = new { enabled = false }, + devices = new[] { new { hostAddress = DeviceA } }, + }); + + await drv.InitializeAsync(config, CancellationToken.None); + + drv.DeviceCount.ShouldBe(1); + drv.GetDeviceState(DeviceA).ShouldNotBeNull(); + } + + // ---- Driver.TwinCAT-002 — 64-bit + unsigned types map without truncation ---- + + [Fact] + public void DataType_mapping_preserves_width_and_signedness() + { + TwinCATDataType.LInt.ToDriverDataType().ShouldBe(DriverDataType.Int64); + TwinCATDataType.ULInt.ToDriverDataType().ShouldBe(DriverDataType.UInt64); + TwinCATDataType.UDInt.ToDriverDataType().ShouldBe(DriverDataType.UInt32); + TwinCATDataType.DInt.ToDriverDataType().ShouldBe(DriverDataType.Int32); + TwinCATDataType.UInt.ToDriverDataType().ShouldBe(DriverDataType.UInt16); + TwinCATDataType.Int.ToDriverDataType().ShouldBe(DriverDataType.Int16); + TwinCATDataType.USInt.ToDriverDataType().ShouldBe(DriverDataType.UInt16); + TwinCATDataType.SInt.ToDriverDataType().ShouldBe(DriverDataType.Int16); + } + + [Fact] + public async Task LInt_read_round_trips_value_above_int_MaxValue() + { + var factory = new FakeTwinCATClientFactory(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Tags = [new TwinCATTagDefinition("Big", DeviceA, "GVL.Big", TwinCATDataType.LInt)], + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + long big = (long)int.MaxValue + 1_000; + factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.Big"] = big } }; + + var snapshot = (await drv.ReadAsync(["Big"], CancellationToken.None)).Single(); + + snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + snapshot.Value.ShouldBe(big); // no truncation into a 32-bit node + } + + // ---- Driver.TwinCAT-007 — concurrent EnsureConnectedAsync creates exactly one client ---- + + [Fact] + public async Task Concurrent_reads_on_one_device_create_a_single_client() + { + var factory = new FakeTwinCATClientFactory(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Tags = [new TwinCATTagDefinition("X", DeviceA, "GVL.X", TwinCATDataType.DInt)], + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.X"] = 7 } }; + + // 32 readers race the lazy connect. Without the per-device gate this leaks clients. + var tasks = Enumerable.Range(0, 32) + .Select(_ => Task.Run(() => drv.ReadAsync(["X"], CancellationToken.None))); + await Task.WhenAll(tasks); + + factory.Clients.Count.ShouldBe(1); + factory.Clients[0].ConnectCount.ShouldBe(1); + } + + [Fact] + public async Task Concurrent_reads_and_writes_share_one_client() + { + var factory = new FakeTwinCATClientFactory(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Tags = [new TwinCATTagDefinition("X", DeviceA, "GVL.X", TwinCATDataType.DInt)], + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.X"] = 1 } }; + + var work = new List(); + for (var i = 0; i < 16; i++) + { + work.Add(Task.Run(() => drv.ReadAsync(["X"], CancellationToken.None))); + work.Add(Task.Run(() => drv.WriteAsync( + [new WriteRequest("X", 5)], CancellationToken.None))); + } + await Task.WhenAll(work); + + factory.Clients.Count.ShouldBe(1); + } + + // ---- Driver.TwinCAT-013 — symbol-version-changed routes to IRediscoverable ---- + + [Fact] + public void TwinCATDriver_implements_IRediscoverable() + { + var drv = new TwinCATDriver(new TwinCATDriverOptions(), "drv-1"); + drv.ShouldBeAssignableTo(); + } + + [Fact] + public async Task Symbol_version_changed_raises_OnRediscoveryNeeded() + { + var factory = new FakeTwinCATClientFactory(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(DeviceA)], + Tags = [new TwinCATTagDefinition("X", DeviceA, "GVL.X", TwinCATDataType.DInt)], + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + RediscoveryEventArgs? raised = null; + drv.OnRediscoveryNeeded += (_, args) => raised = args; + + // Force a connect so the driver wires its handler onto the client. + await drv.ReadAsync(["X"], CancellationToken.None); + factory.Clients.ShouldHaveSingleItem(); + + // The client observed ADS 0x0702 on the wire and signalled it. + factory.Clients[0].FireSymbolVersionChanged(); + + raised.ShouldNotBeNull(); + raised!.Reason.ShouldContain("0x0702"); + } + + [Fact] + public void StatusMapper_recognises_symbol_version_changed_code() + { + TwinCATStatusMapper.AdsSymbolVersionChanged.ShouldBe(0x0702u); + TwinCATStatusMapper.IsSymbolVersionChanged(0x0702u).ShouldBeTrue(); + TwinCATStatusMapper.IsSymbolVersionChanged(0u).ShouldBeFalse(); + } +}