From de21a54949422aa568948a9c0af17f469e9e5d51 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 13:57:01 -0400 Subject: [PATCH 01/12] fix(twincat): publish BadNodeIdUnknown for unresolvable native-subscribe references (U12) The native subscribe path silently skipped a reference it could not resolve (unknown RawPath, or a tag routed to an unconfigured device), leaving the node on whatever status the server seeded. It now publishes one BadNodeIdUnknown snapshot, matching the poll path. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../TwinCATDriver.cs | 16 ++++- .../TwinCATNativeIsolationTests.cs | 59 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs 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 66663ef8..1dda4ad7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -666,8 +666,16 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { foreach (var reference in fullReferences) { - if (!_resolver.TryResolve(reference, out var def)) continue; - if (!_devices.TryGetValue(def.DeviceHostAddress, out var device)) continue; + // An unresolvable reference (unknown RawPath, or a tag routed to an unconfigured device) + // publishes BadNodeIdUnknown once — the poll path's status — instead of leaving the node on + // whatever status the server seeded it with. + if (!_resolver.TryResolve(reference, out var def) + || !_devices.TryGetValue(def.DeviceHostAddress, out var device)) + { + RaiseDataChange(handle, reference, new DataValueSnapshot( + null, TwinCATStatusMapper.BadNodeIdUnknown, null, DateTime.UtcNow)); + continue; + } var parsed = TwinCATSymbolPath.TryParse(def.SymbolPath); var symbolName = parsed?.ToAdsSymbolName() ?? def.SymbolPath; @@ -724,6 +732,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery return handle; } + /// Raises one data change for a native subscription. + private void RaiseDataChange(ISubscriptionHandle handle, string reference, DataValueSnapshot snapshot) => + OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, snapshot)); + /// /// Registers one native notification and records its replayable intent on the device — both /// under the device so the register can't race a diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs new file mode 100644 index 00000000..0d04eaf6 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs @@ -0,0 +1,59 @@ +using System.Collections.Concurrent; +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; + +/// +/// Native-subscribe isolation (#739): an unresolvable reference publishes BadNodeIdUnknown +/// (U12) instead of keeping whatever status the server seeded, and one failing registration does not +/// take down the rest of the subscription (H11). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATNativeIsolationTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + private static TwinCATDriver NewNativeDriver(FakeTwinCATClientFactory factory, params TwinCATTagDefinition[] tags) => + new(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries(tags), + Probe = new TwinCATProbeOptions { Enabled = false }, + UseNativeNotifications = true, + }, "drv-1", factory); + + /// + /// U12: an unknown reference and a tag routed to an unconfigured device each publish one + /// BadNodeIdUnknown snapshot; the resolvable tag still registers and delivers Good (positive control). + /// + [Fact] + public async Task Native_subscribe_publishes_BadNodeIdUnknown_for_unresolvable_references() + { + var factory = new FakeTwinCATClientFactory(); + var drv = NewNativeDriver(factory, + new TwinCATTagDefinition("Speed", Host, "MAIN.Speed", TwinCATDataType.DInt), + new TwinCATTagDefinition("Orphan", "ads://9.9.9.9.1.1:851", "MAIN.Orphan", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + _ = await drv.SubscribeAsync(["Ghost", "Speed", "Orphan"], TimeSpan.FromMilliseconds(100), + TestContext.Current.CancellationToken); + + var ghost = events.Single(e => e.FullReference == "Ghost"); + ghost.Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + ghost.Snapshot.Value.ShouldBeNull(); + events.Single(e => e.FullReference == "Orphan").Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + + // Positive control: the resolvable tag registered and its push arrives Good. + factory.Clients.ShouldHaveSingleItem().Notifications.ShouldHaveSingleItem().SymbolPath.ShouldBe("MAIN.Speed"); + factory.Clients[0].FireNotification("MAIN.Speed", 42); + var good = events.Single(e => e.FullReference == "Speed"); + good.Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + good.Snapshot.Value.ShouldBe(42); + } +} From 17f4bf6d33e9ba30845cfdd61607e9ab77fa44d5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:00:38 -0400 Subject: [PATCH 02/12] fix(twincat): isolate native registrations per tag and per device (H11) Native subscribe was all-or-nothing: any registration failure other than a type refusal disposed every registration and rethrew, so one typo'd symbol or one unreachable PLC in a multi-device driver served zero data, churned register/delete every reconcile tick, and left health Healthy. Now each registration fails on its own: a refusal (the device answered: symbol not found, PLC not ready, type mismatch) publishes its status with no health write; any other per-tag failure publishes BadCommunicationError and degrades; a connect failure marks the device unreachable for the rest of the call and stores its tags as dead intents. Every failed tag is kept as a dead intent that the probe-tick retry and the reconnect replay re-register. The ADS client now classifies a failed symbol resolve / AddDeviceNotification by status instead of throwing InvalidOperationException for all of them. Cancellation still tears the partial subscription down. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AdsTwinCATClient.cs | 21 ++- .../TwinCATDriver.cs | 105 ++++++++--- .../TwinCATNativeIsolationTests.cs | 166 ++++++++++++++++++ .../TwinCATNativeNotificationTests.cs | 24 ++- 4 files changed, 278 insertions(+), 38 deletions(-) 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 c7ab8850..53481b5e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -523,8 +523,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient var (parent, _, _, parentStatus) = await DescribeSymbolAsync(registerPath, cancellationToken) .ConfigureAwait(false); if (parentStatus != TwinCATStatusMapper.Good) - throw new InvalidOperationException( - $"Resolving {registerPath} for a notification failed with status 0x{parentStatus:X8}"); + throw RegistrationFailure(registerPath, "resolving the parent word", parentStatus); if (TwinCATBitAccess.CheckParent(bit, parent) is { } bitReason) throw new TwinCATTypeMismatchException(symbolPath, bitReason); clrType = typeof(byte[]); @@ -538,8 +537,7 @@ internal sealed class AdsTwinCATClient : ITwinCATClient var (shape, array, _, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken) .ConfigureAwait(false); if (symbolStatus != TwinCATStatusMapper.Good) - throw new InvalidOperationException( - $"Resolving {symbolPath} for a notification failed with status 0x{symbolStatus:X8}"); + throw RegistrationFailure(symbolPath, "resolving the symbol", symbolStatus); if (arrayCount is int count) { @@ -582,8 +580,8 @@ internal sealed class AdsTwinCATClient : ITwinCATClient registerPath, settings, userData: null, clrType, anyTypeArgs, cancellationToken) .ConfigureAwait(false); if (result.ErrorCode != AdsErrorCode.NoError) - throw new InvalidOperationException( - $"AddDeviceNotificationExAsync failed with ADS error {result.ErrorCode} for {registerPath}"); + throw RegistrationFailure(registerPath, $"AddDeviceNotificationEx (ADS error {result.ErrorCode})", + MapAndSignal((uint)result.ErrorCode)); var reg = new NotificationRegistration(symbolPath, type, bitIndex, onChange, this, result.Handle) { @@ -594,6 +592,17 @@ internal sealed class AdsTwinCATClient : ITwinCATClient return reg; } + /// + /// Classifies a failed notification registration. A status proving the device ANSWERED (symbol not + /// found, invalid access, PLC not ready, …) is a per-tag refusal the driver isolates without a health + /// write; a comms-class status (BadCommunicationError / BadTimeout) stays an ordinary + /// exception — a comms fault the driver degrades on and retries. + /// + internal static Exception RegistrationFailure(string symbolPath, string step, uint status) => + status is TwinCATStatusMapper.BadCommunicationError or TwinCATStatusMapper.BadTimeout + ? new InvalidOperationException($"{step} for {symbolPath} failed with status 0x{status:X8}") + : new TwinCATNotificationRefusedException(symbolPath, $"{step} failed with status 0x{status:X8}", status); + /// /// Runs on the AMS router thread. Does the cheap decode (bit /// extraction, string, array) then enqueues — no driver logic, no consumer callbacks — so a slow consumer 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 1dda4ad7..021c4e32 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -661,6 +661,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var id = Interlocked.Increment(ref _nextNativeSubId); var handle = new NativeSubscriptionHandle(id); var registrations = new List(fullReferences.Count); + // Devices whose connect failed during THIS call: the rest of their tags are stored as dead + // intents straight away rather than each paying (and logging) its own failed connect. + HashSet? unreachable = null; try { @@ -686,44 +689,69 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var reg = new NativeRegistration( Interlocked.Increment(ref _nextRegId), device, reference, symbolName, def.DataType, bitIndex, def.ArrayLength, publishingInterval, - onChange: (_, value) => OnDataChange?.Invoke(this, - new DataChangeEventArgs(handle, reference, new DataValueSnapshot( - value, TwinCATStatusMapper.Good, DateTime.UtcNow, DateTime.UtcNow))), + onChange: (_, value) => RaiseDataChange(handle, reference, new DataValueSnapshot( + value, TwinCATStatusMapper.Good, DateTime.UtcNow, DateTime.UtcNow)), // When the handle goes dead (a failed replay) push a Bad snapshot so // subscribers see the tag go Bad instead of a frozen-Good-stale value. - onUnavailable: () => OnDataChange?.Invoke(this, - new DataChangeEventArgs(handle, reference, new DataValueSnapshot( - null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)))); + onUnavailable: () => RaiseDataChange(handle, reference, new DataValueSnapshot( + null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow))); + registrations.Add(reg); + + if (unreachable?.Contains(device) == true) + { + // Same device already failed to connect in this call: keep the intent dead for the + // probe-tick retry / reconnect replay and surface Bad now. + device.NativeRegistrations[reg.Id] = reg; + RaiseDataChange(handle, reference, new DataValueSnapshot( + null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); + continue; + } try { await RegisterNotificationAsync(device, reg, cancellationToken).ConfigureAwait(false); } + catch (OperationCanceledException) { throw; } catch (TwinCATNotificationRefusedException refused) { - // The declared type / array length cannot decode this PLC symbol (ignitionoee audit), or the array is over the native-notification size cap: isolate - // THIS tag to Bad — one WARN, one snapshot carrying the refusal's status - // (BadTypeMismatch / BadOutOfRange) — and keep the rest of the subscription. Its - // intent stays on the device with no live handle, so the probe-tick retry re-checks - // it after a PLC program change. + // The device answered and refused THIS tag (declared type cannot decode the symbol, + // array over the notification cap, symbol not found): isolate it — one WARN, one + // snapshot carrying the refusal's status, no health write (a refusal is not a comms + // fault). Its intent stays on the device with no live handle, so the probe-tick retry + // re-checks it after a PLC program change. _logger.LogWarning( "TwinCAT driver '{DriverInstanceId}' refused native notification for '{Reference}': {Reason}", _driverInstanceId, reference, refused.Message); - OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, new DataValueSnapshot( - null, refused.StatusCode, null, DateTime.UtcNow))); + RaiseDataChange(handle, reference, new DataValueSnapshot( + null, refused.StatusCode, null, DateTime.UtcNow)); + } + catch (DeviceUnavailableException unavailable) + { + // The device could not be connected (offline, router down, backoff window open). + // Every tag of this device goes Bad + dead for the retry; other devices carry on. + (unreachable ??= []).Add(device); + MarkCommsFault(unavailable.InnerException?.Message ?? unavailable.Message); + RaiseDataChange(handle, reference, new DataValueSnapshot( + null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); + } + catch (Exception ex) + { + // A per-tag registration failure that is not a refusal (the AddDeviceNotification + // round-trip failed): this tag goes Bad + dead for the retry; the rest keep going. + _logger.LogWarning(ex, + "TwinCAT driver '{DriverInstanceId}' native-notification registration failed for " + + "'{Reference}'; it will be retried on the next healthy probe tick", + _driverInstanceId, reference); + MarkCommsFault(ex.Message); + RaiseDataChange(handle, reference, new DataValueSnapshot( + null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); } - registrations.Add(reg); } } - catch (Exception ex) + catch (OperationCanceledException) { - // On any registration failure, tear down everything we got so far + rethrow. Leaves - // the subscription in a clean "never existed" state rather than a half-registered - // state the caller has to clean up. - _logger.LogWarning(ex, - "TwinCAT driver '{DriverInstanceId}' native-notification registration failed; " + - "tearing down {Count} partial registrations", - _driverInstanceId, registrations.Count); + // Cancelled mid-subscribe: leave the subscription in a clean "never existed" state rather + // than a half-registered one the caller has to clean up. foreach (var r in registrations) { try { r.Dispose(); } catch { } } throw; } @@ -732,6 +760,21 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery return handle; } + /// The connect step of a native registration failed — the whole device is unreachable. + private sealed class DeviceUnavailableException(Exception inner) + : Exception("TwinCAT device unreachable: " + inner.Message, inner); + + /// + /// Records a comms fault: preserving LastSuccessfulRead. + /// Never downgrades . + /// + private void MarkCommsFault(string message) + { + var current = _health; + if (current.State != DriverState.Faulted) + _health = new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, message); + } + /// Raises one data change for a native subscription. private void RaiseDataChange(ISubscriptionHandle handle, string reference, DataValueSnapshot snapshot) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, snapshot)); @@ -748,7 +791,20 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery await device.ConnectGate.WaitAsync(ct).ConfigureAwait(false); try { - var client = await EnsureConnectedUnderGateAsync(device, ct, timeoutOverride: null).ConfigureAwait(false); + ITwinCATClient client; + try + { + client = await EnsureConnectedUnderGateAsync(device, ct, timeoutOverride: null).ConfigureAwait(false); + } + catch (OperationCanceledException) { throw; } + catch (Exception ex) + { + // Keep the intent dead (no live handle): the replay after the next successful connect + // re-registers it. The caller short-circuits the rest of this device's tags. + device.NativeRegistrations[reg.Id] = reg; + throw new DeviceUnavailableException(ex); + } + ITwinCATNotificationHandle newHandle; try { @@ -756,7 +812,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery reg.SymbolName, reg.DataType, reg.BitIndex, reg.ArrayLength, reg.Interval, _options.NotificationMaxDelayMs, reg.OnChange, ct).ConfigureAwait(false); } - catch (TwinCATNotificationRefusedException) + catch (OperationCanceledException) { throw; } + catch { // Keep the intent with no live handle (a "dead" registration) so the probe-tick // retry re-checks it and UnsubscribeAsync still removes it; the caller isolates it. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs index 0d04eaf6..144c7570 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATNativeIsolationTests.cs @@ -56,4 +56,170 @@ public sealed class TwinCATNativeIsolationTests good.Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); good.Snapshot.Value.ShouldBe(42); } + + /// + /// H11: one tag whose AddDeviceNotification round-trip fails (not a refusal) goes Bad on its own and + /// the subscribe call SUCCEEDS — before, the exception disposed every registration and rethrew, so one + /// bad tag served zero data for the whole driver. The good tags keep delivering (positive control). + /// + [Fact] + public async Task One_failing_registration_does_not_tear_down_the_subscription() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.B"] = new InvalidOperationException("AddDeviceNotificationEx blew up"); + var factory = new FakeTwinCATClientFactory { Customise = () => fake }; + var drv = NewNativeDriver(factory, + new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt), + new TwinCATTagDefinition("B", Host, "MAIN.B", TwinCATDataType.DInt), + new TwinCATTagDefinition("C", Host, "MAIN.C", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + var handle = await drv.SubscribeAsync(["A", "B", "C"], TimeSpan.FromMilliseconds(100), + TestContext.Current.CancellationToken); + + fake.Notifications.Select(n => n.SymbolPath).ShouldBe(["MAIN.A", "MAIN.C"], ignoreOrder: true); + events.ShouldHaveSingleItem().FullReference.ShouldBe("B"); + events.Single().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError); + + fake.FireNotification("MAIN.C", 3); + events.Last().FullReference.ShouldBe("C"); + events.Last().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + + // The failed tag is kept as a dead intent and unsubscribe still removes everything. + drv.GetDeviceState(Host)!.NativeRegistrations.Count.ShouldBe(3); + await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + drv.GetDeviceState(Host)!.NativeRegistrations.ShouldBeEmpty(); + fake.Notifications.ShouldBeEmpty(); + } + + /// + /// H11: a refusal (symbol not found — the device answered) isolates the tag with the refusal's own + /// status and leaves driver health alone; the good tag delivers. + /// + [Fact] + public async Task A_refused_symbol_is_isolated_without_a_health_write() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.Typo"] = new TwinCATNotificationRefusedException( + "MAIN.Typo", "resolving the symbol failed", TwinCATStatusMapper.BadNodeIdUnknown); + var factory = new FakeTwinCATClientFactory { Customise = () => fake }; + var drv = NewNativeDriver(factory, + new TwinCATTagDefinition("Typo", Host, "MAIN.Typo", TwinCATDataType.DInt), + new TwinCATTagDefinition("Good", Host, "MAIN.Good", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + _ = await drv.SubscribeAsync(["Typo", "Good"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + + events.ShouldHaveSingleItem().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + fake.FireNotification("MAIN.Good", 1); + events.Last().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + } + + /// + /// H11: in a two-device driver an unreachable device only blacks out ITS tags (Bad + dead intents, + /// health Degraded); the reachable device's tags register and deliver. + /// + [Fact] + public async Task An_unreachable_device_blacks_out_only_its_own_tags() + { + const string hostB = "ads://5.23.91.24.1.1:851"; + var build = 0; + var factory = new FakeTwinCATClientFactory + { + // Device A's client is created first and works; device B's connect throws. + Customise = () => new FakeTwinCATClient { ThrowOnConnect = ++build == 2 }, + }; + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host), new TwinCATDeviceOptions(hostB)], + RawTags = TwinCATRawTags.Entries( + [ + new TwinCATTagDefinition("A1", Host, "MAIN.A1", TwinCATDataType.DInt), + new TwinCATTagDefinition("B1", hostB, "MAIN.B1", TwinCATDataType.DInt), + new TwinCATTagDefinition("B2", hostB, "MAIN.B2", TwinCATDataType.DInt), + new TwinCATTagDefinition("A2", Host, "MAIN.A2", TwinCATDataType.DInt), + ]), + Probe = new TwinCATProbeOptions { Enabled = false }, + UseNativeNotifications = true, + }, "drv-1", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + _ = await drv.SubscribeAsync(["A1", "B1", "B2", "A2"], TimeSpan.FromMilliseconds(100), + TestContext.Current.CancellationToken); + + factory.Clients.Count.ShouldBe(2); // B2 did not attempt a second connect + factory.Clients[0].Notifications.Select(n => n.SymbolPath).ShouldBe(["MAIN.A1", "MAIN.A2"], ignoreOrder: true); + events.Select(e => e.FullReference).ShouldBe(["B1", "B2"], ignoreOrder: true); + events.ShouldAllBe(e => e.Snapshot.StatusCode == TwinCATStatusMapper.BadCommunicationError); + drv.GetDeviceState(hostB)!.NativeRegistrations.Count.ShouldBe(2); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + factory.Clients[0].FireNotification("MAIN.A2", 22); + events.Last().FullReference.ShouldBe("A2"); + events.Last().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + } + + /// + /// H11: the dead intent left by a failed registration is re-registered by the probe-tick retry, and + /// its first push then arrives Good — the subscription heals without a re-subscribe. + /// + [Fact] + public async Task A_failed_registration_is_recovered_by_the_probe_tick_retry() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.B"] = new InvalidOperationException("transient"); + var factory = new FakeTwinCATClientFactory { Customise = () => fake }; + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries([new TwinCATTagDefinition("B", Host, "MAIN.B", TwinCATDataType.DInt)]), + Probe = new TwinCATProbeOptions + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(40), + Timeout = TimeSpan.FromMilliseconds(100), + }, + UseNativeNotifications = true, + }, "drv-1", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + _ = await drv.SubscribeAsync(["B"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + fake.AddNotificationErrors.Clear(); + + await WaitForAsync(() => fake.Notifications.Count == 1, TimeSpan.FromSeconds(3)); + fake.Notifications.Count.ShouldBe(1); + fake.FireNotification("MAIN.B", 5); + events.Last().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// A comms-class registration status stays an exception; any other status is a per-tag refusal. + [Fact] + public void Registration_failures_are_classified_by_status() + { + AdsTwinCATClient.RegistrationFailure("MAIN.X", "step", TwinCATStatusMapper.BadNodeIdUnknown) + .ShouldBeOfType().StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + AdsTwinCATClient.RegistrationFailure("MAIN.X", "step", TwinCATStatusMapper.BadOutOfService) + .ShouldBeOfType(); + AdsTwinCATClient.RegistrationFailure("MAIN.X", "step", TwinCATStatusMapper.BadCommunicationError) + .ShouldBeOfType(); + AdsTwinCATClient.RegistrationFailure("MAIN.X", "step", TwinCATStatusMapper.BadTimeout) + .ShouldBeOfType(); + } + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(20); + } } 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 7b40cbc0..3fdceb13 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 @@ -98,13 +98,14 @@ public sealed class TwinCATNativeNotificationTests events.Count.ShouldBe(afterUnsub); } - /// Verifies that native subscribe failure mid-registration cleans up partial state. + /// + /// A registration failing mid-subscribe isolates that one tag (#739 H11): the first registration + /// stays live, the second is kept as a dead intent and goes Bad, and the call does not throw. + /// [Fact] - public async Task Native_subscribe_failure_mid_registration_cleans_up_partial_state() + public async Task Native_subscribe_failure_mid_registration_isolates_the_failed_tag() { // Fail-on-second-call fake — first AddNotificationAsync succeeds, second throws. - // Subscribe's catch block must tear the first one down before rethrowing so no zombie - // notification lingers. var fake = new FailAfterNAddsFake(new AbTagParamsIrrelevant(), succeedBefore: 1); var factory = new FakeTwinCATClientFactory { Customise = () => fake }; var drv = new TwinCATDriver(new TwinCATDriverOptions @@ -120,12 +121,19 @@ public sealed class TwinCATNativeNotificationTests }, "drv-1", factory); await drv.InitializeAsync("{}", CancellationToken.None); - await Should.ThrowAsync(() => - drv.SubscribeAsync(["A", "B"], TimeSpan.FromMilliseconds(100), CancellationToken.None)); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + var handle = await drv.SubscribeAsync(["A", "B"], TimeSpan.FromMilliseconds(100), CancellationToken.None); - // First registration succeeded then got torn down by the catch; second threw. fake.AddCallCount.ShouldBe(2); - fake.Notifications.Count.ShouldBe(0); // partial handle cleaned up + fake.Notifications.ShouldHaveSingleItem().SymbolPath.ShouldBe("MAIN.A"); + var bad = events.ShouldHaveSingleItem(); + bad.FullReference.ShouldBe("B"); + bad.Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError); + + await drv.UnsubscribeAsync(handle, CancellationToken.None); + fake.Notifications.ShouldBeEmpty(); } /// From ecdd6dbecb53e860dec659b4abc92cf26010bc52 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:03:28 -0400 Subject: [PATCH 03/12] fix(twincat): native/probe evidence advances LastSuccessfulRead; probe failures degrade (H12) In native-notification mode LastSuccessfulRead never moved past the init stamp, so one Degraded write on a connection older than 90 s latched the comms-loss watchdog into a full reconnect (every notification on every device deleted and re-registered). Conversely an unreachable AMS router was swallowed by the probe and never left Healthy. - a native push and a successful probe now stamp LastSuccessfulRead (throttled to one health record per 250 ms) and restore Healthy unless a device is still probe-unreachable; - 3 consecutive failed probe ticks write Degraded with the stamp preserved; - a NotSupportedException write is BadNotSupported with no health write (#611); - a reconnect replay the PLC refuses republishes the refusal's status without degrading (it used to report BadCommunicationError + Degraded on every reconnect for a correctly-isolated type-mismatch tag). Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../TwinCATDriver.cs | 106 ++++++++++-- .../TwinCATHealthContractTests.cs | 161 ++++++++++++++++++ 2 files changed, 252 insertions(+), 15 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs 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 021c4e32..f1c87ebb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -346,7 +346,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { if (status == TwinCATStatusMapper.Good) { - _health = new DriverHealth(DriverState.Healthy, now, null); + RecordDeviceAnswered(now, force: true); return new DataValueSnapshot(value, status, now, now); } @@ -524,10 +524,11 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery results[i] = new WriteResult(status); } catch (OperationCanceledException) { throw; } - catch (NotSupportedException nse) + catch (NotSupportedException) { + // A write the client cannot express is a refusal, not a comms fault (#611): no health + // write, or the comms-loss watchdog would reconnect a healthy device over a config issue. results[i] = new WriteResult(TwinCATStatusMapper.BadNotSupported); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message); } catch (Exception ex) when (ex is FormatException or InvalidCastException) { @@ -540,7 +541,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery catch (Exception ex) { results[i] = new WriteResult(TwinCATStatusMapper.BadCommunicationError); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + MarkCommsFault(ex.Message); } } @@ -689,12 +690,18 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var reg = new NativeRegistration( Interlocked.Increment(ref _nextRegId), device, reference, symbolName, def.DataType, bitIndex, def.ArrayLength, publishingInterval, - onChange: (_, value) => RaiseDataChange(handle, reference, new DataValueSnapshot( - value, TwinCATStatusMapper.Good, DateTime.UtcNow, DateTime.UtcNow)), + onChange: (_, value) => + { + // A push is the device answering: stamp LastSuccessfulRead so a notification-only + // driver does not look silent to the comms-loss watchdog. + var now = DateTime.UtcNow; + RecordDeviceAnswered(now); + RaiseDataChange(handle, reference, new DataValueSnapshot(value, TwinCATStatusMapper.Good, now, now)); + }, // When the handle goes dead (a failed replay) push a Bad snapshot so // subscribers see the tag go Bad instead of a frozen-Good-stale value. - onUnavailable: () => RaiseDataChange(handle, reference, new DataValueSnapshot( - null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow))); + onUnavailable: status => RaiseDataChange(handle, reference, new DataValueSnapshot( + null, status, null, DateTime.UtcNow))); registrations.Add(reg); if (unreachable?.Contains(device) == true) @@ -775,6 +782,43 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _health = new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, message); } + /// + /// Consecutive failed probe ticks (connect failure, failed ReadState) after which a device counts as + /// unreachable and the driver goes with LastSuccessfulRead + /// preserved — so an unreachable AMS router reaches the comms-loss watchdog instead of leaving the + /// driver Healthy with stale-Good values. Three ticks ride out a single dropped ReadState. + /// + internal const int ProbeFailuresBeforeDegraded = 3; + + /// Minimum spacing between LastSuccessfulRead stamps from the notification path. + private static readonly TimeSpan AnsweredStampInterval = TimeSpan.FromMilliseconds(250); + + /// + /// Records that a device answered (a native push, a successful probe, a Good read): advances + /// LastSuccessfulRead and restores unless a device is still + /// probe-unreachable (then the state stays Degraded, the stamp still advances). Throttled so a + /// notification burst does not allocate a health record per value; never touches Faulted. + /// + /// The time the device answered. + /// Write even inside the throttle window (an explicit read / probe outcome). + private void RecordDeviceAnswered(DateTime now, bool force = false) + { + var current = _health; + if (current.State == DriverState.Faulted) return; + // Hot path (a notification on a healthy driver inside the window): no device scan, no allocation. + if (!force && current.State == DriverState.Healthy && current.LastSuccessfulRead is { } recent + && now - recent < AnsweredStampInterval) + return; + var unreachable = _devices.Values.FirstOrDefault(d => d.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded); + var state = unreachable is null ? DriverState.Healthy : DriverState.Degraded; + if (!force && current.State == state && current.LastSuccessfulRead is { } last + && now - last < AnsweredStampInterval) + return; + _health = new DriverHealth(state, now, unreachable is null + ? null + : $"TwinCAT device {unreachable.Options.HostAddress} unreachable ({unreachable.ConsecutiveProbeFailures} consecutive failed probes)"); + } + /// Raises one data change for a native subscription. private void RaiseDataChange(ISubscriptionHandle handle, string reference, DataValueSnapshot snapshot) => OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, snapshot)); @@ -888,7 +932,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// the shape has no quality channel. Closes over the same /// (handle, reference) pair as . /// - public Action OnUnavailable { get; } + public Action OnUnavailableStatus { get; } + + /// Publishes BadCommunicationError for a handle that went dead on a comms failure. + public void OnUnavailable() => OnUnavailableStatus(TwinCATStatusMapper.BadCommunicationError); + + /// Publishes the status of a refusal (the PLC answered and refused this tag). + /// The refusal's OPC UA status. + public void OnRefused(uint status) => OnUnavailableStatus(status); private ITwinCATNotificationHandle? _handle; @@ -908,10 +959,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// Authored array length for an array tag; null for a scalar. /// Requested notification interval. /// Value-change callback (closes over the owning subscription handle + reference). - /// Bad-quality callback fired when the handle goes dead. + /// Bad-quality callback (given the status to publish) fired when the handle goes dead. public NativeRegistration(long id, DeviceState device, string reference, string symbolName, TwinCATDataType dataType, int? bitIndex, int? arrayLength, TimeSpan interval, Action onChange, - Action onUnavailable) + Action onUnavailable) { Id = id; Device = device; @@ -922,7 +973,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery ArrayLength = arrayLength; Interval = interval; OnChange = onChange; - OnUnavailable = onUnavailable; + OnUnavailableStatus = onUnavailable; } /// Installs a freshly-registered handle and returns the previous one (null on first set). @@ -996,6 +1047,17 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } } + if (success) + { + state.ConsecutiveProbeFailures = 0; + RecordDeviceAnswered(DateTime.UtcNow, force: true); + } + else if (++state.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded) + { + MarkCommsFault($"TwinCAT device {state.Options.HostAddress} unreachable " + + $"({state.ConsecutiveProbeFailures} consecutive failed probes)"); + } + TransitionDeviceState(state, success ? HostState.Running : HostState.Stopped); // Opt-in PLC state / task-cycle sampling for /hosts. After the host-state transition so a @@ -1360,6 +1422,19 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } replayed++; } + catch (TwinCATNotificationRefusedException refused) + { + // The PLC answered and refused this tag again (it was already isolated at subscribe, or a + // program change made it undecodable): republish the refusal's status, keep it dead for the + // probe-tick retry, and leave health alone — a refusal is not a comms fault. + _logger.LogDebug( + "TwinCAT driver '{DriverInstanceId}' native notification '{Symbol}' on device '{HostAddress}' " + + "still refused after reconnect: {Reason}", + _driverInstanceId, reg.SymbolName, device.Options.HostAddress, refused.Message); + var dead = reg.MarkHandleDead(); + try { dead?.Dispose(); } catch { /* already-dead handle from the disposed client */ } + try { reg.OnRefused(refused.StatusCode); } catch { /* never abort recovery of the rest */ } + } catch (Exception ex) { _logger.LogWarning(ex, @@ -1373,9 +1448,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var dead = reg.MarkHandleDead(); try { dead?.Dispose(); } catch { /* already-dead handle from the disposed client */ } try { reg.OnUnavailable(); } catch { /* never let a subscriber callback abort recovery of the rest */ } - if (_health.State != DriverState.Faulted) - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, - $"native-notification replay failed for '{reg.SymbolName}' on {device.Options.HostAddress}"); + MarkCommsFault($"native-notification replay failed for '{reg.SymbolName}' on {device.Options.HostAddress}"); } } @@ -1586,6 +1659,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// True once an unexpected sum-read chunk failure was logged for this device. public volatile bool SumChunkFailureWarned; + /// Consecutive failed probe ticks; reset by a successful probe. Written only by this device's probe loop. + public volatile int ConsecutiveProbeFailures; + /// Latest probe-sampled PLC diagnostics; null before the first sample. public TwinCATDeviceDiagnostics? LastDiagnostics { get; set; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs new file mode 100644 index 00000000..f1c06bfd --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs @@ -0,0 +1,161 @@ +using System.Collections.Concurrent; +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; + +/// +/// The driver health contract (#739 H12): data proving the device answered — a native notification or +/// a successful probe — advances LastSuccessfulRead; sustained probe/connect failure degrades with +/// it preserved; a refusal (a write the client cannot express, a replay the PLC refuses) is no comms fault. +/// +[Trait("Category", "Unit")] +public sealed class TwinCATHealthContractTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, TwinCATProbeOptions probe, bool native = true) => + new(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries([new TwinCATTagDefinition("Speed", Host, "MAIN.Speed", TwinCATDataType.DInt)]), + Probe = probe, + UseNativeNotifications = native, + }, "drv-1", factory); + + private static TwinCATProbeOptions FastProbe => new() + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(30), + Timeout = TimeSpan.FromMilliseconds(100), + }; + + /// A native push advances LastSuccessfulRead and restores Healthy after a comms-fault write. + [Fact] + public async Task A_native_notification_advances_LastSuccessfulRead_and_restores_Healthy() + { + var fake = new FakeTwinCATClient(); + var factory = new FakeTwinCATClientFactory { Customise = () => fake }; + var drv = NewDriver(factory, new TwinCATProbeOptions { Enabled = false }); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.SubscribeAsync(["Speed"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead; + initStamp.ShouldNotBeNull(); + + // A write that fails on the wire degrades (comms fault) with the stamp preserved. + fake.ThrowOnWrite = true; + _ = await drv.WriteAsync([new WriteRequest("Speed", 1)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldBe(initStamp); + + await Task.Delay(300, TestContext.Current.CancellationToken); + fake.FireNotification("MAIN.Speed", 7); + + var after = drv.GetHealth(); + after.State.ShouldBe(DriverState.Healthy); + after.LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp!.Value); + } + + /// A successful probe advances LastSuccessfulRead in a driver that serves only notifications. + [Fact] + public async Task A_successful_probe_advances_LastSuccessfulRead() + { + var factory = new FakeTwinCATClientFactory(); + var drv = NewDriver(factory, FastProbe); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead!.Value; + + await WaitForAsync(() => drv.GetHealth().LastSuccessfulRead > initStamp.AddMilliseconds(250), TimeSpan.FromSeconds(3)); + + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp.AddMilliseconds(250)); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// + /// An unreachable AMS router (every probe connect throws) degrades the driver after + /// consecutive failures, preserving the stamp — + /// before, the probe caught it and health stayed Healthy forever. + /// + [Fact] + public async Task Consecutive_probe_failures_degrade_preserving_LastSuccessfulRead() + { + var factory = new FakeTwinCATClientFactory { Customise = () => new FakeTwinCATClient { ThrowOnConnect = true } }; + var drv = NewDriver(factory, FastProbe); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead; + + await WaitForAsync(() => drv.GetHealth().State == DriverState.Degraded, TimeSpan.FromSeconds(3)); + + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Degraded); + health.LastSuccessfulRead.ShouldBe(initStamp); + factory.Clients.Count.ShouldBeGreaterThanOrEqualTo(TwinCATDriver.ProbeFailuresBeforeDegraded); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// + /// A write the client refuses with is BadNotSupported with no + /// health write (#611: a refusal is not a comms fault). Positive control: a wire failure does degrade. + /// + [Fact] + public async Task A_NotSupported_write_leaves_health_alone() + { + var fake = new FakeTwinCATClient { ThrowOnWrite = true, Exception = new NotSupportedException("no") }; + var factory = new FakeTwinCATClientFactory { Customise = () => fake }; + var drv = NewDriver(factory, new TwinCATProbeOptions { Enabled = false }, native: false); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var result = await drv.WriteAsync([new WriteRequest("Speed", 1)], TestContext.Current.CancellationToken); + result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadNotSupported); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + + fake.Exception = new InvalidOperationException("wire"); + result = await drv.WriteAsync([new WriteRequest("Speed", 1)], TestContext.Current.CancellationToken); + result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// + /// A reconnect replay the PLC REFUSES (an isolated type-mismatch tag re-attempted on every reconnect) + /// republishes the refusal's status and does not degrade. Positive control: a replay that fails on the + /// wire degrades (covered for the same flow in TwinCATReconnectReplayTests). + /// + [Fact] + public async Task A_refused_replay_republishes_the_refusal_status_without_degrading() + { + var build = 0; + var factory = new FakeTwinCATClientFactory + { + Customise = () => + { + var c = new FakeTwinCATClient(); + if (++build >= 2) + c.AddNotificationErrors["MAIN.Speed"] = new TwinCATTypeMismatchException("MAIN.Speed", "now a STRING"); + return c; + }, + }; + var drv = NewDriver(factory, new TwinCATProbeOptions { Enabled = false }); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + _ = await drv.SubscribeAsync(["Speed"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + + factory.Clients[0].SimulateWireDrop(); + // A write reconnects (and replays) without itself touching health on success. + _ = await drv.WriteAsync([new WriteRequest("Speed", 1)], TestContext.Current.CancellationToken); + + factory.Clients.Count.ShouldBe(2); + events.ShouldHaveSingleItem().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadTypeMismatch); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(20); + } +} From 4083b4aada5b99ae9b6b48baecad2cc14486a641 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:07:03 -0400 Subject: [PATCH 04/12] fix(twincat): warn once per bad tag; a device refusal is not a comms fault (P10) CompleteRead logged a Warning for every non-Good status on every poll and latched Degraded for all of them. Now a bad tag warns once per bad episode (re-armed when it reads Good, and on re-initialise), a comms-class status (BadCommunicationError / BadTimeout) degrades with LastSuccessfulRead preserved, and any other status - the device answered and refused this tag - advances LastSuccessfulRead with no state change. Read-path exception handlers now go through MarkCommsFault, so they never downgrade Faulted. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../TwinCATDriver.cs | 59 +++++++--- .../TwinCATCapturingLogger.cs | 26 +++++ .../TwinCATReadWarningTests.cs | 103 ++++++++++++++++++ 3 files changed, 175 insertions(+), 13 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapturingLogger.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs 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 f1c87ebb..7c95a8f0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -42,6 +42,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // so concurrent bit-writers to the same word must serialise or they lose each other's // updates. Keyed by device + parent symbol. (Cannot guard against the PLC program writing // the word between our read and write — inherent to RMW.) + // References that already logged their bad-read Warning in the current bad episode (#739 P10). + // Removed when the tag reads Good; cleared on (re-)initialise and shutdown. + private readonly ConcurrentDictionary _warnedRefs = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _bitRmwLocks = new(StringComparer.OrdinalIgnoreCase); @@ -117,6 +121,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // allows it, and a reconnect retry can overlap a running attempt). Release the previous device states' probe loops + clients before // the overwrite below. Deliberately NOT ShutdownAsync: that also drops _poll and the subscriptions. await ReleaseDeviceStatesAsync().ConfigureAwait(false); + _warnedRefs.Clear(); _health = new DriverHealth(DriverState.Initializing, null, null); try { @@ -335,7 +340,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery catch (OperationCanceledException) { throw; } catch (Exception ex) { - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + MarkCommsFault(ex.Message); return new DataValueSnapshot(null, TwinCATStatusMapper.BadCommunicationError, null, now); } } @@ -346,25 +351,53 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { if (status == TwinCATStatusMapper.Good) { + _warnedRefs.TryRemove(reference, out _); // a later failure is a new episode and warns again RecordDeviceAnswered(now, force: true); return new DataValueSnapshot(value, status, now, now); } - if (status == TwinCATStatusMapper.BadTypeMismatch) - _logger.LogWarning( - "TwinCAT driver '{DriverInstanceId}' refused '{Reference}': declared {DataType} " + - "cannot decode the PLC symbol '{Symbol}' (string/numeric kind mismatch, array element type " + - "or length mismatch, or a reference/container)", - _driverInstanceId, reference, def.DataType, symbolName); + // Once per tag per bad episode: a Warning every poll for every bad tag (50 typo'd symbols on a + // 500 ms poll = 100 Warnings/s) buried everything else in the log. + if (_warnedRefs.TryAdd(reference, 0)) + { + if (status == TwinCATStatusMapper.BadTypeMismatch) + _logger.LogWarning( + "TwinCAT driver '{DriverInstanceId}' refused '{Reference}': declared {DataType} " + + "cannot decode the PLC symbol '{Symbol}' (string/numeric kind mismatch, array element type " + + "or length mismatch, or a reference/container); logged once until the tag reads Good", + _driverInstanceId, reference, def.DataType, symbolName); + else + _logger.LogWarning( + "TwinCAT driver '{DriverInstanceId}' ADS read error 0x{Status:X8} for '{Reference}'; " + + "logged once until the tag reads Good", + _driverInstanceId, status, reference); + } + + if (IsCommsStatus(status)) + MarkCommsFault($"ADS status {status:X8} reading {reference}"); else - _logger.LogWarning( - "TwinCAT driver '{DriverInstanceId}' ADS read error 0x{Status:X8} for '{Reference}'", - _driverInstanceId, status, reference); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, - $"ADS status {status:X8} reading {reference}"); + RecordDeviceAnsweredKeepState(now); // a refusal: the device answered — no state change return new DataValueSnapshot(value, status, now, now); } + /// + /// True for a status that means the request did not get a device answer (router / transport / + /// timeout). Every other non-Good status is the device refusing this one tag (symbol not found, type + /// mismatch, PLC not running, access denied): per-tag Bad with no health write. + /// + /// The mapped OPC UA status. + /// True for a comms-class status. + internal static bool IsCommsStatus(uint status) => + status is TwinCATStatusMapper.BadCommunicationError or TwinCATStatusMapper.BadTimeout; + + /// Advances LastSuccessfulRead without changing the state (the device answered with a refusal). + private void RecordDeviceAnsweredKeepState(DateTime now) + { + var current = _health; + if (current.State is DriverState.Faulted or DriverState.Unknown or DriverState.Initializing) return; + _health = current with { LastSuccessfulRead = now }; + } + /// /// Reads one device's gathered tags as ADS sum-reads of at most SumReadMaxItems symbols each. /// A target that refuses the sum command flips and the @@ -430,7 +463,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { foreach (var e in chunk) results[e.Index] = new DataValueSnapshot(null, TwinCATStatusMapper.BadCommunicationError, null, now); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + MarkCommsFault(ex.Message); // Once per device: the health message is overwritten by the next good read in the same poll, so // without this a sum-read path failing on every poll left no trace (hw2 live gate, 2026-10-08). if (connected && !device.SumChunkFailureWarned) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapturingLogger.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapturingLogger.cs new file mode 100644 index 00000000..9df5d1b8 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATCapturingLogger.cs @@ -0,0 +1,26 @@ +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// Collects the driver's Warning-level messages (thread-safe) for once-per-X assertions. +internal sealed class TwinCATCapturingLogger : ILogger +{ + private readonly List _warnings = []; + + /// Gets a snapshot of the formatted Warning messages logged so far. + public IReadOnlyList Warnings { get { lock (_warnings) return [.. _warnings]; } } + + /// + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + /// + public bool IsEnabled(LogLevel logLevel) => true; + + /// + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + if (logLevel == LogLevel.Warning) lock (_warnings) _warnings.Add(formatter(state, exception)); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs new file mode 100644 index 00000000..dd669164 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs @@ -0,0 +1,103 @@ +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; + +/// +/// #739 P10: a bad tag warns ONCE (not once per poll — 50 typo'd symbols on a 500 ms poll used to be 100 +/// Warnings/s forever), re-arms when it reads Good, and a device refusal leaves driver health alone while a +/// comms-class status degrades. +/// +[Trait("Category", "Unit")] +public sealed class TwinCATReadWarningTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + private static (TwinCATDriver drv, FakeTwinCATClient fake, TwinCATCapturingLogger log) NewDriver() + { + var fake = new FakeTwinCATClient(); + var log = new TwinCATCapturingLogger(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries( + [ + new TwinCATTagDefinition("Typo", Host, "MAIN.Typo", TwinCATDataType.DInt), + new TwinCATTagDefinition("Other", Host, "MAIN.Other", TwinCATDataType.DInt), + ]), + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", new FakeTwinCATClientFactory { Customise = () => fake }, log); + return (drv, fake, log); + } + + private static int WarningsFor(TwinCATCapturingLogger log, string reference) => + log.Warnings.Count(w => w.Contains($"'{reference}'", StringComparison.Ordinal)); + + /// Repeated bad reads of one tag log one Warning; a second bad tag gets its own (positive control). + [Fact] + public async Task A_bad_tag_warns_once_across_polls_and_rearms_after_Good() + { + var (drv, fake, log) = NewDriver(); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + fake.ReadStatuses["MAIN.Typo"] = TwinCATStatusMapper.BadNodeIdUnknown; + + for (var i = 0; i < 5; i++) + (await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken)) + .Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + WarningsFor(log, "Typo").ShouldBe(1); + + fake.ReadStatuses["MAIN.Other"] = TwinCATStatusMapper.BadNodeIdUnknown; + _ = await drv.ReadAsync(["Other"], TestContext.Current.CancellationToken); + WarningsFor(log, "Other").ShouldBe(1); + + // Fixed, then broken again: a new episode warns again. + fake.ReadStatuses.Remove("MAIN.Typo"); + _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); + fake.ReadStatuses["MAIN.Typo"] = TwinCATStatusMapper.BadNodeIdUnknown; + _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); + _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); + WarningsFor(log, "Typo").ShouldBe(2); + } + + /// A re-initialise re-arms the warning. + [Fact] + public async Task A_reinitialise_rearms_the_warning() + { + var (drv, fake, log) = NewDriver(); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + fake.ReadStatuses["MAIN.Typo"] = TwinCATStatusMapper.BadNodeIdUnknown; + _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); + + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); + + WarningsFor(log, "Typo").ShouldBe(2); + } + + /// + /// A status proving the device answered (symbol not found) keeps the driver Healthy and advances the + /// stamp; a comms-class status (timeout) degrades with the stamp preserved. + /// + [Fact] + public async Task A_refusal_status_does_not_degrade_but_a_comms_status_does() + { + var (drv, fake, _) = NewDriver(); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead!.Value; + fake.ReadStatuses["MAIN.Typo"] = TwinCATStatusMapper.BadNodeIdUnknown; + await Task.Delay(300, TestContext.Current.CancellationToken); + + _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); + var afterRefusal = drv.GetHealth(); + afterRefusal.State.ShouldBe(DriverState.Healthy); + afterRefusal.LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp); + + fake.ReadStatuses["MAIN.Other"] = TwinCATStatusMapper.BadTimeout; + _ = await drv.ReadAsync(["Other"], TestContext.Current.CancellationToken); + var afterTimeout = drv.GetHealth(); + afterTimeout.State.ShouldBe(DriverState.Degraded); + afterTimeout.LastSuccessfulRead.ShouldBe(afterRefusal.LastSuccessfulRead); + } +} From 40c74938298fc732f2de2e29b3ba133817f2f476 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:10:12 -0400 Subject: [PATCH 05/12] fix(twincat): BOOL-within-word writes RMW the parent at its own width (H18) The bit write read and wrote the parent word as a 32-bit UDInt with no parent check: a bit beyond a 16-bit parent was written into neighbouring bytes, and a signed parent with its sign bit set read back >= 2^31, so the checked conversion turned any bit write into BadOutOfRange. The write now goes through ITwinCATClient.WriteBitAsync: the ADS client resolves the parent symbol, checks the bit against it (TwinCATBitAccess. CheckParent, as the notification path already did), reads exactly parent.ByteSize raw bytes, flips one bit (TwinCATBitAccess.ApplyBitWrite) and writes the bytes back by index group/offset or variable handle. A bit beyond the parent is BadOutOfRange, a non-integer parent BadTypeMismatch, a short parent read BadCommunicationError (never written back), all without a health write. The per-parent RMW lock stays in the driver. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AdsTwinCATClient.cs | 55 +++++++++++++++++ .../ITwinCATClient.cs | 16 +++++ .../TwinCATBitAccess.cs | 30 ++++++++++ .../TwinCATDriver.cs | 31 +++------- .../FakeTwinCATClient.cs | 34 +++++++++++ .../TwinCATBitAccessTests.cs | 46 +++++++++++++++ .../TwinCATReadWriteTests.cs | 59 ++++++++++++------- 7 files changed, 227 insertions(+), 44 deletions(-) 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 53481b5e..a143d018 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -482,6 +482,61 @@ internal sealed class AdsTwinCATClient : ITwinCATClient } } + /// + public async Task WriteBitAsync(string parentPath, int bit, bool value, CancellationToken cancellationToken) + { + try + { + // Resolve the parent so the RMW happens at its OWN width (the notification path's model): + // the old 32-bit UDInt round-trip wrote a 16-bit parent's neighbours and overflowed on a + // signed parent with its sign bit set. + var (parent, _, symbol, status) = await DescribeSymbolAsync(parentPath, cancellationToken) + .ConfigureAwait(false); + if (status != TwinCATStatusMapper.Good) return status; + if (TwinCATBitAccess.CheckParent(bit, parent) is not null) + return TwinCATBitAccess.ApplyBitWrite(parent, ReadOnlySpan.Empty, bit, value, out _); + + var (bytes, readStatus) = await ReadRawAsync(parentPath, symbol!, parent.ByteSize, cancellationToken) + .ConfigureAwait(false); + if (readStatus != TwinCATStatusMapper.Good) return readStatus; + var modify = TwinCATBitAccess.ApplyBitWrite(parent, bytes, bit, value, out var updated); + if (modify != TwinCATStatusMapper.Good) return modify; + return await WriteRawAsync(parentPath, symbol!, updated, cancellationToken).ConfigureAwait(false); + } + catch (AdsErrorException ex) + { + return MapAndSignal((uint)ex.ErrorCode); + } + } + + /// + /// Writes raw bytes to a resolved symbol — the mirror of : by its index + /// group/offset, or through a fresh variable handle for a handle-addressed symbol (0xF005). + /// + private async Task WriteRawAsync( + string symbolPath, IAdsSymbol symbol, ReadOnlyMemory data, CancellationToken cancellationToken) + { + if (symbol.IndexGroup != SymbolValueByHandle) + { + var direct = await _client.WriteAsync(symbol.IndexGroup, symbol.IndexOffset, data, cancellationToken) + .ConfigureAwait(false); + return direct.ErrorCode == AdsErrorCode.NoError ? TwinCATStatusMapper.Good : MapAndSignal((uint)direct.ErrorCode); + } + + var handle = await _client.CreateVariableHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false); + if (handle.ErrorCode != AdsErrorCode.NoError) return MapAndSignal((uint)handle.ErrorCode); + try + { + var write = await _client.WriteAsync(handle.Handle, data, cancellationToken).ConfigureAwait(false); + return write.ErrorCode == AdsErrorCode.NoError ? TwinCATStatusMapper.Good : MapAndSignal((uint)write.ErrorCode); + } + finally + { + try { await _client.DeleteVariableHandleAsync(handle.Handle, CancellationToken.None).ConfigureAwait(false); } + catch { /* best-effort release */ } + } + } + /// public async Task ProbeAsync(CancellationToken cancellationToken) { 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 3849edd0..1ffeb6fa 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -119,6 +119,22 @@ public interface ITwinCATClient : IDisposable object? value, CancellationToken cancellationToken); + /// + /// Sets or clears one bit of an integer parent word — the write half of a BOOL-within-word tag + /// (MAIN.Flags.3; ADS has no symbol for WORD.N). A read-modify-write of the parent's raw + /// bytes at the parent's OWN resolved width (), so a 16-bit + /// parent is never written as 32 bits and a signed parent's sign bit is just a bit. The caller + /// serialises concurrent writers to the same parent; the PLC program writing the word between the + /// read and the write is inherent to RMW. + /// + /// The parent word's ADS symbol path (the bit path without its .N). + /// The bit index (0 = least significant bit of the first byte). + /// The bit value to write. + /// Cancellation token for the read and the write. + /// The mapped OPC UA status: Good; the parent read's status; BadOutOfRange for a bit + /// beyond the parent; BadTypeMismatch for a parent that is not an integer word. + Task WriteBitAsync(string parentPath, int bit, bool value, CancellationToken cancellationToken); + /// /// Cheap health probe — returns true when the target's AMS state is reachable. /// Used by 's probe loop. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATBitAccess.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATBitAccess.cs index 186e1cee..277bd317 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATBitAccess.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATBitAccess.cs @@ -32,6 +32,36 @@ internal static class TwinCATBitAccess : $"bit {bit} is outside the {parent.ByteSize * 8}-bit parent {parent.TypeName}"; } + /// + /// The modify step of a bit write's read-modify-write: checks the bit against the parent + /// () and returns the parent's bytes — exactly parent.ByteSize of them — + /// with one bit set or cleared. Works on raw bytes, so the parent's width and signedness play no part. + /// + /// The parent symbol's resolved shape. + /// The parent's current bytes as read from the PLC. + /// The bit index. + /// The bit value. + /// The bytes to write back when the result is Good; otherwise empty. + /// Good; BadOutOfRange for a bit beyond the parent; BadTypeMismatch for a parent + /// that is not an integer word; BadCommunicationError for a read shorter than the parent + /// (writing it back would zero the missing bytes). + public static uint ApplyBitWrite(TwinCATPlcSymbolShape parent, ReadOnlySpan current, int bit, bool value, + out byte[] updated) + { + updated = []; + if (CheckParent(bit, parent) is not null) + return parent.Kind != TwinCATPlcKind.Scalar || parent.IsFloat + ? TwinCATStatusMapper.BadTypeMismatch + : TwinCATStatusMapper.BadOutOfRange; + if (current.Length < parent.ByteSize) return TwinCATStatusMapper.BadCommunicationError; + + var bytes = current[..parent.ByteSize].ToArray(); + var mask = (byte)(1 << (bit % 8)); + bytes[bit / 8] = value ? (byte)(bytes[bit / 8] | mask) : (byte)(bytes[bit / 8] & ~mask); + updated = bytes; + return TwinCATStatusMapper.Good; + } + /// Reads bit of a little-endian parent word. /// The parent word's raw bytes. /// The bit index (0 = least significant bit of the first byte). 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 7c95a8f0..2a5d4d94 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -511,38 +511,21 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); var parsed = TwinCATSymbolPath.TryParse(def.SymbolPath); - // BOOL-within-word write — read-modify-write of the parent word. Mirrors the bit-read - // (AdsTwinCATClient.ReadValueAsync) which reads the parent as uint: read the parent as - // UDInt (-> uint), flip the bit, write it back, all under a per-parent lock. + // BOOL-within-word write — a read-modify-write of the parent word at the parent's OWN + // width and on raw bytes (the client resolves the parent and checks the bit against it, the + // notification path's model), under a per-parent lock so concurrent bit writers to the same + // word don't lose each other's updates. if (def.DataType == TwinCATDataType.Bool && parsed?.BitIndex is int bit) { var parentPath = (parsed with { BitIndex = null }).ToAdsSymbolName(); + var bitValue = Convert.ToBoolean(w.Value); var gate = _bitRmwLocks.GetOrAdd( $"{def.DeviceHostAddress}|{parentPath}", static _ => new SemaphoreSlim(1, 1)); await gate.WaitAsync(cancellationToken).ConfigureAwait(false); try { - var (parentValue, readStatus) = await client.ReadValueAsync( - parentPath, TwinCATDataType.UDInt, null, null, cancellationToken).ConfigureAwait(false); - if (readStatus != TwinCATStatusMapper.Good) - { - results[i] = new WriteResult(readStatus); - } - else if (parentValue is null) - { - // Good status but no value — treating null as 0 would write a zeroed - // word and clear every bit set on the device. Surface a Bad status and - // write nothing rather than corrupt the parent word. - results[i] = new WriteResult(TwinCATStatusMapper.BadCommunicationError); - } - else - { - var word = Convert.ToUInt32(parentValue); - var updated = Convert.ToBoolean(w.Value) ? word | (1u << bit) : word & ~(1u << bit); - var writeStatus = await client.WriteValueAsync( - parentPath, TwinCATDataType.UDInt, null, updated, cancellationToken).ConfigureAwait(false); - results[i] = new WriteResult(writeStatus); - } + results[i] = new WriteResult(await client.WriteBitAsync( + parentPath, bit, bitValue, cancellationToken).ConfigureAwait(false)); } finally { 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 2ad33e42..e31340a5 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 @@ -136,6 +136,40 @@ internal class FakeTwinCATClient : ITwinCATClient return Task.FromResult(status); } + /// Raw bytes of bit-write parent words, by parent path (#739 H18). The byte count is the parent's width. + public Dictionary ParentWords { get; } = new(StringComparer.OrdinalIgnoreCase); + /// Optional parent shapes; absent = an unsigned integer word as wide as its entry. + public Dictionary ParentShapes { get; } = new(StringComparer.OrdinalIgnoreCase); + /// Gets the log of every call. + public List<(string parent, int bit, bool value)> BitWriteLog { get; } = new(); + /// When set, yields between its read and its write so a missing + /// per-parent lock in the driver loses updates. + public bool YieldInBitWrite { get; set; } + + /// Simulates the parent-width bit RMW with the driver's own . + /// The parent word's symbol path. + /// The bit index. + /// The bit value. + /// The cancellation token. + /// The write status. + public virtual async Task WriteBitAsync(string parentPath, int bit, bool value, CancellationToken ct) + { + if (ThrowOnWrite) throw Exception ?? new InvalidOperationException(); + lock (BitWriteLog) BitWriteLog.Add((parentPath, bit, value)); + if (ReadStatuses.TryGetValue(parentPath, out var readStatus) && readStatus != TwinCATStatusMapper.Good) + return readStatus; + byte[] current; + lock (ParentWords) current = ParentWords.TryGetValue(parentPath, out var w) ? [.. w] : []; + var shape = ParentShapes.TryGetValue(parentPath, out var sh) + ? sh + : new TwinCATPlcSymbolShape(TwinCATPlcKind.Scalar, current.Length == 0 ? 4 : current.Length, false, "UDINT"); + if (YieldInBitWrite) await Task.Yield(); + var status = TwinCATBitAccess.ApplyBitWrite(shape, current, bit, value, out var updated); + if (status != TwinCATStatusMapper.Good) return status; + lock (ParentWords) ParentWords[parentPath] = updated; + return WriteStatuses.TryGetValue(parentPath, out var s) ? s : TwinCATStatusMapper.Good; + } + /// Simulates probing the connection status. /// The cancellation token. /// A task that returns the probe result. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitAccessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitAccessTests.cs index 3dc22507..a5b339c8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitAccessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBitAccessTests.cs @@ -55,4 +55,50 @@ public sealed class TwinCATBitAccessTests TwinCATBitAccess.CheckParent(0, TwinCATPlcSymbolShape.Classify("ST_X", DataTypeCategory.Struct, 8)).ShouldNotBeNull(); TwinCATBitAccess.CheckParent(31, TwinCATPlcSymbolShape.Classify("DWORD", DataTypeCategory.Primitive, 4)).ShouldBeNull(); } + + // ---- #739 H18: the bit WRITE is a read-modify-write at the parent's own width ---- + + /// A bit write flips exactly one bit of the parent's raw bytes at the parent's width. + [Fact] + public void ApplyBitWrite_flips_one_bit_at_the_parent_width() + { + var word = TwinCATPlcSymbolShape.Classify("WORD", DataTypeCategory.Primitive, 2); + TwinCATBitAccess.ApplyBitWrite(word, [0x01, 0x00], 9, true, out var set).ShouldBe(TwinCATStatusMapper.Good); + set.ShouldBe(new byte[] { 0x01, 0x02 }); + TwinCATBitAccess.ApplyBitWrite(word, [0xFF, 0xFF], 3, false, out var cleared).ShouldBe(TwinCATStatusMapper.Good); + cleared.ShouldBe(new byte[] { 0xF7, 0xFF }); + } + + /// + /// A signed parent with its sign bit set is plain bytes — the old UDInt round-trip read it back as + /// a value ≥ 2³¹ and a checked conversion turned ANY bit write into BadOutOfRange. + /// + [Fact] + public void ApplyBitWrite_on_a_negative_signed_parent_preserves_the_sign_bit() + { + var intWord = TwinCATPlcSymbolShape.Classify("INT", DataTypeCategory.Primitive, 2); + TwinCATBitAccess.ApplyBitWrite(intWord, [0x00, 0x80], 0, true, out var updated).ShouldBe(TwinCATStatusMapper.Good); + updated.ShouldBe(new byte[] { 0x01, 0x80 }); + } + + /// A bit beyond the parent is BadOutOfRange; a non-integer parent BadTypeMismatch; nothing is produced. + [Fact] + public void ApplyBitWrite_refuses_a_bit_the_parent_does_not_have() + { + var word = TwinCATPlcSymbolShape.Classify("UINT", DataTypeCategory.Primitive, 2); + TwinCATBitAccess.ApplyBitWrite(word, [0, 0], 16, true, out var none).ShouldBe(TwinCATStatusMapper.BadOutOfRange); + none.ShouldBeEmpty(); + var real = TwinCATPlcSymbolShape.Classify("REAL", DataTypeCategory.Primitive, 4); + TwinCATBitAccess.ApplyBitWrite(real, [0, 0, 0, 0], 1, true, out _).ShouldBe(TwinCATStatusMapper.BadTypeMismatch); + } + + /// A parent read shorter than the parent's width is never written back (it would zero bytes). + [Fact] + public void ApplyBitWrite_refuses_a_short_parent_buffer() + { + var dword = TwinCATPlcSymbolShape.Classify("DWORD", DataTypeCategory.Primitive, 4); + TwinCATBitAccess.ApplyBitWrite(dword, [0xFF, 0xFF], 1, true, out var none) + .ShouldBe(TwinCATStatusMapper.BadCommunicationError); + none.ShouldBeEmpty(); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs index c81e7b88..d7dbcc0d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWriteTests.cs @@ -296,23 +296,23 @@ public sealed class TwinCATReadWriteTests factory.Clients[0].DisposeCount.ShouldBe(1); } - // ---- BOOL-within-word RMW writes ---- + // ---- BOOL-within-word RMW writes (#739 H18: at the parent's own width, on raw bytes) ---- - /// A BOOL-within-word set reads the parent word, ORs the bit, writes it back as UDInt. + /// A BOOL-within-word set read-modify-writes the parent word's raw bytes through WriteBitAsync. [Fact] - public async Task Bit_set_RMWs_parent_word_as_UDInt() + public async Task Bit_set_RMWs_the_parent_word_bytes() { var (drv, factory) = NewDriver( new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); - factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.Flags"] = 0b0001u } }; + factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["MAIN.Flags"] = [0b0001, 0, 0, 0] } }; await drv.InitializeAsync("{}", CancellationToken.None); var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None); results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); - factory.Clients[0].Values["MAIN.Flags"].ShouldBe(0b1001u); - factory.Clients[0].WriteLog.ShouldContain(e => - e.symbol == "MAIN.Flags" && e.type == TwinCATDataType.UDInt && e.bit == null); + factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0b1001, 0, 0, 0 }); + factory.Clients[0].BitWriteLog.ShouldBe([("MAIN.Flags", 3, true)]); + factory.Clients[0].WriteLog.ShouldBeEmpty(); // no 32-bit UDInt round-trip any more } /// A BOOL-within-word clear preserves the other bits in the parent word. @@ -321,12 +321,12 @@ public sealed class TwinCATReadWriteTests { var (drv, factory) = NewDriver( new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); - factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.Flags"] = 0xFFFFu } }; + factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["MAIN.Flags"] = [0xFF, 0xFF] } }; await drv.InitializeAsync("{}", CancellationToken.None); await drv.WriteAsync([new WriteRequest("Flag", false)], CancellationToken.None); - factory.Clients[0].Values["MAIN.Flags"].ShouldBe(0xFFF7u); + factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0xF7, 0xFF }); } /// RMW works on a DWORD parent (bit 20 set above the 16-bit boundary). @@ -335,12 +335,31 @@ public sealed class TwinCATReadWriteTests { var (drv, factory) = NewDriver( new TwinCATTagDefinition("Hi", "ads://5.23.91.23.1.1:851", "GVL.Status.20", TwinCATDataType.Bool)); - factory.Customise = () => new FakeTwinCATClient { Values = { ["GVL.Status"] = 0u } }; + factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["GVL.Status"] = [0, 0, 0, 0] } }; await drv.InitializeAsync("{}", CancellationToken.None); await drv.WriteAsync([new WriteRequest("Hi", true)], CancellationToken.None); - factory.Clients[0].Values["GVL.Status"].ShouldBe(1u << 20); + BitConverter.ToUInt32(factory.Clients[0].ParentWords["GVL.Status"]).ShouldBe(1u << 20); + } + + /// + /// A bit beyond a 16-bit parent is refused BadOutOfRange and nothing is written — before, the hardcoded + /// 32-bit RMW wrote into the neighbouring bytes. No health write: a refusal is not a comms fault. + /// + [Fact] + public async Task Bit_beyond_a_16_bit_parent_is_refused_without_writing() + { + var (drv, factory) = NewDriver( + new TwinCATTagDefinition("Hi", "ads://5.23.91.23.1.1:851", "MAIN.Word.20", TwinCATDataType.Bool)); + factory.Customise = () => new FakeTwinCATClient { ParentWords = { ["MAIN.Word"] = [0, 0] } }; + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.WriteAsync([new WriteRequest("Hi", true)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); + factory.Clients[0].ParentWords["MAIN.Word"].ShouldBe(new byte[] { 0, 0 }); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); } /// A failed parent read short-circuits the RMW and surfaces the read status. @@ -351,6 +370,7 @@ public sealed class TwinCATReadWriteTests new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); factory.Customise = () => new FakeTwinCATClient { + ParentWords = { ["MAIN.Flags"] = [0xAA, 0] }, ReadStatuses = { ["MAIN.Flags"] = TwinCATStatusMapper.BadNodeIdUnknown }, }; await drv.InitializeAsync("{}", CancellationToken.None); @@ -358,25 +378,24 @@ public sealed class TwinCATReadWriteTests var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None); results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); - factory.Clients[0].WriteLog.ShouldBeEmpty(); + factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0xAA, 0 }); } - /// A Good parent read that yields a null value must NOT zero the word — it surfaces - /// a Bad status and writes nothing (treating null as 0 would clear every set bit). + /// A Good parent read that yields no bytes must NOT zero the word — it surfaces a Bad status + /// and writes nothing (treating it as 0 would clear every set bit). [Fact] - public async Task Bit_write_with_null_parent_value_does_not_zero_the_word() + public async Task Bit_write_with_no_parent_bytes_does_not_zero_the_word() { var (drv, factory) = NewDriver( new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool)); - // Do NOT seed Values["MAIN.Flags"] — the fake returns (null, Good) for the parent read. + // Do NOT seed ParentWords["MAIN.Flags"] — the fake answers the parent read Good with no bytes. factory.Customise = () => new FakeTwinCATClient(); await drv.InitializeAsync("{}", CancellationToken.None); var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None); results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError); - // No parent write attempted — the word is left untouched on the device. - factory.Clients[0].WriteLog.ShouldBeEmpty(); + factory.Clients[0].ParentWords.ShouldNotContainKey("MAIN.Flags"); } /// Concurrent bit writes to the same word compose correctly (per-parent lock). @@ -387,12 +406,12 @@ public sealed class TwinCATReadWriteTests .Select(b => new TwinCATTagDefinition($"Bit{b}", "ads://5.23.91.23.1.1:851", $"MAIN.Flags.{b}", TwinCATDataType.Bool)) .ToArray(); var (drv, factory) = NewDriver(tags); - factory.Customise = () => new FakeTwinCATClient { Values = { ["MAIN.Flags"] = 0u } }; + factory.Customise = () => new FakeTwinCATClient { YieldInBitWrite = true, ParentWords = { ["MAIN.Flags"] = [0, 0] } }; await drv.InitializeAsync("{}", CancellationToken.None); await Task.WhenAll(Enumerable.Range(0, 8).Select(b => drv.WriteAsync([new WriteRequest($"Bit{b}", true)], CancellationToken.None))); - Convert.ToUInt32(factory.Clients[0].Values["MAIN.Flags"]).ShouldBe(0xFFu); + factory.Clients[0].ParentWords["MAIN.Flags"].ShouldBe(new byte[] { 0xFF, 0 }); } } From f17ba8e2585e0036bfe83f036c37f23bc141d3e6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:15:29 -0400 Subject: [PATCH 06/12] feat(twincat): array and STRING writes through the codecs (U9) Array tags were advertised writable but every write threw InvalidCastException in ConvertForWrite (reported BadTypeMismatch); STRING writes went through the Beckhoff marshaller with no length check against STRING(n) and the host's ANSI code page, which the read side deliberately avoids. - Array write: TwinCATArrayCodec.ToWriteElements converts exactly ArrayLength elements to the declared type (CLR lists convert - ScadaBridge writes int[] to Int16 nodes; a length mismatch or out-of-range element is BadOutOfRange, a scalar value BadTypeMismatch) before any wire traffic; the ADS client plans the layout from the resolved PLC array (TwinCATTypeGuard.CheckArray, as the read does), encodes with TwinCATArrayCodec.Encode - the exact inverse of Decode - and writes the raw bytes (new ITwinCATClient.WriteArrayAsync). - STRING / WSTRING write: encoded at the PLC's declared width by TwinCATStringCodec.EncodeString/EncodeWString (Windows-1252 or UTF-8 per target, NUL-padded, strict: an over-long value or unrepresentable character is OverflowException -> BadOutOfRange) and written as raw bytes. - TwinCAT CLI read/write gain --array-length (+ --separator) for the hardware gate. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Commands/ReadCommand.cs | 11 +- .../Commands/WriteCommand.cs | 38 +++- .../AdsTwinCATClient.cs | 39 ++++ .../ITwinCATClient.cs | 17 ++ .../TwinCATArrayCodec.cs | 78 +++++++ .../TwinCATDriver.cs | 26 ++- .../TwinCATStringCodec.cs | 51 +++++ .../WriteCommandParseValueTests.cs | 20 ++ .../FakeTwinCATClient.cs | 18 ++ .../TwinCATArrayStringWriteTests.cs | 199 ++++++++++++++++++ 10 files changed, 490 insertions(+), 7 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArrayStringWriteTests.cs diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs index 96fe1d87..4603825d 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/ReadCommand.cs @@ -25,6 +25,11 @@ public sealed class ReadCommand : TwinCATTagCommandBase "String / WString / Time / Date / DateTime / TimeOfDay (default DInt).")] public TwinCATDataType DataType { get; init; } = TwinCATDataType.DInt; + /// Gets the element count for an array read; null reads a scalar. + [CommandOption("array-length", Description = + "Read the symbol as a 1-D array of this many --type elements (the leading sub-range when shorter).")] + public int? ArrayLength { get; init; } + /// public override async ValueTask ExecuteAsync(IConsole console) { @@ -38,7 +43,8 @@ public sealed class ReadCommand : TwinCATTagCommandBase DeviceHostAddress: Gateway, SymbolPath: SymbolPath, DataType: DataType, - Writable: false); + Writable: false, + ArrayLength: ArrayLength); var options = BuildOptions([tag]); await using var driver = new TwinCATDriver(options, DriverInstanceId); @@ -47,6 +53,9 @@ public sealed class ReadCommand : TwinCATTagCommandBase await driver.InitializeAsync("{}", ct); var snapshot = await driver.ReadAsync([tagName], ct); await console.Output.WriteLineAsync(SnapshotFormatter.Format(SymbolPath, snapshot[0])); + if (snapshot[0].Value is Array elements) + await console.Output.WriteLineAsync("Elements: [" + string.Join(", ", + elements.Cast().Select(e => Convert.ToString(e, System.Globalization.CultureInfo.InvariantCulture))) + "]"); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs index f72adea4..a1f882ef 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli/Commands/WriteCommand.cs @@ -30,6 +30,16 @@ public sealed class WriteCommand : TwinCATTagCommandBase IsRequired = true)] public string Value { get; init; } = default!; + /// Gets the element count for an array write; null writes a scalar. + [CommandOption("array-length", Description = + "Write the symbol as a 1-D array of this many --type elements. --value then carries the elements " + + "separated by --separator (e.g. -v 1,2,3 --array-length 3); the count must match.")] + public int? ArrayLength { get; init; } + + /// Gets the element separator for an array write. + [CommandOption("separator", Description = "Element separator for --array-length writes (default ',').")] + public string Separator { get; init; } = ","; + /// public override async ValueTask ExecuteAsync(IConsole console) { @@ -48,10 +58,13 @@ public sealed class WriteCommand : TwinCATTagCommandBase DeviceHostAddress: Gateway, SymbolPath: SymbolPath, DataType: DataType, - Writable: true); + Writable: true, + ArrayLength: ArrayLength); var options = BuildOptions([tag]); - var parsed = ParseValue(Value, DataType); + var parsed = ArrayLength is int count + ? ParseArrayValue(Value, DataType, count, Separator) + : ParseValue(Value, DataType); await using var driver = new TwinCATDriver(options, DriverInstanceId); try @@ -113,6 +126,27 @@ public sealed class WriteCommand : TwinCATTagCommandBase } } + /// + /// Parse an array --value: -separated elements, each parsed by + /// . The element count must equal — the driver + /// refuses a mismatch with BadOutOfRange, but the CLI says so before touching the PLC. + /// + /// The raw separated element list. + /// The element type. + /// The expected element count (--array-length). + /// The element separator. + /// The parsed elements. + internal static object[] ParseArrayValue(string raw, TwinCATDataType type, int count, string separator) + { + if (count < 1) + throw new CliFx.Exceptions.CommandException("--array-length must be >= 1."); + var parts = raw.Split(string.IsNullOrEmpty(separator) ? "," : separator); + if (parts.Length != count) + throw new CliFx.Exceptions.CommandException( + $"--value has {parts.Length} element(s) but --array-length is {count}."); + return [.. parts.Select(p => ParseValue(type is TwinCATDataType.String or TwinCATDataType.WString ? p : p.Trim(), type))]; + } + private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch { "1" or "true" or "on" or "yes" => true, 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 a143d018..1781bedf 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -469,6 +469,23 @@ internal sealed class AdsTwinCATClient : ITwinCATClient { try { + if (type is TwinCATDataType.String or TwinCATDataType.WString) + { + // A STRING / WSTRING is written as raw bytes at the PLC's own declared width, encoded by the + // same codec the read decodes with: Windows-1252 (or UTF-8 when the target says so) rather + // than the host's ANSI code page, NUL-padded so no stale tail survives, and refused + // (OverflowException → BadOutOfRange) when it does not fit STRING(n). + var (shape, _, symbol, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken) + .ConfigureAwait(false); + if (symbolStatus != TwinCATStatusMapper.Good) return symbolStatus; + if (TwinCATTypeGuard.CheckRead(type, shape) is not null) return TwinCATStatusMapper.BadTypeMismatch; + var text = Convert.ToString(value, System.Globalization.CultureInfo.InvariantCulture) ?? string.Empty; + var encoded = type == TwinCATDataType.WString + ? TwinCATStringCodec.EncodeWString(text, shape.ByteSize) + : TwinCATStringCodec.EncodeString(text, shape.ByteSize, StringsAreUtf8()); + return await WriteRawAsync(symbolPath, symbol!, encoded, cancellationToken).ConfigureAwait(false); + } + var converted = ConvertForWrite(type, value); var result = await _client.WriteValueAsync(symbolPath, converted, cancellationToken) .ConfigureAwait(false); @@ -482,6 +499,28 @@ internal sealed class AdsTwinCATClient : ITwinCATClient } } + /// + public async Task WriteArrayAsync(string symbolPath, TwinCATDataType type, IReadOnlyList elements, + CancellationToken cancellationToken) + { + try + { + // Plan the layout from the RESOLVED PLC array exactly as the array read does, so the element + // stride (incl. a STRING(n) element's n+1) and the strict element-type rule are shared. + var (shape, array, symbol, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken) + .ConfigureAwait(false); + if (symbolStatus != TwinCATStatusMapper.Good) return symbolStatus; + if (TwinCATTypeGuard.CheckArray(type, elements.Count, shape, array, out var layout) is not null) + return TwinCATStatusMapper.BadTypeMismatch; + var bytes = TwinCATArrayCodec.Encode(type, elements, layout, StringsAreUtf8()); + return await WriteRawAsync(symbolPath, symbol!, bytes, cancellationToken).ConfigureAwait(false); + } + catch (AdsErrorException ex) + { + return MapAndSignal((uint)ex.ErrorCode); + } + } + /// public async Task WriteBitAsync(string parentPath, int bit, bool value, CancellationToken cancellationToken) { 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 1ffeb6fa..b44f3038 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -119,6 +119,23 @@ public interface ITwinCATClient : IDisposable object? value, CancellationToken cancellationToken); + /// + /// Writes an array tag: (already converted to the declared element type by + /// ) as raw bytes at the layout planned from the RESOLVED + /// PLC array () and encoded by + /// — the exact inverse of the array read, so an array tag writes + /// what it reads. Fewer elements than the PLC array writes its leading sub-range. + /// + /// The ADS symbol path of the array. + /// The declared element type. + /// The converted elements (the authored ArrayLength of them). + /// Cancellation token for the write. + /// The mapped OPC UA status: Good; BadTypeMismatch when the declared element type / + /// length cannot address the PLC array; the ADS write's status otherwise. + /// A string element does not fit the PLC element's width. + Task WriteArrayAsync(string symbolPath, TwinCATDataType type, IReadOnlyList elements, + CancellationToken cancellationToken); + /// /// Sets or clears one bit of an integer parent word — the write half of a BOOL-within-word tag /// (MAIN.Flags.3; ADS has no symbol for WORD.N). A read-modify-write of the parent's raw diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATArrayCodec.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATArrayCodec.cs index d008a714..d8fa56c1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATArrayCodec.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATArrayCodec.cs @@ -19,6 +19,84 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; /// internal static class TwinCATArrayCodec { + /// + /// Converts a client-supplied array value into exactly elements of the + /// declared type, each converted the way a scalar write converts ( + /// — so TIME/DATE elements become their encoded uint). Any CLR collection is accepted and the + /// element type is CONVERTED, not exact-matched: ScadaBridge writes an int[] to an Int16 node. + /// + /// The tag's declared element type. + /// The written value — an array or list. + /// The tag's authored ArrayLength. + /// The converted elements, in order. + /// The value is not a collection (a scalar, a string, null), or + /// an element cannot convert. + /// The element count differs from , or an + /// element is outside the declared type's range. + public static object[] ToWriteElements(TwinCATDataType declared, object? value, int count) + { + if (value is null or string || value is not System.Collections.IEnumerable items) + throw new InvalidCastException( + $"an array tag of {count} {declared} takes an array value, not {value?.GetType().Name ?? "null"}"); + var elements = new List(count); + foreach (var item in items) elements.Add(AdsTwinCATClient.ConvertForWrite(declared, item)); + if (elements.Count != count) + throw new OverflowException($"wrote {elements.Count} elements to an array tag of arrayLength {count}"); + return [.. elements]; + } + + /// + /// Encodes converted elements () into the PLC's raw bytes at the planned + /// layout — the exact inverse of : little-endian at the element stride, a + /// STRING / WSTRING element encoded by within its fixed stride. + /// + /// The tag's declared element type. + /// The converted elements; exactly . + /// The layout planned from the PLC array (). + /// True when the target reports UTF-8 STRING data (else Windows-1252). + /// Exactly bytes. + /// The element count differs from the layout. + /// A string element does not fit its stride. + public static byte[] Encode(TwinCATDataType declared, IReadOnlyList elements, TwinCATArrayLayout layout, bool utf8) + { + if (layout.Stride <= 0 || elements.Count != layout.ElementCount) + throw new ArgumentException( + $"{elements.Count} elements do not match a layout of {layout.ElementCount} × {layout.Stride} B", nameof(elements)); + + var bytes = new byte[layout.ByteLength]; + var stride = layout.Stride; + for (var i = 0; i < elements.Count; i++) + { + var slot = bytes.AsSpan(i * stride, stride); + var e = elements[i]; + switch (declared) + { + case TwinCATDataType.Bool: slot[0] = Convert.ToBoolean(e) ? (byte)1 : (byte)0; break; + case TwinCATDataType.SInt: slot[0] = unchecked((byte)Convert.ToSByte(e)); break; + case TwinCATDataType.USInt: slot[0] = Convert.ToByte(e); break; + case TwinCATDataType.Int: BinaryPrimitives.WriteInt16LittleEndian(slot, Convert.ToInt16(e)); break; + case TwinCATDataType.UInt: BinaryPrimitives.WriteUInt16LittleEndian(slot, Convert.ToUInt16(e)); break; + case TwinCATDataType.DInt: BinaryPrimitives.WriteInt32LittleEndian(slot, Convert.ToInt32(e)); break; + case TwinCATDataType.UDInt or TwinCATDataType.Time or TwinCATDataType.TimeOfDay + or TwinCATDataType.Date or TwinCATDataType.DateTime: + BinaryPrimitives.WriteUInt32LittleEndian(slot, Convert.ToUInt32(e)); break; + case TwinCATDataType.LInt: BinaryPrimitives.WriteInt64LittleEndian(slot, Convert.ToInt64(e)); break; + case TwinCATDataType.ULInt: BinaryPrimitives.WriteUInt64LittleEndian(slot, Convert.ToUInt64(e)); break; + case TwinCATDataType.Real: BinaryPrimitives.WriteSingleLittleEndian(slot, Convert.ToSingle(e)); break; + case TwinCATDataType.LReal: BinaryPrimitives.WriteDoubleLittleEndian(slot, Convert.ToDouble(e)); break; + case TwinCATDataType.String: + TwinCATStringCodec.EncodeString(Convert.ToString(e) ?? string.Empty, stride, utf8).CopyTo(slot); + break; + case TwinCATDataType.WString: + TwinCATStringCodec.EncodeWString(Convert.ToString(e) ?? string.Empty, stride).CopyTo(slot); + break; + default: + throw new NotSupportedException($"TwinCATDataType {declared} has no array element encoding"); + } + } + return bytes; + } + /// Decodes .ElementCount elements of . /// The tag's declared element type. /// The PLC bytes from the array's first element (at least ). 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 2a5d4d94..30d9f488 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -508,17 +508,26 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery try { - var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); var parsed = TwinCATSymbolPath.TryParse(def.SymbolPath); + var bitIndex = def.DataType == TwinCATDataType.Bool ? parsed?.BitIndex : null; + // Shape checks run BEFORE any wire traffic (connect included): an array write needs exactly + // ArrayLength elements, each converted to the declared element type (a CLR int[] to an Int16 + // array converts; it is not exact-matched) — a length mismatch / out-of-range element is + // OverflowException (BadOutOfRange), a scalar value InvalidCastException (BadTypeMismatch). + var bitValue = bitIndex is not null ? Convert.ToBoolean(w.Value) : false; + var elements = bitIndex is null && def.ArrayLength is int arrayLength + ? TwinCATArrayCodec.ToWriteElements(def.DataType, w.Value, arrayLength) + : null; + + var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); // BOOL-within-word write — a read-modify-write of the parent word at the parent's OWN // width and on raw bytes (the client resolves the parent and checks the bit against it, the // notification path's model), under a per-parent lock so concurrent bit writers to the same // word don't lose each other's updates. - if (def.DataType == TwinCATDataType.Bool && parsed?.BitIndex is int bit) + if (bitIndex is int bit) { - var parentPath = (parsed with { BitIndex = null }).ToAdsSymbolName(); - var bitValue = Convert.ToBoolean(w.Value); + var parentPath = (parsed! with { BitIndex = null }).ToAdsSymbolName(); var gate = _bitRmwLocks.GetOrAdd( $"{def.DeviceHostAddress}|{parentPath}", static _ => new SemaphoreSlim(1, 1)); await gate.WaitAsync(cancellationToken).ConfigureAwait(false); @@ -535,6 +544,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } var symbolName = parsed?.ToAdsSymbolName() ?? def.SymbolPath; + if (elements is not null) + { + // Array tag: raw bytes encoded by TwinCATArrayCodec at the layout planned from the PLC + // array — the inverse of the array read. + results[i] = new WriteResult(await client.WriteArrayAsync( + symbolName, def.DataType, elements, cancellationToken).ConfigureAwait(false)); + continue; + } + var status = await client.WriteValueAsync( symbolName, def.DataType, parsed?.BitIndex, w.Value, cancellationToken).ConfigureAwait(false); results[i] = new WriteResult(status); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStringCodec.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStringCodec.cs index d3a90c1f..bdaff2ae 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStringCodec.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStringCodec.cs @@ -50,6 +50,57 @@ internal static class TwinCATStringCodec return (utf8 ? Encoding.UTF8 : Windows1252).GetString(bytes); } + // Strict encoders for the WRITE side: a character the target encoding cannot hold is refused rather + // than silently replaced with '?' (the replacement would be written to the PLC as if it were the value). + private static readonly Encoding StrictWindows1252 = CreateStrict(Windows1252.CodePage); + private static readonly Encoding StrictUtf8 = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); + private static readonly Encoding StrictUtf16 = new UnicodeEncoding(bigEndian: false, byteOrderMark: false, throwOnInvalidBytes: true); + + private static Encoding CreateStrict(int codePage) => + Encoding.GetEncoding(codePage, EncoderFallback.ExceptionFallback, DecoderFallback.ReplacementFallback); + + /// + /// Encodes a value into an IEC STRING(n) buffer of the PLC's declared width (n+1 bytes): the + /// inverse of — Windows-1252 unless , NUL-terminated + /// and NUL-padded so no stale tail of a longer earlier value survives. + /// + /// The value to write. + /// The PLC symbol's byte width (n+1 for a STRING(n)). + /// True when the target reports UTF-8 string data. + /// Exactly bytes. + /// The value does not fit STRING(n), or holds a character the + /// target encoding cannot represent. + public static byte[] EncodeString(string value, int byteWidth, bool utf8) => + Encode(value, byteWidth, utf8 ? StrictUtf8 : StrictWindows1252, terminatorBytes: 1, "STRING"); + + /// + /// Encodes a value into an IEC WSTRING(n) buffer (UTF-16LE, 2(n+1) bytes): the inverse of + /// , terminated by an aligned U+0000 and zero-padded. + /// + /// The value to write. + /// The PLC symbol's byte width. + /// Exactly bytes. + /// The value does not fit WSTRING(n). + public static byte[] EncodeWString(string value, int byteWidth) => + Encode(value, byteWidth, StrictUtf16, terminatorBytes: 2, "WSTRING"); + + private static byte[] Encode(string value, int byteWidth, Encoding encoding, int terminatorBytes, string kind) + { + ArgumentNullException.ThrowIfNull(value); + byte[] encoded; + try { encoded = encoding.GetBytes(value); } + catch (ArgumentException ex) // EncoderFallbackException: a character the encoding cannot hold + { + throw new OverflowException($"'{value}' cannot be represented in the PLC's {kind} encoding: {ex.Message}", ex); + } + if (encoded.Length + terminatorBytes > byteWidth) + throw new OverflowException( + $"'{value}' is {encoded.Length} B but the PLC {kind} holds {Math.Max(0, byteWidth - terminatorBytes)} B"); + var buffer = new byte[byteWidth]; + encoded.CopyTo(buffer, 0); + return buffer; + } + /// Decodes an IEC WSTRING(n) buffer (UTF-16LE): ends at the first aligned U+0000. /// The raw symbol bytes (2(n+1) for a WSTRING(n)). /// The decoded value. diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs index 89ed5c22..a06662a7 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs @@ -184,4 +184,24 @@ public sealed class WriteCommandParseValueTests { ReadCommand.SynthesiseTagName(symbol, type).ShouldBe(expected); } + + /// An array value splits on the separator and parses each element per --type (#739 U9 gate tooling). + [Fact] + public void ParseArrayValue_parses_each_element() + { + WriteCommand.ParseArrayValue("1, -2,3", TwinCATDataType.Int, 3, ",") + .ShouldBe(new object[] { (short)1, (short)-2, (short)3 }); + WriteCommand.ParseArrayValue("a b|c", TwinCATDataType.String, 2, "|") + .ShouldBe(new object[] { "a b", "c" }); + } + + /// A count that differs from --array-length is refused before the PLC is touched. + [Fact] + public void ParseArrayValue_refuses_a_count_mismatch() + { + Should.Throw( + () => WriteCommand.ParseArrayValue("1,2", TwinCATDataType.Int, 3, ",")); + Should.Throw( + () => WriteCommand.ParseArrayValue("1", TwinCATDataType.Int, 0, ",")); + } } 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 e31340a5..39f795be 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 @@ -136,6 +136,24 @@ internal class FakeTwinCATClient : ITwinCATClient return Task.FromResult(status); } + /// Gets the log of every call. + public List<(string symbol, TwinCATDataType type, IReadOnlyList elements)> ArrayWriteLog { get; } = new(); + + /// Simulates an array write: records it, stores the elements and answers from . + /// The array symbol path. + /// The declared element type. + /// The converted elements. + /// The cancellation token. + /// The write status. + public virtual Task WriteArrayAsync(string symbolPath, TwinCATDataType type, IReadOnlyList elements, + CancellationToken ct) + { + if (ThrowOnWrite) throw Exception ?? new InvalidOperationException(); + ArrayWriteLog.Add((symbolPath, type, elements)); + Values[symbolPath] = elements; + return Task.FromResult(WriteStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good); + } + /// Raw bytes of bit-write parent words, by parent path (#739 H18). The byte count is the parent's width. public Dictionary ParentWords { get; } = new(StringComparer.OrdinalIgnoreCase); /// Optional parent shapes; absent = an unsigned integer word as wide as its entry. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArrayStringWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArrayStringWriteTests.cs new file mode 100644 index 00000000..b42c3f0a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATArrayStringWriteTests.cs @@ -0,0 +1,199 @@ +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; + +/// +/// #739 U9: array writes and STRING writes go through the same codecs the read side decodes with. +/// Arrays were advertised writable but every write threw InvalidCastException (reported BadTypeMismatch); +/// strings bypassed the codec (host-ANSI encoding, no length check against STRING(n)). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATArrayStringWriteTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + // ---- string codec: encode ---- + + /// A STRING(n) encodes NUL-terminated and NUL-padded to the PLC width, in Windows-1252. + [Fact] + public void EncodeString_is_NUL_padded_Windows1252_at_the_PLC_width() + { + TwinCATStringCodec.EncodeString("AB", 4, utf8: false).ShouldBe(new byte[] { 0x41, 0x42, 0, 0 }); + TwinCATStringCodec.EncodeString("ä€", 3, utf8: false).ShouldBe(new byte[] { 0xE4, 0x80, 0 }); + TwinCATStringCodec.EncodeString("ä", 3, utf8: true).ShouldBe(new byte[] { 0xC3, 0xA4, 0 }); + TwinCATStringCodec.EncodeString("", 2, utf8: false).ShouldBe(new byte[] { 0, 0 }); + } + + /// The encode is the decode's inverse (a STRING(n) of exactly n characters fits). + [Fact] + public void EncodeString_round_trips_through_DecodeString() + { + foreach (var utf8 in new[] { false, true }) + { + var bytes = TwinCATStringCodec.EncodeString("Grüße 40°", 81, utf8); + TwinCATStringCodec.DecodeString(bytes, utf8).ShouldBe("Grüße 40°"); + } + TwinCATStringCodec.EncodeString("ABC", 4, utf8: false).Length.ShouldBe(4); + } + + /// A value longer than STRING(n), or a character the target encoding cannot hold, is out of range. + [Fact] + public void EncodeString_refuses_a_value_that_does_not_fit() + { + Should.Throw(() => TwinCATStringCodec.EncodeString("ABC", 3, utf8: false)); + Should.Throw(() => TwinCATStringCodec.EncodeString("中", 10, utf8: false)); + Should.Throw(() => TwinCATStringCodec.EncodeString("ää", 4, utf8: true)); // 4 bytes + NUL + } + + /// A WSTRING(n) encodes UTF-16LE with a 2-byte terminator and round-trips. + [Fact] + public void EncodeWString_is_UTF16LE_with_a_terminator() + { + TwinCATStringCodec.EncodeWString("Hi", 6).ShouldBe(new byte[] { 0x48, 0, 0x69, 0, 0, 0 }); + TwinCATStringCodec.DecodeWString(TwinCATStringCodec.EncodeWString("中文", 22)).ShouldBe("中文"); + Should.Throw(() => TwinCATStringCodec.EncodeWString("Hi", 4)); + } + + // ---- array codec: elements + encode ---- + + /// + /// A CLR list converts element-wise to the declared type (ScadaBridge writes an int[] to an + /// Int16 node); the element type is converted, not exact-matched. + /// + [Fact] + public void ToWriteElements_converts_each_element_to_the_declared_type() + { + var elements = TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Int, new[] { 1, -2, 300 }, 3); + elements.ShouldBe(new object[] { (short)1, (short)-2, (short)300 }); + TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Real, new List { 1, 2.5 }, 2) + .ShouldBe(new object[] { 1f, 2.5f }); + TwinCATArrayCodec.ToWriteElements(TwinCATDataType.String, new[] { "a", "b" }, 2) + .ShouldBe(new object[] { "a", "b" }); + } + + /// A length mismatch and an out-of-range element are out of range; a scalar is a type mismatch. + [Fact] + public void ToWriteElements_refuses_a_wrong_shape() + { + Should.Throw(() => TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Int, new[] { 1, 2 }, 3)); + Should.Throw(() => TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Int, new[] { 70_000 }, 1)); + Should.Throw(() => TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Int, 5, 1)); + Should.Throw(() => TwinCATArrayCodec.ToWriteElements(TwinCATDataType.String, "abc", 3)); + Should.Throw(() => TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Int, null, 1)); + } + + /// Encode lays elements out little-endian at the PLC stride — the exact inverse of Decode. + [Fact] + public void Encode_is_the_inverse_of_Decode() + { + var intLayout = new TwinCATArrayLayout(2, 2); + var intBytes = TwinCATArrayCodec.Encode(TwinCATDataType.Int, [(short)1, (short)-2], intLayout, utf8: false); + intBytes.ShouldBe(new byte[] { 0x01, 0x00, 0xFE, 0xFF }); + + RoundTrip(TwinCATDataType.Bool, [true, false, true], new TwinCATArrayLayout(3, 1)); + RoundTrip(TwinCATDataType.DInt, [int.MinValue, 7], new TwinCATArrayLayout(2, 4)); + RoundTrip(TwinCATDataType.ULInt, [ulong.MaxValue], new TwinCATArrayLayout(1, 8)); + RoundTrip(TwinCATDataType.LReal, [1.5, -2.25], new TwinCATArrayLayout(2, 8)); + RoundTrip(TwinCATDataType.String, ["ab", "Grüße"], new TwinCATArrayLayout(2, 11)); + RoundTrip(TwinCATDataType.WString, ["中", "x"], new TwinCATArrayLayout(2, 8)); + + // TIME elements arrive as the encoded uint ms (ConvertForWrite) and decode back to double ms. + var time = TwinCATArrayCodec.Encode(TwinCATDataType.Time, + TwinCATArrayCodec.ToWriteElements(TwinCATDataType.Time, new[] { 1500.0, 20.0 }, 2), new TwinCATArrayLayout(2, 4), false); + TwinCATArrayCodec.Decode(TwinCATDataType.Time, time, new TwinCATArrayLayout(2, 4), false) + .ShouldBe(new[] { 1500.0, 20.0 }); + } + + /// A string element longer than the element's STRING(n) is out of range. + [Fact] + public void Encode_refuses_a_string_element_wider_than_its_stride() + { + Should.Throw(() => + TwinCATArrayCodec.Encode(TwinCATDataType.String, ["toolong"], new TwinCATArrayLayout(1, 4), utf8: false)); + Should.Throw(() => + TwinCATArrayCodec.Encode(TwinCATDataType.Int, [(short)1], new TwinCATArrayLayout(2, 2), utf8: false)); + } + + private static void RoundTrip(TwinCATDataType type, object[] elements, TwinCATArrayLayout layout) + { + var bytes = TwinCATArrayCodec.Encode(type, elements, layout, utf8: false); + bytes.Length.ShouldBe(layout.ByteLength); + var decoded = TwinCATArrayCodec.Decode(type, bytes, layout, utf8: false); + decoded.Cast().ShouldBe(elements); + } + + // ---- driver ---- + + private static (TwinCATDriver drv, FakeTwinCATClient fake) NewDriver(params TwinCATTagDefinition[] tags) + { + var fake = new FakeTwinCATClient(); + var drv = new TwinCATDriver(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries(tags), + Probe = new TwinCATProbeOptions { Enabled = false }, + }, "drv-1", new FakeTwinCATClientFactory { Customise = () => fake }); + return (drv, fake); + } + + /// An array tag's write sends ArrayLength converted elements through WriteArrayAsync. + [Fact] + public async Task An_array_write_sends_converted_elements() + { + var (drv, fake) = NewDriver(new TwinCATTagDefinition("Buf", Host, "MAIN.Buf", TwinCATDataType.Int, ArrayLength: 3)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var result = await drv.WriteAsync([new WriteRequest("Buf", new[] { 1, 2, 3 })], TestContext.Current.CancellationToken); + + result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); + var write = fake.ArrayWriteLog.ShouldHaveSingleItem(); + write.symbol.ShouldBe("MAIN.Buf"); + write.elements.ShouldBe(new object[] { (short)1, (short)2, (short)3 }); + fake.WriteLog.ShouldBeEmpty(); // never the scalar path (that threw InvalidCastException) + } + + /// + /// A length mismatch is BadOutOfRange and a scalar value BadTypeMismatch — nothing reaches the wire + /// and health is untouched (a refusal is not a comms fault). Positive control: the good write above. + /// + [Fact] + public async Task A_wrong_shape_array_write_is_refused_before_the_wire() + { + var (drv, fake) = NewDriver(new TwinCATTagDefinition("Buf", Host, "MAIN.Buf", TwinCATDataType.Int, ArrayLength: 3)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var results = await drv.WriteAsync( + [ + new WriteRequest("Buf", new[] { 1, 2 }), + new WriteRequest("Buf", 5), + new WriteRequest("Buf", new[] { 1, 2, 70_000 }), + ], TestContext.Current.CancellationToken); + + results.Select(r => r.StatusCode).ShouldBe( + [ + TwinCATStatusMapper.BadOutOfRange, + TwinCATStatusMapper.BadTypeMismatch, + TwinCATStatusMapper.BadOutOfRange, + ]); + fake.ArrayWriteLog.ShouldBeEmpty(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// An over-long string surfaced by the client as OverflowException is BadOutOfRange, not a comms fault. + [Fact] + public async Task An_over_long_string_write_is_BadOutOfRange() + { + var (drv, fake) = NewDriver(new TwinCATTagDefinition("Name", Host, "MAIN.Name", TwinCATDataType.String)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + fake.ThrowOnWrite = true; + fake.Exception = new OverflowException("6 bytes do not fit STRING(4)"); + + var result = await drv.WriteAsync([new WriteRequest("Name", "toolong")], TestContext.Current.CancellationToken); + + result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } +} From cbacaee4ee3289e3b830997570cf4b4d374417eb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:24:10 -0400 Subject: [PATCH 07/12] fix(twincat): bounded, race-safe lifecycle; drop counter; post-unsubscribe purge (L8) - Device and tag tables are build-and-swap: InitializeAsync builds a fresh generation and publishes it by one swap under a lifecycle lock, then releases the replaced generation; teardown swaps in empty tables. A retained poll loop never sees a table mid-Clear (it used to read _tagsByRawPath while a re-init only ever added to it), and each device set is released by exactly one caller. - A lifecycle generation makes an InitializeAsync overtaken by a shutdown (or a newer init) release what it built instead of installing it - the Lane A abandoned-init-overlaps-shutdown case. - ShutdownAsync joins all probe loops together within 2 s (it awaited each unbounded - a probe wedged in the blocking ADS Connect hung the actor's teardown) in parallel with the poll-engine dispose, the whole bounded at 4.5 s; DisposeAsync may run concurrently and returns at once. A connect that completes on a released device disposes its client. - Releasing a generation disposes its native registrations, so a direct re-init no longer leaves live notifications bound to dead DeviceStates. - Connect gates and RMW semaphores are no longer disposed under in-flight users (no kernel handle is ever allocated); a fault on a released device is not recorded as driver health. - The notification channel counts DropWrite overflows (ITwinCATClient.DroppedNotificationCount); the probe tick warns with the delta. A queued value whose registration was disposed is purged in the client dispatch loop and dropped driver-side for any client. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AdsTwinCATClient.cs | 17 +- .../ITwinCATClient.cs | 6 + .../TwinCATDriver.cs | 359 +++++++++++++----- .../FakeTwinCATClient.cs | 12 + .../TwinCATLifecycleTests.cs | 271 +++++++++++++ 5 files changed, 561 insertions(+), 104 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.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 1781bedf..a779b09b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -721,15 +721,24 @@ internal sealed class AdsTwinCATClient : ITwinCATClient value = DecodeString(reg.Type, raw, reg.StringsUtf8); else if (TwinCATTemporalCodec.IsTemporal(reg.Type) && value is uint rawTemporal) value = TwinCATTemporalCodec.Decode(reg.Type, rawTemporal); - _notificationQueue.Writer.TryWrite(new PendingNotification(reg, value)); + if (!_notificationQueue.Writer.TryWrite(new PendingNotification(reg, value))) + Interlocked.Increment(ref _droppedNotifications); // queue full (DropWrite): count, never block } + private long _droppedNotifications; + + /// + public long DroppedNotificationCount => Interlocked.Read(ref _droppedNotifications); + private async Task DispatchLoopAsync(CancellationToken ct) { try { await foreach (var pending in _notificationQueue.Reader.ReadAllAsync(ct).ConfigureAwait(false)) { + // A value queued before its registration was deleted is purged here rather than delivered + // after the unsubscribe. + if (pending.Registration.IsDeleted) continue; try { pending.Registration.OnChange(pending.Registration.SymbolPath, pending.Value); } catch { /* consumer-side errors stay on this managed task, not the ADS thread */ } } @@ -980,9 +989,15 @@ internal sealed class AdsTwinCATClient : ITwinCATClient /// public void Dispose() { + IsDeleted = true; // Fire-and-forget AMS call — caller has already committed to the tear-down. _ = owner.DeleteNotificationAsync(handle, CancellationToken.None); } + + /// Gets a value indicating whether the registration was disposed; queued values for it are dropped. + public bool IsDeleted { get => Volatile.Read(ref _deleted); private set => Volatile.Write(ref _deleted, value); } + + private bool _deleted; } private static Type MapToClrType(TwinCATDataType type) => type switch 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 b44f3038..cb552f0c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs @@ -102,6 +102,12 @@ public interface ITwinCATClient : IDisposable /// The overall ADS code of the most recent such fallback (0 = a malformed response). uint LastSumFallbackCode => 0; + /// + /// Native notifications this client dropped because its bounded dispatch queue was full (the router + /// thread never blocks on a slow consumer). The driver warns when it grows. Default 0. + /// + long DroppedNotificationCount => 0; + /// /// Write a symbolic value. Returns the mapped OPC UA status for the operation /// (0 = Good, non-zero = error mapped via ). 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 30d9f488..8542bcad 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -22,30 +22,60 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public SkippedTagReport? GetSkippedTags() => _skippedTags.Snapshot(); // Mutable so ReinitializeAsync can apply a new config generation. The constructor seeds - // it; InitializeAsync re-parses driverConfigJson over the top of it. - private TwinCATDriverOptions _options; + // it; InitializeAsync re-parses driverConfigJson over the top of it (committed under _lifecycleLock). + private volatile TwinCATDriverOptions _options; private readonly string _driverInstanceId; private readonly ITwinCATClientFactory _clientFactory; private readonly ILogger _logger; private readonly PollGroupEngine _poll; - // ConcurrentDictionary so ShutdownAsync (Clear) and ReadAsync/WriteAsync/SubscribeAsync - // (TryGetValue) don't race — plain Dictionary is not safe for concurrent read+write. - private readonly ConcurrentDictionary _devices = - new(StringComparer.OrdinalIgnoreCase); + + // Device + tag tables are BUILD-AND-SWAP (#739 L8): InitializeAsync builds fresh tables and publishes + // them by reference under _lifecycleLock; ShutdownAsync / Dispose swap in empty ones and release what + // they took. A retained poll loop, the probe tick or an in-flight write therefore always sees one whole + // generation — never a table mid-Clear — and two overlapping teardowns can never release the same + // device twice (each device set is taken by exactly one swap). + private volatile ConcurrentDictionary _devices = NewDeviceTable(); // v3: the authored RawPath → definition table. Keyed case-sensitive ordinal (the RawPath is // the exact wire reference). Built in InitializeAsync from _options.RawTags via the pure mapper. - private readonly ConcurrentDictionary _tagsByRawPath = - new(StringComparer.Ordinal); + private volatile IReadOnlyDictionary _tagsByRawPath = EmptyTags; + + private static readonly IReadOnlyDictionary EmptyTags = + new Dictionary(StringComparer.Ordinal); + + private static ConcurrentDictionary NewDeviceTable() => new(StringComparer.OrdinalIgnoreCase); + + // Serialises the table swaps against each other. _lifecycleGeneration is bumped by every + // InitializeAsync and every teardown, so an InitializeAsync that was overtaken while it awaited + // (an abandoned init overlapping the runtime's next ShutdownAsync, or a newer init) sees a + // different generation at its commit and releases what it built instead of installing it. + private readonly object _lifecycleLock = new(); + private long _lifecycleGeneration; + + /// Test seam: awaited after the new generation is built and before it is committed, so a test can + /// interleave a teardown deterministically. Never set in production. + internal Func? BeforeCommitForTest { get; set; } + + /// Upper bound on joining released devices' probe loops (one wait for all of them). + internal static readonly TimeSpan ProbeJoinTimeout = TimeSpan.FromSeconds(2); + + /// + /// Upper bound on the whole of . The runtime gives a final stop 5 s, so + /// the driver stops waiting a little earlier; anything still running past it (a poll loop wedged in + /// a read) is cancelled already and finishes in the background. + /// + internal static readonly TimeSpan ShutdownBudget = TimeSpan.FromSeconds(4.5); + + // References that already logged their bad-read Warning in the current bad episode (#739 P10). + // Removed when the tag reads Good; cleared on (re-)initialise and shutdown. + private readonly ConcurrentDictionary _warnedRefs = new(StringComparer.Ordinal); // Per-parent-word RMW gate for BOOL-within-word writes: a single-bit write is a // read-modify-write of the parent word (TwinCAT's symbol table doesn't expose "Word.N"), // so concurrent bit-writers to the same word must serialise or they lose each other's // updates. Keyed by device + parent symbol. (Cannot guard against the PLC program writing - // the word between our read and write — inherent to RMW.) - // References that already logged their bad-read Warning in the current bad episode (#739 P10). - // Removed when the tag reads Good; cleared on (re-)initialise and shutdown. - private readonly ConcurrentDictionary _warnedRefs = new(StringComparer.Ordinal); - + // the word between our read and write — inherent to RMW.) Teardown only CLEARS it: a + // SemaphoreSlim owns no kernel handle unless AvailableWaitHandle is read, and disposing one an + // in-flight write still holds turns its Release into an ObjectDisposedException. private readonly ConcurrentDictionary _bitRmwLocks = new(StringComparer.OrdinalIgnoreCase); @@ -118,29 +148,40 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { // InitializeAsync is reachable on a live driver with no teardown first (the IDriver contract - // allows it, and a reconnect retry can overlap a running attempt). Release the previous device states' probe loops + clients before - // the overwrite below. Deliberately NOT ShutdownAsync: that also drops _poll and the subscriptions. - await ReleaseDeviceStatesAsync().ConfigureAwait(false); + // allows it, and a reconnect retry can overlap a running attempt). The new generation is BUILT + // first and published by one swap; the previous device states (probe loops, clients, native + // registrations) are released right after the swap — never orphaned, and a retained poll loop + // never sees an empty table in between. Deliberately NOT ShutdownAsync: that also drops _poll and + // the subscriptions. + long generation; + lock (_lifecycleLock) + { + generation = ++_lifecycleGeneration; + _health = new DriverHealth(DriverState.Initializing, null, null); + } _warnedRefs.Clear(); - _health = new DriverHealth(DriverState.Initializing, null, null); + + var devices = NewDeviceTable(); + var committed = false; try { // Apply the supplied config generation. 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). + var options = _options; if (!string.IsNullOrWhiteSpace(driverConfigJson)) { var parsed = TwinCATDriverFactoryExtensions.ParseOptions(driverConfigJson, _driverInstanceId); if (parsed.Devices.Count > 0 || parsed.RawTags.Count > 0) - _options = parsed; + options = parsed; } - foreach (var device in _options.Devices) + foreach (var device in options.Devices) { var addr = TwinCATAmsAddress.TryParse(device.HostAddress) ?? throw new InvalidOperationException( $"TwinCAT device has invalid HostAddress '{device.HostAddress}' — expected 'ads://{{netId}}:{{port}}'."); - _devices[device.HostAddress] = new DeviceState(addr, device); + devices[device.HostAddress] = new DeviceState(addr, device); } // Build the RawPath → definition table from the authored raw tags. Each entry's TagConfig @@ -149,14 +190,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // DeviceName is resolved to the target's live host (def.DeviceHostAddress) via ResolveDeviceHost // so the existing device routing (_devices keyed by HostAddress) is unchanged. A mapper miss is // skipped (logged), never thrown — a bad tag must not fail the whole driver init. - _skippedTags.BeginRebuild(_options.RawTags.Count); - foreach (var entry in _options.RawTags) + var tags = new Dictionary(StringComparer.Ordinal); + _skippedTags.BeginRebuild(options.RawTags.Count); + foreach (var entry in options.RawTags) { if (TwinCATTagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def)) - _tagsByRawPath[entry.RawPath] = def with + tags[entry.RawPath] = def with { WriteIdempotent = entry.WriteIdempotent, - DeviceHostAddress = ResolveDeviceHost(entry.DeviceName), + DeviceHostAddress = ResolveDeviceHost(options, entry.DeviceName), }; else { @@ -168,47 +210,113 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } _skippedTags.Commit(); - if (_options.Probe.Enabled) + if (BeforeCommitForTest is { } pause) await pause().ConfigureAwait(false); + + ConcurrentDictionary previous; + lock (_lifecycleLock) { - foreach (var state in _devices.Values) + if (generation != _lifecycleGeneration) + throw new OperationCanceledException( + "TwinCAT InitializeAsync was superseded by a shutdown or a newer initialise while it ran."); + + _options = options; + _tagsByRawPath = tags; + previous = _devices; + _devices = devices; + committed = true; + // Probe loops start INSIDE the commit so a teardown that swaps this generation out right + // after always finds (and cancels) them. + if (options.Probe.Enabled) { - state.ProbeCts = new CancellationTokenSource(); - var ct = state.ProbeCts.Token; - // Store the task so ShutdownAsync can await it. - state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); + foreach (var state in devices.Values) + { + state.ProbeCts = new CancellationTokenSource(); + var ct = state.ProbeCts.Token; + // Store the task so the teardown can join it. + state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); + } + } + _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); + } + + // The replaced generation is now unreachable from the tables: release it (bounded). + await ReleaseDevicesAsync(previous.Values).ConfigureAwait(false); + } + catch (Exception ex) when (!committed) + { + // Nothing of this generation was published: release what was built. A superseded init leaves + // health to whoever overtook it. + await ReleaseDevicesAsync(devices.Values).ConfigureAwait(false); + if (ex is not OperationCanceledException) + { + lock (_lifecycleLock) + { + if (generation == _lifecycleGeneration) + _health = new DriverHealth(DriverState.Faulted, null, ex.Message); } } - _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); - } - catch (Exception ex) - { - _health = new DriverHealth(DriverState.Faulted, null, ex.Message); throw; } } /// - /// Cancels, awaits and disposes every existing device state's probe loop, client and gate, then - /// clears the device table. Device part of only. + /// Releases a set of device states that have already been swapped out of : + /// marks them released (so a probe wedged in the blocking ADS connect disposes the client it + /// eventually gets), disposes their native registrations (a re-initialise must not leave live ADS + /// notifications bound to a dead generation), cancels every probe loop and joins them all together + /// within , then disposes their clients. The connect gates are NOT + /// disposed — an in-flight read/write may still hold one. /// - private async Task ReleaseDeviceStatesAsync() + /// The released device states (each owned by exactly one caller). + private async Task ReleaseDevicesAsync(ICollection states) { - if (_devices.IsEmpty) return; - foreach (var state in _devices.Values) + if (states.Count == 0) return; + var probes = BeginRelease(states); + if (probes.Count > 0) { - try { state.ProbeCts?.Cancel(); } catch { } - if (state.ProbeTask is Task pt) - { - try { await pt.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); } - catch { /* cancelled, faulted or wedged — teardown continues */ } - } - state.ProbeCts?.Dispose(); + try { await Task.WhenAll(probes).WaitAsync(ProbeJoinTimeout).ConfigureAwait(false); } + catch { /* cancelled, faulted or wedged in the synchronous ADS connect — teardown continues */ } + } + EndRelease(states); + } + + /// The non-blocking first half of a release: mark, retract registrations, cancel probes. + private static List BeginRelease(ICollection states) + { + var probes = new List(states.Count); + foreach (var state in states) + { + state.Released = true; + foreach (var reg in state.NativeRegistrations.Values) { try { reg.Dispose(); } catch { } } + try { state.ProbeCts?.Cancel(); } catch { /* already disposed */ } + if (state.ProbeTask is Task pt) probes.Add(pt); + } + return probes; + } + + /// The second half of a release, after the bounded probe join: dispose CTSs and clients. + private static void EndRelease(ICollection states) + { + foreach (var state in states) + { + try { state.ProbeCts?.Dispose(); } catch { } state.ProbeCts = null; state.ProbeTask = null; - state.DisposeClient(); - state.DisposeGate(); + try { state.DisposeClient(); } catch { /* best-effort */ } + } + } + + /// Swaps out every table under the lifecycle lock and returns the device set the caller now owns. + private ICollection TakeAllForTeardown() + { + lock (_lifecycleLock) + { + _lifecycleGeneration++; + var taken = _devices; + _devices = NewDeviceTable(); + _tagsByRawPath = EmptyTags; + return taken.Values; } - _devices.Clear(); } /// @@ -219,6 +327,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } /// + /// + /// Safe to run concurrently with itself, with / and with + /// an abandoned : the tables are taken by one atomic swap, so each device + /// is released exactly once and an overtaken init releases its own generation. Bounded by + /// . + /// public async Task ShutdownAsync(CancellationToken cancellationToken) { // Native subs first — disposing the handles is cheap + lets the client close its @@ -227,30 +341,22 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery foreach (var r in sub.Registrations) { try { r.Dispose(); } catch { } } _nativeSubs.Clear(); - await _poll.DisposeAsync().ConfigureAwait(false); + var taken = TakeAllForTeardown(); - // Cancel every probe loop and await its task before disposing the client + gate so the - // loop can never touch a disposed object. - foreach (var state in _devices.Values) + // The poll loops and the device release wind down in parallel (releasing a client can unblock a + // poll loop stuck in a read on it); the whole teardown is bounded. + var teardown = Task.WhenAll(_poll.DisposeAsync().AsTask(), ReleaseDevicesAsync(taken)); + try { await teardown.WaitAsync(ShutdownBudget).ConfigureAwait(false); } + catch (TimeoutException) { - try { state.ProbeCts?.Cancel(); } catch { } - if (state.ProbeTask is Task pt) - { - try { await pt.ConfigureAwait(false); } - catch (OperationCanceledException) { /* expected — probe loop exits on cancel */ } - catch { /* other probe errors are not fatal to shutdown */ } - } - state.ProbeCts?.Dispose(); - state.ProbeCts = null; - state.DisposeClient(); - state.DisposeGate(); + _logger.LogWarning( + "TwinCAT driver '{DriverInstanceId}' shutdown exceeded {Budget}; the remaining teardown continues in the background", + _driverInstanceId, ShutdownBudget); } - _devices.Clear(); - _tagsByRawPath.Clear(); + catch { /* best-effort teardown */ } + + _warnedRefs.Clear(); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - // Dispose + clear the per-parent-word RMW gates so ReinitializeAsync cycles don't orphan - // their SemaphoreSlim instances (each leaks a wait handle once contended). - foreach (var sem in _bitRmwLocks.Values) sem.Dispose(); _bitRmwLocks.Clear(); _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); } @@ -340,7 +446,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery catch (OperationCanceledException) { throw; } catch (Exception ex) { - MarkCommsFault(ex.Message); + MarkCommsFault(ex.Message, device); return new DataValueSnapshot(null, TwinCATStatusMapper.BadCommunicationError, null, now); } } @@ -463,7 +569,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { foreach (var e in chunk) results[e.Index] = new DataValueSnapshot(null, TwinCATStatusMapper.BadCommunicationError, null, now); - MarkCommsFault(ex.Message); + MarkCommsFault(ex.Message, device); // Once per device: the health message is overwritten by the next good read in the same poll, so // without this a sum-read path failing on every poll left no trace (hw2 live gate, 2026-10-08). if (connected && !device.SumChunkFailureWarned) @@ -575,7 +681,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery catch (Exception ex) { results[i] = new WriteResult(TwinCATStatusMapper.BadCommunicationError); - MarkCommsFault(ex.Message); + MarkCommsFault(ex.Message, device); } } @@ -771,7 +877,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // The device could not be connected (offline, router down, backoff window open). // Every tag of this device goes Bad + dead for the retry; other devices carry on. (unreachable ??= []).Add(device); - MarkCommsFault(unavailable.InnerException?.Message ?? unavailable.Message); + MarkCommsFault(unavailable.InnerException?.Message ?? unavailable.Message, device); RaiseDataChange(handle, reference, new DataValueSnapshot( null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); } @@ -783,7 +889,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery "TwinCAT driver '{DriverInstanceId}' native-notification registration failed for " + "'{Reference}'; it will be retried on the next healthy probe tick", _driverInstanceId, reference); - MarkCommsFault(ex.Message); + MarkCommsFault(ex.Message, device); RaiseDataChange(handle, reference, new DataValueSnapshot( null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); } @@ -801,6 +907,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery return handle; } + /// + /// The device state was released (re-initialise / teardown) while a connect on it ran. Not an + /// : the caller's token was not cancelled, so the data path maps + /// it like any failed request (Bad for this call) and ignores it. + /// + private sealed class DeviceReleasedException(string host) + : Exception($"TwinCAT device {host} was released during connect."); + /// The connect step of a native registration failed — the whole device is unreachable. private sealed class DeviceUnavailableException(Exception inner) : Exception("TwinCAT device unreachable: " + inner.Message, inner); @@ -809,8 +923,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// Records a comms fault: preserving LastSuccessfulRead. /// Never downgrades . /// - private void MarkCommsFault(string message) + /// The fault description. + /// The device the fault was observed on; a fault on a device already swapped out of + /// the table (a read/write racing a re-initialise or teardown) belongs to a dead generation and is + /// not recorded. + private void MarkCommsFault(string message, DeviceState? device = null) { + if (device?.Released == true) return; var current = _health; if (current.State != DriverState.Faulted) _health = new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, message); @@ -1006,8 +1125,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery BitIndex = bitIndex; ArrayLength = arrayLength; Interval = interval; - OnChange = onChange; - OnUnavailableStatus = onUnavailable; + // A notification already queued in the client's dispatch channel can be delivered after this + // registration is disposed (unsubscribe / teardown): drop it here, for every client. + OnChange = (symbol, value) => { if (!IsDisposed) onChange(symbol, value); }; + OnUnavailableStatus = status => { if (!IsDisposed) onUnavailable(status); }; } /// Installs a freshly-registered handle and returns the previous one (null on first set). @@ -1025,9 +1146,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public ITwinCATNotificationHandle? MarkHandleDead() => Interlocked.Exchange(ref _handle, null); + private volatile bool _disposed; + + /// Gets a value indicating whether this registration has been disposed (its callbacks are dropped). + public bool IsDisposed => _disposed; + /// Removes this registration from its device and disposes its live handle. public void Dispose() { + _disposed = true; Device.NativeRegistrations.TryRemove(Id, out _); var h = Interlocked.Exchange(ref _handle, null); try { h?.Dispose(); } catch { /* best-effort */ } @@ -1085,6 +1212,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { state.ConsecutiveProbeFailures = 0; RecordDeviceAnswered(DateTime.UtcNow, force: true); + if (probed is not null) ReportNotificationDrops(state, probed); } else if (++state.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded) { @@ -1107,6 +1235,24 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } } + /// + /// Logs a Warning when the client's native-notification queue dropped values since the last probe + /// tick (the queue is bounded and drops on overflow — silently, before #739 L8). At most one + /// Warning per probe interval, carrying the delta and the total. + /// + private void ReportNotificationDrops(DeviceState state, ITwinCATClient client) + { + var total = client.DroppedNotificationCount; + var delta = total - state.ReportedNotificationDrops; + if (delta <= 0) return; + state.ReportedNotificationDrops = total; + _logger.LogWarning( + "TwinCAT driver '{DriverInstanceId}' device '{HostAddress}' dropped {Dropped} native notification(s) " + + "since the last probe ({Total} on this connection): the notification queue was full — the consumer " + + "is not keeping up with the PLC's change rate", + _driverInstanceId, state.Options.HostAddress, delta, total); + } + // ---- IDriverDiagnostics ---- // Well-known system symbols, exported by TC3 (and most TC2) PLC runtimes under TwinCAT_SystemInfoVarList. @@ -1296,11 +1442,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// CLI, pass the host string as the DeviceName). An empty DeviceName routes to the sole device /// on a single-device instance; an unmatched name leaves the def unrouted (→ BadNodeIdUnknown). /// + /// The options generation being initialised. /// The owning device's name (or host) from the raw-tag entry. /// The resolved device host address, or empty when unresolved. - private string ResolveDeviceHost(string deviceName) + private static string ResolveDeviceHost(TwinCATDriverOptions options, string deviceName) { - var devices = _options.Devices; + var devices = options.Devices; if (string.IsNullOrEmpty(deviceName)) return devices.Count == 1 ? devices[0].HostAddress : ""; foreach (var d in devices) @@ -1372,6 +1519,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { // Re-check under the gate: another caller may have connected while we waited. if (device.Client is { IsConnected: true } c) return c; + // A released device (swapped out by a re-initialise / teardown) never connects again. + if (device.Released) throw new DeviceReleasedException(device.Options.HostAddress); // Inside an open backoff window a data-path caller fails fast — no client // create, no wire connect. The probe loop bypasses this (its interval is the recovery @@ -1412,7 +1561,16 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery device.Backoff.RecordFailure(DateTime.UtcNow); // open the backoff window throw; } + if (device.Released) + { + // This device was torn down while the (blocking) connect ran: never install a client on a + // released generation, where nothing would ever dispose it. + client.OnSymbolVersionChanged -= HandleSymbolVersionChanged; + try { client.Dispose(); } catch { /* best-effort */ } + throw new DeviceReleasedException(device.Options.HostAddress); + } device.Client = client; + device.ReportedNotificationDrops = 0; device.UnavailableDiagSymbols.Clear(); // a rebuilt client may face a PLC that now exports them device.Backoff.RecordSuccess(); // recovery resets the window immediately @@ -1608,29 +1766,17 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery 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 { } + var taken = TakeAllForTeardown(); + var probes = BeginRelease(taken); - 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(); - _tagsByRawPath.Clear(); + // PollGroupEngine.DisposeAsync awaits loop tasks; drive it synchronously with a bounded Wait() on + // the returned task (no sync-context capture), in parallel with the probe join. + var poll = _poll.DisposeAsync().AsTask(); + try { Task.WhenAll([poll, .. probes]).Wait(ProbeJoinTimeout); } catch { /* cancel races are expected */ } + EndRelease(taken); + + _warnedRefs.Clear(); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - // Dispose + clear the per-parent-word RMW gates (mirrors ShutdownAsync) so the - // SemaphoreSlim instances aren't orphaned on disposal. - foreach (var sem in _bitRmwLocks.Values) sem.Dispose(); _bitRmwLocks.Clear(); _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); } @@ -1711,7 +1857,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery UnavailableDiagSymbols.Clear(); } - /// Disposes the connection gate semaphore. - public void DisposeGate() => ConnectGate.Dispose(); + /// + /// Set once this device state has been swapped out of the driver's table. A connect that + /// completes afterwards (a probe wedged in the blocking ADS Connect past the bounded join) + /// disposes its client instead of installing it on a dead generation. + /// + public volatile bool Released; + + /// Total notification drops last reported for this device's current client. + public long ReportedNotificationDrops; } } 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 39f795be..909fc31f 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 @@ -56,10 +56,22 @@ internal class FakeTwinCATClient : ITwinCATClient { ConnectCount++; if (ThrowOnConnect) throw Exception ?? new InvalidOperationException(); + if (BlockConnect is { } block) + { + // Models the real client's blocking SDK Connect: ignores the token until released. + return block.Task.ContinueWith(_ => { IsConnected = true; }, TaskScheduler.Default); + } IsConnected = true; return Task.CompletedTask; } + /// When set, does not complete (and ignores cancellation) until this + /// is completed — a probe wedged in the synchronous ADS Connect. + public TaskCompletionSource? BlockConnect { get; set; } + + /// Gets or sets the simulated notification-queue drop count. + public long DroppedNotificationCount { get; set; } + /// Simulates reading a value from a symbol. /// The path to the symbol to read. /// The data type of the symbol. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs new file mode 100644 index 00000000..c419289b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs @@ -0,0 +1,271 @@ +using System.Collections.Concurrent; +using System.Diagnostics; +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; + +/// +/// #739 L8 + the Lane A runtime contract (final stop = ShutdownAsync then DisposeAsync, +/// bounded 5 s + 250 ms): a probe wedged in the blocking ADS connect cannot hang teardown; dispose may run +/// while a shutdown is still running; an abandoned initialise overlapping the next shutdown never installs +/// its generation; a value queued before unsubscribe is not delivered after it; re-init swaps its tables +/// and retracts the previous generation's notifications; queue drops are reported. +/// +[Trait("Category", "Unit")] +public sealed class TwinCATLifecycleTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, bool probe = true, bool native = true, + TwinCATCapturingLogger? log = null, params TwinCATTagDefinition[] tags) => + new(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries(tags.Length > 0 + ? tags + : [new TwinCATTagDefinition("Speed", Host, "MAIN.Speed", TwinCATDataType.DInt)]), + Probe = probe + ? new TwinCATProbeOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(30), Timeout = TimeSpan.FromMilliseconds(100) } + : new TwinCATProbeOptions { Enabled = false }, + UseNativeNotifications = native, + }, "drv-1", factory, log); + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(10); + } + + /// + /// A probe wedged in the (uncancellable) connect no longer hangs ShutdownAsync — the join is bounded — + /// and the client the wedged connect eventually produces is disposed, not installed on a dead device. + /// + [Fact] + public async Task Shutdown_is_bounded_when_a_probe_is_wedged_in_connect() + { + var wedge = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeTwinCATClientFactory { Customise = () => new FakeTwinCATClient { BlockConnect = wedge } }; + var drv = NewDriver(factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); + factory.Clients.Count.ShouldBe(1); // the probe is inside the wedged connect + + var sw = Stopwatch.StartNew(); + await drv.ShutdownAsync(TestContext.Current.CancellationToken).WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ShutdownBudget + TimeSpan.FromMilliseconds(500)); + + wedge.SetResult(); + await WaitForAsync(() => factory.Clients[0].DisposeCount == 1, TimeSpan.FromSeconds(2)); + factory.Clients[0].DisposeCount.ShouldBe(1); + drv.DeviceCount.ShouldBe(0); + } + + /// + /// DisposeAsync while a ShutdownAsync is still running (joining a wedged probe) returns promptly and + /// both complete without throwing; the device is released exactly once. + /// + [Fact] + public async Task Dispose_concurrent_with_a_running_shutdown_completes_promptly() + { + var wedge = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeTwinCATClientFactory { Customise = () => new FakeTwinCATClient { BlockConnect = wedge } }; + var drv = NewDriver(factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + var sw = Stopwatch.StartNew(); + await drv.DisposeAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(250)); + shutdown.IsCompleted.ShouldBeFalse("the positive control: the first shutdown really was still joining the probe"); + + wedge.SetResult(); + await shutdown.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + drv.Dispose(); // and a late sync Dispose is a no-op + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + } + + /// + /// An InitializeAsync overtaken by a ShutdownAsync before it commits must not install its devices or + /// start probe loops when it resumes. Positive control: a fresh initialise afterwards installs normally. + /// + [Fact] + public async Task An_initialise_overtaken_by_shutdown_before_commit_installs_nothing() + { + var factory = new FakeTwinCATClientFactory(); + var drv = NewDriver(factory); + var paused = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var resume = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + drv.BeforeCommitForTest = async () => { paused.TrySetResult(); await resume.Task; }; + + var abandoned = drv.InitializeAsync("{}", CancellationToken.None); + await paused.Task.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + resume.SetResult(); + + await Should.ThrowAsync(() => abandoned.WaitAsync(TimeSpan.FromSeconds(5))); + drv.DeviceCount.ShouldBe(0); + await Task.Delay(150, TestContext.Current.CancellationToken); // several probe intervals + factory.Clients.ShouldBeEmpty("no probe loop of the overtaken generation may be running"); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + + drv.BeforeCommitForTest = null; + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + drv.DeviceCount.ShouldBe(1); + await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); + factory.Clients.Count.ShouldBe(1); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// + /// The Lane A shape: an InitializeAsync the runtime abandoned while it is still releasing the previous + /// generation (joining a probe wedged in connect) overlaps the next ShutdownAsync. Shutdown stays + /// bounded and, once everything settles, no device, probe loop or live client survives. + /// + [Fact] + public async Task An_abandoned_initialise_overlapping_shutdown_leaves_nothing_running() + { + var wedge = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var first = true; + var factory = new FakeTwinCATClientFactory + { + Customise = () => + { + var c = new FakeTwinCATClient { BlockConnect = first ? wedge : null }; + first = false; + return c; + }, + }; + var drv = NewDriver(factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); + + var abandoned = drv.InitializeAsync("{}", CancellationToken.None); // joins the wedged generation-1 probe + await Task.Delay(50, TestContext.Current.CancellationToken); + var sw = Stopwatch.StartNew(); + await drv.ShutdownAsync(TestContext.Current.CancellationToken).WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ShutdownBudget + TimeSpan.FromMilliseconds(500)); + + wedge.SetResult(); + try { await abandoned.WaitAsync(TimeSpan.FromSeconds(5)); } catch (OperationCanceledException) { } + + drv.DeviceCount.ShouldBe(0); + await WaitForAsync(() => factory.Clients.All(c => c.DisposeCount > 0 || !c.IsConnected), TimeSpan.FromSeconds(2)); + var settled = factory.Clients.Count; + await Task.Delay(150, TestContext.Current.CancellationToken); + factory.Clients.Count.ShouldBe(settled, "no probe loop may still be creating clients"); + factory.Clients.ShouldAllBe(c => c.DisposeCount > 0 || !c.IsConnected); + } + + /// + /// A value the client queued before the unsubscribe is dropped if dispatched after it. + /// Positive control: the same callback delivers while subscribed. + /// + [Fact] + public async Task A_notification_dispatched_after_unsubscribe_is_dropped() + { + var factory = new FakeTwinCATClientFactory(); + var drv = NewDriver(factory, probe: false); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + var handle = await drv.SubscribeAsync(["Speed"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + var callback = factory.Clients[0].Notifications.Single().OnChange; // what the client's queue holds + + callback("MAIN.Speed", 1); + events.Count.ShouldBe(1); + + await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + callback("MAIN.Speed", 2); + events.Count.ShouldBe(1); + } + + /// + /// A direct re-initialise with a changed config swaps the tag table (the removed tag is unknown, the + /// added one resolves) and retracts the previous generation's live notifications. + /// + [Fact] + public async Task Reinitialise_swaps_tables_and_retracts_the_previous_generations_notifications() + { + var factory = new FakeTwinCATClientFactory(); + var drv = NewDriver(factory, probe: false); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.SubscribeAsync(["Speed"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + factory.Clients[0].Notifications.Count.ShouldBe(1); + + var next = System.Text.Json.JsonSerializer.Serialize(new + { + devices = new[] { new { hostAddress = Host } }, + rawTags = new[] + { + new + { + rawPath = "Torque", + tagConfig = TwinCATTagDefinitionFactory.ToTagConfig( + new TwinCATTagDefinition("Torque", Host, "MAIN.Torque", TwinCATDataType.DInt)), + writeIdempotent = false, + deviceName = Host, + }, + }, + probe = new { enabled = false }, + }); + await drv.InitializeAsync(next, TestContext.Current.CancellationToken); + + factory.Clients[0].Notifications.ShouldBeEmpty(); + factory.Clients[0].DisposeCount.ShouldBe(1); + (await drv.ReadAsync(["Speed"], TestContext.Current.CancellationToken)).Single().StatusCode + .ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + (await drv.ReadAsync(["Torque"], TestContext.Current.CancellationToken)).Single().StatusCode + .ShouldBe(TwinCATStatusMapper.Good); + } + + /// Reads racing a stream of re-initialises never throw (no table is ever mid-Clear). + [Fact] + public async Task Reads_racing_reinitialise_never_throw() + { + var factory = new FakeTwinCATClientFactory(); + var drv = NewDriver(factory, probe: false); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + using var stop = new CancellationTokenSource(TimeSpan.FromMilliseconds(400)); + var good = 0; + var reader = Task.Run(async () => + { + while (!stop.IsCancellationRequested) + { + var r = await drv.ReadAsync(["Speed"], CancellationToken.None); + if (r[0].StatusCode == TwinCATStatusMapper.Good) Interlocked.Increment(ref good); + } + }, TestContext.Current.CancellationToken); + while (!stop.IsCancellationRequested) + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + await reader; // throws if any read threw + good.ShouldBeGreaterThan(0); + } + + /// Notification-queue drops are reported once per probe tick that sees new drops. + [Fact] + public async Task Notification_queue_drops_are_reported() + { + var fake = new FakeTwinCATClient(); + var log = new TwinCATCapturingLogger(); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, log: log); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + await WaitForAsync(() => fake.IsConnected, TimeSpan.FromSeconds(2)); + await Task.Delay(100, TestContext.Current.CancellationToken); + log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(0); // no drops, no warning + + fake.DroppedNotificationCount = 5; + await WaitForAsync(() => log.Warnings.Any(w => w.Contains("dropped 5")), TimeSpan.FromSeconds(2)); + await Task.Delay(100, TestContext.Current.CancellationToken); + log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(1); + + fake.DroppedNotificationCount = 8; + await WaitForAsync(() => log.Warnings.Any(w => w.Contains("dropped 3")), TimeSpan.FromSeconds(2)); + log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(2); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } +} From 9beeed7a7649956b793e9015623348bac0c1ba35 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:25:20 -0400 Subject: [PATCH 08/12] feat(twincat): implement IBatchSubscribable (R02:P1) The poll engine already builds a per-cycle batch; TwinCAT now forwards it as OnDataChangeBatch, so DriverInstanceActor gets one message per poll cycle instead of one per changed tag. Because the actor subscribes ONLY the batch event of a batch-capable driver, every native-path data change (pushes, the BadNodeIdUnknown / refusal / unavailable snapshots) is raised through a single RaiseDataChange that fires both the per-tag event and a fresh one-item batch. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../TwinCATDriver.cs | 19 +++- .../TwinCATBatchSubscribeTests.cs | 89 +++++++++++++++++++ 2 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBatchSubscribeTests.cs 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 8542bcad..fc0469ae 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -11,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; /// the skeleton; read / write / discover / subscribe / probe / host- /// resolver land in PRs 2 and 3. /// -public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery, ISubscribable, +public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery, IBatchSubscribable, IHostConnectivityProbe, IPerCallHostResolver, IRediscoverable, ISkippedTagReporter, IDriverDiagnostics, IDisposable, IAsyncDisposable { @@ -88,6 +88,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// Occurs when a subscribed tag value changes. public event EventHandler? OnDataChange; + /// + /// Batched data changes (R02:P1): one raise per poll cycle on the poll path, one per notification on + /// the native path (each ADS notification is its own burst). DriverInstanceActor subscribes only this + /// event of a batch-capable driver, so EVERY data change the driver publishes goes through both. + /// + public event EventHandler? OnDataChangeBatch; /// Occurs when a device host connectivity status changes. public event EventHandler? OnHostStatusChanged; /// Occurs when the Galaxy object hierarchy or TwinCAT symbol table is rediscovered. @@ -115,6 +121,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), onError: HandlePollError, backoffCap: PollBackoffCap, + onBatch: (handle, items) => OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items)), // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this // closure IS the production consumer of its overrun signal. onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, @@ -972,9 +979,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery : $"TwinCAT device {unreachable.Options.HostAddress} unreachable ({unreachable.ConsecutiveProbeFailures} consecutive failed probes)"); } - /// Raises one data change for a native subscription. - private void RaiseDataChange(ISubscriptionHandle handle, string reference, DataValueSnapshot snapshot) => + /// + /// Raises one data change for a native subscription on BOTH events: the per-tag one (legacy consumers) + /// and a one-item batch (a fresh list per raise — the consumer may retain it). + /// + private void RaiseDataChange(ISubscriptionHandle handle, string reference, DataValueSnapshot snapshot) + { OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, reference, snapshot)); + OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, [new DataChangeItem(reference, snapshot)])); + } /// /// Registers one native notification and records its replayable intent on the device — both diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBatchSubscribeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBatchSubscribeTests.cs new file mode 100644 index 00000000..29148186 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATBatchSubscribeTests.cs @@ -0,0 +1,89 @@ +using System.Collections.Concurrent; +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; + +/// +/// #739 / R02:P1 — TwinCAT is an . DriverInstanceActor subscribes ONLY the +/// batch event of a batch-capable driver, so every data change — poll cycle AND native push, including the +/// Bad snapshots the native path publishes itself — must reach OnDataChangeBatch; the per-tag event +/// keeps firing for legacy consumers (the CLI). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATBatchSubscribeTests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, bool native) => + new(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries( + [ + new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt), + new TwinCATTagDefinition("B", Host, "MAIN.B", TwinCATDataType.DInt), + ]), + Probe = new TwinCATProbeOptions { Enabled = false }, + UseNativeNotifications = native, + }, "drv-1", factory); + + /// The driver advertises the batch capability. + [Fact] + public void Driver_is_batch_subscribable() => + NewDriver(new FakeTwinCATClientFactory(), native: false).ShouldBeAssignableTo(); + + /// A poll cycle that changes two tags raises ONE batch carrying both, and the per-tag event twice. + [Fact] + public async Task A_poll_cycle_raises_one_batch_with_every_changed_tag() + { + var factory = new FakeTwinCATClientFactory + { + Customise = () => new FakeTwinCATClient { Values = { ["MAIN.A"] = 1, ["MAIN.B"] = 2 } }, + }; + var drv = NewDriver(factory, native: false); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var batches = new ConcurrentQueue(); + var single = new ConcurrentQueue(); + drv.OnDataChangeBatch += (_, e) => batches.Enqueue(e); + drv.OnDataChange += (_, e) => single.Enqueue(e); + + var handle = await drv.SubscribeAsync(["A", "B"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + await WaitForAsync(() => !batches.IsEmpty, TimeSpan.FromSeconds(3)); + await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + + var first = batches.First(); + first.SubscriptionHandle.ShouldBe(handle); + first.Items.Select(i => i.FullReference).ShouldBe(["A", "B"], ignoreOrder: true); + single.Count(e => e.FullReference is "A" or "B").ShouldBeGreaterThanOrEqualTo(2); + } + + /// A native push and the native path's own Bad snapshot both reach the batch event. + [Fact] + public async Task Native_pushes_and_native_bad_snapshots_reach_the_batch_event() + { + var fake = new FakeTwinCATClient(); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, native: true); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var batches = new ConcurrentQueue(); + drv.OnDataChangeBatch += (_, e) => batches.Enqueue(e); + + _ = await drv.SubscribeAsync(["A", "Ghost"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + fake.FireNotification("MAIN.A", 42); + + var items = batches.SelectMany(b => b.Items).ToList(); + items.Single(i => i.FullReference == "Ghost").Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + var a = items.Single(i => i.FullReference == "A"); + a.Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.Good); + a.Snapshot.Value.ShouldBe(42); + } + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(20); + } +} From 6201bafea4714d92f2926719de3e6871421d280c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:28:53 -0400 Subject: [PATCH 09/12] test(twincat): pass the test token to the abandoned-init wait Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../TwinCATLifecycleTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs index c419289b..222495fd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs @@ -151,7 +151,8 @@ public sealed class TwinCATLifecycleTests sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ShutdownBudget + TimeSpan.FromMilliseconds(500)); wedge.SetResult(); - try { await abandoned.WaitAsync(TimeSpan.FromSeconds(5)); } catch (OperationCanceledException) { } + try { await abandoned.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); } + catch (OperationCanceledException) when (!TestContext.Current.CancellationToken.IsCancellationRequested) { } drv.DeviceCount.ShouldBe(0); await WaitForAsync(() => factory.Clients.All(c => c.DisposeCount > 0 || !c.IsConnected), TimeSpan.FromSeconds(2)); From 8de5947e2d48d42799ac6b610bf9eb47817225c9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:30:05 -0400 Subject: [PATCH 10/12] test(twincat): XAR-gated wire cases for H11/H12/H18/U9 Native-subscribe isolation of an unknown symbol, LastSuccessfulRead advance from pushes alone, a WORD bit write flipping exactly one bit (bit 16 refused), a whole-array REAL write round-trip (length mismatch refused) and a STRING(80) write round-trip (overflow refused). Every writing case restores the seed it touched. Skipped without the TwinCAT XAR fixture. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../TwinCATRemediationGateTests.cs | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATRemediationGateTests.cs diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATRemediationGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATRemediationGateTests.cs new file mode 100644 index 00000000..c592495b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests/TwinCATRemediationGateTests.cs @@ -0,0 +1,146 @@ +using System.Collections.Concurrent; +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.IntegrationTests; + +/// +/// Wire-level cases for the #739 High/Medium findings, against the live TwinCAT 3 XAR fixture +/// (skipped without it, like ). Every case that writes restores what it +/// changed: GVL_Primitives.vWord and vString are seeds the primitives theory reads. +/// +[Collection("TwinCATXar")] +[Trait("Category", "Integration")] +[Trait("Simulator", "TwinCAT-XAR")] +public sealed class TwinCATRemediationGateTests(TwinCATXarFixture sim) +{ + private string Host => $"ads://{sim.TargetNetId}:{sim.AmsPort}"; + + private TwinCATDriver NewDriver(string id, bool native, params TwinCATTagDefinition[] tags) => new(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host, "XAR-VM")], + RawTags = TwinCATRawTags.Entries(tags), + UseNativeNotifications = native, + Timeout = TimeSpan.FromSeconds(5), + Probe = new TwinCATProbeOptions { Enabled = false }, + }, id); + + /// H11: a typo'd symbol in a native subscribe is isolated; the real counter keeps delivering. + [TwinCATFact] + public async Task Native_subscribe_isolates_an_unknown_symbol() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + await using var drv = NewDriver("tc3-gate-h11", native: true, + new TwinCATTagDefinition("Counter", Host, "GVL_Fixture.nCounter", TwinCATDataType.DInt), + new TwinCATTagDefinition("Typo", Host, "GVL_Fixture.nCounterTypo", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + var handle = await drv.SubscribeAsync(["Counter", "Typo"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (events.Count(e => e.FullReference == "Counter") < 2 && DateTime.UtcNow < deadline) + await Task.Delay(50, TestContext.Current.CancellationToken); + await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + + events.Count(e => e.FullReference == "Counter" && e.Snapshot.StatusCode == 0u).ShouldBeGreaterThanOrEqualTo(2); + events.First(e => e.FullReference == "Typo").Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refused symbol is not a comms fault"); + } + + /// H12: native pushes alone advance LastSuccessfulRead. + [TwinCATFact] + public async Task Native_pushes_advance_LastSuccessfulRead() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + await using var drv = NewDriver("tc3-gate-h12", native: true, + new TwinCATTagDefinition("Counter", Host, "GVL_Fixture.nCounter", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead!.Value; + + var handle = await drv.SubscribeAsync(["Counter"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + await Task.Delay(1500, TestContext.Current.CancellationToken); + await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp.AddMilliseconds(500)); + } + + /// + /// H18: a bit write on the 16-bit vWord (0xBEEF) flips exactly that bit at the WORD's own width + /// (the old 32-bit UDInt RMW could not even address a WORD), and a bit beyond it is refused. + /// + [TwinCATFact] + public async Task Bit_write_on_a_WORD_flips_one_bit_and_refuses_bit_16() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + await using var drv = NewDriver("tc3-gate-h18", native: false, + new TwinCATTagDefinition("Word", Host, "GVL_Primitives.vWord", TwinCATDataType.UInt, Writable: false), + new TwinCATTagDefinition("Bit4", Host, "GVL_Primitives.vWord.4", TwinCATDataType.Bool), + new TwinCATTagDefinition("Bit16", Host, "GVL_Primitives.vWord.16", TwinCATDataType.Bool)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var before = Convert.ToUInt16((await drv.ReadAsync(["Word"], TestContext.Current.CancellationToken))[0].Value); + var bit4WasSet = (before & 0x10) != 0; + try + { + (await drv.WriteAsync([new WriteRequest("Bit4", !bit4WasSet)], TestContext.Current.CancellationToken))[0] + .StatusCode.ShouldBe(0u); + var after = Convert.ToUInt16((await drv.ReadAsync(["Word"], TestContext.Current.CancellationToken))[0].Value); + after.ShouldBe((ushort)(before ^ 0x10), "only bit 4 may change"); + + (await drv.WriteAsync([new WriteRequest("Bit16", true)], TestContext.Current.CancellationToken))[0] + .StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); + } + finally + { + await drv.WriteAsync([new WriteRequest("Bit4", bit4WasSet)], CancellationToken.None); + } + Convert.ToUInt16((await drv.ReadAsync(["Word"], TestContext.Current.CancellationToken))[0].Value).ShouldBe(before); + } + + /// U9: a whole-array write round-trips through the codec (CLR double[] converts to REAL elements). + [TwinCATFact] + public async Task Whole_array_write_round_trips() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + await using var drv = NewDriver("tc3-gate-u9-array", native: false, + new TwinCATTagDefinition("Arr", Host, "GVL_Arrays.aReal1D", TwinCATDataType.Real, ArrayLength: 32)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var values = Enumerable.Range(0, 32).Select(i => i * 1.5 + 0.25).ToArray(); + + (await drv.WriteAsync([new WriteRequest("Arr", values)], TestContext.Current.CancellationToken))[0] + .StatusCode.ShouldBe(0u); + var read = (await drv.ReadAsync(["Arr"], TestContext.Current.CancellationToken))[0]; + read.StatusCode.ShouldBe(0u); + ((float[])read.Value!).ShouldBe(values.Select(v => (float)v).ToArray()); + + (await drv.WriteAsync([new WriteRequest("Arr", values[..31])], TestContext.Current.CancellationToken))[0] + .StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); + } + + /// U9: a STRING write is NUL-padded at the PLC width (no stale tail) and an over-long one is refused. + [TwinCATFact] + public async Task String_write_round_trips_and_refuses_overflow() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + await using var drv = NewDriver("tc3-gate-u9-string", native: false, + new TwinCATTagDefinition("S", Host, "GVL_Primitives.vString", TwinCATDataType.String)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var original = (string)(await drv.ReadAsync(["S"], TestContext.Current.CancellationToken))[0].Value!; + try + { + (await drv.WriteAsync([new WriteRequest("S", "Grüße 40°")], TestContext.Current.CancellationToken))[0] + .StatusCode.ShouldBe(0u); + (await drv.ReadAsync(["S"], TestContext.Current.CancellationToken))[0].Value.ShouldBe("Grüße 40°"); + + (await drv.WriteAsync([new WriteRequest("S", new string('x', 81))], TestContext.Current.CancellationToken))[0] + .StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange, "vString is STRING(80)"); + } + finally + { + await drv.WriteAsync([new WriteRequest("S", original)], CancellationToken.None); + } + (await drv.ReadAsync(["S"], TestContext.Current.CancellationToken))[0].Value.ShouldBe(original); + } +} From f179829ca6ccf3a99428fd9eb59df32c7b32f331 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:47:24 -0400 Subject: [PATCH 11/12] fix(twincat): review round 1 - PLC-not-in-RUN degrades, per-device health, retry backoff - IMPORTANT: a successful probe records the PLC's ADS state; a PLC not in Run keeps the driver Degraded with 'PLC in {state}' while LastSuccessfulRead still advances (the device answered). RecordDeviceAnswered never restores Healthy while any device is out of Run. - Health is per device, rolled up worst-of: a comms fault is remembered on the device it was seen on and only that device answering clears it (device A answering no longer clears device B's fault). - Refused (non-comms) dead registrations back off 30 s doubling to 10 min and are re-armed by a symbol-version change or retried by the reconnect replay; comms-class failures keep the every-tick retry. A retry whose failure class changes republishes the tag's status, once. - Bit-RMW gates are kept across generations and teardowns (never cleared). - ShutdownAsync honours its CancellationToken: it stops waiting, finishes the bookkeeping and throws OperationCanceledException. - Remarks on why a superseded InitializeAsync throws OperationCanceledException. - REAL / LREAL values (scalar or array element) outside the target range are OverflowException -> BadOutOfRange instead of being written as Infinity. - String / array write encoding factored into AdsTwinCATClient.PlanStringWrite / PlanArrayWrite; the fake runs them so a driver-level test covers the real encode path against a fake resolved symbol. - Timing-fragile tests use condition waits / measured probe-tick windows; the dispose-during-shutdown test asserts ordering with a 1 s bound. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AdsTwinCATClient.cs | 83 ++++- .../TwinCATDriver.cs | 184 +++++++++-- .../FakeTwinCATClient.cs | 40 ++- .../TwinCATHealthContractTests.cs | 4 +- .../TwinCATLifecycleTests.cs | 56 +++- .../TwinCATReadWarningTests.cs | 3 +- .../TwinCATReviewFixRound1Tests.cs | 294 ++++++++++++++++++ 7 files changed, 608 insertions(+), 56 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.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 a779b09b..9eecbd34 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -478,11 +478,8 @@ internal sealed class AdsTwinCATClient : ITwinCATClient var (shape, _, symbol, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken) .ConfigureAwait(false); if (symbolStatus != TwinCATStatusMapper.Good) return symbolStatus; - if (TwinCATTypeGuard.CheckRead(type, shape) is not null) return TwinCATStatusMapper.BadTypeMismatch; - var text = Convert.ToString(value, System.Globalization.CultureInfo.InvariantCulture) ?? string.Empty; - var encoded = type == TwinCATDataType.WString - ? TwinCATStringCodec.EncodeWString(text, shape.ByteSize) - : TwinCATStringCodec.EncodeString(text, shape.ByteSize, StringsAreUtf8()); + var (planStatus, encoded) = PlanStringWrite(type, shape, value, StringsAreUtf8()); + if (planStatus != TwinCATStatusMapper.Good) return planStatus; return await WriteRawAsync(symbolPath, symbol!, encoded, cancellationToken).ConfigureAwait(false); } @@ -499,6 +496,47 @@ internal sealed class AdsTwinCATClient : ITwinCATClient } } + /// + /// The pure encode plan of a STRING / WSTRING write against the RESOLVED symbol: the declared kind must + /// match the PLC's (, else BadTypeMismatch), then the value is + /// encoded at the PLC's byte width by . + /// + /// The declared String / WString type. + /// The resolved PLC symbol shape. + /// The written value. + /// True when the target reports UTF-8 STRING data. + /// Good with the bytes to write, or the refusal status with no bytes. + /// The value does not fit the PLC string. + internal static (uint status, byte[] bytes) PlanStringWrite( + TwinCATDataType type, TwinCATPlcSymbolShape shape, object? value, bool utf8) + { + if (TwinCATTypeGuard.CheckRead(type, shape) is not null) return (TwinCATStatusMapper.BadTypeMismatch, []); + var text = Convert.ToString(value, System.Globalization.CultureInfo.InvariantCulture) ?? string.Empty; + return (TwinCATStatusMapper.Good, type == TwinCATDataType.WString + ? TwinCATStringCodec.EncodeWString(text, shape.ByteSize) + : TwinCATStringCodec.EncodeString(text, shape.ByteSize, utf8)); + } + + /// + /// The pure encode plan of an array write against the RESOLVED PLC array: the layout comes from + /// (the array read's rule, else BadTypeMismatch) and the + /// bytes from . + /// + /// The declared element type. + /// The converted elements. + /// The resolved PLC symbol shape. + /// The resolved PLC array shape, or null for a non-array symbol. + /// True when the target reports UTF-8 STRING data. + /// Good with the bytes to write, or the refusal status with no bytes. + /// A string element does not fit its stride. + internal static (uint status, byte[] bytes) PlanArrayWrite(TwinCATDataType type, IReadOnlyList elements, + TwinCATPlcSymbolShape shape, TwinCATPlcArrayShape? array, bool utf8) + { + if (TwinCATTypeGuard.CheckArray(type, elements.Count, shape, array, out var layout) is not null) + return (TwinCATStatusMapper.BadTypeMismatch, []); + return (TwinCATStatusMapper.Good, TwinCATArrayCodec.Encode(type, elements, layout, utf8)); + } + /// public async Task WriteArrayAsync(string symbolPath, TwinCATDataType type, IReadOnlyList elements, CancellationToken cancellationToken) @@ -510,9 +548,8 @@ internal sealed class AdsTwinCATClient : ITwinCATClient var (shape, array, symbol, symbolStatus) = await DescribeSymbolAsync(symbolPath, cancellationToken) .ConfigureAwait(false); if (symbolStatus != TwinCATStatusMapper.Good) return symbolStatus; - if (TwinCATTypeGuard.CheckArray(type, elements.Count, shape, array, out var layout) is not null) - return TwinCATStatusMapper.BadTypeMismatch; - var bytes = TwinCATArrayCodec.Encode(type, elements, layout, StringsAreUtf8()); + var (planStatus, bytes) = PlanArrayWrite(type, elements, shape, array, StringsAreUtf8()); + if (planStatus != TwinCATStatusMapper.Good) return planStatus; return await WriteRawAsync(symbolPath, symbol!, bytes, cancellationToken).ConfigureAwait(false); } catch (AdsErrorException ex) @@ -1034,14 +1071,40 @@ internal sealed class AdsTwinCATClient : ITwinCATClient TwinCATDataType.UDInt => Convert.ToUInt32(value), TwinCATDataType.LInt => Convert.ToInt64(value), TwinCATDataType.ULInt => Convert.ToUInt64(value), - TwinCATDataType.Real => Convert.ToSingle(value), - TwinCATDataType.LReal => Convert.ToDouble(value), + TwinCATDataType.Real => ToSingleInRange(value), + TwinCATDataType.LReal => ToDoubleInRange(value), TwinCATDataType.String or TwinCATDataType.WString => Convert.ToString(value) ?? string.Empty, TwinCATDataType.Time or TwinCATDataType.Date or TwinCATDataType.DateTime or TwinCATDataType.TimeOfDay => TwinCATTemporalCodec.Encode(type, value), _ => throw new NotSupportedException($"TwinCATDataType {type} not writable."), }; + // Convert.ToSingle(1e300) is +Infinity, and Convert.ToDouble("1e400") too: writing that to the PLC + // would store a value nobody wrote. A finite source that overflows the target is out of range; an + // explicit infinity (or NaN) passes through as the caller's own value. + private static float ToSingleInRange(object? value) + { + var f = Convert.ToSingle(value, System.Globalization.CultureInfo.InvariantCulture); + if (float.IsInfinity(f) && !IsExplicitInfinity(value)) + throw new OverflowException($"{value} is outside the REAL range (±{float.MaxValue:R})"); + return f; + } + + private static double ToDoubleInRange(object? value) + { + var d = Convert.ToDouble(value, System.Globalization.CultureInfo.InvariantCulture); + if (double.IsInfinity(d) && !IsExplicitInfinity(value)) + throw new OverflowException($"{value} is outside the LREAL range"); + return d; + } + + private static bool IsExplicitInfinity(object? value) => value switch + { + float f => float.IsInfinity(f), + double d => double.IsInfinity(d), + _ => false, + }; + private static bool ExtractBit(object? rawWord, int bit) => rawWord switch { short s => (s & (1 << bit)) != 0, 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 fc0469ae..4d9be07b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -51,6 +51,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery private readonly object _lifecycleLock = new(); private long _lifecycleGeneration; + /// Test accessor: the RMW gate for a device + parent word, or null. + internal SemaphoreSlim? BitRmwGateForTest(string host, string parentPath) => + _bitRmwLocks.TryGetValue($"{host}|{parentPath}", out var g) ? g : null; + /// Test seam: awaited after the new generation is built and before it is committed, so a test can /// interleave a teardown deterministically. Never set in production. internal Func? BeforeCommitForTest { get; set; } @@ -73,9 +77,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // read-modify-write of the parent word (TwinCAT's symbol table doesn't expose "Word.N"), // so concurrent bit-writers to the same word must serialise or they lose each other's // updates. Keyed by device + parent symbol. (Cannot guard against the PLC program writing - // the word between our read and write — inherent to RMW.) Teardown only CLEARS it: a - // SemaphoreSlim owns no kernel handle unless AvailableWaitHandle is read, and disposing one an - // in-flight write still holds turns its Release into an ObjectDisposedException. + // the word between our read and write — inherent to RMW.) The gates are KEPT across generations + // and teardowns (never cleared or disposed): a write still in flight on the old generation and the + // first write of the next one must serialise on the SAME gate. A SemaphoreSlim owns no kernel handle + // unless AvailableWaitHandle is read, and the key set is bounded by the authored bit tags. private readonly ConcurrentDictionary _bitRmwLocks = new(StringComparer.OrdinalIgnoreCase); @@ -152,6 +157,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public string DriverType => "TwinCAT"; /// + /// + /// An initialise that is overtaken before it commits — a / + /// or a newer bumped the lifecycle generation while it was building — releases + /// what it built and throws . Cancellation is the honest outcome: + /// the work was abandoned by a later lifecycle call, not failed, so it must not write Faulted over + /// the newer state or let a caller believe this generation is live; returning normally would claim a + /// driver that was never installed. The runtime has usually abandoned the task by then, and an + /// overlapping retry treats it like any other cancelled attempt. + /// public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { // InitializeAsync is reachable on a live driver with no teardown first (the IDriver contract @@ -353,7 +367,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // The poll loops and the device release wind down in parallel (releasing a client can unblock a // poll loop stuck in a read on it); the whole teardown is bounded. var teardown = Task.WhenAll(_poll.DisposeAsync().AsTask(), ReleaseDevicesAsync(taken)); - try { await teardown.WaitAsync(ShutdownBudget).ConfigureAwait(false); } + var cancelled = false; + try { await teardown.WaitAsync(ShutdownBudget, cancellationToken).ConfigureAwait(false); } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // The caller stopped waiting. The tables are already swapped out and every loop is cancelled, so + // the rest of the teardown finishes in the background; report the cancellation after the cheap + // bookkeeping below. + cancelled = true; + } catch (TimeoutException) { _logger.LogWarning( @@ -364,8 +386,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _warnedRefs.Clear(); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - _bitRmwLocks.Clear(); _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + if (cancelled) throw new OperationCanceledException(cancellationToken); } /// @@ -448,7 +470,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var (value, status) = await client.ReadValueAsync( symbolName, def.DataType, parsed?.BitIndex, def.ArrayLength, cancellationToken) .ConfigureAwait(false); - return CompleteRead(reference, def, symbolName, value, status, now); + return CompleteRead(device, reference, def, symbolName, value, status, now); } catch (OperationCanceledException) { throw; } catch (Exception ex) @@ -459,13 +481,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } /// Maps one read outcome to a snapshot and applies the health / log side effects. - private DataValueSnapshot CompleteRead( + private DataValueSnapshot CompleteRead(DeviceState device, string reference, TwinCATTagDefinition def, string symbolName, object? value, uint status, DateTime now) { if (status == TwinCATStatusMapper.Good) { _warnedRefs.TryRemove(reference, out _); // a later failure is a new episode and warns again - RecordDeviceAnswered(now, force: true); + RecordDeviceAnswered(now, device, force: true); return new DataValueSnapshot(value, status, now, now); } @@ -487,7 +509,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } if (IsCommsStatus(status)) - MarkCommsFault($"ADS status {status:X8} reading {reference}"); + MarkCommsFault($"ADS status {status:X8} reading {reference}", device); else RecordDeviceAnsweredKeepState(now); // a refusal: the device answered — no state change return new DataValueSnapshot(value, status, now, now); @@ -558,7 +580,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { var e = chunk[k]; var (value, status) = k < bulk.Count ? bulk[k] : (null, TwinCATStatusMapper.BadCommunicationError); - results[e.Index] = CompleteRead(e.Reference, e.Def, e.SymbolName, value, status, now); + results[e.Index] = CompleteRead(device, e.Reference, e.Def, e.SymbolName, value, status, now); } } catch (OperationCanceledException) { throw; } @@ -842,7 +864,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // A push is the device answering: stamp LastSuccessfulRead so a notification-only // driver does not look silent to the comms-loss watchdog. var now = DateTime.UtcNow; - RecordDeviceAnswered(now); + RecordDeviceAnswered(now, device); RaiseDataChange(handle, reference, new DataValueSnapshot(value, TwinCATStatusMapper.Good, now, now)); }, // When the handle goes dead (a failed replay) push a Bad snapshot so @@ -856,6 +878,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // Same device already failed to connect in this call: keep the intent dead for the // probe-tick retry / reconnect replay and surface Bad now. device.NativeRegistrations[reg.Id] = reg; + NoteDeadFailure(reg, TwinCATStatusMapper.BadCommunicationError, refused: false, DateTime.UtcNow); RaiseDataChange(handle, reference, new DataValueSnapshot( null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); continue; @@ -876,6 +899,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _logger.LogWarning( "TwinCAT driver '{DriverInstanceId}' refused native notification for '{Reference}': {Reason}", _driverInstanceId, reference, refused.Message); + NoteDeadFailure(reg, refused.StatusCode, refused: true, DateTime.UtcNow); RaiseDataChange(handle, reference, new DataValueSnapshot( null, refused.StatusCode, null, DateTime.UtcNow)); } @@ -885,6 +909,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // Every tag of this device goes Bad + dead for the retry; other devices carry on. (unreachable ??= []).Add(device); MarkCommsFault(unavailable.InnerException?.Message ?? unavailable.Message, device); + NoteDeadFailure(reg, TwinCATStatusMapper.BadCommunicationError, refused: false, DateTime.UtcNow); RaiseDataChange(handle, reference, new DataValueSnapshot( null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); } @@ -897,6 +922,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery "'{Reference}'; it will be retried on the next healthy probe tick", _driverInstanceId, reference); MarkCommsFault(ex.Message, device); + NoteDeadFailure(reg, TwinCATStatusMapper.BadCommunicationError, refused: false, DateTime.UtcNow); RaiseDataChange(handle, reference, new DataValueSnapshot( null, TwinCATStatusMapper.BadCommunicationError, null, DateTime.UtcNow)); } @@ -928,7 +954,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// /// Records a comms fault: preserving LastSuccessfulRead. - /// Never downgrades . + /// Never downgrades . A fault observed on a device is remembered ON + /// that device () and only that device answering clears it — the + /// driver state is the worst of every device (#739 fix round 1). /// /// The fault description. /// The device the fault was observed on; a fault on a device already swapped out of @@ -937,6 +965,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery private void MarkCommsFault(string message, DeviceState? device = null) { if (device?.Released == true) return; + if (device is not null) device.CommsFault = message; var current = _health; if (current.State != DriverState.Faulted) _health = new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, message); @@ -954,29 +983,90 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery private static readonly TimeSpan AnsweredStampInterval = TimeSpan.FromMilliseconds(250); /// - /// Records that a device answered (a native push, a successful probe, a Good read): advances - /// LastSuccessfulRead and restores unless a device is still - /// probe-unreachable (then the state stays Degraded, the stamp still advances). Throttled so a + /// Why one device keeps the driver Degraded, or null when it does not: probe-unreachable, a PLC whose + /// ADS state is not Run (coordinator ruling: a stopped / config-mode PLC is Degraded with a + /// reason even though it answers), or an uncleared comms fault seen on it. + /// + private static string? DeviceDegradedReason(DeviceState d) + { + if (d.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded) + return $"TwinCAT device {d.Options.HostAddress} unreachable ({d.ConsecutiveProbeFailures} consecutive failed probes)"; + if (d.PlcState is { } st && !string.Equals(st, "Run", StringComparison.OrdinalIgnoreCase)) + return $"TwinCAT device {d.Options.HostAddress}: PLC in {st}"; + return d.CommsFault; + } + + /// + /// Records that a device answered (a native push, a successful probe, a Good read): clears THAT + /// device's comms fault, advances LastSuccessfulRead, and sets the state to the worst of every + /// device — Healthy only when no device is unreachable, out of RUN or still faulted (then Degraded + /// with that device's reason; the stamp still advances, since a device did answer). Throttled so a /// notification burst does not allocate a health record per value; never touches Faulted. /// /// The time the device answered. + /// The device that answered; null when not attributable. /// Write even inside the throttle window (an explicit read / probe outcome). - private void RecordDeviceAnswered(DateTime now, bool force = false) + private void RecordDeviceAnswered(DateTime now, DeviceState? device = null, bool force = false) { + if (device?.Released == true) return; + if (device?.CommsFault is not null) + { + device.CommsFault = null; + force = true; + } var current = _health; if (current.State == DriverState.Faulted) return; // Hot path (a notification on a healthy driver inside the window): no device scan, no allocation. if (!force && current.State == DriverState.Healthy && current.LastSuccessfulRead is { } recent && now - recent < AnsweredStampInterval) return; - var unreachable = _devices.Values.FirstOrDefault(d => d.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded); - var state = unreachable is null ? DriverState.Healthy : DriverState.Degraded; + string? reason = null; + foreach (var d in _devices.Values) + if ((reason = DeviceDegradedReason(d)) is not null) break; + var state = reason is null ? DriverState.Healthy : DriverState.Degraded; if (!force && current.State == state && current.LastSuccessfulRead is { } last && now - last < AnsweredStampInterval) return; - _health = new DriverHealth(state, now, unreachable is null - ? null - : $"TwinCAT device {unreachable.Options.HostAddress} unreachable ({unreachable.ConsecutiveProbeFailures} consecutive failed probes)"); + _health = new DriverHealth(state, now, reason); + } + + /// First probe-tick retry delay of a REFUSED dead registration (the PLC answered and refused it). + internal static readonly TimeSpan RefusedRetryInitial = TimeSpan.FromSeconds(30); + + /// Cap of the refused-registration retry backoff. + internal static readonly TimeSpan RefusedRetryMax = TimeSpan.FromMinutes(10); + + /// + /// Books a failed registration attempt. A REFUSAL is deterministic until the PLC program changes, so it + /// backs off exponentially (30 s doubling to 10 min) and is re-armed at once by a symbol-version change + /// or retried by the reconnect replay; a comms-class failure stays on the every-tick retry. + /// + /// True when the published status changes (the caller republishes the tag). + private static bool NoteDeadFailure(NativeRegistration reg, uint status, bool refused, DateTime now) + { + if (refused) + { + reg.RefusalBackoff = reg.RefusalBackoff == TimeSpan.Zero + ? RefusedRetryInitial + : TimeSpan.FromTicks(Math.Min(reg.RefusalBackoff.Ticks * 2, RefusedRetryMax.Ticks)); + reg.NextRetryUtc = now + reg.RefusalBackoff; + } + else + { + reg.RefusalBackoff = TimeSpan.Zero; + reg.NextRetryUtc = DateTime.MinValue; + } + var changed = reg.DeadStatus != status; + reg.DeadStatus = status; + return changed; + } + + /// Clears the dead-registration bookkeeping after a successful (re-)registration. + private static void NoteLive(NativeRegistration reg) + { + reg.DeadStatus = null; + reg.RefusalBackoff = TimeSpan.Zero; + reg.NextRetryUtc = DateTime.MinValue; } /// @@ -1159,6 +1249,15 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery public ITwinCATNotificationHandle? MarkHandleDead() => Interlocked.Exchange(ref _handle, null); + /// The Bad status last published for this registration while dead; null while live. + public uint? DeadStatus { get; set; } + + /// Earliest time the probe-tick retry may re-attempt this dead registration. + public DateTime NextRetryUtc { get; set; } + + /// Current refusal backoff (zero after a comms-class failure or a success). + public TimeSpan RefusalBackoff { get; set; } + private volatile bool _disposed; /// Gets a value indicating whether this registration has been disposed (its callbacks are dropped). @@ -1195,6 +1294,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var client = await EnsureConnectedAsync(state, ct, _options.Probe.Timeout, bypassBackoff: true) .ConfigureAwait(false); success = await client.ProbeAsync(ct).ConfigureAwait(false); + // The probe's ReadState also says whether the PLC is in RUN; a stopped PLC answers but + // serves no live data, so it keeps the driver Degraded (RecordDeviceAnswered below). + if (success) state.PlcState = client.LastAdsState; // A locally-connected client whose wire probe FAILS is wire-dead but port-open: // force a recycle so the next tick rebuilds + replays native notifications. Without @@ -1224,13 +1326,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery if (success) { state.ConsecutiveProbeFailures = 0; - RecordDeviceAnswered(DateTime.UtcNow, force: true); + RecordDeviceAnswered(DateTime.UtcNow, state, force: true); if (probed is not null) ReportNotificationDrops(state, probed); } else if (++state.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded) { MarkCommsFault($"TwinCAT device {state.Options.HostAddress} unreachable " + - $"({state.ConsecutiveProbeFailures} consecutive failed probes)"); + $"({state.ConsecutiveProbeFailures} consecutive failed probes)", state); } TransitionDeviceState(state, success ? HostState.Running : HostState.Stopped); @@ -1625,6 +1727,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery try { orphan?.Dispose(); } catch { /* best-effort */ } continue; } + NoteLive(reg); replayed++; } catch (TwinCATNotificationRefusedException refused) @@ -1638,7 +1741,8 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _driverInstanceId, reg.SymbolName, device.Options.HostAddress, refused.Message); var dead = reg.MarkHandleDead(); try { dead?.Dispose(); } catch { /* already-dead handle from the disposed client */ } - try { reg.OnRefused(refused.StatusCode); } catch { /* never abort recovery of the rest */ } + if (NoteDeadFailure(reg, refused.StatusCode, refused: true, DateTime.UtcNow)) + try { reg.OnRefused(refused.StatusCode); } catch { /* never abort recovery of the rest */ } } catch (Exception ex) { @@ -1652,8 +1756,9 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // no quality channel), and degrade health preserving the last successful read. var dead = reg.MarkHandleDead(); try { dead?.Dispose(); } catch { /* already-dead handle from the disposed client */ } - try { reg.OnUnavailable(); } catch { /* never let a subscriber callback abort recovery of the rest */ } - MarkCommsFault($"native-notification replay failed for '{reg.SymbolName}' on {device.Options.HostAddress}"); + if (NoteDeadFailure(reg, TwinCATStatusMapper.BadCommunicationError, refused: false, DateTime.UtcNow)) + try { reg.OnUnavailable(); } catch { /* never let a subscriber callback abort recovery of the rest */ } + MarkCommsFault($"native-notification replay failed for '{reg.SymbolName}' on {device.Options.HostAddress}", device); } } @@ -1682,9 +1787,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery if (device.Client is not { IsConnected: true } client) return; var recovered = 0; + var now = DateTime.UtcNow; foreach (var reg in device.NativeRegistrations.Values) { if (reg.HasLiveHandle) continue; + // A refused registration waits out its backoff (a symbol-version change re-arms it). + if (now < reg.NextRetryUtc) continue; try { var newHandle = await client.AddNotificationAsync( @@ -1698,11 +1806,18 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery try { orphan?.Dispose(); } catch { /* best-effort */ } continue; } + NoteLive(reg); recovered++; } catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } catch (Exception ex) { + // Republish only when the failure CLASS changes (a comms blip that turned into a refusal, + // or the reverse); the same failure again publishes nothing. + var refusal = ex as TwinCATNotificationRefusedException; + var status = refusal?.StatusCode ?? TwinCATStatusMapper.BadCommunicationError; + if (NoteDeadFailure(reg, status, refusal is not null, now)) + try { reg.OnRefused(status); } catch { /* never abort the rest */ } _logger.LogDebug(ex, "TwinCAT driver '{DriverInstanceId}' probe-tick retry of dead notification " + "'{Symbol}' on device '{HostAddress}' failed; will retry next tick", @@ -1758,10 +1873,18 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// space must be rebuilt — this is the documented TwinCAT failure mode, not a transient /// connection error. /// - private void HandleSymbolVersionChanged(object? sender, EventArgs e) => + private void HandleSymbolVersionChanged(object? sender, EventArgs e) + { + // A program change can make a refused registration valid: re-arm every backed-off dead intent of the + // device whose client saw it, so the next probe tick re-checks them at once. + foreach (var d in _devices.Values) + if (ReferenceEquals(d.Client, sender)) + foreach (var reg in d.NativeRegistrations.Values) + reg.NextRetryUtc = DateTime.MinValue; OnRediscoveryNeeded?.Invoke(this, new RediscoveryEventArgs( "TwinCAT symbol-version-changed (DeviceSymbolVersionInvalid 0x0711) — PLC program re-downloaded", ScopeHint: "TwinCAT")); + } /// /// Synchronous teardown — no await, no captured sync context. The OPC UA stack @@ -1790,7 +1913,6 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _warnedRefs.Clear(); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - _bitRmwLocks.Clear(); _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); } @@ -1852,6 +1974,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// True once an unexpected sum-read chunk failure was logged for this device. public volatile bool SumChunkFailureWarned; + /// The last comms fault seen on this device; cleared when this device answers. Null = none. + public volatile string? CommsFault; + + /// The ADS state (Run, Stop, Config, …) the last successful probe read; null = unknown. + public volatile string? PlcState; + /// Consecutive failed probe ticks; reset by a successful probe. Written only by this device's probe loop. public volatile int ConsecutiveProbeFailures; 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 909fc31f..49a6deea 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 @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Runtime.CompilerServices; using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; @@ -142,6 +143,14 @@ internal class FakeTwinCATClient : ITwinCATClient string symbolPath, TwinCATDataType type, int? bitIndex, object? value, CancellationToken ct) { if (ThrowOnWrite) throw Exception ?? new InvalidOperationException(); + if (type is TwinCATDataType.String or TwinCATDataType.WString + && SymbolShapes.TryGetValue(symbolPath, out var shape)) + { + // The real client's encode plan (resolve → CheckRead → codec at the PLC width). + var (planStatus, bytes) = AdsTwinCATClient.PlanStringWrite(type, shape, value, utf8: false); + if (planStatus != TwinCATStatusMapper.Good) return Task.FromResult(planStatus); + RawWrites[symbolPath] = bytes; + } WriteLog.Add((symbolPath, type, bitIndex, value)); Values[symbolPath] = value; var status = WriteStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good; @@ -161,11 +170,26 @@ internal class FakeTwinCATClient : ITwinCATClient CancellationToken ct) { if (ThrowOnWrite) throw Exception ?? new InvalidOperationException(); + if (ArrayShapes.TryGetValue(symbolPath, out var array) && SymbolShapes.TryGetValue(symbolPath, out var shape)) + { + // The real client's encode plan (CheckArray layout → TwinCATArrayCodec.Encode). + var (planStatus, bytes) = AdsTwinCATClient.PlanArrayWrite(type, elements, shape, array, utf8: false); + if (planStatus != TwinCATStatusMapper.Good) return Task.FromResult(planStatus); + RawWrites[symbolPath] = bytes; + } ArrayWriteLog.Add((symbolPath, type, elements)); Values[symbolPath] = elements; return Task.FromResult(WriteStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good); } + /// Resolved PLC symbol shapes; a string / array write to a symbol listed here runs the real client's + /// encode plan and lands its bytes in (#739 fix round 1). + public Dictionary SymbolShapes { get; } = new(StringComparer.OrdinalIgnoreCase); + /// Resolved PLC array shapes for entries that are arrays. + public Dictionary ArrayShapes { get; } = new(StringComparer.OrdinalIgnoreCase); + /// The raw bytes the encode plan produced, by symbol. + public ConcurrentDictionary RawWrites { get; } = new(StringComparer.OrdinalIgnoreCase); + /// Raw bytes of bit-write parent words, by parent path (#739 H18). The byte count is the parent's width. public Dictionary ParentWords { get; } = new(StringComparer.OrdinalIgnoreCase); /// Optional parent shapes; absent = an unsigned integer word as wide as its entry. @@ -205,10 +229,16 @@ internal class FakeTwinCATClient : ITwinCATClient /// A task that returns the probe result. public virtual Task ProbeAsync(CancellationToken ct) { + Interlocked.Increment(ref _probeCount); if (ThrowOnProbe) return Task.FromResult(false); return Task.FromResult(ProbeResult); } + private int _probeCount; + + /// Gets how many times ran — a measured window for absence assertions. + public int ProbeCount => Volatile.Read(ref _probeCount); + /// Releases unmanaged resources. public virtual void Dispose() { @@ -224,7 +254,14 @@ internal class FakeTwinCATClient : ITwinCATClient public bool ThrowOnAddNotification { get; set; } /// Per-symbol exceptions throws (e.g. a /// for one refused tag). - public Dictionary AddNotificationErrors { get; } = new(StringComparer.OrdinalIgnoreCase); + public ConcurrentDictionary AddNotificationErrors { get; } = new(StringComparer.OrdinalIgnoreCase); + + private readonly ConcurrentDictionary _addAttempts = new(StringComparer.OrdinalIgnoreCase); + + /// How many times was called for . + /// The symbol path. + /// The attempt count. + public int AttemptsFor(string symbolPath) => _addAttempts.TryGetValue(symbolPath, out var n) ? n : 0; /// Records the most recently-supplied maxDelayMs for Driver.TwinCAT-014 tests. public int LastMaxDelayMs { get; private set; } /// When set, awaits this gate before creating the handle — @@ -245,6 +282,7 @@ internal class FakeTwinCATClient : ITwinCATClient string symbolPath, TwinCATDataType type, int? bitIndex, int? arrayCount, TimeSpan cycleTime, int maxDelayMs, Action onChange, CancellationToken cancellationToken) { + _addAttempts.AddOrUpdate(symbolPath, 1, (_, n) => n + 1); if (ThrowOnAddNotification) throw Exception ?? new InvalidOperationException("fake AddNotification failure"); if (AddNotificationErrors.TryGetValue(symbolPath, out var perSymbol)) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs index f1c06bfd..19beaeff 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATHealthContractTests.cs @@ -50,7 +50,9 @@ public sealed class TwinCATHealthContractTests drv.GetHealth().State.ShouldBe(DriverState.Degraded); drv.GetHealth().LastSuccessfulRead.ShouldBe(initStamp); - await Task.Delay(300, TestContext.Current.CancellationToken); + // Condition, not a fixed delay: the clock only has to move past the stamp (the Degraded state means + // the notification is not inside the Healthy throttle window). + await WaitForAsync(() => DateTime.UtcNow > initStamp!.Value, TimeSpan.FromSeconds(2)); fake.FireNotification("MAIN.Speed", 7); var after = drv.GetHealth(); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs index 222495fd..915316f8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATLifecycleTests.cs @@ -80,8 +80,10 @@ public sealed class TwinCATLifecycleTests var shutdown = drv.ShutdownAsync(CancellationToken.None); var sw = Stopwatch.StartNew(); await drv.DisposeAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken); - sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(250)); + // Ordering is the real assertion: dispose finished while the first shutdown was still joining the + // wedged probe. The elapsed bound is generous (the runtime allows 250 ms; a loaded CI box gets 1 s). shutdown.IsCompleted.ShouldBeFalse("the positive control: the first shutdown really was still joining the probe"); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(1)); wedge.SetResult(); await shutdown.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); @@ -109,15 +111,18 @@ public sealed class TwinCATLifecycleTests await Should.ThrowAsync(() => abandoned.WaitAsync(TimeSpan.FromSeconds(5))); drv.DeviceCount.ShouldBe(0); - await Task.Delay(150, TestContext.Current.CancellationToken); // several probe intervals - factory.Clients.ShouldBeEmpty("no probe loop of the overtaken generation may be running"); + factory.Clients.ShouldBeEmpty(); drv.GetHealth().State.ShouldBe(DriverState.Unknown); + // Positive control + measured window: a fresh generation's probe creates exactly one client and + // ticks several times; an overtaken-generation probe loop (same interval, started earlier) would have + // created a second client within that window. drv.BeforeCommitForTest = null; await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); drv.DeviceCount.ShouldBe(1); - await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); - factory.Clients.Count.ShouldBe(1); + await WaitForAsync(() => factory.Clients.Count >= 1 && factory.Clients[0].ProbeCount >= 4, TimeSpan.FromSeconds(3)); + factory.Clients[0].ProbeCount.ShouldBeGreaterThanOrEqualTo(4); + factory.Clients.Count.ShouldBe(1, "no probe loop of the overtaken generation may be running"); await drv.ShutdownAsync(TestContext.Current.CancellationToken); } @@ -145,7 +150,7 @@ public sealed class TwinCATLifecycleTests await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); var abandoned = drv.InitializeAsync("{}", CancellationToken.None); // joins the wedged generation-1 probe - await Task.Delay(50, TestContext.Current.CancellationToken); + await WaitForAsync(() => drv.DeviceCount == 1 && factory.Clients.Count >= 2, TimeSpan.FromSeconds(3)); // gen 2 committed + probing var sw = Stopwatch.StartNew(); await drv.ShutdownAsync(TestContext.Current.CancellationToken).WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ShutdownBudget + TimeSpan.FromMilliseconds(500)); @@ -157,7 +162,15 @@ public sealed class TwinCATLifecycleTests drv.DeviceCount.ShouldBe(0); await WaitForAsync(() => factory.Clients.All(c => c.DisposeCount > 0 || !c.IsConnected), TimeSpan.FromSeconds(2)); var settled = factory.Clients.Count; - await Task.Delay(150, TestContext.Current.CancellationToken); + + // Measured window: a sibling driver on the same probe interval ticks 4 times; any surviving probe + // loop of the torn-down driver would have built a new client in that time. + var witnessFactory = new FakeTwinCATClientFactory(); + var witness = NewDriver(witnessFactory); + await witness.InitializeAsync("{}", TestContext.Current.CancellationToken); + await WaitForAsync(() => witnessFactory.Clients.Count >= 1 && witnessFactory.Clients[0].ProbeCount >= 4, TimeSpan.FromSeconds(3)); + witnessFactory.Clients[0].ProbeCount.ShouldBeGreaterThanOrEqualTo(4); + await witness.ShutdownAsync(TestContext.Current.CancellationToken); factory.Clients.Count.ShouldBe(settled, "no probe loop may still be creating clients"); factory.Clients.ShouldAllBe(c => c.DisposeCount > 0 || !c.IsConnected); } @@ -231,20 +244,33 @@ public sealed class TwinCATLifecycleTests var factory = new FakeTwinCATClientFactory(); var drv = NewDriver(factory, probe: false); await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); - using var stop = new CancellationTokenSource(TimeSpan.FromMilliseconds(400)); + // Run for at least 300 ms of overlap AND until the reader has seen a Good read mid-churn (positive + // control), bounded at 10 s — a condition, not a fixed window, so a loaded machine cannot starve it. var good = 0; + var reads = 0; + var inits = 0; + var minUntil = DateTime.UtcNow.AddMilliseconds(300); + var deadline = DateTime.UtcNow.AddSeconds(10); + bool Done() => (DateTime.UtcNow > minUntil && Volatile.Read(ref good) > 0 && Volatile.Read(ref inits) > 10) + || DateTime.UtcNow > deadline; var reader = Task.Run(async () => { - while (!stop.IsCancellationRequested) + while (!Done()) { var r = await drv.ReadAsync(["Speed"], CancellationToken.None); + Interlocked.Increment(ref reads); if (r[0].StatusCode == TwinCATStatusMapper.Good) Interlocked.Increment(ref good); } }, TestContext.Current.CancellationToken); - while (!stop.IsCancellationRequested) + while (!Done()) + { await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + Interlocked.Increment(ref inits); + await Task.Yield(); // let the reader in between generations + } await reader; // throws if any read threw good.ShouldBeGreaterThan(0); + inits.ShouldBeGreaterThan(10); } /// Notification-queue drops are reported once per probe tick that sees new drops. @@ -255,14 +281,14 @@ public sealed class TwinCATLifecycleTests var log = new TwinCATCapturingLogger(); var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, log: log); await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); - await WaitForAsync(() => fake.IsConnected, TimeSpan.FromSeconds(2)); - await Task.Delay(100, TestContext.Current.CancellationToken); - log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(0); // no drops, no warning + await WaitForAsync(() => fake.ProbeCount >= 3, TimeSpan.FromSeconds(3)); + log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(0); // 3 ticks, no drops, no warning fake.DroppedNotificationCount = 5; await WaitForAsync(() => log.Warnings.Any(w => w.Contains("dropped 5")), TimeSpan.FromSeconds(2)); - await Task.Delay(100, TestContext.Current.CancellationToken); - log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(1); + var ticks = fake.ProbeCount; + await WaitForAsync(() => fake.ProbeCount >= ticks + 3, TimeSpan.FromSeconds(3)); + log.Warnings.Count(w => w.Contains("dropped")).ShouldBe(1); // 3 more ticks, no new drops, no repeat fake.DroppedNotificationCount = 8; await WaitForAsync(() => log.Warnings.Any(w => w.Contains("dropped 3")), TimeSpan.FromSeconds(2)); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs index dd669164..eeb78937 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReadWarningTests.cs @@ -87,7 +87,8 @@ public sealed class TwinCATReadWarningTests await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); var initStamp = drv.GetHealth().LastSuccessfulRead!.Value; fake.ReadStatuses["MAIN.Typo"] = TwinCATStatusMapper.BadNodeIdUnknown; - await Task.Delay(300, TestContext.Current.CancellationToken); + var deadline = DateTime.UtcNow.AddSeconds(2); + while (DateTime.UtcNow <= initStamp && DateTime.UtcNow < deadline) await Task.Delay(5, TestContext.Current.CancellationToken); _ = await drv.ReadAsync(["Typo"], TestContext.Current.CancellationToken); var afterRefusal = drv.GetHealth(); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs new file mode 100644 index 00000000..35ff945b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs @@ -0,0 +1,294 @@ +using System.Collections.Concurrent; +using System.Diagnostics; +using Shouldly; +using TwinCAT.TypeSystem; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// +/// #739 review fix round 1: the PLC-not-in-RUN health ruling, refused-registration retry backoff, +/// per-device worst-of health, RMW gates surviving teardown, a cancellable shutdown, the real +/// STRING / array encode path through the client planning helpers, and REAL range checks. +/// +[Trait("Category", "Unit")] +public sealed class TwinCATReviewFixRound1Tests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + private const string HostB = "ads://5.23.91.24.1.1:851"; + + private static TwinCATProbeOptions FastProbe => new() + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(30), + Timeout = TimeSpan.FromMilliseconds(100), + }; + + private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, TwinCATProbeOptions probe, bool native, + params TwinCATTagDefinition[] tags) => + new(new TwinCATDriverOptions + { + Devices = [.. tags.Select(t => t.DeviceHostAddress).Append(Host).Distinct().Select(h => new TwinCATDeviceOptions(h))], + RawTags = TwinCATRawTags.Entries(tags), + Probe = probe, + UseNativeNotifications = native, + }, "drv-1", factory); + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(10); + } + + // ---- IMPORTANT: a PLC not in RUN leaves the driver Degraded ---- + + /// + /// A probe that answers with ADS state Stop writes Degraded "PLC in Stop" and still advances + /// LastSuccessfulRead (the device answered); back in Run the driver is Healthy (positive control). + /// + [Fact] + public async Task A_PLC_not_in_Run_is_Degraded_and_Run_restores_Healthy() + { + var fake = new FakeTwinCATClient { AdsStateText = "Stop" }; + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: false, + new TwinCATTagDefinition("Speed", Host, "MAIN.Speed", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead!.Value; + + await WaitForAsync(() => drv.GetHealth().State == DriverState.Degraded, TimeSpan.FromSeconds(3)); + var stopped = drv.GetHealth(); + stopped.State.ShouldBe(DriverState.Degraded); + stopped.LastError.ShouldNotBeNull().ShouldContain("PLC in Stop"); + await WaitForAsync(() => drv.GetHealth().LastSuccessfulRead > initStamp, TimeSpan.FromSeconds(3)); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp); + + // A Good read while the PLC is stopped must not paper over it. + _ = await drv.ReadAsync(["Speed"], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + fake.AdsStateText = "Run"; + await WaitForAsync(() => drv.GetHealth().State == DriverState.Healthy, TimeSpan.FromSeconds(3)); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + // ---- MINOR 1: refused dead registrations back off; a class change republishes ---- + + /// + /// A REFUSED dead registration is not re-attempted every probe tick; a comms-failed one is (positive + /// control: it recovers). A symbol-version-changed signal re-arms the refused one at once. + /// + [Fact] + public async Task Refused_registrations_back_off_while_comms_failures_retry_each_tick() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.T"] = new TwinCATTypeMismatchException("MAIN.T", "declared DInt, PLC STRING"); + fake.AddNotificationErrors["MAIN.C"] = new InvalidOperationException("transient"); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: true, + new TwinCATTagDefinition("T", Host, "MAIN.T", TwinCATDataType.DInt), + new TwinCATTagDefinition("C", Host, "MAIN.C", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.SubscribeAsync(["T", "C"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + + await WaitForAsync(() => fake.AttemptsFor("MAIN.C") >= 3, TimeSpan.FromSeconds(3)); + fake.AttemptsFor("MAIN.C").ShouldBeGreaterThanOrEqualTo(3); // comms class: every tick + fake.AttemptsFor("MAIN.T").ShouldBe(1); // refusal: backed off + + fake.AddNotificationErrors.TryRemove("MAIN.C", out _); + await WaitForAsync(() => fake.Notifications.Any(n => n.SymbolPath == "MAIN.C"), TimeSpan.FromSeconds(3)); + fake.Notifications.ShouldContain(n => n.SymbolPath == "MAIN.C"); + + fake.FireSymbolVersionChanged(); + await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 2, TimeSpan.FromSeconds(3)); + fake.AttemptsFor("MAIN.T").ShouldBe(2); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// When a retry fails with a different failure class the tag's status is republished, once. + [Fact] + public async Task A_retry_whose_failure_class_changes_republishes_the_status() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.X"] = new InvalidOperationException("router hiccup"); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: true, + new TwinCATTagDefinition("X", Host, "MAIN.X", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + _ = await drv.SubscribeAsync(["X"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + events.ShouldHaveSingleItem().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError); + await WaitForAsync(() => fake.AttemptsFor("MAIN.X") >= 3, TimeSpan.FromSeconds(3)); + events.Count.ShouldBe(1, "a repeat of the same failure class publishes nothing"); + + fake.AddNotificationErrors["MAIN.X"] = new TwinCATNotificationRefusedException( + "MAIN.X", "symbol gone", TwinCATStatusMapper.BadNodeIdUnknown); + await WaitForAsync(() => events.Count >= 2, TimeSpan.FromSeconds(3)); + events.Last().Snapshot.StatusCode.ShouldBe(TwinCATStatusMapper.BadNodeIdUnknown); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + // ---- MINOR 2: per-device worst-of health ---- + + /// + /// A comms fault on device B is not cleared by device A answering; B answering clears it + /// (positive control). + /// + [Fact] + public async Task One_devices_answer_does_not_clear_another_devices_comms_fault() + { + var clients = new ConcurrentDictionary(); + var build = 0; + var factory = new FakeTwinCATClientFactory + { + Customise = () => + { + var c = new FakeTwinCATClient(); + clients[Interlocked.Increment(ref build)] = c; + return c; + }, + }; + var drv = NewDriver(factory, new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt), + new TwinCATTagDefinition("B", HostB, "MAIN.B", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.ReadAsync(["A"], TestContext.Current.CancellationToken); // client 1 = device A + _ = await drv.ReadAsync(["B"], TestContext.Current.CancellationToken); // client 2 = device B + clients[2].ReadStatuses["MAIN.B"] = TwinCATStatusMapper.BadTimeout; + + _ = await drv.ReadAsync(["B"], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + _ = await drv.ReadAsync(["A"], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded, "device A answering says nothing about device B"); + + clients[2].ReadStatuses.Remove("MAIN.B"); + _ = await drv.ReadAsync(["B"], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + // ---- MINOR 3: RMW gates are kept across generations ---- + + /// The per-parent RMW gate survives a teardown (a write still in flight keeps the same gate). + [Fact] + public async Task Bit_RMW_gates_survive_a_shutdown() + { + var fake = new FakeTwinCATClient { ParentWords = { ["MAIN.Flags"] = [0, 0] } }; + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("Bit", Host, "MAIN.Flags.1", TwinCATDataType.Bool)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.WriteAsync([new WriteRequest("Bit", true)], TestContext.Current.CancellationToken); + var gate = drv.BitRmwGateForTest(Host, "MAIN.Flags").ShouldNotBeNull(); + + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.WriteAsync([new WriteRequest("Bit", false)], TestContext.Current.CancellationToken); + + drv.BitRmwGateForTest(Host, "MAIN.Flags").ShouldBeSameAs(gate); + } + + // ---- MINOR 4: ShutdownAsync honours its token ---- + + /// + /// A cancelled ShutdownAsync stops waiting on a probe wedged in connect and throws + /// OperationCanceledException; the teardown itself still happened (tables emptied). + /// + [Fact] + public async Task Shutdown_honours_its_cancellation_token() + { + var wedge = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeTwinCATClientFactory { Customise = () => new FakeTwinCATClient { BlockConnect = wedge } }; + var drv = NewDriver(factory, FastProbe, native: false); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + await WaitForAsync(() => factory.Clients.Count == 1, TimeSpan.FromSeconds(2)); + + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(100)); + var sw = Stopwatch.StartNew(); + await Should.ThrowAsync(() => drv.ShutdownAsync(cts.Token)); + sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ProbeJoinTimeout, "the wait ended on the token, not the join bound"); + drv.DeviceCount.ShouldBe(0); + wedge.SetResult(); + } + + // ---- MINOR 7: the real STRING / array encode path through the client planning helpers ---- + + /// + /// A STRING write through the driver lands as NUL-padded Windows-1252 bytes at the PLC symbol's width; + /// an over-long value is BadOutOfRange and a WSTRING symbol under a String tag BadTypeMismatch. + /// + [Fact] + public async Task String_writes_go_through_the_client_encode_plan() + { + var fake = new FakeTwinCATClient(); + fake.SymbolShapes["MAIN.Name"] = TwinCATPlcSymbolShape.Classify("STRING(4)", DataTypeCategory.String, 5); + fake.SymbolShapes["MAIN.Wide"] = TwinCATPlcSymbolShape.Classify("WSTRING(4)", DataTypeCategory.String, 10); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("Name", Host, "MAIN.Name", TwinCATDataType.String), + new TwinCATTagDefinition("Wide", Host, "MAIN.Wide", TwinCATDataType.String)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var results = await drv.WriteAsync( + [ + new WriteRequest("Name", "äb"), + new WriteRequest("Name", "toolong"), + new WriteRequest("Wide", "x"), + ], TestContext.Current.CancellationToken); + + results.Select(r => r.StatusCode).ShouldBe( + [TwinCATStatusMapper.Good, TwinCATStatusMapper.BadOutOfRange, TwinCATStatusMapper.BadTypeMismatch]); + fake.RawWrites["MAIN.Name"].ShouldBe(new byte[] { 0xE4, (byte)'b', 0, 0, 0 }); + fake.RawWrites.ShouldNotContainKey("MAIN.Wide"); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// An array write through the driver lands as the codec's bytes at the PLC element stride. + [Fact] + public async Task Array_writes_go_through_the_client_encode_plan() + { + var fake = new FakeTwinCATClient(); + var element = TwinCATPlcSymbolShape.Classify("INT", DataTypeCategory.Primitive, 2); + fake.SymbolShapes["MAIN.Buf"] = TwinCATPlcSymbolShape.Classify("ARRAY [0..3] OF INT", DataTypeCategory.Array, 8); + fake.ArrayShapes["MAIN.Buf"] = new TwinCATPlcArrayShape([4], 8, element); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("Buf", Host, "MAIN.Buf", TwinCATDataType.Int, ArrayLength: 2), + new TwinCATTagDefinition("BufAsReal", Host, "MAIN.Buf", TwinCATDataType.Real, ArrayLength: 2)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var results = await drv.WriteAsync( + [ + new WriteRequest("Buf", new[] { 1, -2 }), + new WriteRequest("BufAsReal", new[] { 1.0, 2.0 }), + ], TestContext.Current.CancellationToken); + + results[0].StatusCode.ShouldBe(TwinCATStatusMapper.Good); + fake.RawWrites["MAIN.Buf"].ShouldBe(new byte[] { 0x01, 0x00, 0xFE, 0xFF }); // leading sub-range only + results[1].StatusCode.ShouldBe(TwinCATStatusMapper.BadTypeMismatch); + } + + // ---- MINOR 8: REAL / LREAL range ---- + + /// A REAL (scalar or array element) beyond float range is BadOutOfRange, never written as Infinity. + [Fact] + public async Task Out_of_range_REAL_values_are_BadOutOfRange() + { + Should.Throw(() => AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, 1e300)); + Should.Throw(() => AdsTwinCATClient.ConvertForWrite(TwinCATDataType.LReal, "1e400")); + AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, 3.5).ShouldBe(3.5f); + AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, float.PositiveInfinity).ShouldBe(float.PositiveInfinity); + float.IsNaN((float)AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, double.NaN)).ShouldBeTrue(); + + var fake = new FakeTwinCATClient(); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("Arr", Host, "MAIN.Arr", TwinCATDataType.Real, ArrayLength: 2)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var result = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1.0, 1e300 })], TestContext.Current.CancellationToken); + result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); + fake.ArrayWriteLog.ShouldBeEmpty(); + } +} From a8d313ed5ade26d3041da863776d979507f25fcd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:00:23 -0400 Subject: [PATCH 12/12] fix(twincat): review round 2 - teardown confined to its generation; write health; re-arm epoch - I1: the poll engine belongs to a teardown generation. ShutdownAsync/Dispose retire the current engine and install a fresh one in the same swap as the device/tag tables, then dispose the retired one (possibly in the background after a cancelled or over-budget shutdown). A subscription made after the next InitializeAsync can no longer land in an engine whose late dispose tail would dispose its CTS without cancelling it (a loop no UnsubscribeAsync could find). Poll handles are generation-qualified (TwinCATPollHandle) because each engine numbers handles from 1; SubscribeAsync re-homes a subscription that raced a retirement. - M1: a write's status feeds per-device health - Good clears that device's comms fault and advances LastSuccessfulRead, a comms status degrades, a refusal keeps the state. - M2: a symbol-version re-arm bumps a per-registration epoch; an in-flight retry that fails afterwards no longer re-imposes the backoff. - M3: a re-arm restarts the refusal backoff at 30 s. - M4: documented that PLC-not-in-RUN detection needs the probe. - M5: WSTRING and multi-dimensional array encode-plan tests; the cancelled- shutdown test asserts Unknown health and awaits its background teardown. - M6: an infinity spelled as a string ('Infinity', '-inf') passes like a float/double infinity; a finite overflow stays BadOutOfRange. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AdsTwinCATClient.cs | 3 + .../TwinCATDriver.cs | 143 ++++++++-- .../FakeTwinCATClient.cs | 23 +- .../TwinCATReviewFixRound1Tests.cs | 8 + .../TwinCATReviewFixRound2Tests.cs | 262 ++++++++++++++++++ 5 files changed, 417 insertions(+), 22 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound2Tests.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 9eecbd34..649998b0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -1098,10 +1098,13 @@ internal sealed class AdsTwinCATClient : ITwinCATClient return d; } + // An explicit infinity — a float/double infinity, or a string that SPELLS one ("Infinity", "-inf", "∞") + // — is the caller's own value and passes; a finite number that overflows ("1e400") does not. private static bool IsExplicitInfinity(object? value) => value switch { float f => float.IsInfinity(f), double d => double.IsInfinity(d), + string s => s.Contains("inf", StringComparison.OrdinalIgnoreCase) || s.Contains('∞'), _ => false, }; 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 4d9be07b..8b6c0927 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATDriver.cs @@ -27,7 +27,28 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery private readonly string _driverInstanceId; private readonly ITwinCATClientFactory _clientFactory; private readonly ILogger _logger; - private readonly PollGroupEngine _poll; + // The poll engine belongs to a teardown GENERATION (#739 fix round 2): ShutdownAsync / Dispose retire the + // current engine and install a fresh one in the same swap, then dispose the retired one — possibly in the + // background after a cancelled or over-budget shutdown. PollGroupEngine.DisposeAsync clears whatever its + // own table holds when it finishes, so a subscription made after a later InitializeAsync must never land + // in a retiring engine, or the old teardown would dispose its CTS without cancelling it (a poll loop no + // UnsubscribeAsync can find, running forever). + private volatile PollGeneration _poll; + private long _nextPollGeneration; + + /// One generation's poll engine plus its serial (the serial is part of every handle it issues). + private sealed record PollGeneration(long Serial, PollGroupEngine Engine); + + /// + /// A poll subscription handle, qualified by the engine generation that issued it. Each engine numbers its + /// handles from 1, so without the generation a retired engine's late callback would carry a handle EQUAL + /// to a new subscription's and be taken for it. + /// + private sealed record TwinCATPollHandle(long Generation, ISubscriptionHandle Inner) : ISubscriptionHandle + { + /// + public string DiagnosticId => $"twincat-poll-g{Generation}-{Inner.DiagnosticId}"; + } // Device + tag tables are BUILD-AND-SWAP (#739 L8): InitializeAsync builds fresh tables and publishes // them by reference under _lifecycleLock; ShutdownAsync / Dispose swap in empty ones and release what @@ -120,18 +141,27 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery _logger = logger ?? NullLogger.Instance; _resolver = new EquipmentTagRefResolver( r => _tagsByRawPath.TryGetValue(r, out var t) ? t : null); - _poll = new PollGroupEngine( + _poll = CreatePollEngine(); + } + + /// Builds one generation's poll engine; its callbacks publish generation-qualified handles. + private PollGeneration CreatePollEngine() + { + var driverInstanceId = _driverInstanceId; + var serial = Interlocked.Increment(ref _nextPollGeneration); + return new PollGeneration(serial, new PollGroupEngine( reader: ReadAsync, onChange: (handle, tagRef, snapshot) => - OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), + OnDataChange?.Invoke(this, new DataChangeEventArgs(new TwinCATPollHandle(serial, handle), tagRef, snapshot)), onError: HandlePollError, backoffCap: PollBackoffCap, - onBatch: (handle, items) => OnDataChangeBatch?.Invoke(this, new DataChangeBatchEventArgs(handle, items)), + onBatch: (handle, items) => OnDataChangeBatch?.Invoke(this, + new DataChangeBatchEventArgs(new TwinCATPollHandle(serial, handle), items)), // The engine owns no Meter (Core.Abstractions takes no metrics dependency), so this // closure IS the production consumer of its overrun signal. onOverrun: _ => OtOpcUaTelemetry.PollOverruns.Add(1, new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverTypeTag, DriverTypeNames.TwinCAT), - new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId))); + new KeyValuePair(OtOpcUaTelemetry.PollOverrunDriverInstanceTag, driverInstanceId)))); } /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide. @@ -327,8 +357,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } } - /// Swaps out every table under the lifecycle lock and returns the device set the caller now owns. - private ICollection TakeAllForTeardown() + /// + /// Swaps out every table AND the poll engine under the lifecycle lock and returns what the caller now + /// owns. Everything returned belongs to the retired generation only; nothing a later + /// / creates is reachable from it. + /// + private (ICollection devices, PollGroupEngine poll) TakeAllForTeardown() { lock (_lifecycleLock) { @@ -336,10 +370,16 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery var taken = _devices; _devices = NewDeviceTable(); _tagsByRawPath = EmptyTags; - return taken.Values; + var poll = _poll; + _poll = CreatePollEngine(); + return (taken.Values, poll.Engine); } } + /// The most recent teardown's background work (poll-engine dispose + device release). Test + /// visibility only: a cancelled shutdown leaves it running. + internal Task? LastTeardownForTest { get; private set; } + /// public async Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { @@ -362,11 +402,13 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery foreach (var r in sub.Registrations) { try { r.Dispose(); } catch { } } _nativeSubs.Clear(); - var taken = TakeAllForTeardown(); + var (taken, retiredPoll) = TakeAllForTeardown(); // The poll loops and the device release wind down in parallel (releasing a client can unblock a - // poll loop stuck in a read on it); the whole teardown is bounded. - var teardown = Task.WhenAll(_poll.DisposeAsync().AsTask(), ReleaseDevicesAsync(taken)); + // poll loop stuck in a read on it); the whole teardown is bounded. Both halves own ONLY the retired + // generation, so finishing in the background after a cancelled wait is safe. + var teardown = Task.WhenAll(retiredPoll.DisposeAsync().AsTask(), ReleaseDevicesAsync(taken)); + LastTeardownForTest = teardown; var cancelled = false; try { await teardown.WaitAsync(ShutdownBudget, cancellationToken).ConfigureAwait(false); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) @@ -515,6 +557,19 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery return new DataValueSnapshot(value, status, now, now); } + /// + /// Applies the health contract to a write the client returned a status for: Good is the device + /// answering (clears that device's comms fault, advances LastSuccessfulRead); a comms-class + /// status is a comms fault; anything else is a refusal — the device answered, the state is unchanged. + /// + private void NoteWriteOutcome(DeviceState device, uint status, string reference) + { + var now = DateTime.UtcNow; + if (status == TwinCATStatusMapper.Good) RecordDeviceAnswered(now, device, force: true); + else if (IsCommsStatus(status)) MarkCommsFault($"ADS status {status:X8} writing {reference}", device); + else RecordDeviceAnsweredKeepState(now); + } + /// /// True for a status that means the request did not get a device answer (router / transport / /// timeout). Every other non-Good status is the device refusing this one tag (symbol not found, type @@ -675,6 +730,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery { gate.Release(); } + NoteWriteOutcome(device, results[i].StatusCode, w.FullReference); continue; } @@ -685,12 +741,14 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // array — the inverse of the array read. results[i] = new WriteResult(await client.WriteArrayAsync( symbolName, def.DataType, elements, cancellationToken).ConfigureAwait(false)); + NoteWriteOutcome(device, results[i].StatusCode, w.FullReference); continue; } var status = await client.WriteValueAsync( symbolName, def.DataType, parsed?.BitIndex, w.Value, cancellationToken).ConfigureAwait(false); results[i] = new WriteResult(status); + NoteWriteOutcome(device, status, w.FullReference); } catch (OperationCanceledException) { throw; } catch (NotSupportedException) @@ -826,7 +884,17 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery IReadOnlyList fullReferences, TimeSpan publishingInterval, CancellationToken cancellationToken) { if (!_options.UseNativeNotifications) - return _poll.Subscribe(fullReferences, publishingInterval); + { + // A teardown racing this call may retire the engine we read: re-home the subscription on the + // current one so it never lives in an engine that is being disposed. + while (true) + { + var generation = _poll; + var inner = generation.Engine.Subscribe(fullReferences, publishingInterval); + if (ReferenceEquals(generation, _poll)) return new TwinCATPollHandle(generation.Serial, inner); + generation.Engine.Unsubscribe(inner); + } + } var id = Interlocked.Increment(ref _nextNativeSubId); var handle = new NativeSubscriptionHandle(id); @@ -987,6 +1055,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// ADS state is not Run (coordinator ruling: a stopped / config-mode PLC is Degraded with a /// reason even though it answers), or an uncleared comms fault seen on it. /// + /// + /// The ADS state comes from the probe's ReadState, so PLC-not-in-RUN detection needs the probe + /// (Probe.Enabled, the default); with the probe off the state is unknown and never degrades. + /// private static string? DeviceDegradedReason(DeviceState d) { if (d.ConsecutiveProbeFailures >= ProbeFailuresBeforeDegraded) @@ -1042,9 +1114,20 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// or retried by the reconnect replay; a comms-class failure stays on the every-tick retry. /// /// True when the published status changes (the caller republishes the tag). - private static bool NoteDeadFailure(NativeRegistration reg, uint status, bool refused, DateTime now) + /// The registration. + /// The status the failure maps to. + /// True for a refusal (backs off), false for a comms-class failure. + /// The failure time. + /// The registration's when the attempt + /// started; when a re-arm happened while it was in flight the backoff is NOT re-imposed (the re-arm wins). + private static bool NoteDeadFailure(NativeRegistration reg, uint status, bool refused, DateTime now, + int? epochAtAttempt = null) { - if (refused) + if (epochAtAttempt is int epoch && epoch != reg.RearmEpoch) + { + // A symbol-version change re-armed this registration mid-attempt: keep it re-armed. + } + else if (refused) { reg.RefusalBackoff = reg.RefusalBackoff == TimeSpan.Zero ? RefusedRetryInitial @@ -1141,7 +1224,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery } // Async teardown: the engine drains the in-flight poll without blocking the caller's // thread — this runs inside a DriverInstanceActor mailbox turn. - await _poll.UnsubscribeAsync(handle).ConfigureAwait(false); + // A handle of a retired generation has nothing left to stop: its engine was disposed with it. + var current = _poll; + if (handle is TwinCATPollHandle poll && poll.Generation == current.Serial) + await current.Engine.UnsubscribeAsync(poll.Inner).ConfigureAwait(false); } private sealed record NativeSubscriptionHandle(long Id) : ISubscriptionHandle @@ -1255,9 +1341,25 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery /// Earliest time the probe-tick retry may re-attempt this dead registration. public DateTime NextRetryUtc { get; set; } - /// Current refusal backoff (zero after a comms-class failure or a success). + /// Current refusal backoff (zero after a comms-class failure, a success or a re-arm). public TimeSpan RefusalBackoff { get; set; } + private int _rearmEpoch; + + /// Bumped by every symbol-version re-arm; a retry that started before it must not re-impose a backoff. + public int RearmEpoch => Volatile.Read(ref _rearmEpoch); + + /// + /// Re-arms the retry after a PLC program change: retry on the next tick and restart the backoff + /// from (the refusal it backed off from may no longer apply). + /// + public void Rearm() + { + Interlocked.Increment(ref _rearmEpoch); + RefusalBackoff = TimeSpan.Zero; + NextRetryUtc = DateTime.MinValue; + } + private volatile bool _disposed; /// Gets a value indicating whether this registration has been disposed (its callbacks are dropped). @@ -1793,6 +1895,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery if (reg.HasLiveHandle) continue; // A refused registration waits out its backoff (a symbol-version change re-arms it). if (now < reg.NextRetryUtc) continue; + var epoch = reg.RearmEpoch; try { var newHandle = await client.AddNotificationAsync( @@ -1816,7 +1919,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery // or the reverse); the same failure again publishes nothing. var refusal = ex as TwinCATNotificationRefusedException; var status = refusal?.StatusCode ?? TwinCATStatusMapper.BadCommunicationError; - if (NoteDeadFailure(reg, status, refusal is not null, now)) + if (NoteDeadFailure(reg, status, refusal is not null, now, epoch)) try { reg.OnRefused(status); } catch { /* never abort the rest */ } _logger.LogDebug(ex, "TwinCAT driver '{DriverInstanceId}' probe-tick retry of dead notification " + @@ -1880,7 +1983,7 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery foreach (var d in _devices.Values) if (ReferenceEquals(d.Client, sender)) foreach (var reg in d.NativeRegistrations.Values) - reg.NextRetryUtc = DateTime.MinValue; + reg.Rearm(); OnRediscoveryNeeded?.Invoke(this, new RediscoveryEventArgs( "TwinCAT symbol-version-changed (DeviceSymbolVersionInvalid 0x0711) — PLC program re-downloaded", ScopeHint: "TwinCAT")); @@ -1902,12 +2005,12 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery foreach (var r in sub.Registrations) { try { r.Dispose(); } catch { } } _nativeSubs.Clear(); - var taken = TakeAllForTeardown(); + var (taken, retiredPoll) = TakeAllForTeardown(); var probes = BeginRelease(taken); // PollGroupEngine.DisposeAsync awaits loop tasks; drive it synchronously with a bounded Wait() on // the returned task (no sync-context capture), in parallel with the probe join. - var poll = _poll.DisposeAsync().AsTask(); + var poll = retiredPoll.DisposeAsync().AsTask(); try { Task.WhenAll([poll, .. probes]).Wait(ProbeJoinTimeout); } catch { /* cancel races are expected */ } EndRelease(taken); 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 49a6deea..8559f9af 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 @@ -83,16 +83,31 @@ internal class FakeTwinCATClient : ITwinCATClient public virtual Task<(object? value, uint status)> ReadValueAsync( string symbolPath, TwinCATDataType type, int? bitIndex, int? arrayCount, CancellationToken ct) { + if (BlockRead is { } block) + { + // A read wedged in the (uncancellable) wire call; completes with the normal answer once released. + return block.Task.ContinueWith(_ => + { + BlockRead = null; + return ReadValueAsync(symbolPath, type, bitIndex, arrayCount, CancellationToken.None).Result; + }, TaskScheduler.Default); + } if (ThrowOnRead) throw Exception ?? new InvalidOperationException(); - SingleReadCount++; + Interlocked.Increment(ref _singleReadCount); LastReadArrayCount = arrayCount; var status = ReadStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good; var value = Values.TryGetValue(symbolPath, out var v) ? v : null; return Task.FromResult((value, status)); } + /// When set, does not complete (and ignores cancellation) until this is + /// completed — a poll read wedged in the wire call. + public TaskCompletionSource? BlockRead { get; set; } + + private int _singleReadCount; + /// Gets the number of individual calls served. - public int SingleReadCount { get; private set; } + public int SingleReadCount { get => Volatile.Read(ref _singleReadCount); private set => Volatile.Write(ref _singleReadCount, value); } /// Gets the size of each batch the driver issued, in order (#645). public List SumReadBatches { get; } = new(); /// Gets or sets an exception throws instead of answering — a @@ -264,6 +279,9 @@ internal class FakeTwinCATClient : ITwinCATClient public int AttemptsFor(string symbolPath) => _addAttempts.TryGetValue(symbolPath, out var n) ? n : 0; /// Records the most recently-supplied maxDelayMs for Driver.TwinCAT-014 tests. public int LastMaxDelayMs { get; private set; } + /// When set, awaits this gate BEFORE applying + /// — wedges an attempt in flight whose outcome is a failure. + public TaskCompletionSource? PreAttemptGate { get; set; } /// When set, awaits this gate before creating the handle — /// lets a test wedge a replay/register mid-flight (STAB-17 orphan race). public TaskCompletionSource? AddNotificationGate { get; set; } @@ -283,6 +301,7 @@ internal class FakeTwinCATClient : ITwinCATClient int maxDelayMs, Action onChange, CancellationToken cancellationToken) { _addAttempts.AddOrUpdate(symbolPath, 1, (_, n) => n + 1); + if (PreAttemptGate is { } pre) await pre.Task.ConfigureAwait(false); if (ThrowOnAddNotification) throw Exception ?? new InvalidOperationException("fake AddNotification failure"); if (AddNotificationErrors.TryGetValue(symbolPath, out var perSymbol)) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs index 35ff945b..a25d5292 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound1Tests.cs @@ -210,7 +210,15 @@ public sealed class TwinCATReviewFixRound1Tests await Should.ThrowAsync(() => drv.ShutdownAsync(cts.Token)); sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ProbeJoinTimeout, "the wait ended on the token, not the join bound"); drv.DeviceCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + + // The background teardown finishes once the wedge releases, and disposes the client the wedged + // connect produced (no dangling teardown left behind the test). + var teardown = drv.LastTeardownForTest.ShouldNotBeNull(); wedge.SetResult(); + await teardown.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + await WaitForAsync(() => factory.Clients[0].DisposeCount > 0, TimeSpan.FromSeconds(3)); + factory.Clients[0].DisposeCount.ShouldBeGreaterThan(0); } // ---- MINOR 7: the real STRING / array encode path through the client planning helpers ---- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound2Tests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound2Tests.cs new file mode 100644 index 00000000..0acc860f --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATReviewFixRound2Tests.cs @@ -0,0 +1,262 @@ +using System.Collections.Concurrent; +using Shouldly; +using TwinCAT.TypeSystem; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// +/// #739 review fix round 2: a cancelled shutdown's background teardown is confined to its own generation +/// (I1); write outcomes feed per-device health (M1); a symbol-version re-arm survives an in-flight retry +/// and restarts the backoff (M2/M3); WSTRING / multi-dimensional encode plans (M5); infinity strings (M6). +/// +[Trait("Category", "Unit")] +public sealed class TwinCATReviewFixRound2Tests +{ + private const string Host = "ads://5.23.91.23.1.1:851"; + + private static TwinCATProbeOptions FastProbe => new() + { + Enabled = true, + Interval = TimeSpan.FromMilliseconds(30), + Timeout = TimeSpan.FromMilliseconds(100), + }; + + private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, TwinCATProbeOptions probe, bool native, + params TwinCATTagDefinition[] tags) => + new(new TwinCATDriverOptions + { + Devices = [new TwinCATDeviceOptions(Host)], + RawTags = TwinCATRawTags.Entries(tags), + Probe = probe, + UseNativeNotifications = native, + }, "drv-1", factory); + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(10); + } + + // ---- I1 ---- + + /// + /// The exact reviewed sequence: a poll read is wedged; ShutdownAsync is cancelled (its teardown + /// continues in the background); the same instance is re-initialised and subscribed; the wedge is + /// released and the OLD teardown completes. The NEW subscription keeps polling and delivering (positive + /// control), UnsubscribeAsync finds and stops it, and a final shutdown leaves nothing polling. + /// + [Fact] + public async Task A_cancelled_shutdowns_background_teardown_never_touches_the_next_generations_subscription() + { + var wedge = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var counter = 0; + var clients = new ConcurrentQueue(); + var factory = new FakeTwinCATClientFactory + { + Customise = () => + { + // Client 1 (generation 1) wedges its first read; later clients answer an ever-changing value + // so every poll publishes. + var c = new ChangingValueFake(() => Interlocked.Increment(ref counter)) + { + BlockRead = clients.IsEmpty ? wedge : null, + }; + clients.Enqueue(c); + return c; + }, + }; + var tag = new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt); + var drv = NewDriver(factory, new TwinCATProbeOptions { Enabled = false }, native: false, tag); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var events = new ConcurrentQueue(); + drv.OnDataChange += (_, e) => events.Enqueue(e); + + _ = await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken); + await WaitForAsync(() => !clients.IsEmpty, TimeSpan.FromSeconds(2)); // gen-1 poll read is wedged + + using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(100))) + await Should.ThrowAsync(() => drv.ShutdownAsync(cts.Token)); + var oldTeardown = drv.LastTeardownForTest.ShouldNotBeNull(); + oldTeardown.IsCompleted.ShouldBeFalse("the old teardown must still be running in the background"); + + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var handle = await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken); + + wedge.SetResult(); + await oldTeardown.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + + // Positive control: the new subscription delivers AFTER the old teardown finished. + var afterTeardown = events.Count(e => e.SubscriptionHandle.Equals(handle)); + await WaitForAsync(() => events.Count(e => e.SubscriptionHandle.Equals(handle)) >= afterTeardown + 3, TimeSpan.FromSeconds(3)); + events.Count(e => e.SubscriptionHandle.Equals(handle)).ShouldBeGreaterThanOrEqualTo(afterTeardown + 3); + + // UnsubscribeAsync can still find and stop it. + await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken); + var stoppedAt = events.Count(e => e.SubscriptionHandle.Equals(handle)); + // Measured window: a sibling driver's poll publishes 5 times; the stopped loop must publish nothing. + var witnessFactory = new FakeTwinCATClientFactory { Customise = () => new ChangingValueFake(() => Environment.TickCount) }; + var witnessDrv = NewDriver(witnessFactory, new TwinCATProbeOptions { Enabled = false }, native: false, tag); + await witnessDrv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var witnessEvents = 0; + witnessDrv.OnDataChange += (_, _) => Interlocked.Increment(ref witnessEvents); + _ = await witnessDrv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken); + await WaitForAsync(() => Volatile.Read(ref witnessEvents) >= 5, TimeSpan.FromSeconds(3)); + Volatile.Read(ref witnessEvents).ShouldBeGreaterThanOrEqualTo(5); + await witnessDrv.ShutdownAsync(TestContext.Current.CancellationToken); + + events.Count(e => e.SubscriptionHandle.Equals(handle)).ShouldBe(stoppedAt, "the unsubscribed loop must have stopped"); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// Answers each read with a new value so every poll cycle publishes. + private sealed class ChangingValueFake(Func next) : FakeTwinCATClient + { + public override Task<(object? value, uint status)> ReadValueAsync( + string symbolPath, TwinCATDataType type, int? bitIndex, int? arrayCount, CancellationToken ct) + { + if (BlockRead is not null) return base.ReadValueAsync(symbolPath, type, bitIndex, arrayCount, ct); + Values[symbolPath] = next(); + return base.ReadValueAsync(symbolPath, type, bitIndex, arrayCount, ct); + } + } + + // ---- M1 ---- + + /// + /// A write-only device with the probe off: a wire-failed write degrades; a later Good write is the device + /// answering — Healthy again with the stamp advanced. A refused write (BadNotWritable) changes nothing. + /// + [Fact] + public async Task A_good_write_clears_that_devices_comms_fault() + { + var fake = new FakeTwinCATClient(); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("W", Host, "MAIN.W", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + var initStamp = drv.GetHealth().LastSuccessfulRead!.Value; + + fake.ThrowOnWrite = true; + _ = await drv.WriteAsync([new WriteRequest("W", 1)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + fake.ThrowOnWrite = false; + fake.WriteStatuses["MAIN.W"] = TwinCATStatusMapper.BadNotWritable; + _ = await drv.WriteAsync([new WriteRequest("W", 1)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Degraded, "a refusal says nothing about the comms fault"); + + await WaitForAsync(() => DateTime.UtcNow > initStamp, TimeSpan.FromSeconds(1)); + fake.WriteStatuses.Remove("MAIN.W"); + _ = await drv.WriteAsync([new WriteRequest("W", 1)], TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp); + } + + // ---- M2 + M3 ---- + + /// + /// A symbol-version re-arm that lands while a retry of the refused registration is in flight is not + /// overwritten by that retry's failure: the next tick re-attempts at once. (Without the epoch the + /// failure re-imposed a 30 s+ backoff.) + /// + [Fact] + public async Task A_rearm_during_an_in_flight_retry_is_not_lost() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.T"] = new TwinCATTypeMismatchException("MAIN.T", "PLC STRING"); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: true, + new TwinCATTagDefinition("T", Host, "MAIN.T", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.SubscribeAsync(["T"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + fake.AttemptsFor("MAIN.T").ShouldBe(1); + + var inFlight = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + fake.PreAttemptGate = inFlight; + fake.FireSymbolVersionChanged(); // re-arm: the next tick starts a retry, which wedges on the gate + await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 2, TimeSpan.FromSeconds(3)); + fake.FireSymbolVersionChanged(); // a second program change while that retry is in flight + fake.PreAttemptGate = null; + inFlight.SetResult(); // the in-flight retry now fails (still refused) + + await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 3, TimeSpan.FromSeconds(3)); + fake.AttemptsFor("MAIN.T").ShouldBeGreaterThanOrEqualTo(3); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + /// A re-arm restarts the refusal backoff from the initial delay instead of doubling the old one. + [Fact] + public async Task A_rearm_restarts_the_refusal_backoff() + { + var fake = new FakeTwinCATClient(); + fake.AddNotificationErrors["MAIN.T"] = new TwinCATTypeMismatchException("MAIN.T", "PLC STRING"); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: true, + new TwinCATTagDefinition("T", Host, "MAIN.T", TwinCATDataType.DInt)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + _ = await drv.SubscribeAsync(["T"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + var reg = drv.GetDeviceState(Host)!.NativeRegistrations.Values.Single(); + reg.RefusalBackoff.ShouldBe(TwinCATDriver.RefusedRetryInitial); + + fake.FireSymbolVersionChanged(); + await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 2, TimeSpan.FromSeconds(3)); + await WaitForAsync(() => reg.RefusalBackoff != TimeSpan.Zero, TimeSpan.FromSeconds(1)); + reg.RefusalBackoff.ShouldBe(TwinCATDriver.RefusedRetryInitial); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + } + + // ---- M5 ---- + + /// A WSTRING tag over a WSTRING symbol writes UTF-16LE with an aligned 2-byte terminator, zero-padded. + [Fact] + public async Task A_WSTRING_write_goes_through_the_client_encode_plan() + { + var fake = new FakeTwinCATClient(); + fake.SymbolShapes["MAIN.W"] = TwinCATPlcSymbolShape.Classify("WSTRING(3)", DataTypeCategory.String, 8); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("W", Host, "MAIN.W", TwinCATDataType.WString)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var results = await drv.WriteAsync( + [new WriteRequest("W", "中x"), new WriteRequest("W", "abcd")], TestContext.Current.CancellationToken); + + results[0].StatusCode.ShouldBe(TwinCATStatusMapper.Good); + results[1].StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); // 4 chars do not fit WSTRING(3) + fake.RawWrites["MAIN.W"].ShouldBe(new byte[] { 0x2D, 0x4E, (byte)'x', 0, 0, 0, 0, 0 }); + } + + /// A multi-dimensional array is written flattened in PLC memory order (last index fastest). + [Fact] + public async Task A_multi_dimensional_array_write_is_flattened_row_major() + { + var fake = new FakeTwinCATClient(); + var element = TwinCATPlcSymbolShape.Classify("INT", DataTypeCategory.Primitive, 2); + fake.SymbolShapes["MAIN.M"] = TwinCATPlcSymbolShape.Classify("ARRAY [0..1,0..2] OF INT", DataTypeCategory.Array, 12); + fake.ArrayShapes["MAIN.M"] = new TwinCATPlcArrayShape([2, 3], 12, element); + var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, + new TwinCATProbeOptions { Enabled = false }, native: false, + new TwinCATTagDefinition("M", Host, "MAIN.M", TwinCATDataType.Int, ArrayLength: 6)); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var result = await drv.WriteAsync([new WriteRequest("M", new[] { 1, 2, 3, 4, 5, 6 })], TestContext.Current.CancellationToken); + + result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good); + fake.RawWrites["MAIN.M"].ShouldBe(new byte[] { 1, 0, 2, 0, 3, 0, 4, 0, 5, 0, 6, 0 }); + TwinCATArrayCodec.Decode(TwinCATDataType.Int, fake.RawWrites["MAIN.M"], new TwinCATArrayLayout(6, 2), false) + .ShouldBe(new short[] { 1, 2, 3, 4, 5, 6 }); + } + + // ---- M6 ---- + + /// An infinity spelled as a string passes like a float/double infinity; a finite overflow does not. + [Fact] + public void Infinity_strings_are_explicit_infinities() + { + AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, "Infinity").ShouldBe(float.PositiveInfinity); + AdsTwinCATClient.ConvertForWrite(TwinCATDataType.LReal, "-Infinity").ShouldBe(double.NegativeInfinity); + Should.Throw(() => AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, "1e300")); + } +}