From f179829ca6ccf3a99428fd9eb59df32c7b32f331 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:47:24 -0400 Subject: [PATCH] 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(); + } +}