From a32894e93b5eaa1339be6afa9f4976b4bf072b8d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:03:08 -0400 Subject: [PATCH 01/12] fix(focas): build-and-swap the device table; make the lifecycle overlap-safe (R07:H13) _devices was a plain Dictionary cleared and refilled in place by InitializeAsync / ShutdownAsync while the retained poll engine, the fixed-tree and alarm loops and the actor's health tick enumerated it ("Collection was modified"). It is now built whole and published by reference swap, and a release swaps it out to empty before tearing it down. Lane A's runtime overlaps lifecycle calls, so the driver now tolerates both shapes: - a lifecycle generation is bumped by every Initialize/Shutdown; an initialise that finds itself superseded (abandoned by the runtime, then a shutdown ran) publishes nothing, starts no loops, writes no health and throws OperationCanceledException; - a shutdown overlapping another (dispose beside a still-running shutdown) waits for the first's teardown via a release tail instead of returning on an empty table; - released devices are Retired: EnsureConnectedAsync refuses to reconnect them, and a connect that completes after retirement disposes what it opened. Clients are disposed under the device ConnectGate (bounded). Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 334 ++++++++++++------ .../FakeFocasClient.cs | 2 +- .../FocasDeviceTableLifecycleTests.cs | 200 +++++++++++ 3 files changed, 424 insertions(+), 112 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDeviceTableLifecycleTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index cecc6299..f0c0226f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -43,7 +43,40 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, private readonly Func? _rebind; private readonly PollGroupEngine _poll; private readonly ILogger _logger; - private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); + /// + /// Host address → device state. Built whole in and SWAPPED in (and swapped + /// out whole by a release), never cleared or refilled in place: the retained poll engine, the fixed-tree + /// and alarm loops and the actor's health tick all read it concurrently with a re-initialise, and a + /// mutated under an enumerator throws "Collection was modified". + /// Readers take one Volatile.Read snapshot and use it for the whole operation. + /// + private IReadOnlyDictionary _devices = EmptyDevices; + + private static readonly IReadOnlyDictionary EmptyDevices = + new Dictionary(StringComparer.OrdinalIgnoreCase); + + /// The live device table (one consistent snapshot). + private IReadOnlyDictionary Devices => Volatile.Read(ref _devices); + + /// + /// Guards the publish/retire of the device table against an overlapping lifecycle call. Held only for + /// synchronous swaps — never across an await, and nothing a loop runs ever takes it. + /// + private readonly object _lifecycleLock = new(); + + /// + /// Bumped by every and . An initialise that finds + /// the counter moved on by the time it would publish has been superseded (the runtime abandoned it and + /// tore the driver down) and must start nothing. + /// + private int _lifecycleGeneration; + + /// + /// Completes when every device table released so far has finished its teardown. A shutdown that overlaps + /// another (Lane A's dispose beside a still-running shutdown) awaits it, so it never returns while the + /// first one is still joining loops and disposing clients. + /// + private Task _releaseTail = Task.CompletedTask; // v3: RawPath → definition, case-sensitive ordinal. Built in InitializeAsync from _options.RawTags // via the pure FocasTagDefinitionFactory mapper (the reference is always a RawPath). // Built whole in InitializeAsync and SWAPPED in: the retained poll engine keeps calling @@ -196,9 +229,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // release the previous device states' loops + clients BEFORE they are overwritten (a live driver can be // re-initialised without a teardown first). Not ShutdownAsync: that also drops _poll // and subscriptions, which are owned by the shutdown path (the actor re-subscribes after every successful init). + // + // The generation is taken FIRST: an initialise the runtime abandons (it timed out and moved on to a + // shutdown) is superseded by that shutdown's bump, and checks for it before publishing anything. + var generation = Interlocked.Increment(ref _lifecycleGeneration); await ReleaseDeviceStatesAsync().ConfigureAwait(false); ResetAlarmHealth(); - Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null)); + WriteHealthIfCurrent(generation, new DriverHealth(DriverState.Initializing, null, null)); + var devices = new Dictionary(StringComparer.OrdinalIgnoreCase); try { // adopt a changed config. Options and the backend client factory are re-derived by ONE @@ -222,7 +260,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, var addr = FocasHostAddress.TryParse(device.HostAddress) ?? throw new InvalidOperationException( $"FOCAS device has invalid HostAddress '{device.HostAddress}' — expected 'focas://{{ip}}[:{{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 // blob is mapped by the pure factory; the entry's WriteIdempotent flag + DeviceName are @@ -256,7 +294,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, throw new InvalidOperationException( $"FOCAS tag '{entry.RawPath}' references device '{entry.DeviceName}' " + $"which is not in the Devices list. Check for a typo (e.g. wrong port or hostname)."); - if (!_devices.TryGetValue(deviceHost, out var device)) + if (!devices.TryGetValue(deviceHost, out var device)) throw new InvalidOperationException( $"FOCAS tag '{entry.RawPath}' references device '{entry.DeviceName}' " + $"which is not in the Devices list. Check for a typo (e.g. wrong port or hostname)."); @@ -309,75 +347,135 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // above; reusing the parsed record avoids per-tick allocs on subscription pollers. parsedAddresses[entry.RawPath] = parsed; } - // Publish the skip tally only once the whole table is built. An unknown-device throw above - // leaves the previous snapshot standing rather than a truncated one that would read as - // "we now skip fewer tags". - _skippedTags.Commit(); - Volatile.Write(ref _parsedAddressesByTagName, parsedAddresses); - Volatile.Write(ref _tagsByRawPath, tagsByRawPath); - Volatile.Write(ref _alarmTags, alarmTags); - - if (_options.Probe.Enabled) + // Publish everything at once, and only if no later lifecycle call has superseded this one. The check + // and the publish share the lock with the shutdown's swap-out, so either the shutdown sees (and + // releases) this table, or this initialise sees the shutdown and publishes nothing. + lock (_lifecycleLock) { - foreach (var state in _devices.Values) - { - state.ProbeCts = new CancellationTokenSource(); - var ct = state.ProbeCts.Token; - state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); - } - } + if (generation != Volatile.Read(ref _lifecycleGeneration)) + throw new OperationCanceledException( + $"FOCAS initialise of {_driverInstanceId} was superseded by a later shutdown/initialise; nothing was started."); - if (_options.HandleRecycle.Enabled) - { - foreach (var state in _devices.Values) - { - state.RecycleCts = new CancellationTokenSource(); - var ct = state.RecycleCts.Token; - state.RecycleTask = Task.Run(() => RecycleLoopAsync(state, ct), ct); - } - } - - // See _fixedTreeActive: an authored FIXED: tag arms the loop on its own, because the - // config flag defaults off and the operator who committed those tags has already said - // what they want. - _fixedTreeActive = _options.FixedTree.Enabled - || _parsedAddressesByTagName.Values.Any(a => a.Kind == FocasAreaKind.Fixed); - if (_fixedTreeActive) - { - foreach (var state in _devices.Values) - { - state.FixedTreeCts = new CancellationTokenSource(); - var ct = state.FixedTreeCts.Token; - state.FixedTreeTask = Task.Run(() => FixedTreeLoopAsync(state, ct), ct); - } + // Publish the skip tally only once the whole table is built. An unknown-device throw above + // leaves the previous snapshot standing rather than a truncated one that would read as + // "we now skip fewer tags". + _skippedTags.Commit(); + Volatile.Write(ref _parsedAddressesByTagName, parsedAddresses); + Volatile.Write(ref _tagsByRawPath, tagsByRawPath); + Volatile.Write(ref _alarmTags, alarmTags); + // See _fixedTreeActive: an authored FIXED: tag arms the loop on its own, because the + // config flag defaults off and the operator who committed those tags has already said + // what they want. + _fixedTreeActive = _options.FixedTree.Enabled + || parsedAddresses.Values.Any(a => a.Kind == FocasAreaKind.Fixed); + Volatile.Write(ref _devices, devices); + StartDeviceLoops(devices.Values); } // Restart the alarm poll a re-initialise paused, against the tables just rebuilt. A // driver with no subscription yet starts nothing — the actor subscribes once it is Connected. await _alarmProjection.ResumeAsync().ConfigureAwait(false); + // A shutdown that landed while the resume ran owns the projection now: put it back the way it left it. + if (generation != Volatile.Read(ref _lifecycleGeneration)) + { + await _alarmProjection.PauseAsync().ConfigureAwait(false); + throw new OperationCanceledException( + $"FOCAS initialise of {_driverInstanceId} was superseded while resuming the alarm poll."); + } - Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); + WriteHealthIfCurrent(generation, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); + } + catch (OperationCanceledException) when (generation != Volatile.Read(ref _lifecycleGeneration)) + { + // Superseded: whatever this attempt built but never published is released here (a published table + // is the superseding call's to release); health belongs to that call too. + if (!ReferenceEquals(Devices, devices)) await ReleaseTableAsync(devices.Values).ConfigureAwait(false); + throw; } catch (Exception ex) { - Volatile.Write(ref _health, new DriverHealth(DriverState.Faulted, null, ex.Message)); + WriteHealthIfCurrent(generation, new DriverHealth(DriverState.Faulted, null, ex.Message)); throw; } } + /// Writes unless a later lifecycle call has taken over. + private void WriteHealthIfCurrent(int generation, DriverHealth health) + { + lock (_lifecycleLock) + { + if (generation == Volatile.Read(ref _lifecycleGeneration)) Volatile.Write(ref _health, health); + } + } + + /// Starts the probe / recycle / fixed-tree loops for a freshly built table. Caller holds the lifecycle lock. + private void StartDeviceLoops(IEnumerable states) + { + foreach (var state in states) + { + if (_options.Probe.Enabled) + { + state.ProbeCts = new CancellationTokenSource(); + var ct = state.ProbeCts.Token; + state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); + } + if (_options.HandleRecycle.Enabled) + { + state.RecycleCts = new CancellationTokenSource(); + var ct = state.RecycleCts.Token; + state.RecycleTask = Task.Run(() => RecycleLoopAsync(state, ct), ct); + } + if (_fixedTreeActive) + { + state.FixedTreeCts = new CancellationTokenSource(); + var ct = state.FixedTreeCts.Token; + state.FixedTreeTask = Task.Run(() => FixedTreeLoopAsync(state, ct), ct); + } + } + } + /// - /// Cancels, bounded-joins and disposes every existing device state (probe / recycle / fixed-tree - /// loops + client) and the alarm projection, then clears the table. Device part of - /// only (no poll / subscription teardown). + /// Swaps the live device table out (to empty) and releases it: cancels, bounded-joins and disposes every + /// device state's loops and client, after pausing the alarm projection. Device part of + /// only (no poll / subscription teardown). Also waits for any release an + /// overlapping lifecycle call started earlier, so it never returns while that one is still running. /// private async Task ReleaseDeviceStatesAsync() { // Pause, not dispose: the subscription and what the server was told survive the rebuild. await _alarmProjection.PauseAsync().ConfigureAwait(false); - if (_devices.Count == 0) return; - var loops = new List(); - foreach (var state in _devices.Values) + IReadOnlyDictionary old; + Task prior; + var done = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + lock (_lifecycleLock) { + old = _devices; + Volatile.Write(ref _devices, EmptyDevices); + prior = _releaseTail; + _releaseTail = done.Task; + } + try + { + if (old.Count > 0) await ReleaseTableAsync(old.Values).ConfigureAwait(false); + await prior.ConfigureAwait(false); + } + finally { done.TrySetResult(); } + } + + /// + /// Retires and tears down one swapped-out table: retire every device first (so a connect still in flight + /// disposes what it opens), cancel every loop, join them (bounded), then dispose the CTSs and — under each + /// device's connect gate — its client. + /// + private async Task ReleaseTableAsync(IEnumerable table) + { + var states = table.ToList(); + var loops = new List(); + foreach (var state in states) + { + state.Retire(); + // Cancel-then-dispose can race in tight shutdown loops; swallowing is intentional + // but we log the cause so a noisy shutdown leaves a Debug trace. try { state.ProbeCts?.Cancel(); } catch (Exception ex) { _logger.LogDebug(ex, "Cancelling probe CTS for {Host} failed", state.Options.HostAddress); } try { state.RecycleCts?.Cancel(); } @@ -388,15 +486,18 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (state.RecycleTask is { } rt) loops.Add(rt); if (state.FixedTreeTask is { } ft) loops.Add(ft); } + // Cancel every loop first, JOIN them (bounded), and only then dispose the CTSs and the + // client. Fire-and-forget loops used to outlive shutdown: one parked in EnsureConnectedAsync could + // finish its connect AFTER DisposeClient and leave a live socket on a shut-down driver. if (loops.Count > 0) { var all = Task.WhenAll(loops); var finished = await Task.WhenAny(all, Task.Delay(LoopJoinTimeout, CancellationToken.None)).ConfigureAwait(false); if (finished != all) - _logger.LogWarning("FOCAS re-initialise: {Count} background loop(s) did not stop within {Timeout}; proceeding", + _logger.LogWarning("FOCAS teardown: {Count} background loop(s) did not stop within {Timeout}; proceeding", loops.Count, LoopJoinTimeout); } - foreach (var state in _devices.Values) + foreach (var state in states) { state.ProbeCts?.Dispose(); state.ProbeCts = null; @@ -407,9 +508,22 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, state.FixedTreeCts?.Dispose(); state.FixedTreeCts = null; state.FixedTreeTask = null; - state.DisposeClient(); + await DisposeClientGatedAsync(state).ConfigureAwait(false); } - _devices.Clear(); + } + + /// + /// Disposes a device's client under its , so it can neither null a + /// client a concurrent just installed nor race that connect. The wait + /// is bounded: a connect wedged past may not wedge teardown too (the device + /// is retired, so that connect disposes its own client when it finally returns). + /// + private async Task DisposeClientGatedAsync(DeviceState state) + { + var entered = await state.ConnectGate.WaitAsync(LoopJoinTimeout).ConfigureAwait(false); + try { state.DisposeClient(); } + catch (Exception ex) { _logger.LogDebug(ex, "FOCAS client dispose failed for {Host}", state.Options.HostAddress); } + finally { if (entered) state.ConnectGate.Release(); } } /// @@ -423,58 +537,28 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, internal static readonly TimeSpan LoopJoinTimeout = TimeSpan.FromSeconds(5); /// + // Safe to overlap with itself (Lane A disposes beside a still-running shutdown — the second call waits for + // the first's teardown via _releaseTail) and with an abandoned InitializeAsync (the generation bump below + // supersedes it, so it publishes nothing). public async Task ShutdownAsync(CancellationToken cancellationToken) { + var generation = Interlocked.Increment(ref _lifecycleGeneration); await _poll.DisposeAsync().ConfigureAwait(false); - // Pause, not dispose — see _alarmProjection. A later InitializeAsync resumes it. - await _alarmProjection.PauseAsync().ConfigureAwait(false); - // cancel every loop first, JOIN them (bounded), and only then dispose the CTSs and the - // client. Fire-and-forget loops used to outlive shutdown: one parked in EnsureConnectedAsync could - // finish its connect AFTER DisposeClient and leave a live socket on a shut-down driver. - var loops = new List(); - foreach (var state in _devices.Values) + // Pauses (not disposes) the alarm projection — see _alarmProjection; a later InitializeAsync resumes it. + await ReleaseDeviceStatesAsync().ConfigureAwait(false); + lock (_lifecycleLock) { - // Cancel-then-dispose can race in tight shutdown loops; swallowing is intentional - // but we now log the cause so a noisy shutdown leaves a Debug trace. - try { state.ProbeCts?.Cancel(); } - catch (Exception ex) { _logger.LogDebug(ex, "Cancelling probe CTS for {Host} failed", state.Options.HostAddress); } - try { state.RecycleCts?.Cancel(); } - catch (Exception ex) { _logger.LogDebug(ex, "Cancelling recycle CTS for {Host} failed", state.Options.HostAddress); } - try { state.FixedTreeCts?.Cancel(); } - catch (Exception ex) { _logger.LogDebug(ex, "Cancelling fixed-tree CTS for {Host} failed", state.Options.HostAddress); } - if (state.ProbeTask is { } pt) loops.Add(pt); - if (state.RecycleTask is { } rt) loops.Add(rt); - if (state.FixedTreeTask is { } ft) loops.Add(ft); + // A later initialise owns the tables (and health) now — leave them alone. + if (generation != Volatile.Read(ref _lifecycleGeneration)) return; + Volatile.Write(ref _tagsByRawPath, new Dictionary(StringComparer.Ordinal)); + Volatile.Write(ref _alarmTags, new Dictionary(StringComparer.Ordinal)); + Volatile.Write(ref _parsedAddressesByTagName, + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase)); + _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry + _fixedTreeActive = false; + ResetAlarmHealth(); + Volatile.Write(ref _health, new DriverHealth(DriverState.Unknown, Volatile.Read(ref _health).LastSuccessfulRead, null)); } - if (loops.Count > 0) - { - var all = Task.WhenAll(loops); - var finished = await Task.WhenAny(all, Task.Delay(LoopJoinTimeout, CancellationToken.None)).ConfigureAwait(false); - if (finished != all) - _logger.LogWarning("FOCAS shutdown: {Count} background loop(s) did not stop within {Timeout}; proceeding with teardown", - loops.Count, LoopJoinTimeout); - } - foreach (var state in _devices.Values) - { - state.ProbeCts?.Dispose(); - state.ProbeCts = null; - state.ProbeTask = null; - state.RecycleCts?.Dispose(); - state.RecycleCts = null; - state.RecycleTask = null; - state.FixedTreeCts?.Dispose(); - state.FixedTreeCts = null; - state.FixedTreeTask = null; - state.DisposeClient(); - } - _devices.Clear(); - Volatile.Write(ref _tagsByRawPath, new Dictionary(StringComparer.Ordinal)); - Volatile.Write(ref _alarmTags, new Dictionary(StringComparer.Ordinal)); - _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - _parsedAddressesByTagName.Clear(); - _fixedTreeActive = false; - ResetAlarmHealth(); - Volatile.Write(ref _health, new DriverHealth(DriverState.Unknown, Volatile.Read(ref _health).LastSuccessfulRead, null)); } /// @@ -499,12 +583,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// Gets the number of configured devices. - internal int DeviceCount => _devices.Count; + internal int DeviceCount => Devices.Count; + /// Test seam — the live device table snapshot (swapped whole, never mutated in place). + internal IReadOnlyDictionary DeviceTable => Devices; /// Gets the state of a device by host address. /// The host address of the device. /// The device state if found; otherwise null. internal DeviceState? GetDeviceState(string hostAddress) => - _devices.TryGetValue(hostAddress, out var s) ? s : null; + Devices.TryGetValue(hostAddress, out var s) ? s : null; /// Test seam — the resolved options the factory built this driver from. internal FocasDriverOptions Options => _options; @@ -592,7 +678,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadNodeIdUnknown, null, now); continue; } - if (!_devices.TryGetValue(def.DeviceHostAddress, out var device)) + if (!Devices.TryGetValue(def.DeviceHostAddress, out var device)) { results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadNodeIdUnknown, null, now); continue; @@ -680,7 +766,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, var reference = references[i]; if (_fixedTreeActive && TryReadFixedTree(reference, now) is not null) continue; if (!_resolver.TryResolve(reference, out var def)) continue; - if (!_devices.TryGetValue(def.DeviceHostAddress, out var device)) continue; + if (!Devices.TryGetValue(def.DeviceHostAddress, out var device)) continue; var parsed = ResolveParsedAddress(def); if (parsed.Kind != FocasAreaKind.Pmc || parsed.PmcLetter is null) continue; (byDevice ??= []).TryAdd(device, []); @@ -802,7 +888,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[i] = new WriteResult(FocasStatusMapper.BadNotWritable); continue; } - if (!_devices.TryGetValue(def.DeviceHostAddress, out var device)) + if (!Devices.TryGetValue(def.DeviceHostAddress, out var device)) { results[i] = new WriteResult(FocasStatusMapper.BadNodeIdUnknown); continue; @@ -890,7 +976,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Fixed-tree T1 — Identity + Axes subtrees, populated once per session // from cnc_sysinfo + cnc_rdaxisname at init time and kept in DeviceState. if (_fixedTreeActive - && _devices.TryGetValue(device.HostAddress, out var state) + && Devices.TryGetValue(device.HostAddress, out var state) && state.FixedTreeCache is { } cache) { var identity = deviceFolder.Folder("Identity", "Identity"); @@ -1142,7 +1228,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// public IReadOnlyList GetHostStatuses() => - [.. _devices.Values.Select(s => new HostConnectivityStatus(s.Options.HostAddress, s.HostState, s.HostStateChangedUtc))]; + [.. Devices.Values.Select(s => new HostConnectivityStatus(s.Options.HostAddress, s.HostState, s.HostStateChangedUtc))]; private async Task ProbeLoopAsync(DeviceState state, CancellationToken ct) { @@ -1510,7 +1596,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// private DataValueSnapshot? TryReadFixedTree(string reference, DateTime now) { - foreach (var state in _devices.Values) + foreach (var state in Devices.Values) { if (!reference.StartsWith(state.Options.HostAddress + "/", StringComparison.OrdinalIgnoreCase)) continue; if (TryReadFixedTreeOnDevice(state, reference[(state.Options.HostAddress.Length + 1)..], now) is { } hit) @@ -1776,7 +1862,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// The device's active alarms. internal async Task> ReadDeviceAlarmsAsync(string hostAddress, CancellationToken ct) { - if (!_devices.TryGetValue(hostAddress, out var state)) + if (!Devices.TryGetValue(hostAddress, out var state)) throw new InvalidOperationException($"FOCAS device '{hostAddress}' is not configured."); var client = await EnsureConnectedAsync(state, ct).ConfigureAwait(false); return await client.ReadAlarmsAsync(ct).ConfigureAwait(false); @@ -1822,6 +1908,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, await device.ConnectGate.WaitAsync(ct).ConfigureAwait(false); try { + // A released device (re-initialise / shutdown) is never reconnected: whoever still holds it read + // the table before the swap, and a client opened now would be orphaned on a state nobody tears down. + if (device.IsRetired) + throw new ObjectDisposedException(nameof(DeviceState), + $"FOCAS device {device.Options.HostAddress} was released by a re-initialise or shutdown."); + // Another caller may have connected while this one waited. if (device.Client is { IsConnected: true } connected) return connected; @@ -1864,6 +1956,15 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, throw; } device.Backoff.RecordSuccess(); // recovery resets the window immediately + // Retired WHILE this connect ran (the release waits on this gate only up to its bound): the connect + // is ours to undo, or it leaves a live session on a shut-down driver. + if (device.IsRetired) + { + client.Dispose(); + if (ReferenceEquals(device.Client, client)) device.Client = null; + throw new ObjectDisposedException(nameof(DeviceState), + $"FOCAS device {device.Options.HostAddress} was released while connecting."); + } return client; } finally { device.ConnectGate.Release(); } @@ -2020,6 +2121,17 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// public ConcurrentDictionary LastSpindleMotorSpeeds { get; } = new(); + private int _retired; + + /// + /// True once a re-initialise / shutdown has released this device. A retired device never connects + /// again, and a connect that was already running when it retired disposes what it opened. + /// + public bool IsRetired => Volatile.Read(ref _retired) != 0; + + /// Marks the device released. Idempotent. + public void Retire() => Volatile.Write(ref _retired, 1); + /// Disposes the FOCAS client instance. public void DisposeClient() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FakeFocasClient.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FakeFocasClient.cs index 00cf61d5..370bff15 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FakeFocasClient.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FakeFocasClient.cs @@ -259,7 +259,7 @@ internal sealed class FakeFocasClientFactory : IFocasClientFactory public IFocasClient Create() { var c = Customise?.Invoke() ?? new FakeFocasClient(); - Clients.Add(c); + lock (Clients) Clients.Add(c); return c; } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDeviceTableLifecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDeviceTableLifecycleTests.cs new file mode 100644 index 00000000..056b7ca8 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDeviceTableLifecycleTests.cs @@ -0,0 +1,200 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:H13 — the device table is read by the retained poll engine, the fixed-tree loop, the alarm +/// loop and the actor's health tick, so a re-initialise must build a fresh table and swap it in rather than +/// clearing and refilling the live one. Also pins the lifecycle overlaps the runtime produces since Lane A: +/// a dispose running beside a still-running shutdown, and an abandoned initialise overlapping the next +/// attempt's shutdown. +/// +[Trait("Category", "Unit")] +public sealed class FocasDeviceTableLifecycleTests +{ + private const string Host = "focas://10.0.0.5:8193"; + + private static FocasDriverOptions Options(bool probe = false, bool fixedTree = false) => new() + { + Devices = [new FocasDeviceOptions(Host)], + RawTags = [FocasRawTags.Entry(new FocasTagDefinition("raw/R", Host, "R100", FocasDataType.Byte))], + Probe = new FocasProbeOptions { Enabled = probe, Interval = TimeSpan.FromMilliseconds(20) }, + FixedTree = new FocasFixedTreeOptions { Enabled = fixedTree, PollInterval = TimeSpan.FromMilliseconds(10) }, + }; + + /// A re-initialise publishes a NEW table; a reader holding the old one never sees it emptied. + [Fact] + public async Task Reinitialize_swaps_in_a_new_table_and_never_mutates_the_one_a_reader_holds() + { + var drv = new FocasDriver(Options(), "focas-swap", new FakeFocasClientFactory()); + await drv.InitializeAsync("{}", CancellationToken.None); + var before = drv.DeviceTable; + before.Count.ShouldBe(1); + + await drv.InitializeAsync("{}", CancellationToken.None); + + drv.DeviceTable.ShouldNotBeSameAs(before); + drv.DeviceTable.Count.ShouldBe(1); + before.Count.ShouldBe(1, "the table a concurrent reader enumerates must not be cleared under it"); + await drv.ShutdownAsync(CancellationToken.None); + before.Count.ShouldBe(1); + drv.DeviceTable.Count.ShouldBe(0); + } + + /// Hammering every table reader during repeated re-initialises throws nothing — and reads still succeed. + [Fact] + public async Task Readers_survive_repeated_reinitialise() + { + var factory = new FakeFocasClientFactory(); + var drv = new FocasDriver(Options(fixedTree: true), "focas-hammer", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + using var stop = new CancellationTokenSource(); + var good = 0; + var errors = new List(); + var reader = Task.Run(async () => + { + while (!stop.IsCancellationRequested) + { + try + { + _ = drv.GetHostStatuses(); + var snaps = await drv.ReadAsync(["raw/R", $"{Host}/Identity/SeriesNumber"], CancellationToken.None); + if (snaps[0].StatusCode == FocasStatusMapper.Good) Interlocked.Increment(ref good); + } + catch (Exception ex) { lock (errors) errors.Add(ex); } + } + }); + + for (var i = 0; i < 100; i++) + await drv.InitializeAsync("{}", CancellationToken.None); + await Task.Delay(100); + stop.Cancel(); + await reader; + await drv.ShutdownAsync(CancellationToken.None); + + errors.ShouldBeEmpty(); + good.ShouldBeGreaterThan(0, "positive control: the reader really did read through the table"); + } + + /// A client whose connect parks until released and ignores cancellation. + private sealed class ParkedConnectClient(TaskCompletionSource entered, Task release) : FakeFocasClient + { + public override async Task ConnectAsync(FocasHostAddress address, TimeSpan timeout, CancellationToken ct) + { + entered.TrySetResult(); + await release.ConfigureAwait(false); + await base.ConnectAsync(address, timeout, CancellationToken.None).ConfigureAwait(false); + } + } + + /// + /// A data-path read parked in connect when shutdown runs must not leave a live client on the released + /// device: the device is retired, so the connect that completes afterwards disposes what it opened. + /// + [Fact] + public async Task Connect_completing_after_shutdown_does_not_leave_a_live_client() + { + var entered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeFocasClientFactory { Customise = () => new ParkedConnectClient(entered, release.Task) }; + var drv = new FocasDriver(Options(), "focas-retire", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var read = drv.ReadAsync(["raw/R"], CancellationToken.None); + await entered.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + await Task.Delay(50); + release.SetResult(); + await shutdown.WaitAsync(TimeSpan.FromSeconds(10)); + var snaps = await read.WaitAsync(TimeSpan.FromSeconds(5)); + + factory.Clients.Count.ShouldBe(1); + factory.Clients[0].IsConnected.ShouldBeFalse("a connect finishing on a retired device must dispose its client"); + factory.Clients[0].DisposeCount.ShouldBeGreaterThanOrEqualTo(1); + snaps[0].StatusCode.ShouldBe(FocasStatusMapper.BadCommunicationError); + } + + /// + /// An initialise abandoned mid-way (the runtime timed it out and moved on to a shutdown) must not + /// resurrect the driver when it finally completes: no device table, no loops, no connection. + /// + [Fact] + public async Task Abandoned_initialise_finishing_after_shutdown_starts_nothing() + { + var factory = new FakeFocasClientFactory(); + var options = Options(probe: true); + using var gate = new ManualResetEventSlim(false); + var inRebind = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var drv = new FocasDriver(options, "focas-abandoned", factory, rebind: _ => + { + inRebind.TrySetResult(); + gate.Wait(TimeSpan.FromSeconds(10)); + return new FocasDriverBinding(options, factory); + }); + + var init = Task.Run(() => drv.InitializeAsync("{\"devices\":[]}", CancellationToken.None)); + await inRebind.Task.WaitAsync(TimeSpan.FromSeconds(5)); + await drv.ShutdownAsync(CancellationToken.None); + gate.Set(); + await Should.ThrowAsync(() => init.WaitAsync(TimeSpan.FromSeconds(10))); + + await Task.Delay(200); // several probe intervals — a started loop would have connected by now + drv.DeviceCount.ShouldBe(0); + factory.Clients.ShouldBeEmpty(); + drv.GetHealth().State.ShouldNotBe(DriverState.Healthy); + + // Positive control: the same driver, initialised without being superseded, does start its probe. + await drv.InitializeAsync("{\"devices\":[]}", CancellationToken.None); + await WaitUntil(() => factory.Clients.Count > 0); + drv.DeviceCount.ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.DisposeAsync(); + } + + /// A loop wedged in a read that ignores cancellation for a while. + private sealed class SlowProbeClient : FakeFocasClient + { + public override async Task ProbeAsync(CancellationToken ct) + { + await Task.Delay(400, CancellationToken.None).ConfigureAwait(false); + return true; + } + } + + /// + /// Lane A disposes while its own shutdown may still be running. The dispose must not return before the + /// teardown it overlaps has released the client — it waits for it rather than finding an empty table. + /// + [Fact] + public async Task Dispose_concurrent_with_shutdown_waits_for_the_teardown() + { + var factory = new FakeFocasClientFactory { Customise = () => new SlowProbeClient() }; + var drv = new FocasDriver(Options(probe: true), "focas-dispose", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntil(() => factory.Clients.Count > 0 && factory.Clients[0].IsConnected); + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + var dispose = drv.DisposeAsync().AsTask(); + await dispose.WaitAsync(TimeSpan.FromSeconds(10)); + + factory.Clients.ShouldAllBe(c => !c.IsConnected); + factory.Clients.ShouldAllBe(c => c.DisposeCount >= 1); + await shutdown.WaitAsync(TimeSpan.FromSeconds(10)); + drv.DeviceCount.ShouldBe(0); + } + + private static async Task WaitUntil(Func condition) + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("condition not reached"); + await Task.Delay(10); + } + } +} From 12b981dc1bd522e27b9b4c4eb7858226a37e9491 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:07:42 -0400 Subject: [PATCH 02/12] fix(focas): carry macro/parameter numbers to the CNC unnarrowed (R07:H14) The capability matrix admits #0..99999 on 30i/31i/32i (and anything on Unknown), and the FOCAS/2 request block has always carried 32-bit arguments, but WireFocasClient cast every number to short: MACRO:99999 asked the CNC for #-31073 and published that variable at Good under the authored tag. FocasWireClient's macro/parameter reads, WireMacro.Number and WireParameter.DataNumber are now int; the request blocks come from MacroRequest / ParameterRequest builders (byte-identical to the live 31i-B capture for #500). Same class on PMC: a range whose last byte passes 65535 (Unknown series admits it) is now refused BadOutOfRange instead of wrapping onto a different 16-bit address. The wire client is in-process (the other end is the CNC itself), so this is not a cross-repo contract change. Whether a real 30i honours cnc_rdmacro Arg1 > 32767 is the hardware userGate. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Wire/FocasWireClient.cs | 54 ++++++++---- .../Wire/FocasWireModels.cs | 4 +- .../Wire/WireFocasClient.cs | 39 +++++++-- .../Series/WireBackendCoverageTests.cs | 69 +++++++++++++++ .../FocasWideNumberTests.cs | 84 +++++++++++++++++++ 5 files changed, 225 insertions(+), 25 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWideNumberTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs index db407d18..53a23d2c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs @@ -467,18 +467,41 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional path ID override; defaults to . /// The raw parameter payload bytes, wrapped in a . public async Task> ReadParameterBytesAsync( - short dataNumber, + int dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) { using var callTimeout = CreateCallTimeout(cancellationToken, timeout); - var secondArg = axis == 0 ? dataNumber : axis; - var block = await SendSingleRequestAsync(callTimeout.Token, new RequestBlock(0x000e, dataNumber, secondArg, PathId: EffectivePathId(pathId))).ConfigureAwait(false); + var block = await SendSingleRequestAsync(callTimeout.Token, ParameterRequest(dataNumber, axis, EffectivePathId(pathId))).ConfigureAwait(false); return ToResult(block, payload => payload); } + /// + /// The cnc_rdparam (0x000e) request block. The data number travels in the 32-bit + /// Arg1 UNNARROWED — the request block has always carried arguments, and + /// a short cast in the adapter once asked the CNC for a different (wrapped) number than the + /// one authored. A global parameter repeats the number in Arg2; an axis parameter carries + /// the axis there. + /// + /// The parameter data number. + /// Axis selector; zero means a global parameter. + /// The CNC path. + /// The request block. + internal static RequestBlock ParameterRequest(int dataNumber, short axis, ushort pathId) => + new(0x000e, dataNumber, axis == 0 ? dataNumber : axis, PathId: pathId); + + /// + /// The cnc_rdmacro (0x0015) request block: the variable number, unnarrowed, in both 32-bit + /// arguments (start / end of a one-variable range) — byte-identical to the live 31i-B capture for #500. + /// + /// The macro variable number. + /// The CNC path. + /// The request block. + internal static RequestBlock MacroRequest(int number, ushort pathId) => + new(0x0015, number, number, PathId: pathId); + /// Typed Int32 parameter read — convenience over . /// The parameter data number. /// The FOCAS parameter type code. @@ -487,7 +510,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional path ID override; defaults to . /// The typed parameter value, wrapped in a . public async Task> ReadParameterAsync( - short dataNumber, + int dataNumber, short type = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, @@ -507,7 +530,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional per-call timeout override. /// Optional path ID override; defaults to . /// The typed 8-bit parameter value, wrapped in a . - public async Task> ReadParameterByteAsync(short dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) + public async Task> ReadParameterByteAsync(int dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) { var result = await ReadParameterBytesAsync(dataNumber, axis, cancellationToken, timeout, pathId).ConfigureAwait(false); return !result.IsOk || result.Value is null @@ -522,7 +545,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional per-call timeout override. /// Optional path ID override; defaults to . /// The typed 16-bit parameter value, wrapped in a . - public async Task> ReadParameterInt16Async(short dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) + public async Task> ReadParameterInt16Async(int dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) { var result = await ReadParameterBytesAsync(dataNumber, axis, cancellationToken, timeout, pathId).ConfigureAwait(false); return !result.IsOk || result.Value is null @@ -537,7 +560,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional per-call timeout override. /// Optional path ID override; defaults to . /// The typed 32-bit parameter value, wrapped in a . - public async Task> ReadParameterInt32Async(short dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) + public async Task> ReadParameterInt32Async(int dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) { var result = await ReadParameterBytesAsync(dataNumber, axis, cancellationToken, timeout, pathId).ConfigureAwait(false); return !result.IsOk || result.Value is null @@ -552,7 +575,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional per-call timeout override. /// Optional path ID override; defaults to . /// The typed single-precision parameter value, wrapped in a . - public async Task> ReadParameterFloat32Async(short dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) + public async Task> ReadParameterFloat32Async(int dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) { var result = await ReadParameterBytesAsync(dataNumber, axis, cancellationToken, timeout, pathId).ConfigureAwait(false); return !result.IsOk || result.Value is null || result.Value.Length < 4 @@ -567,7 +590,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional per-call timeout override. /// Optional path ID override; defaults to . /// The typed double-precision parameter value, wrapped in a . - public async Task> ReadParameterFloat64Async(short dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) + public async Task> ReadParameterFloat64Async(int dataNumber, short axis = 0, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) { var result = await ReadParameterBytesAsync(dataNumber, axis, cancellationToken, timeout, pathId).ConfigureAwait(false); return !result.IsOk || result.Value is null || result.Value.Length < 8 @@ -582,14 +605,17 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// Optional path ID override; defaults to . /// The macro variable value, wrapped in a . public Task> ReadMacroAsync( - short number, + int number, CancellationToken cancellationToken = default, TimeSpan? timeout = null, ushort? pathId = null) - => ReadSingleWithTimeoutAsync( - 0x0015, + { + var request = MacroRequest(number, EffectivePathId(pathId)); + return ReadSingleWithTimeoutAsync( + request.Command, payload => ParseMacro(number, payload), - cancellationToken, timeout, EffectivePathId(pathId), number, number); + cancellationToken, timeout, request.PathId, request.Arg1, request.Arg2); + } /// /// Decode a cnc_rdmacro (0x0015) payload. The on-wire ODBM record is 8 @@ -609,7 +635,7 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// The response-block payload. /// The decoded macro. /// The payload is shorter than 8 bytes. - internal static WireMacro ParseMacro(short number, byte[] payload) + internal static WireMacro ParseMacro(int number, byte[] payload) { RequireLength(payload, 8, "cnc_rdmacro"); return new WireMacro(number, ReadInt32(payload, 0), ReadInt16(payload, 6)); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireModels.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireModels.cs index ac6f32ee..70ca55f6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireModels.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireModels.cs @@ -170,7 +170,7 @@ public readonly record struct WireSpindleRecord(short Index, string Name); /// Parameter value returned by cnc_rdparam, interpreted as a scalar Int32. public sealed record WireParameter( - short DataNumber, + int DataNumber, short Type, int Value); @@ -184,7 +184,7 @@ public sealed record WireParameter( /// The 9-significant-digit mantissa. /// Implied decimal places; negative (0xFFFF on the wire) = vacant. public sealed record WireMacro( - short Number, + int Number, int Value, short Decimal) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs index d6d5a3a4..ec44e6e3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs @@ -80,6 +80,7 @@ public sealed class WireFocasClient : IFocasClient var area = FocasPmcAreaLookup.FromLetter(letter); if (area is null) return (null, FocasStatusMapper.BadNodeIdUnknown); var dataType = FocasPmcDataTypeLookup.FromFocasDataType(slotType); + if (start < 0 || end > ushort.MaxValue || end < start) return (null, FocasStatusMapper.BadOutOfRange); try { var result = await _wire.ReadPmcRangeAsync(area.Value, dataType, (ushort)start, (ushort)end, cancellationToken) @@ -334,18 +335,38 @@ public sealed class WireFocasClient : IFocasClient // ---- PMC / Parameter / Macro read paths ------------------------------------------ + /// + /// The 16-bit byte range pmc_rdpmcrng addresses for a slot of bytes at + /// . False when any byte of it falls outside 0..65535 — the range is refused + /// rather than wrapped onto a different PMC address (an Unknown-series device admits any number). + /// + /// The authored PMC byte address. + /// The slot width in bytes (≥ 1). + /// The first byte, when representable. + /// The last byte, when representable. + /// True when the whole slot is addressable. + internal static bool TryPmcRange(int number, int width, out ushort start, out ushort end) + { + start = end = 0; + var last = (long)number + Math.Max(width, 1) - 1; + if (number < 0 || last > ushort.MaxValue) return false; + start = (ushort)number; + end = (ushort)last; + return true; + } + private async Task<(object? value, uint status)> ReadPmcAsync( FocasAddress address, FocasDataType type, CancellationToken cancellationToken) { var area = FocasPmcAreaLookup.FromLetter(address.PmcLetter ?? string.Empty); if (area is null) return (null, FocasStatusMapper.BadNodeIdUnknown); var dataType = FocasPmcDataTypeLookup.FromFocasDataType(type); - var start = (ushort)address.Number; // pmc_rdpmcrng returns (end-start+1) BYTES. A multi-byte slot (Word/Long/Real/Double) needs // the range widened to its byte width, else the value parser gets too few bytes → 0 values // → spurious BadOutOfRange (live-confirmed on the 31i-B: a Word read with end=start returned // a single byte). Bit/Byte stay width 1 so end==start. 2026-06-25. - var end = (ushort)(start + FocasWireClient.PmcByteWidth((short)dataType) - 1); + if (!TryPmcRange(address.Number, FocasWireClient.PmcByteWidth((short)dataType), out var start, out var end)) + return (null, FocasStatusMapper.BadOutOfRange); try { @@ -373,23 +394,23 @@ public sealed class WireFocasClient : IFocasClient switch (type) { case FocasDataType.Byte: - var b = await _wire.ReadParameterByteAsync((short)address.Number, 0, cancellationToken).ConfigureAwait(false); + var b = await _wire.ReadParameterByteAsync(address.Number, 0, cancellationToken).ConfigureAwait(false); return b.IsOk ? ((object)(sbyte)b.Value, FocasStatusMapper.Good) : (null, FocasStatusMapper.MapFocasReturn(b.Rc)); case FocasDataType.Int16: - var s = await _wire.ReadParameterInt16Async((short)address.Number, 0, cancellationToken).ConfigureAwait(false); + var s = await _wire.ReadParameterInt16Async(address.Number, 0, cancellationToken).ConfigureAwait(false); return s.IsOk ? ((object)s.Value, FocasStatusMapper.Good) : (null, FocasStatusMapper.MapFocasReturn(s.Rc)); case FocasDataType.Float32: - var f = await _wire.ReadParameterFloat32Async((short)address.Number, 0, cancellationToken).ConfigureAwait(false); + var f = await _wire.ReadParameterFloat32Async(address.Number, 0, cancellationToken).ConfigureAwait(false); return f.IsOk ? ((object)f.Value, FocasStatusMapper.Good) : (null, FocasStatusMapper.MapFocasReturn(f.Rc)); case FocasDataType.Float64: - var d = await _wire.ReadParameterFloat64Async((short)address.Number, 0, cancellationToken).ConfigureAwait(false); + var d = await _wire.ReadParameterFloat64Async(address.Number, 0, cancellationToken).ConfigureAwait(false); return d.IsOk ? ((object)d.Value, FocasStatusMapper.Good) : (null, FocasStatusMapper.MapFocasReturn(d.Rc)); case FocasDataType.Bit when address.BitIndex is int bit: - var bi = await _wire.ReadParameterInt32Async((short)address.Number, 0, cancellationToken).ConfigureAwait(false); + var bi = await _wire.ReadParameterInt32Async(address.Number, 0, cancellationToken).ConfigureAwait(false); if (!bi.IsOk) return (null, FocasStatusMapper.MapFocasReturn(bi.Rc)); return ((object)((bi.Value >> bit & 1) != 0), FocasStatusMapper.Good); default: - var i = await _wire.ReadParameterInt32Async((short)address.Number, 0, cancellationToken).ConfigureAwait(false); + var i = await _wire.ReadParameterInt32Async(address.Number, 0, cancellationToken).ConfigureAwait(false); return i.IsOk ? ((object)i.Value, FocasStatusMapper.Good) : (null, FocasStatusMapper.MapFocasReturn(i.Rc)); } } @@ -404,7 +425,7 @@ public sealed class WireFocasClient : IFocasClient { try { - var result = await _wire.ReadMacroAsync((short)address.Number, cancellationToken).ConfigureAwait(false); + var result = await _wire.ReadMacroAsync(address.Number, cancellationToken).ConfigureAwait(false); if (!result.IsOk || result.Value is null) return (null, FocasStatusMapper.MapFocasReturn(result.Rc)); return MapMacro(result.Value, type, address.BitIndex); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs index 314fda86..4cbf460e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs @@ -72,6 +72,75 @@ public sealed class WireBackendCoverageTests } } + /// + /// #740 / R07:H14 — a macro / parameter number above 32767 reaches the CNC unnarrowed. The mock decodes + /// the request's 32-bit Arg1 as the number, so a decoy is seeded at the address the old + /// (short) cast produced (99999 → -31073, 40000 → -25536, as the unsigned key the mock sees): + /// reading the decoy's value means the number was wrapped. #500 / #6711 are the positive control that + /// ordinary numbers still read. + /// + [Fact] + public async Task Macro_and_parameter_numbers_above_short_range_read_the_authored_variable() + { + if (_fx.SkipReason is not null) Assert.Skip(_fx.SkipReason); + var ct = TestContext.Current.CancellationToken; + + static string Wrapped(int number) => ((uint)(int)(short)number).ToString(System.Globalization.CultureInfo.InvariantCulture); + + await _fx.LoadProfileAsync("FWLIB64", ct); + await _fx.PatchStateAsync(new + { + parameters = new Dictionary + { + ["6711"] = new { type = "long", value = 1234, @decimal = 0 }, + ["40000"] = new { type = "long", value = 4242, @decimal = 0 }, + [Wrapped(40000)] = new { type = "long", value = -1, @decimal = 0 }, + }, + macros = new Dictionary + { + ["500"] = new { value = 42000, @decimal = 3 }, + ["99999"] = new { value = 7, @decimal = 0 }, + [Wrapped(99999)] = new { value = 666, @decimal = 0 }, + }, + }, ct); + + var drv = new FocasDriver(new FocasDriverOptions + { + // ThirtyOne_i: the capability matrix admits #99999 (its documented ceiling); the parameter is read on + // an Unknown-series device, which admits any number. + Devices = [new FocasDeviceOptions(DeviceHost, Series: FocasCncSeries.ThirtyOne_i)], + RawTags = FocasRawTags.Entries( + new FocasTagDefinition("Macro500", DeviceHost, "MACRO:500", FocasDataType.Float64, Writable: false), + new FocasTagDefinition("Macro99999", DeviceHost, "MACRO:99999", FocasDataType.Int32, Writable: false), + new FocasTagDefinition("Param6711", DeviceHost, "PARAM:6711", FocasDataType.Int32, Writable: false)), + Probe = new FocasProbeOptions { Enabled = false }, + }, driverInstanceId: "wire-wide-numbers", clientFactory: new WireFocasClientFactory()); + + var unknownSeries = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(DeviceHost)], + RawTags = FocasRawTags.Entries( + new FocasTagDefinition("Param40000", DeviceHost, "PARAM:40000", FocasDataType.Int32, Writable: false)), + Probe = new FocasProbeOptions { Enabled = false }, + }, driverInstanceId: "wire-wide-param", clientFactory: new WireFocasClientFactory()); + + await using (drv) + await using (unknownSeries) + { + await drv.InitializeAsync("{}", ct); + var snaps = await drv.ReadAsync(["Macro500", "Macro99999", "Param6711"], ct); + snaps.ShouldAllBe(s => s.StatusCode == FocasStatusMapper.Good); + Convert.ToDouble(snaps[0].Value).ShouldBe(42.0, tolerance: 0.001); + snaps[1].Value.ShouldBe(7, "MACRO:99999 must read #99999, not the wrapped #-31073 decoy (666)"); + Convert.ToInt32(snaps[2].Value).ShouldBe(1234); + + await unknownSeries.InitializeAsync("{}", ct); + var param = await unknownSeries.ReadAsync(["Param40000"], ct); + param[0].StatusCode.ShouldBe(FocasStatusMapper.Good); + Convert.ToInt32(param[0].Value).ShouldBe(4242, "PARAM:40000 must not read the wrapped decoy (-1)"); + } + } + /// Verifies that discover emits device folder and tag variables. [Fact] public async Task Discover_emits_device_folder_and_tag_variables() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWideNumberTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWideNumberTests.cs new file mode 100644 index 00000000..7bf63a40 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasWideNumberTests.cs @@ -0,0 +1,84 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:H14 — the capability matrix admits macro numbers up to 99999 (30i/31i/32i) and parameters up +/// to 29999 (Unknown: anything), and the FOCAS/2 request block carries 32-bit arguments, but the adapter +/// cast every number to short: MACRO:99999 asked the CNC for #-31073 and published +/// that variable Good under the authored tag. Numbers now travel as int end to end. +/// +[Trait("Category", "Unit")] +public sealed class FocasWideNumberTests +{ + /// The macro request carries the full number in both 32-bit argument slots. + [Theory] + [InlineData(500)] + [InlineData(32767)] + [InlineData(32768)] + [InlineData(99999)] + public void Macro_request_carries_the_unnarrowed_number(int number) + { + var block = FocasWireClient.MacroRequest(number, pathId: 1); + block.Command.ShouldBe((ushort)0x0015); + block.Arg1.ShouldBe(number); + block.Arg2.ShouldBe(number); + } + + /// + /// Positive control on the wire encoding: the request for #500 is byte-identical to the one the real + /// 31i-B answered in the ref-macro-500-0x0015 capture, so widening changed nothing below 32768. + /// + [Fact] + public void Macro_request_for_500_matches_the_live_capture_bytes() + { + var body = FocasWireProtocol.BuildRequestBody([FocasWireClient.MacroRequest(500, pathId: 1)]); + Convert.ToHexString(body).ShouldBe("0001001C0001000100150000 01F4000001F4000000000000000000000000".Replace(" ", "")); + } + + /// A global parameter puts its number in both arguments; an axis parameter puts the axis second. + [Theory] + [InlineData(6711, (short)0, 6711)] + [InlineData(40000, (short)0, 40000)] + [InlineData(40000, (short)2, 2)] + public void Parameter_request_carries_the_unnarrowed_number(int number, short axis, int expectedArg2) + { + var block = FocasWireClient.ParameterRequest(number, axis, pathId: 1); + block.Command.ShouldBe((ushort)0x000e); + block.Arg1.ShouldBe(number); + block.Arg2.ShouldBe(expectedArg2); + } + + /// The decoded macro reports the number that was asked for, not a wrapped one. + [Fact] + public void Parsed_macro_keeps_a_number_above_short_range() + { + var payload = new byte[] { 0x05, 0xF5, 0xE1, 0x00, 0x00, 0x0A, 0x00, 0x08 }; + var m = FocasWireClient.ParseMacro(99999, payload); + m.Number.ShouldBe(99999); + m.Scaled.ShouldBe(1.0); + } + + /// + /// pmc_rdpmcrng addresses are 16-bit on the wire; a PMC number whose slot runs past 65535 is refused + /// rather than wrapped onto a different address (Unknown series admits any number). + /// + [Theory] + [InlineData(100, 1, true)] + [InlineData(65535, 1, true)] + [InlineData(65534, 2, true)] + [InlineData(65535, 2, false)] + [InlineData(70000, 1, false)] + public void Pmc_range_outside_16_bits_is_refused_not_wrapped(int number, int width, bool ok) + { + WireFocasClient.TryPmcRange(number, width, out var start, out var end).ShouldBe(ok); + if (ok) + { + ((int)start).ShouldBe(number); + ((int)end).ShouldBe(number + width - 1); + } + } +} From 6423ed272fafb7dde5f349bcbdd0319d00b64e47 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:11:25 -0400 Subject: [PATCH 03/12] fix(focas): the fixed-tree loop reports device silence to driver health (R07:H15) The fixed-tree loop swallowed every failure at Debug and never touched health, so a driver serving only FIXED: tags (Identity/Axes/Program/Status, the typical FOCAS shape) read Healthy on /hosts with every node Bad and the #527 watchdog could never fire. The fixed tree is now a third health path beside the value and alarm paths, combined in GetHealth (worst state, latest LastSuccessfulRead): - an answered bootstrap / axis pass clears the device's failure and advances LastSuccessfulRead; - a transport failure degrades the driver once the device has been silent past the same staleness budget that turns its values Bad (from its last answer, or from loop start when it never answered), with LastSuccessfulRead frozen; - a CNC refusal (non-OK rc) is not a comms loss: it clears the failure and credits nothing. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 86 +++++++++-- .../FocasFixedTreeHealthTests.cs | 133 ++++++++++++++++++ 2 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index f0c0226f..c8dad722 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -562,24 +562,27 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// - // The worst of the value path (_health) and the alarm path (_alarmTransportFailures), - // computed together. LastSuccessfulRead is the later of the two paths' last successes: while either - // path still reads, the comms-loss watchdog stays quiet; when both have stopped it is frozen. - // Faulted / Unknown / Initializing come from the value (lifecycle) path unchanged. + // The worst of the value path (_health), the alarm path (_alarmTransportFailures) and the fixed-tree path + // (_fixedTreeFailures), computed together. LastSuccessfulRead is the latest of the three paths' last + // successes: while any path still reads, the comms-loss watchdog stays quiet; when all have stopped it is + // frozen. Faulted / Unknown / Initializing come from the value (lifecycle) path unchanged. public DriverHealth GetHealth() { var value = Volatile.Read(ref _health); - var ticks = Interlocked.Read(ref _alarmLastSuccessTicks); - DateTime? alarmLast = ticks == 0 ? null : new DateTime(ticks, DateTimeKind.Utc); - var last = value.LastSuccessfulRead is { } v && (alarmLast is null || v >= alarmLast) ? v : alarmLast; + var last = Latest(Latest(value.LastSuccessfulRead, Stamp(Interlocked.Read(ref _alarmLastSuccessTicks))), + Stamp(Interlocked.Read(ref _fixedTreeLastSuccessTicks))); if (value.State is not (DriverState.Healthy or DriverState.Degraded)) return value; - if (value.State == DriverState.Healthy && !_alarmTransportFailures.IsEmpty) - { - var reason = _alarmTransportFailures.OrderBy(kv => kv.Key, StringComparer.OrdinalIgnoreCase).First().Value; + if (value.State == DriverState.Healthy + && (FirstFailure(_alarmTransportFailures) ?? FirstFailure(_fixedTreeFailures)) is { } reason) return new DriverHealth(DriverState.Degraded, last, reason); - } return value.LastSuccessfulRead == last ? value : value with { LastSuccessfulRead = last }; + + static DateTime? Stamp(long ticks) => ticks == 0 ? null : new DateTime(ticks, DateTimeKind.Utc); + static DateTime? Latest(DateTime? a, DateTime? b) => a is null ? b : b is null || a >= b ? a : b; + static string? FirstFailure(ConcurrentDictionary failures) => failures.IsEmpty + ? null + : failures.OrderBy(kv => kv.Key, StringComparer.OrdinalIgnoreCase).Select(kv => kv.Value).FirstOrDefault(); } /// Gets the number of configured devices. @@ -1290,6 +1293,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Each optional call is attempted once; failures (EW_FUNC / EW_NOOPT / EW_VERSION) // record the capability as unsupported and suppress the corresponding nodes // in DiscoverAsync + the poll loop. + state.FixedTreeStartedUtc = DateTime.UtcNow; while (!ct.IsCancellationRequested && state.FixedTreeCache is null) { try @@ -1320,6 +1324,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // so stamp liveness here as well as per tick, or a CNC whose axis poll never // succeeds would serve a perfectly valid sysinfo as stale. state.LastFixedPollUtc = DateTime.UtcNow; + NoteFixedTreeAnswered(state); // Published LAST (release write): LastStatus is the field callers watch for "the first // answer is in", and an authored FIXED:Status/* read is only servable once // FixedTreeCache and LastFixedPollUtc are set (TryReadFixedTreeOnDevice). Publishing it @@ -1331,6 +1336,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { _logger.LogDebug(ex, "FOCAS fixed-tree bootstrap failed for {Host} — retrying", state.Options.HostAddress); + NoteFixedTreeFailed(state, ex); try { await Task.Delay(TimeSpan.FromSeconds(2), ct).ConfigureAwait(false); } catch (OperationCanceledException) { return; } } @@ -1379,6 +1385,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // of the loop: EnsureConnectedAsync can hand back a cached handle without any wire // traffic, so it alone does not prove the CNC is still there. state.LastFixedPollUtc = DateTime.UtcNow; + // …and it is the driver's evidence of life too: a fixed-only driver has no value read to credit. + NoteFixedTreeAnswered(state); // Servo loads + spindle loads — both return bulk arrays, so folding // into the axis cadence is cheap. Each is gated by the bootstrap @@ -1474,6 +1482,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /* next tick retries — transient blips are expected */ _logger.LogDebug(ex, "FOCAS fixed-tree poll tick failed for {Host}", state.Options.HostAddress); + // …but a sustained silence is a comms loss, and nothing else will report it for a fixed-only driver. + NoteFixedTreeFailed(state, ex); } try { await Task.Delay(_options.FixedTree.PollInterval, ct).ConfigureAwait(false); } @@ -1635,7 +1645,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { var polledUtc = state.LastFixedPollUtc; if (polledUtc is null) return null; - var status = now - polledUtc.Value > FixedTreeStaleAfter(_options.FixedTree.PollInterval) + var status = now - polledUtc.Value > FixedTreeStaleBudget ? FocasStatusMapper.BadCommunicationError : FocasStatusMapper.Good; var sourceUtc = polledUtc.Value; @@ -1850,6 +1860,55 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { _alarmTransportFailures.Clear(); Interlocked.Exchange(ref _alarmLastSuccessTicks, 0); + _fixedTreeFailures.Clear(); + Interlocked.Exchange(ref _fixedTreeLastSuccessTicks, 0); + } + + /// + /// The FIXED-TREE half of driver health: device host → why its fixed-tree loop has had no answer for longer + /// than the staleness budget. Kept apart from like the alarm path, and for the same + /// reason — a driver serving only FIXED: tags has no value read to degrade it, so before this it + /// read Healthy on /hosts with every node Bad and the #527 watchdog could never fire. + /// + private readonly ConcurrentDictionary _fixedTreeFailures = new(StringComparer.OrdinalIgnoreCase); + + /// UTC ticks of the last answered fixed-tree poll on any device (0 = none). + private long _fixedTreeLastSuccessTicks; + + /// Test seam: replaces the fixed-tree staleness budget (production derives it from the poll interval). + internal TimeSpan? FixedTreeStaleBudgetOverride { get; init; } + + /// + /// How long a device's fixed tree may go unanswered before its values read Bad AND the driver degrades — + /// one budget for both, so the node quality and the health surface cannot disagree. + /// + private TimeSpan FixedTreeStaleBudget => + FixedTreeStaleBudgetOverride ?? FixedTreeStaleAfter(_options.FixedTree.PollInterval); + + /// An answered fixed-tree pass of : clears its failure, advances LastSuccessfulRead. + private void NoteFixedTreeAnswered(DeviceState state) + { + _fixedTreeFailures.TryRemove(state.Options.HostAddress, out _); + Interlocked.Exchange(ref _fixedTreeLastSuccessTicks, DateTime.UtcNow.Ticks); + } + + /// + /// A failed fixed-tree tick. A CNC REFUSAL (non-OK rc, transport fine) is not a comms loss and clears any + /// failure without crediting a read; a transport failure degrades the driver only once the device has been + /// silent past (measured from its last answer, or from loop start when + /// it never answered), so one dropped tick on a slow control does not flap /hosts. + /// + private void NoteFixedTreeFailed(DeviceState state, Exception error) + { + if (FocasAlarmProjection.IsCncRefusal(error)) + { + _fixedTreeFailures.TryRemove(state.Options.HostAddress, out _); + return; + } + var since = state.LastFixedPollUtc ?? state.FixedTreeStartedUtc; + if (DateTime.UtcNow - since > FixedTreeStaleBudget) + _fixedTreeFailures[state.Options.HostAddress] = + $"FOCAS fixed-tree poll of {state.Options.HostAddress} has had no answer since {since:O}: {error.Message}"; } /// @@ -2082,6 +2141,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// it falls outside . /// public DateTime? LastFixedPollUtc { get; set; } + + /// When this device's fixed-tree loop started — the silence origin for a device that never answered. + public DateTime FixedTreeStartedUtc { get; set; } = DateTime.UtcNow; /// /// Gets the last fixed-tree snapshots by field name. Double-typed so axis /// positions can carry the engineering-unit scale applied at diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs new file mode 100644 index 00000000..b83898e8 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs @@ -0,0 +1,133 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:H15 — the fixed-tree loop swallowed every failure at Debug and never touched driver health, so a +/// driver serving only FIXED: tags (Identity / Axes / Program / Status — the typical FOCAS shape) read +/// Healthy on /hosts with every node Bad, and the #527 comms-loss watchdog could never fire. The loop +/// now credits each answered axis pass (Healthy + LastSuccessfulRead) and, once a device has been silent past +/// the fixed-tree staleness budget, degrades the driver with LastSuccessfulRead frozen. +/// +[Trait("Category", "Unit")] +public sealed class FocasFixedTreeHealthTests +{ + private const string Host = "focas://10.0.0.5:8193"; + private static readonly TimeSpan Stale = TimeSpan.FromMilliseconds(300); + + /// A CNC that can be switched off (transport failure) or made to refuse the dynamic read. + private sealed class CncSwitch + { + public volatile bool Down; + public volatile bool Refuse; + } + + private sealed class SwitchableCnc(CncSwitch sw) : FakeFocasClient + { + private bool Down => sw.Down; + private bool Refuse => sw.Refuse; + + public override Task GetSysInfoAsync(CancellationToken ct) => + Down ? throw new IOException("socket closed") : base.GetSysInfoAsync(ct); + + public override Task ReadDynamicAsync(int axisIndex, CancellationToken ct) + { + if (Down) throw new IOException("socket closed"); + if (Refuse) throw new FocasWireException("cnc_rddynamic2 refused", rc: 6); + return base.ReadDynamicAsync(axisIndex, ct); + } + } + + private static FocasDriver FixedOnlyDriver(CncSwitch sw) => new(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host)], + RawTags = [FocasRawTags.Entry(new FocasTagDefinition("raw/X", Host, "FIXED:Axes/X/AbsolutePosition", FocasDataType.Float64))], + Probe = new FocasProbeOptions { Enabled = false }, + FixedTree = new FocasFixedTreeOptions + { + PollInterval = TimeSpan.FromMilliseconds(20), + ProgramPollInterval = TimeSpan.Zero, + TimerPollInterval = TimeSpan.Zero, + }, + }, "focas-fixed-health", new FakeFocasClientFactory { Customise = () => new SwitchableCnc(sw) }) + { + FixedTreeStaleBudgetOverride = Stale, + }; + + /// + /// Positive control first, then the outage: an answering CNC keeps the driver Healthy with a moving + /// LastSuccessfulRead; a CNC that stops answering degrades it once the budget passes, with + /// LastSuccessfulRead frozen (the watchdog's conjunction); and its return restores Healthy. + /// + [Fact] + public async Task Fixed_only_driver_degrades_when_its_cnc_disappears_and_recovers_when_it_returns() + { + var sw = new CncSwitch(); + await using var drv = FixedOnlyDriver(sw); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntil(() => drv.GetDeviceState(Host)?.LastFixedPollUtc is not null); + + var first = drv.GetHealth().LastSuccessfulRead; + await Task.Delay(100); + var healthy = drv.GetHealth(); + healthy.State.ShouldBe(DriverState.Healthy); + healthy.LastSuccessfulRead!.Value.ShouldBeGreaterThan(first!.Value, "an answered axis pass is a successful read"); + + sw.Down = true; + await Task.Delay(100); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a blip inside the staleness budget does not degrade"); + await WaitUntil(() => drv.GetHealth().State == DriverState.Degraded); + var frozen = drv.GetHealth().LastSuccessfulRead; + await Task.Delay(150); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldBe(frozen, "LastSuccessfulRead must stay frozen while the CNC is silent"); + + sw.Down = false; + await WaitUntil(() => drv.GetHealth().State == DriverState.Healthy); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(frozen!.Value); + } + + /// A CNC that is down from the start (bootstrap never succeeds) degrades once the budget passes. + [Fact] + public async Task Bootstrap_that_never_succeeds_degrades_the_driver() + { + var sw = new CncSwitch { Down = true }; + await using var drv = FixedOnlyDriver(sw); + await drv.InitializeAsync("{}", CancellationToken.None); + // The bootstrap retries every 2 s, so the first failure is what has to trip it. + await WaitUntil(() => drv.GetHealth().State == DriverState.Degraded, TimeSpan.FromSeconds(6)); + sw.Down = false; + } + + /// + /// A CNC that ANSWERS with a refusal is not a comms loss: the transport is fine, so the driver is not + /// degraded (else the watchdog would reconnect-loop a production control). + /// + [Fact] + public async Task Cnc_refusal_of_the_dynamic_read_does_not_degrade() + { + var sw = new CncSwitch(); + await using var drv = FixedOnlyDriver(sw); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntil(() => drv.GetDeviceState(Host)?.LastFixedPollUtc is not null); + + sw.Refuse = true; + await Task.Delay(Stale * 3); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + sw.Refuse = false; + } + + private static async Task WaitUntil(Func condition, TimeSpan? timeout = null) + { + var deadline = DateTime.UtcNow + (timeout ?? TimeSpan.FromSeconds(5)); + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("condition not reached"); + await Task.Delay(10); + } + } +} From cc140593a02ee2e05c20633dd28234a6d12446c7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:15:35 -0400 Subject: [PATCH 04/12] fix(focas): refuse String on PMC/PARAM/MACRO in the factory, editor and CLI (R07:U8) FocasDataType.String was authorable on any address, but no area read returns text: PMC fell through to a byte read, PARAM to Int32, MACRO to a double, and the String node received a boxed number at Good. One rule, FocasTagDefinitionFactory.DescribeDataTypeMismatch, now backs every path: - FromTagConfig rejects the pair, so the driver skips + reports it (BadNodeIdUnknown, never a Good number) and Inspect names the reason at deploy; - FocasTagConfigModel.Validate refuses it on the typed fields; - the AdminUI editor only offers String where it is readable and shows the rule's text inline (kept visible for a loaded blob that already carries it); - the CLI read/subscribe/probe/write commands refuse it at the boundary. String stays valid on FIXED: paths (Identity/Program name/mode text) and is ignored on ALARM: tags; an unparseable address keeps its own diagnosis. Razor change (FocasTagConfigEditor.razor) needs the Wave 4 live check. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Commands/ProbeCommand.cs | 1 + .../Commands/ReadCommand.cs | 3 +- .../Commands/SubscribeCommand.cs | 3 +- .../Commands/WriteCommand.cs | 3 +- .../FocasCommandBase.cs | 13 +++ .../FocasTagDefinitionFactory.cs | 27 +++++ .../Uns/TagEditors/FocasTagConfigEditor.razor | 18 ++- .../Uns/TagEditors/FocasTagConfigModel.cs | 4 + .../FocasCommandBaseValidationTests.cs | 21 ++++ .../FocasStringDataTypeTests.cs | 104 ++++++++++++++++++ .../Uns/FocasTagConfigModelTests.cs | 22 ++++ 11 files changed, 214 insertions(+), 5 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasStringDataTypeTests.cs diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs index d2d46458..9c31bffe 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ProbeCommand.cs @@ -28,6 +28,7 @@ public sealed class ProbeCommand : FocasCommandBase { ConfigureLogging(); ValidateOptions(); + ValidateDataType(Address, DataType); var ct = console.RegisterCancellationHandler(); var probeTag = new FocasTagDefinition( diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs index df042344..9218a2c9 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/ReadCommand.cs @@ -19,7 +19,7 @@ public sealed class ReadCommand : FocasCommandBase /// Gets the data type to interpret the address as. [CommandOption("type", 't', Description = - "Bit / Byte / Int16 / Int32 / Float32 / Float64 / String (default Int16).")] + "Bit / Byte / Int16 / Int32 / Float32 / Float64 / String (default Int16). String only on FIXED: paths.")] public FocasDataType DataType { get; init; } = FocasDataType.Int16; /// @@ -28,6 +28,7 @@ public sealed class ReadCommand : FocasCommandBase ConfigureLogging(); // Validate numeric option ranges before any driver work. ValidateOptions(); + ValidateDataType(Address, DataType); var ct = console.RegisterCancellationHandler(); var tagName = SynthesiseTagName(Address, DataType); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs index 1a3cb5f4..ee126fc0 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/SubscribeCommand.cs @@ -18,7 +18,7 @@ public sealed class SubscribeCommand : FocasCommandBase /// Gets the data type of the address. [CommandOption("type", 't', Description = - "Bit / Byte / Int16 / Int32 / Float32 / Float64 / String (default Int16).")] + "Bit / Byte / Int16 / Int32 / Float32 / Float64 / String (default Int16). String only on FIXED: paths.")] public FocasDataType DataType { get; init; } = FocasDataType.Int16; /// Gets the polling interval in milliseconds. @@ -33,6 +33,7 @@ public sealed class SubscribeCommand : FocasCommandBase // --interval-ms) before any driver work so a zero/negative interval surfaces as a // clean CommandException rather than a tight-spinning poll loop. ValidateOptions(IntervalMs); + ValidateDataType(Address, DataType); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Address, DataType); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs index bd383041..22cfb431 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/Commands/WriteCommand.cs @@ -20,7 +20,7 @@ public sealed class WriteCommand : FocasCommandBase /// Gets the data type of the value to write. [CommandOption("type", 't', Description = - "Bit / Byte / Int16 / Int32 / Float32 / Float64 / String (default Int16).")] + "Bit / Byte / Int16 / Int32 / Float32 / Float64 / String (default Int16). String only on FIXED: paths.")] public FocasDataType DataType { get; init; } = FocasDataType.Int16; /// Gets the value to write. @@ -37,6 +37,7 @@ public sealed class WriteCommand : FocasCommandBase // port/timeout surfaces as a clean CommandException rather than an opaque // downstream exception. ValidateOptions(); + ValidateDataType(Address, DataType); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Address, DataType); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs index 62965d52..e57ce8bf 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli/FocasCommandBase.cs @@ -89,4 +89,17 @@ public abstract class FocasCommandBase : DriverCommandBase throw new CliFx.Exceptions.CommandException( $"--interval-ms must be positive (got {iv})."); } + + /// + /// Refuses an --address / --type pair the driver cannot serve (String on a PMC / PARAM / MACRO + /// address — the driver skips such a tag, so the command would only ever print BadNodeIdUnknown). Same + /// rule as the driver and the AdminUI editor: . + /// + /// The FOCAS address. + /// The data type. + protected static void ValidateDataType(string address, FocasDataType type) + { + if (FocasTagDefinitionFactory.DescribeDataTypeMismatch(type, address) is { } error) + throw new CliFx.Exceptions.CommandException(error); + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs index d495dad1..b65150f6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs @@ -53,6 +53,9 @@ public static class FocasTagDefinitionFactory // Strict enum read: a typo'd dataType rejects the tag (→ BadNodeIdUnknown) // instead of silently defaulting to a wrong-width Good. if (!TagConfigJson.TryReadEnumStrict(root, "dataType", FocasDataType.Int32, out var dataType)) return false; + // A data type no read of this address can produce (String on PMC / PARAM / MACRO) rejects the tag: + // served, it published a boxed number at Good into a String node. + if (DescribeDataTypeMismatch(dataType, address) is not null) return false; // FOCAS authored tags carry no `writable` key (writability lives on the Tag entity / node ACL): // absent ⇒ read-only. An explicit writable:true is honoured for the driver's internal write gate // (default-false, so a missing key stays read-only — the note is preserved). @@ -73,6 +76,26 @@ public static class FocasTagDefinitionFactory catch (InvalidOperationException) { return false; } } + /// + /// The ONE address/data-type compatibility rule, shared by the mapper (and so the driver's init and the + /// deploy inspector), the AdminUI tag editor and the CLI. is only + /// servable from the fixed tree (Identity / Program name / mode text …): a PMC, parameter or macro read + /// returns a number, which a String node would receive at Good. An ALARM: tag has no value, so its + /// data type is irrelevant; an unparseable address is left to the address check, which names it better. + /// + /// The authored data type. + /// The authored address. + /// The reason the pair cannot be served, or when it can. + public static string? DescribeDataTypeMismatch(FocasDataType dataType, string? address) + { + if (dataType != FocasDataType.String) return null; + var parsed = FocasAddress.TryParse(address); + if (parsed is null || parsed.Kind is FocasAreaKind.Fixed or FocasAreaKind.Alarm) return null; + return $"FOCAS data type 'String' is not readable at '{address!.Trim()}' — PMC, PARAM: and MACRO: reads " + + "return numbers (String is only valid on FIXED: paths such as FIXED:Identity/SeriesNumber). " + + "Choose Bit / Byte / Int16 / Int32 / Float32 / Float64."; + } + /// /// The tag's cross-driver alarm object, or when it carries none. /// Lenient like the host's TagConfigIntent: a non-integer severity is simply absent. @@ -236,6 +259,10 @@ public static class FocasTagDefinitionFactory } if (TagConfigJson.DescribeInvalidEnum(root, "dataType") is { } w) rejections.Add(w); + else if (addr.ValueKind == JsonValueKind.String + && TagConfigJson.TryReadEnumStrict(root, "dataType", FocasDataType.Int32, out var dataType) + && DescribeDataTypeMismatch(dataType, addr.GetString()) is { } mismatch) + rejections.Add(mismatch + " The driver will skip this tag and it will read BadNodeIdUnknown."); } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/FocasTagConfigEditor.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/FocasTagConfigEditor.razor index 8911b438..03fd6a15 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/FocasTagConfigEditor.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/FocasTagConfigEditor.razor @@ -15,8 +15,22 @@
+ @foreach (var v in Enum.GetValues()) + { + @* String is only readable from the fixed tree (FocasTagDefinitionFactory.DescribeDataTypeMismatch): + offered there, and kept visible when a loaded blob already carries it so the select never lies. *@ + if (v != FocasDataType.String || _m.DataType == FocasDataType.String + || FocasTagDefinitionFactory.DescribeDataTypeMismatch(v, _m.Address) is null) + { + + } + } + + @if (FocasTagDefinitionFactory.DescribeDataTypeMismatch(_m.DataType, _m.Address) is { } typeError) + { +
@typeError
+ } +
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs index 91851b8e..68904dff 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs @@ -80,6 +80,10 @@ public sealed class FocasTagConfigModel // ahead of the parse consult below and is never replaced by it. if (FocasAddress.TryParse(Address) is null) return DescribeUnparseableAddress(Address); + // The driver's own address/data-type rule (String only on FIXED: paths) — checked on the typed fields so a + // model built in the editor is judged too, not only a loaded blob the parse gate below can see. + if (FocasTagDefinitionFactory.DescribeDataTypeMismatch(DataType, Address) is { } typeError) return typeError; + // Parse-gate backstop. ⚠️ It gates on the PARSE VERDICT, never on Inspect being non-empty: FOCAS's // inspector emits advisories for blobs the reader ACCEPTS (an unparseable address, an authored // 'writable':true on a read-only wire), and promoting those to authoring errors would make the modal diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs index 2efff255..66dcabde 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/FocasCommandBaseValidationTests.cs @@ -33,6 +33,27 @@ public sealed class FocasCommandBaseValidationTests /// Invokes option validation without interval. public void InvokeValidateNoInterval() => ValidateOptions(intervalMs: null); + + /// Invokes the address/data-type check. + public void InvokeValidateDataType(string address, FocasDataType type) => ValidateDataType(address, type); + } + + /// + /// #740 / R07:U8 — --type String on a PMC / PARAM / MACRO address is refused at the CLI boundary + /// (the driver would skip the tag and the read would print BadNodeIdUnknown); a fixed-tree path keeps it. + /// + [Theory] + [InlineData("R100", FocasDataType.String, false)] + [InlineData("MACRO:500", FocasDataType.String, false)] + [InlineData("PARAM:6711", FocasDataType.String, false)] + [InlineData("R100", FocasDataType.Byte, true)] + [InlineData("FIXED:Identity/SeriesNumber", FocasDataType.String, true)] + public void Validate_data_type_against_address(string address, FocasDataType type, bool ok) + { + var sut = new Probe { CncHost = "host", IntervalMs = 1000 }; + if (ok) Should.NotThrow(() => sut.InvokeValidateDataType(address, type)); + else Should.Throw(() => sut.InvokeValidateDataType(address, type)) + .Message.ShouldContain("String"); } /// Verifies that default options are accepted. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasStringDataTypeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasStringDataTypeTests.cs new file mode 100644 index 00000000..600a205a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasStringDataTypeTests.cs @@ -0,0 +1,104 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:U8 — was authorable on any address, but no area read +/// produces a string: PMC fell through to a byte read, PARAM to an Int32, MACRO to a double, and a +/// String-typed node received a boxed number at Good. String is now refused on PMC / PARAM / MACRO by the +/// one shared check, everywhere a tag is authored or loaded; the fixed tree (Identity / Program name, …), +/// whose values really are strings, keeps it. +/// +[Trait("Category", "Unit")] +public sealed class FocasStringDataTypeTests +{ + private const string Host = "focas://10.0.0.5:8193"; + + private static string Config(string address, string dataType) => + $$"""{"address":"{{address}}","dataType":"{{dataType}}"}"""; + + /// String on an area read is refused by the mapper. + [Theory] + [InlineData("R100")] + [InlineData("X0.0")] + [InlineData("PARAM:6711")] + [InlineData("MACRO:500")] + public void Mapper_refuses_string_on_an_area_read(string address) + { + FocasTagDefinitionFactory.FromTagConfig(Config(address, "String"), "raw/T", out _).ShouldBeFalse(); + FocasTagDefinitionFactory.DescribeDataTypeMismatch(FocasDataType.String, address).ShouldNotBeNull(); + } + + /// Positive control: numeric types on the same addresses, and String on the fixed tree, still map. + [Theory] + [InlineData("R100", "Byte")] + [InlineData("PARAM:6711", "Int32")] + [InlineData("MACRO:500", "Float64")] + [InlineData("FIXED:Identity/SeriesNumber", "String")] + [InlineData("FIXED:Program/Name", "String")] + public void Mapper_accepts_numeric_area_reads_and_string_fixed_tree_paths(string address, string dataType) + { + FocasTagDefinitionFactory.FromTagConfig(Config(address, dataType), "raw/T", out var def).ShouldBeTrue(); + def.Address.ShouldBe(address); + } + + /// An ALARM: tag has no value, so its data type is irrelevant and String is not refused. + [Fact] + public void Mapper_accepts_string_on_an_alarm_tag() + { + FocasTagDefinitionFactory.FromTagConfig( + """{"address":"ALARM:*","dataType":"String","alarm":{"severity":500}}""", "raw/A", out _).ShouldBeTrue(); + } + + /// An unparseable address is left to the address check — it still draws THAT diagnosis. + [Fact] + public void Unparseable_address_keeps_its_own_diagnosis() + { + FocasTagDefinitionFactory.DescribeDataTypeMismatch(FocasDataType.String, "not-an-address").ShouldBeNull(); + FocasTagDefinitionFactory.FromTagConfig(Config("not-an-address", "String"), "raw/T", out _).ShouldBeTrue(); + } + + /// The deploy-time inspector names the reason rather than falling to the generic backstop. + [Fact] + public void Inspect_names_the_string_refusal() + { + var warnings = FocasTagDefinitionFactory.Inspect(Config("MACRO:500", "String")); + warnings.ShouldContain(w => w.Contains("String", StringComparison.Ordinal) && w.Contains("MACRO:500", StringComparison.Ordinal)); + FocasTagDefinitionFactory.Inspect(Config("MACRO:500", "Float64")).ShouldBeEmpty(); + } + + /// + /// At init a String area tag is skipped and reported (never served as a Good number); the numeric tag + /// beside it reads normally (positive control). + /// + [Fact] + public async Task Driver_skips_and_reports_a_string_area_tag() + { + var factory = new FakeFocasClientFactory + { + Customise = () => new FakeFocasClient { Values = { ["R100"] = (sbyte)7 } }, + }; + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host)], + RawTags = + [ + new RawTagEntry("raw/Str", Config("R100", "String"), false, Host), + new RawTagEntry("raw/Num", Config("R100", "Byte"), false, Host), + ], + Probe = new FocasProbeOptions { Enabled = false }, + }, "focas-u8", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/Str", "raw/Num"], CancellationToken.None); + snaps[0].StatusCode.ShouldBe(FocasStatusMapper.BadNodeIdUnknown); + snaps[0].Value.ShouldBeNull(); + snaps[1].StatusCode.ShouldBe(FocasStatusMapper.Good); + snaps[1].Value.ShouldBe((sbyte)7); + drv.GetSkippedTags()!.Sample.ShouldContain(s => s.RawPath == "raw/Str"); + await drv.ShutdownAsync(CancellationToken.None); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/FocasTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/FocasTagConfigModelTests.cs index 334637cd..b3f16e8b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/FocasTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/FocasTagConfigModelTests.cs @@ -169,4 +169,26 @@ public sealed class FocasTagConfigModelTests [InlineData("""{"address":"R100","dataType":2}""")] public void Validate_accepts_every_dataType_the_driver_accepts(string json) => FocasTagConfigModel.FromJson(json).Validate().ShouldBeNull(); + + /// + /// #740 / R07:U8 — String on a PMC / PARAM / MACRO address is refused by the driver (no area read + /// returns text), so the modal refuses it too, for a model built in the editor and for a loaded blob. + /// + [Theory] + [InlineData("R100")] + [InlineData("PARAM:6711")] + [InlineData("MACRO:500")] + public void Validate_refuses_string_on_an_area_address(string address) + { + new FocasTagConfigModel { Address = address, DataType = FocasDataType.String } + .Validate().ShouldNotBeNull().ShouldContain("String"); + FocasTagConfigModel.FromJson($$"""{"address":"{{address}}","dataType":"String"}""") + .Validate().ShouldNotBeNull().ShouldContain("String"); + } + + /// Positive control: the fixed tree's string fields keep String. + [Fact] + public void Validate_accepts_string_on_a_fixed_tree_path() + => new FocasTagConfigModel { Address = "FIXED:Identity/SeriesNumber", DataType = FocasDataType.String } + .Validate().ShouldBeNull(); } From 256915b7e45a7bd757c24ad573431a2246212c8d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:18:16 -0400 Subject: [PATCH 05/12] perf(focas): read devices concurrently within a batch (R07:P9) ReadAsync walked the whole batch sequentially, so one SLOW CNC (TCP-accept-then-stall, the live 31i-B case) cost Timeout per tag per poll while no other device was read. ReadAsync now resolves every reference against one table snapshot, buckets the rest by device and reads the devices with Task.WhenAll; each device's tags still go in order on its own serialised socket (the PMC coalescing pre-pass runs per device, first). Results land at their own indices. Also precomputes each device's fixed-tree key prefix (it was built per reference, twice per poll). Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 223 ++++++++++-------- .../FocasConcurrentDeviceReadTests.cs | 115 +++++++++ 2 files changed, 245 insertions(+), 93 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index c8dad722..5c4953d3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -637,23 +637,23 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // ---- IReadable ---- /// + // Devices are read CONCURRENTLY, each one's tags in order on its own (serialised) socket: a slow CNC — a + // TCP-accept-then-stall control costs its Timeout per tag — used to hold every other device's read in the + // batch hostage, because the whole batch was one sequential walk. public async Task> ReadAsync( IReadOnlyList fullReferences, CancellationToken cancellationToken) { ArgumentNullException.ThrowIfNull(fullReferences); var now = DateTime.UtcNow; var results = new DataValueSnapshot[fullReferences.Count]; + // One snapshot of each swapped table for the whole call (see _devices). + var devices = Devices; + var alarmTags = Volatile.Read(ref _alarmTags); + var fixedTreeActive = _fixedTreeActive; - // Merge adjacent / near PMC reads of one device into fewer pmc_rdpmcrng calls. Indices it - // serves are marked here and skipped by the per-tag loop below; everything else (including every - // single-tag range and any range the CNC rejected) takes the unchanged per-tag path. - var served = _options.PmcCoalesce.Enabled - ? await CoalescePmcReadsAsync(fullReferences, results, now, cancellationToken).ConfigureAwait(false) - : null; - + Dictionary>? byDevice = null; for (var i = 0; i < fullReferences.Count; i++) { - if (served is not null && served[i]) continue; var reference = fullReferences[i]; // Fixed-tree T1 — fixed-tree references are synthesized from the cached @@ -663,25 +663,66 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // This branch matches a reference that IS a "{host}/{path}" fixed-tree key. Authored // tags do not take it — their reference is a RawPath — and are served further down by // their FIXED: address instead. - if (_fixedTreeActive && TryReadFixedTree(reference, now) is { } fx) + if (fixedTreeActive && TryReadFixedTree(devices, reference, now) is { } fx) { results[i] = fx; continue; } // An ALARM: tag is a Part 9 condition, not a value — there is nothing to read. - if (Volatile.Read(ref _alarmTags).ContainsKey(reference)) + if (alarmTags.ContainsKey(reference)) { results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadNotSupported, null, now); continue; } - if (!_resolver.TryResolve(reference, out var def)) + if (!_resolver.TryResolve(reference, out var def) + || !devices.TryGetValue(def.DeviceHostAddress, out var device)) { results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadNodeIdUnknown, null, now); continue; } - if (!Devices.TryGetValue(def.DeviceHostAddress, out var device)) + + byDevice ??= []; + if (!byDevice.TryGetValue(device, out var indices)) byDevice[device] = indices = []; + indices.Add(i); + } + + if (byDevice is null) return results; + if (byDevice.Count == 1) + { + foreach (var (device, indices) in byDevice) + await ReadDeviceAsync(device, indices, fullReferences, results, now, cancellationToken).ConfigureAwait(false); + } + else + { + await Task.WhenAll(byDevice.Select(kv => + ReadDeviceAsync(kv.Key, kv.Value, fullReferences, results, now, cancellationToken))).ConfigureAwait(false); + } + return results; + } + + /// + /// Reads one device's share of a batch, in request order: the PMC coalescing pre-pass first, then every + /// index it did not serve through the per-tag path. Each result lands at its own index, so concurrent + /// device reads never touch the same slot. + /// + private async Task ReadDeviceAsync( + DeviceState device, List indices, IReadOnlyList fullReferences, + DataValueSnapshot[] results, DateTime now, CancellationToken cancellationToken) + { + // Merge adjacent / near PMC reads of the device into fewer pmc_rdpmcrng calls. Indices it serves are + // marked here and skipped by the per-tag loop below; everything else (including every single-tag range + // and any range the CNC rejected) takes the unchanged per-tag path. + var served = _options.PmcCoalesce.Enabled + ? await CoalescePmcReadsAsync(device, indices, fullReferences, results, now, cancellationToken).ConfigureAwait(false) + : null; + + foreach (var i in indices) + { + if (served is not null && served.Contains(i)) continue; + var reference = fullReferences[i]; + if (!_resolver.TryResolve(reference, out var def)) { results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadNodeIdUnknown, null, now); continue; @@ -738,12 +779,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, Volatile.Read(ref _health).LastSuccessfulRead, ex.Message)); } } - - return results; } /// - /// PMC coalescing pre-pass. Groups the batch's PMC tags by device, plans merged ranges + /// PMC coalescing pre-pass for ONE device's share of a batch. Plans merged ranges /// () and serves every multi-member range with ONE /// pmc_rdpmcrng, slicing each tag's value out of the returned slots. /// @@ -756,106 +795,98 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// leaves the members UNSERVED, so the per-tag loop reads them individually and each gets its own /// true status — and the range is remembered so later polls skip the doomed merged read. /// - /// Returns null when nothing was served. + /// Returns the served indices, or null when nothing was served. /// - private async Task CoalescePmcReadsAsync( - IReadOnlyList references, DataValueSnapshot[] results, DateTime now, CancellationToken ct) + private async Task?> CoalescePmcReadsAsync( + DeviceState device, List indices, IReadOnlyList references, DataValueSnapshot[] results, + DateTime now, CancellationToken ct) { - Dictionary>? byDevice = null; - for (var i = 0; i < references.Count; i++) + List? items = null; + foreach (var i in indices) { try { - var reference = references[i]; - if (_fixedTreeActive && TryReadFixedTree(reference, now) is not null) continue; - if (!_resolver.TryResolve(reference, out var def)) continue; - if (!Devices.TryGetValue(def.DeviceHostAddress, out var device)) continue; + if (!_resolver.TryResolve(references[i], out var def)) continue; var parsed = ResolveParsedAddress(def); if (parsed.Kind != FocasAreaKind.Pmc || parsed.PmcLetter is null) continue; - (byDevice ??= []).TryAdd(device, []); - byDevice[device].Add(new FocasPmcReadItem(i, parsed.PmcLetter, parsed.Number, def.DataType, parsed.BitIndex)); + (items ??= []).Add(new FocasPmcReadItem(i, parsed.PmcLetter, parsed.Number, def.DataType, parsed.BitIndex)); } catch (Exception ex) when (ex is not OperationCanceledException) { /* malformed / unresolvable — the per-tag loop reports it with its usual status */ } } - if (byDevice is null) return null; + if (items is null || items.Count < 2) return null; - bool[]? served = null; - foreach (var (device, items) in byDevice) + var plans = FocasPmcCoalescer.Plan(items, _options.PmcCoalesce.MaxSpanBytes, _options.PmcCoalesce.MaxGapBytes); + if (!plans.Any(p => p.Members.Count >= 2 && !device.CoalesceRejected.ContainsKey(p.Key))) return null; + + HashSet? served = null; + // One connect attempt per device per batch: if it fails, every PMC tag of the device fails with + // that status here instead of paying a connect attempt per plan and again per tag. + IFocasClient client; + try { client = await EnsureConnectedAsync(device, ct).ConfigureAwait(false); } + catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } + catch (Exception) { - if (items.Count < 2) continue; - var plans = FocasPmcCoalescer.Plan(items, _options.PmcCoalesce.MaxSpanBytes, _options.PmcCoalesce.MaxGapBytes); - if (!plans.Any(p => p.Members.Count >= 2 && !device.CoalesceRejected.ContainsKey(p.Key))) continue; + FailMembers(items, FocasStatusMapper.BadCommunicationError, + $"FOCAS connect failed reading PMC tags of {device.Options.HostAddress}"); + return served; + } - // One connect attempt per device per batch: if it fails, every PMC tag of the device fails with - // that status here instead of paying a connect attempt per plan and again per tag. - IFocasClient client; - try { client = await EnsureConnectedAsync(device, ct).ConfigureAwait(false); } + foreach (var plan in plans) + { + if (plan.Members.Count < 2) continue; + if (device.CoalesceRejected.ContainsKey(plan.Key)) continue; + + var members = plan.Members; + try + { + var (slots, status) = await client + .ReadPmcSlotsAsync(plan.Letter, plan.SlotType, plan.Start, plan.End, ct) + .ConfigureAwait(false); + + if (status == FocasStatusMapper.Good && slots is not null) + { + foreach (var m in members) + { + var slot = plan.SlotOf(m); + results[m.Index] = slot < slots.Count + ? new DataValueSnapshot(FocasPmcCoalescer.MapSlot(slots[slot], m.Type, m.BitIndex), status, now, now) + : new DataValueSnapshot(null, FocasStatusMapper.BadOutOfRange, now, now); + (served ??= []).Add(m.Index); + } + Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null)); + } + else + { + // The CNC REFUSED the merged range (any return code, mapped or not). Don't guess which + // member is at fault: let the per-tag loop give each its own answer. Remember the range + // so later polls skip the doomed read — except for a transient refusal (EW_BUSY maps to + // BadDeviceFailure), which says nothing about the range. + if (status != FocasStatusMapper.BadDeviceFailure) + device.CoalesceRejected[plan.Key] = 0; + _logger.LogDebug( + "FOCAS PMC range {Key} on {Host} rejected (0x{Status:X8}) — reading its {Count} tags individually", + plan.Key, device.Options.HostAddress, status, members.Count); + } + } catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } catch (Exception) { - FailMembers(items, FocasStatusMapper.BadCommunicationError, references, results, now, ref served, - $"FOCAS connect failed reading PMC tags of {device.Options.HostAddress}"); - continue; - } - - foreach (var plan in plans) - { - if (plan.Members.Count < 2) continue; - if (device.CoalesceRejected.ContainsKey(plan.Key)) continue; - - var members = plan.Members; - try - { - var (slots, status) = await client - .ReadPmcSlotsAsync(plan.Letter, plan.SlotType, plan.Start, plan.End, ct) - .ConfigureAwait(false); - - if (status == FocasStatusMapper.Good && slots is not null) - { - foreach (var m in members) - { - var slot = plan.SlotOf(m); - results[m.Index] = slot < slots.Count - ? new DataValueSnapshot(FocasPmcCoalescer.MapSlot(slots[slot], m.Type, m.BitIndex), status, now, now) - : new DataValueSnapshot(null, FocasStatusMapper.BadOutOfRange, now, now); - (served ??= new bool[references.Count])[m.Index] = true; - } - Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null)); - } - else - { - // The CNC REFUSED the merged range (any return code, mapped or not). Don't guess which - // member is at fault: let the per-tag loop give each its own answer. Remember the range - // so later polls skip the doomed read — except for a transient refusal (EW_BUSY maps to - // BadDeviceFailure), which says nothing about the range. - if (status != FocasStatusMapper.BadDeviceFailure) - device.CoalesceRejected[plan.Key] = 0; - _logger.LogDebug( - "FOCAS PMC range {Key} on {Host} rejected (0x{Status:X8}) — reading its {Count} tags individually", - plan.Key, device.Options.HostAddress, status, members.Count); - } - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; } - catch (Exception) - { - // Transport failure / timeout: the link is suspect, so fail the members without per-tag retries. - FailMembers(members, FocasStatusMapper.BadCommunicationError, references, results, now, ref served, - $"FOCAS transport failure reading PMC range {plan.Key}"); - } + // Transport failure / timeout: the link is suspect, so fail the members without per-tag retries. + FailMembers(members, FocasStatusMapper.BadCommunicationError, + $"FOCAS transport failure reading PMC range {plan.Key}"); } } return served; - void FailMembers(IEnumerable failed, uint status, IReadOnlyList refs, - DataValueSnapshot[] res, DateTime stamp, ref bool[]? servedRef, string reason) + void FailMembers(IEnumerable failed, uint status, string reason) { foreach (var m in failed) { - res[m.Index] = new DataValueSnapshot(null, status, null, stamp); - (servedRef ??= new bool[refs.Count])[m.Index] = true; + results[m.Index] = new DataValueSnapshot(null, status, null, now); + (served ??= []).Add(m.Index); } Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, Volatile.Read(ref _health).LastSuccessfulRead, @@ -1604,12 +1635,13 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// the reference looks like a fixed-tree FullName; null when it doesn't /// (callers fall through to the user-authored tag path). ///
- private DataValueSnapshot? TryReadFixedTree(string reference, DateTime now) + private DataValueSnapshot? TryReadFixedTree( + IReadOnlyDictionary devices, string reference, DateTime now) { - foreach (var state in Devices.Values) + foreach (var state in devices.Values) { - if (!reference.StartsWith(state.Options.HostAddress + "/", StringComparison.OrdinalIgnoreCase)) continue; - if (TryReadFixedTreeOnDevice(state, reference[(state.Options.HostAddress.Length + 1)..], now) is { } hit) + if (!reference.StartsWith(state.FixedTreePrefix, StringComparison.OrdinalIgnoreCase)) continue; + if (TryReadFixedTreeOnDevice(state, reference[state.FixedTreePrefix.Length..], now) is { } hit) return hit; } return null; @@ -2088,6 +2120,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, public FocasHostAddress ParsedAddress { get; } = parsedAddress; /// Gets the device configuration options. public FocasDeviceOptions Options { get; } = options; + /// + /// "{HostAddress}/" — the prefix of this device's fixed-tree cache keys, built once rather than + /// per reference per poll (twice per poll before #740). + /// + public string FixedTreePrefix { get; } = options.HostAddress + "/"; /// Gets or sets the FOCAS client instance. public IFocasClient? Client { get; set; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs new file mode 100644 index 00000000..9ca35cf7 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs @@ -0,0 +1,115 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:P9 — walked the whole batch sequentially, so a SLOW CNC +/// (TCP-accept-then-stall, the live 31i-B case) cost its timeout per tag per poll during which no other +/// device was read. Devices are now read concurrently, each still serialised on its own socket. +/// +[Trait("Category", "Unit")] +public sealed class FocasConcurrentDeviceReadTests +{ + private const string HostA = "focas://10.0.0.5:8193"; + private const string HostB = "focas://10.0.0.6:8193"; + + /// + /// A client that records which device it dialled. Device A's read only completes once device B's read has + /// STARTED — so a sequential walk (A first) can never finish A, while a concurrent one does. Also counts + /// the peak number of reads in flight on any one client. + /// + private sealed class OrderingClient(TaskCompletionSource bStarted) : FakeFocasClient + { + private string _host = ""; + private int _inFlight; + public int PeakInFlight; + + public override Task ConnectAsync(FocasHostAddress address, TimeSpan timeout, CancellationToken ct) + { + _host = address.ToString(); + return base.ConnectAsync(address, timeout, ct); + } + + public override async Task<(object? value, uint status)> ReadAsync( + FocasAddress address, FocasDataType type, CancellationToken ct) + { + var now = Interlocked.Increment(ref _inFlight); + InterlockedMax(ref PeakInFlight, now); + try + { + if (_host.Contains("10.0.0.6", StringComparison.Ordinal)) bStarted.TrySetResult(); + else await bStarted.Task.WaitAsync(TimeSpan.FromSeconds(2), ct).ConfigureAwait(false); + await Task.Delay(20, ct).ConfigureAwait(false); + return ((sbyte)1, FocasStatusMapper.Good); + } + finally { Interlocked.Decrement(ref _inFlight); } + } + + private static void InterlockedMax(ref int target, int value) + { + int seen; + while ((seen = Volatile.Read(ref target)) < value + && Interlocked.CompareExchange(ref target, value, seen) != seen) { } + } + } + + /// A stalled device no longer holds every other device's read hostage — and each device stays serialised. + [Fact] + public async Task Devices_are_read_concurrently_and_each_stays_serialised() + { + var bStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeFocasClientFactory { Customise = () => new OrderingClient(bStarted) }; + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(HostA, DeviceName: "A"), new FocasDeviceOptions(HostB, DeviceName: "B")], + RawTags = + [ + FocasRawTags.Entry(new FocasTagDefinition("raw/A1", "A", "R100", FocasDataType.Byte)), + FocasRawTags.Entry(new FocasTagDefinition("raw/A2", "A", "R101", FocasDataType.Byte)), + FocasRawTags.Entry(new FocasTagDefinition("raw/B1", "B", "R100", FocasDataType.Byte)), + ], + Probe = new FocasProbeOptions { Enabled = false }, + // Off so every tag takes the per-tag path — this pins device-level concurrency, not coalescing. + PmcCoalesce = new FocasPmcCoalesceOptions { Enabled = false }, + }, "focas-p9", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/A1", "raw/A2", "raw/B1"], CancellationToken.None); + + snaps.ShouldAllBe(s => s.StatusCode == FocasStatusMapper.Good, + "device A's read can only finish once device B's has started — a sequential walk times A out"); + factory.Clients.Count.ShouldBe(2); + factory.Clients.Cast().ShouldAllBe(c => c.PeakInFlight == 1, + "one device's socket is still request→response: never two reads in flight on it"); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// Results land at their own indices whatever order the devices finish in. + [Fact] + public async Task Results_keep_their_request_order_across_devices() + { + var factory = new FakeFocasClientFactory(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(HostA, DeviceName: "A"), new FocasDeviceOptions(HostB, DeviceName: "B")], + RawTags = + [ + FocasRawTags.Entry(new FocasTagDefinition("raw/A1", "A", "R100", FocasDataType.Byte)), + FocasRawTags.Entry(new FocasTagDefinition("raw/B1", "B", "R200", FocasDataType.Byte)), + ], + Probe = new FocasProbeOptions { Enabled = false }, + }, "focas-p9-order", factory) + ; + factory.Customise = () => new FakeFocasClient { Values = { ["R100"] = (sbyte)1, ["R200"] = (sbyte)2 } }; + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/B1", "nope", "raw/A1"], CancellationToken.None); + + snaps[0].Value.ShouldBe((sbyte)2); + snaps[1].StatusCode.ShouldBe(FocasStatusMapper.BadNodeIdUnknown); + snaps[2].Value.ShouldBe((sbyte)1); + await drv.ShutdownAsync(CancellationToken.None); + } +} From 4c4dfaadcc1721f79f8bf6bfb2bab21f325e3ec6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:21:02 -0400 Subject: [PATCH 06/12] fix(focas): recycle the device handle under ConnectGate and drain it first (R07:L6) The handle-recycle loop disposed the device client with no gate. It could null a client a concurrent EnsureConnectedAsync had just installed (orphaning a live two-socket session), or dispose one a poll was mid-read on (ObjectDisposedException -> BadCommunicationError + Degraded on every recycle interval). The recycle now takes the device's ConnectGate, detaches the client, and disposes it via SynchronizedFocasClient.DisposeWhenIdleAsync, which waits (bounded by the call timeout plus slack) for ops that already hold the client. The gate stays held through the drain, so no fresh session opens beside the old one. SynchronizedFocasClient no longer disposes its call gate. Releasing a disposed semaphore threw out of reads that had otherwise completed. Ops that reach a disposed client now get ObjectDisposedException and are never run on the dead socket. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 30 ++++++- .../SynchronizedFocasClient.cs | 79 +++++++++++++++---- .../FocasHandleRecycleTests.cs | 67 ++++++++++++++++ 3 files changed, 158 insertions(+), 18 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 5c4953d3..f65bb2f4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -1779,6 +1779,13 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, return null; } + /// + /// How long a handle recycle waits for in-flight ops on the detached client: one call timeout (each op is + /// bounded by it) plus slack, so a recycle never kills a read that would have completed. + /// + private TimeSpan RecycleDrainBudget => + (_options.Timeout > TimeSpan.Zero ? _options.Timeout : TimeSpan.FromSeconds(2)) + TimeSpan.FromMilliseconds(500); + private async Task RecycleLoopAsync(DeviceState state, CancellationToken ct) { while (!ct.IsCancellationRequested) @@ -1788,14 +1795,31 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Close the current handle — the next Read / Write / Probe call triggers // EnsureConnectedAsync, which reopens a fresh one. We don't block here on - // reconnect because the goal is just to release the FWLIB handle slot; a - // readable tick one probe cycle later is an acceptable cost. - try { state.DisposeClient(); } + // reconnect because the goal is just to release the FWLIB handle slot. + // + // Under the device's ConnectGate, exactly like EnsureConnectedAsync's install: unserialised, the + // recycle could null a client a concurrent connect had just installed (orphaning a live two-socket + // session) or dispose one a poll was mid-read on (BadCommunicationError + Degraded every interval). + // The gate is held through the drain so no fresh session opens beside the old one — the CNC's + // handle pool is small — and the detached client is disposed only once the ops already holding it + // have finished (bounded by the call timeout). + try { await state.ConnectGate.WaitAsync(ct).ConfigureAwait(false); } + catch (OperationCanceledException) { break; } + try + { + var old = state.Client; + state.Client = null; + if (old is SynchronizedFocasClient sync) + await sync.DisposeWhenIdleAsync(RecycleDrainBudget).ConfigureAwait(false); + else + old?.Dispose(); + } catch (Exception ex) { /* already disposed or race — next EnsureConnected recovers */ _logger.LogDebug(ex, "FOCAS handle-recycle dispose failed for {Host}", state.Options.HostAddress); } + finally { state.ConnectGate.Release(); } } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/SynchronizedFocasClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/SynchronizedFocasClient.cs index 9dc81fca..a0bade40 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/SynchronizedFocasClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/SynchronizedFocasClient.cs @@ -32,6 +32,9 @@ public sealed class SynchronizedFocasClient : IFocasClient private readonly IFocasClient _inner; private readonly TimeSpan _callTimeout; private readonly SemaphoreSlim _gate = new(1, 1); + /// Ops holding or queued for — what drains. + private int _pending; + private int _disposed; /// Wraps with per-device serialization + a per-call timeout. /// The underlying FOCAS client to serialize access to. @@ -115,26 +118,66 @@ public sealed class SynchronizedFocasClient : IFocasClient public Task> GetSpindleMotorSpeedsAsync(CancellationToken cancellationToken) => RunBoundedAsync(ct => _inner.GetSpindleMotorSpeedsAsync(ct), cancellationToken); - /// Disposes the inner client and the call gate. + /// + /// Disposes the inner client. Ops still queued on the call gate then fail with + /// instead of running on a disposed socket. The gate itself is NOT + /// disposed: an op that already holds it releases it on the way out, and releasing a disposed + /// threw out of a read that had otherwise completed. + /// public void Dispose() { + if (Interlocked.Exchange(ref _disposed, 1) != 0) return; _inner.Dispose(); - _gate.Dispose(); + } + + /// + /// Disposes once every op that already holds (or is queued for) this client has finished, waiting at most + /// . Used by the handle-recycle loop, which detaches the client under the device's + /// connect gate first — so nothing new can reach it — and then must not kill a read that is mid-flight on + /// it (that read used to surface BadCommunicationError + Degraded on every recycle tick). + /// + /// Upper bound on the wait; the client is disposed regardless once it passes. + /// A task that completes when the client is disposed. + public async Task DisposeWhenIdleAsync(TimeSpan maxWait) + { + var deadline = DateTime.UtcNow + maxWait; + while (Volatile.Read(ref _pending) > 0 && DateTime.UtcNow < deadline) + await Task.Delay(5, CancellationToken.None).ConfigureAwait(false); + Dispose(); + } + + /// Gets the number of ops currently holding or queued for the call gate. Test seam. + internal int PendingOperations => Volatile.Read(ref _pending); + + private void ThrowIfDisposed() + { + if (Volatile.Read(ref _disposed) != 0) + throw new ObjectDisposedException(nameof(SynchronizedFocasClient), "The FOCAS client was recycled or disposed."); } // Gate only — the caller already governs the budget (connect timeout arg / probe linked token). private async Task RunGatedAsync(Func> op, CancellationToken ct) { - await _gate.WaitAsync(ct).ConfigureAwait(false); - try { return await op(ct).ConfigureAwait(false); } - finally { _gate.Release(); } + Interlocked.Increment(ref _pending); + try + { + await _gate.WaitAsync(ct).ConfigureAwait(false); + try { ThrowIfDisposed(); return await op(ct).ConfigureAwait(false); } + finally { _gate.Release(); } + } + finally { Interlocked.Decrement(ref _pending); } } private async Task RunGatedAsync(Func op, CancellationToken ct) { - await _gate.WaitAsync(ct).ConfigureAwait(false); - try { await op(ct).ConfigureAwait(false); } - finally { _gate.Release(); } + Interlocked.Increment(ref _pending); + try + { + await _gate.WaitAsync(ct).ConfigureAwait(false); + try { ThrowIfDisposed(); await op(ct).ConfigureAwait(false); } + finally { _gate.Release(); } + } + finally { Interlocked.Decrement(ref _pending); } } // Gate + per-call timeout. A fired timeout surfaces as OperationCanceledException whose token is @@ -142,16 +185,22 @@ public sealed class SynchronizedFocasClient : IFocasClient // their own token's IsCancellationRequested. private async Task RunBoundedAsync(Func> op, CancellationToken ct) { - await _gate.WaitAsync(ct).ConfigureAwait(false); + Interlocked.Increment(ref _pending); try { - if (_callTimeout <= TimeSpan.Zero) - return await op(ct).ConfigureAwait(false); + await _gate.WaitAsync(ct).ConfigureAwait(false); + try + { + ThrowIfDisposed(); + if (_callTimeout <= TimeSpan.Zero) + return await op(ct).ConfigureAwait(false); - using var linked = CancellationTokenSource.CreateLinkedTokenSource(ct); - linked.CancelAfter(_callTimeout); - return await op(linked.Token).ConfigureAwait(false); + using var linked = CancellationTokenSource.CreateLinkedTokenSource(ct); + linked.CancelAfter(_callTimeout); + return await op(linked.Token).ConfigureAwait(false); + } + finally { _gate.Release(); } } - finally { _gate.Release(); } + finally { Interlocked.Decrement(ref _pending); } } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasHandleRecycleTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasHandleRecycleTests.cs index 97042231..4c287b4f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasHandleRecycleTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasHandleRecycleTests.cs @@ -62,6 +62,73 @@ public sealed class FocasHandleRecycleTests await drv.ShutdownAsync(CancellationToken.None); } + /// A client whose reads take a while — long enough for a recycle tick to land mid-read. + private sealed class SlowReadClient : FakeFocasClient + { + public override async Task<(object? value, uint status)> ReadAsync( + FocasAddress address, FocasDataType type, CancellationToken ct) + { + await Task.Delay(200, ct).ConfigureAwait(false); + if (!IsConnected) throw new ObjectDisposedException(nameof(SlowReadClient), "disposed mid-read"); + return ((sbyte)5, FocasStatusMapper.Good); + } + } + + /// + /// #740 / R07:L6 — the recycle loop disposed the device client with no gate, so a recycle tick landing + /// mid-read killed that read (BadCommunicationError + Degraded, every interval). The recycle now takes the + /// device's connect gate and lets the in-flight read finish before disposing the client it detached. + /// + [Fact] + public async Task Recycle_tick_landing_mid_read_lets_the_read_finish() + { + var factory = new FakeFocasClientFactory { Customise = () => new SlowReadClient() }; + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")], + RawTags = [FocasRawTags.Entry(new FocasTagDefinition("R", "focas://10.0.0.5:8193", "R100", FocasDataType.Byte))], + Probe = new FocasProbeOptions { Enabled = false }, + HandleRecycle = new FocasHandleRecycleOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(50) }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + // Each 200 ms read spans at least three recycle ticks. + for (var i = 0; i < 3; i++) + { + var snap = (await drv.ReadAsync(["R"], CancellationToken.None))[0]; + snap.StatusCode.ShouldBe(FocasStatusMapper.Good, $"read {i} was killed by a recycle tick"); + snap.Value.ShouldBe((sbyte)5); + } + drv.GetHealth().State.ShouldBe(Core.Abstractions.DriverState.Healthy); + + // Positive control: the recycle really ran — the first client was disposed and a fresh one opened. + await WaitFor(() => factory.Clients[0].DisposeCount > 0, TimeSpan.FromSeconds(3)); + factory.Clients[0].DisposeCount.ShouldBeGreaterThan(0); + factory.Clients.Count.ShouldBeGreaterThan(1); + await drv.ShutdownAsync(CancellationToken.None); + factory.Clients.ShouldAllBe(c => !c.IsConnected, "no client opened around a recycle may be orphaned"); + } + + /// DisposeWhenIdleAsync drains the in-flight op, then disposes; a later op is refused, not run on a dead socket. + [Fact] + public async Task DisposeWhenIdle_drains_the_in_flight_op_then_refuses_new_ones() + { + var inner = new SlowReadClient(); + await inner.ConnectAsync(new FocasHostAddress("10.0.0.5", 8193), TimeSpan.FromSeconds(1), CancellationToken.None); + var sync = new SynchronizedFocasClient(inner, TimeSpan.FromSeconds(5)); + var addr = FocasAddress.TryParse("R100")!; + + var read = sync.ReadAsync(addr, FocasDataType.Byte, CancellationToken.None); + await Task.Delay(20); + sync.PendingOperations.ShouldBe(1); + var dispose = sync.DisposeWhenIdleAsync(TimeSpan.FromSeconds(5)); + + (await read).status.ShouldBe(FocasStatusMapper.Good); + await dispose; + inner.DisposeCount.ShouldBe(1); + await Should.ThrowAsync(() => sync.ReadAsync(addr, FocasDataType.Byte, CancellationToken.None)); + } + private static async Task WaitFor(Func pred, TimeSpan timeout) { var deadline = DateTime.UtcNow + timeout; From 29b566d51b435d383a47566839706f8a0c85abe4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:24:05 -0400 Subject: [PATCH 07/12] fix(focas): disambiguate the cnc_rdalmmsg2 record layout at 1,520-byte multiples (R07:H20) ParseAlarms took the 76-byte vendor layout whenever Length % 76 == 0, so a legacy 80-byte reply of exactly 19 records (1,520 B = 20 x 76) decoded as 20 garbage vendor records. The response block carries no record count (its 16-byte header is length/class/path/ command/rc/payload length), so the layout is chosen from what is known: the record count the request asked for (a reply never holds more), then each layout's field invariants on every record. For legacy records, the int32 type/axis/length must fit a short and the length must be at most 64. For vendor records, the i16 length must be at most 64. If both still fit, the layout whose declared lengths match the decoded text wins. Vendor stays the default when both readings are equally plausible, and unambiguous lengths are unchanged. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Wire/FocasWireClient.cs | 77 ++++++++++++++- .../FocasAlarmLayoutTests.cs | 96 +++++++++++++++++++ 2 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs index 53a23d2c..fd76bf66 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs @@ -1137,15 +1137,88 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// The requested record count. /// The decoded alarms. /// The payload length matches neither record layout. + /// + /// Choosing the layout. The response block carries no record count, so a length that is a + /// multiple of BOTH strides (1,520 B = 20 × 76 = 19 × 80, and its multiples) is ambiguous; picking the + /// vendor stride on Length % 76 == 0 alone decoded 19 legacy records as 20 garbage ones. The choice + /// now uses what IS known: (1) the record count the REQUEST asked for — a reply never holds more, so a + /// layout implying more records than is ruled out; (2) each layout's own field + /// invariants, checked on every record ( / ). + /// Only when both readings remain plausible does the vendor layout win, as before. + /// internal static IReadOnlyList ParseAlarms(byte[] payload, short count) { - if (payload.Length % 76 == 0) return ParseVendorAlarms(payload, count); - if (payload.Length % 80 == 0) return ParseLegacyWireAlarms(payload, count); + if (payload.Length == 0) return []; + var vendor = payload.Length % VendorAlarmStride == 0 && payload.Length / VendorAlarmStride <= Math.Max((int)count, 1); + var legacy = payload.Length % LegacyAlarmStride == 0 && payload.Length / LegacyAlarmStride <= Math.Max((int)count, 1); + if (vendor && legacy) + { + // Ambiguous length: a legacy record's int32 type/axis/length fields must each fit the short they are + // read into and its length must fit the 64-byte message — a vendor payload read at the legacy stride + // fails that on its first non-empty message. Prefer the reading whose every record is self-consistent. + var legacyOk = LegacyRecordsValid(payload); + var vendorOk = VendorRecordsValid(payload); + if (legacyOk && !vendorOk) vendor = false; + else if (legacyOk && vendorOk && LengthsMatchText(payload, LegacyAlarmStride, 12, 4, 16) + && !LengthsMatchText(payload, VendorAlarmStride, 10, 2, 12)) vendor = false; + } + if (vendor) return ParseVendorAlarms(payload, count); + if (legacy) return ParseLegacyWireAlarms(payload, count); + if (payload.Length % VendorAlarmStride == 0 || payload.Length % LegacyAlarmStride == 0) + { + // Fits a stride but holds more records than were requested: fall back to the stride it fits, capped + // at the count, exactly as before this check existed. + return payload.Length % VendorAlarmStride == 0 + ? ParseVendorAlarms(payload, count) + : ParseLegacyWireAlarms(payload, count); + } throw new FocasWireException( $"cnc_rdalmmsg2 returned a {payload.Length}-byte payload, which fits neither the 76- nor the 80-byte " + "record layout; refusing to read it as an empty alarm list.", rc: null); } + private const int VendorAlarmStride = 76; + private const int LegacyAlarmStride = 80; + private const int AlarmMessageBytes = 64; + + /// Every 80-byte record's int32 type / axis / length fit a short, and the length fits the message. + private static bool LegacyRecordsValid(byte[] payload) + { + for (var o = 0; o + LegacyAlarmStride <= payload.Length; o += LegacyAlarmStride) + { + var type = ReadInt32(payload, o + 4); + var axis = ReadInt32(payload, o + 8); + var length = ReadInt32(payload, o + 12); + if (type is < short.MinValue or > short.MaxValue) return false; + if (axis is < short.MinValue or > short.MaxValue) return false; + if (length is < 0 or > AlarmMessageBytes) return false; + } + return true; + } + + /// Every 76-byte record's i16 message length fits the 64-byte message. + private static bool VendorRecordsValid(byte[] payload) + { + for (var o = 0; o + VendorAlarmStride <= payload.Length; o += VendorAlarmStride) + { + var length = ReadInt16(payload, o + 10); + if (length is < 0 or > AlarmMessageBytes) return false; + } + return true; + } + + /// Every record's declared message length equals the length of the text actually in its message field. + private static bool LengthsMatchText(byte[] payload, int stride, int lengthOffset, int lengthWidth, int messageOffset) + { + for (var o = 0; o + stride <= payload.Length; o += stride) + { + var length = lengthWidth == 4 ? ReadInt32(payload, o + lengthOffset) : ReadInt16(payload, o + lengthOffset); + var text = FocasWireProtocol.ReadAscii(payload.AsSpan(o + messageOffset, AlarmMessageBytes)); + if (length != text.Length) return false; + } + return true; + } + private static IReadOnlyList ParseVendorAlarms(byte[] payload, short count) { var alarms = new List(); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs new file mode 100644 index 00000000..bc1bc1b2 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs @@ -0,0 +1,96 @@ +using System.Buffers.Binary; +using System.Text; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:H20 — ParseAlarms picked the 76-byte vendor layout whenever Length % 76 == 0, so a +/// legacy 80-byte reply of exactly 19 records (1,520 B = 20 × 76) decoded as 20 garbage vendor records. The +/// response block carries no record count, so the layout is now chosen from the two things that are known: the +/// count the REQUEST asked for (a reply never holds more) and each layout's own field invariants. +/// +[Trait("Category", "Unit")] +public sealed class FocasAlarmLayoutTests +{ + private static byte[] Legacy(params (int no, int type, int axis, string msg)[] records) + { + var buf = new byte[records.Length * 80]; + for (var i = 0; i < records.Length; i++) + { + var o = i * 80; + var (no, type, axis, msg) = records[i]; + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o, 4), no); + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o + 4, 4), type); + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o + 8, 4), axis); + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o + 12, 4), msg.Length); + Encoding.ASCII.GetBytes(msg).CopyTo(buf.AsSpan(o + 16)); + } + return buf; + } + + private static byte[] Vendor(params (int no, short type, short axis, string msg)[] records) + { + var buf = new byte[records.Length * 76]; + for (var i = 0; i < records.Length; i++) + { + var o = i * 76; + var (no, type, axis, msg) = records[i]; + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o, 4), no); + BinaryPrimitives.WriteInt16BigEndian(buf.AsSpan(o + 4, 2), type); + BinaryPrimitives.WriteInt16BigEndian(buf.AsSpan(o + 6, 2), axis); + BinaryPrimitives.WriteInt16BigEndian(buf.AsSpan(o + 10, 2), (short)msg.Length); + Encoding.ASCII.GetBytes(msg).CopyTo(buf.AsSpan(o + 12)); + } + return buf; + } + + private static (int, int, int, string)[] LegacySet(int n) => + [.. Enumerable.Range(0, n).Select(i => (400 + i, 2 + (i % 3), 1 + (i % 4), $"SERVO ALARM {400 + i}"))]; + + private static (int, short, short, string)[] VendorSet(int n) => + [.. Enumerable.Range(0, n).Select(i => (1000 + i, (short)(4 + (i % 3)), (short)(i % 2), $"OVERTRAVEL +{i}"))]; + + /// The ambiguous length: 19 legacy records decode as 19 legacy records, not 20 vendor ones. + [Fact] + public void Nineteen_legacy_records_decode_as_legacy() + { + var payload = Legacy(LegacySet(19)); + payload.Length.ShouldBe(1520); + (payload.Length % 76).ShouldBe(0, "the length really is ambiguous"); + + var alarms = FocasWireClient.ParseAlarms(payload, 32); + + alarms.Count.ShouldBe(19); + alarms[0].AlarmNumber.ShouldBe(400); + alarms[0].Message.ShouldBe("SERVO ALARM 400"); + alarms[18].AlarmNumber.ShouldBe(418); + alarms[18].Axis.ShouldBe((short)(1 + (18 % 4))); + } + + /// Positive control at the same length: 20 vendor records still decode as vendor. + [Fact] + public void Twenty_vendor_records_decode_as_vendor() + { + var payload = Vendor(VendorSet(20)); + payload.Length.ShouldBe(1520); + + var alarms = FocasWireClient.ParseAlarms(payload, 32); + + alarms.Count.ShouldBe(20); + alarms[0].AlarmNumber.ShouldBe(1000); + alarms[19].Message.ShouldBe("OVERTRAVEL +19"); + } + + /// The unambiguous lengths are unchanged. + [Fact] + public void Unambiguous_lengths_are_unchanged() + { + FocasWireClient.ParseAlarms(Vendor(VendorSet(3)), 32).Count.ShouldBe(3); + FocasWireClient.ParseAlarms(Legacy(LegacySet(3)), 32).Count.ShouldBe(3); + FocasWireClient.ParseAlarms([], 32).ShouldBeEmpty(); + Should.Throw(() => FocasWireClient.ParseAlarms(new byte[77], 32)); + } +} From 2724d7dae84d5e5ddaeef727ca9f9456203a7968 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:26:28 -0400 Subject: [PATCH 08/12] fix(focas): a CNC refusal is per-tag Bad, not a driver Degraded (lane health checklist) Applies the driver-lane health contract to the value path. Every non-Good read status used to write Degraded on every poll, so one vacant macro (BadNoData), one EW_NUMBER address (BadOutOfRange) or one fractional macro on an integer tag made a reachable CNC look sick forever. Now: - a comms-class status (BadCommunicationError / BadInternalError (EW_HANDLE) / BadTimeout) still degrades, with LastSuccessfulRead preserved; - any other non-Good status is a refusal: the tag reads Bad, one Warning is logged per tag (re-armed when it next reads Good, cleared at init), driver state is untouched, and LastSuccessfulRead advances because the CNC answered (CAS, since devices now read concurrently); - a write refused with NotSupportedException no longer degrades the driver. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 54 ++++++- .../FocasAlarmProjectionTests.cs | 3 +- .../FocasReadHealthContractTests.cs | 140 ++++++++++++++++++ 3 files changed, 193 insertions(+), 4 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index f65bb2f4..c5bb0dda 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -235,6 +235,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, var generation = Interlocked.Increment(ref _lifecycleGeneration); await ReleaseDeviceStatesAsync().ConfigureAwait(false); ResetAlarmHealth(); + _refusalWarned.Clear(); WriteHealthIfCurrent(generation, new DriverHealth(DriverState.Initializing, null, null)); var devices = new Dictionary(StringComparer.OrdinalIgnoreCase); try @@ -756,11 +757,20 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[i] = new DataValueSnapshot(value, status, now, now); if (status == FocasStatusMapper.Good) + { + _refusalWarned.TryRemove(reference, out _); Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null)); - else + } + else if (IsCommsStatus(status)) + { Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, Volatile.Read(ref _health).LastSuccessfulRead, $"FOCAS status 0x{status:X8} reading {reference}")); + } + else + { + NoteRefusal(reference, def.Address, status, now); + } } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (OperationCanceledException) @@ -781,6 +791,43 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } + /// + /// RawPaths whose CNC refusal has already been logged — a refusal warns ONCE per tag, not once per poll, + /// and re-arms when the tag next reads Good. Cleared at initialise. + /// + private readonly ConcurrentDictionary _refusalWarned = new(StringComparer.Ordinal); + + /// + /// True for a status that says the LINK failed (or the read never got an answer), as opposed to the CNC + /// answering with a refusal. maps EW_SOCKET / EW_UNEXP / + /// EW_PARITY / unknown codes to BadCommunicationError and EW_HANDLE to BadInternalError; the wire client + /// reports a dead session as BadCommunicationError. + /// + private static bool IsCommsStatus(uint status) => status is FocasStatusMapper.BadCommunicationError + or FocasStatusMapper.BadInternalError or FocasStatusMapper.BadTimeout; + + /// + /// A deterministic CNC refusal (drivers.md health contract): per-tag Bad only — one Warning per tag, no + /// state change — but the CNC DID answer, so LastSuccessfulRead advances (else a reachable control whose + /// only tags are refused would look silent to the comms-loss watchdog). + /// + private void NoteRefusal(string reference, string address, uint status, DateTime now) + { + // CAS, not a plain write: devices are read concurrently, and a read-modify-write could otherwise put back + // the state another device's comms fault had just replaced. + while (true) + { + var current = Volatile.Read(ref _health); + if (current.LastSuccessfulRead is { } last && last >= now) break; + if (ReferenceEquals(Interlocked.CompareExchange(ref _health, current with { LastSuccessfulRead = now }, current), current)) + break; + } + if (_refusalWarned.TryAdd(reference, 0)) + _logger.LogWarning( + "FOCAS CNC refused {RawPath} ({Address}) with 0x{Status:X8}; the tag reads Bad until it is fixed (warned once). Driver={DriverInstanceId}", + reference, address, status, _driverInstanceId); + } + /// /// PMC coalescing pre-pass for ONE device's share of a batch. Plans merged ranges /// () and serves every multi-member range with ONE @@ -956,9 +1003,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (NotSupportedException nse) { + // A refusal, not a comms fault (drivers.md, #611): nothing was wrong with the link, so health is + // left alone — degrading here handed the comms-loss watchdog a reason to reconnect a healthy CNC. results[i] = new WriteResult(FocasStatusMapper.BadNotSupported); - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, nse.Message)); + _logger.LogDebug(nse, "FOCAS write to {RawPath} refused as unsupported", w.FullReference); } catch (Exception ex) when (ex is FormatException or InvalidCastException) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs index 9006936d..0749aafc 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmProjectionTests.cs @@ -645,7 +645,8 @@ public sealed class FocasAlarmProjectionTests drv.GetHealth().State.ShouldBe(DriverState.Healthy); // Value path Degraded, alarm path good ⇒ Degraded: an alarm success cannot mask it. - factory.Clients.Single().ReadStatuses["R100"] = FocasStatusMapper.BadOutOfRange; + // A COMMS-class status: a CNC refusal (e.g. BadOutOfRange) is not a value-path Degraded since #740. + factory.Clients.Single().ReadStatuses["R100"] = FocasStatusMapper.BadCommunicationError; await drv.ReadAsync([valuePath], CancellationToken.None); drv.NoteAlarmReadSucceeded(Host); drv.GetHealth().State.ShouldBe(DriverState.Degraded); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs new file mode 100644 index 00000000..88e7f206 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs @@ -0,0 +1,140 @@ +using Microsoft.Extensions.Logging; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 driver-lane health checklist (drivers.md, #527/#611): a deterministic CNC refusal — the control +/// answered with a non-OK return code, or with a value the tag's type cannot hold (vacant macro, fractional +/// macro on an integer tag) — is per-tag Bad with ONE Warning per tag and NO health write, and it still +/// proves the device answered (LastSuccessfulRead advances). Only a comms-class status degrades the driver. +/// Before this, every non-Good status latched Degraded every poll, so one vacant macro made a reachable CNC +/// look sick forever. +/// +[Trait("Category", "Unit")] +public sealed class FocasReadHealthContractTests +{ + private const string Host = "focas://10.0.0.5:8193"; + + private sealed class WarningCounter : ILogger + { + private int _warnings; + public int Warnings => Volatile.Read(ref _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 && formatter(state, exception).Contains("refused", StringComparison.Ordinal)) + Interlocked.Increment(ref _warnings); + } + } + + private static (FocasDriver drv, FakeFocasClient client, WarningCounter log) NewDriver(uint r100Status) + { + var client = new FakeFocasClient { Values = { ["R101"] = (sbyte)1 } }; + client.ReadStatuses["R100"] = r100Status; + var log = new WarningCounter(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host)], + RawTags = + [ + FocasRawTags.Entry(new FocasTagDefinition("raw/R100", Host, "R100", FocasDataType.Byte)), + FocasRawTags.Entry(new FocasTagDefinition("raw/R101", Host, "R101", FocasDataType.Byte)), + ], + Probe = new FocasProbeOptions { Enabled = false }, + PmcCoalesce = new FocasPmcCoalesceOptions { Enabled = false }, + }, "focas-contract", new FakeFocasClientFactory { Customise = () => client }, log); + return (drv, client, log); + } + + /// A refusal leaves health alone, warns once, and still credits the answer. + [Theory] + [InlineData(FocasStatusMapper.BadOutOfRange)] // EW_NUMBER + [InlineData(FocasStatusMapper.BadNotSupported)] // EW_FUNC / EW_NOOPT + [InlineData(FocasStatusMapper.BadNoData)] // vacant macro + [InlineData(FocasStatusMapper.BadTypeMismatch)] // fractional macro on an integer tag + [InlineData(FocasStatusMapper.BadDeviceFailure)] // EW_BUSY + public async Task Cnc_refusal_is_per_tag_bad_with_one_warning_and_no_health_write(uint refusal) + { + var (drv, _, log) = NewDriver(refusal); + await drv.InitializeAsync("{}", CancellationToken.None); + var atInit = drv.GetHealth().LastSuccessfulRead; + + for (var i = 0; i < 3; i++) + { + await Task.Delay(5); + var snap = (await drv.ReadAsync(["raw/R100"], CancellationToken.None))[0]; + snap.StatusCode.ShouldBe(refusal); + } + + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Healthy, "a CNC refusal is not a comms fault"); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(atInit!.Value, "the CNC answered — that is evidence of life"); + log.Warnings.ShouldBe(1, "one Warning per refused tag, not one per poll"); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// A refused tag that recovers and is refused again warns again (the dedup clears on Good). + [Fact] + public async Task Refusal_warning_rearms_after_the_tag_reads_good() + { + var (drv, client, log) = NewDriver(FocasStatusMapper.BadOutOfRange); + await drv.InitializeAsync("{}", CancellationToken.None); + await drv.ReadAsync(["raw/R100"], CancellationToken.None); + client.ReadStatuses.Remove("R100"); + client.Values["R100"] = (sbyte)3; + (await drv.ReadAsync(["raw/R100"], CancellationToken.None))[0].StatusCode.ShouldBe(FocasStatusMapper.Good); + client.ReadStatuses["R100"] = FocasStatusMapper.BadOutOfRange; + await drv.ReadAsync(["raw/R100"], CancellationToken.None); + log.Warnings.ShouldBe(2); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// Positive control: a comms-class status still degrades, preserving LastSuccessfulRead. + [Fact] + public async Task Comms_status_still_degrades_with_last_successful_read_preserved() + { + var (drv, _, _) = NewDriver(FocasStatusMapper.BadCommunicationError); + await drv.InitializeAsync("{}", CancellationToken.None); + await drv.ReadAsync(["raw/R101"], CancellationToken.None); + var lastGood = drv.GetHealth().LastSuccessfulRead; + + await drv.ReadAsync(["raw/R100"], CancellationToken.None); + + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Degraded); + health.LastSuccessfulRead.ShouldBe(lastGood); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// A write the driver refuses (NotSupportedException) is not a comms fault either. + [Fact] + public async Task Write_refusal_does_not_degrade() + { + var client = new ThrowingWriteClient(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host)], + RawTags = [FocasRawTags.Entry(new FocasTagDefinition("raw/W", Host, "R100", FocasDataType.Byte, Writable: true))], + Probe = new FocasProbeOptions { Enabled = false }, + }, "focas-write-refusal", new FakeFocasClientFactory { Customise = () => client }); + await drv.InitializeAsync("{}", CancellationToken.None); + + var result = await drv.WriteAsync([new WriteRequest("raw/W", (sbyte)1)], CancellationToken.None); + + result[0].StatusCode.ShouldBe(FocasStatusMapper.BadNotSupported); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.ShutdownAsync(CancellationToken.None); + } + + private sealed class ThrowingWriteClient : FakeFocasClient + { + public override Task WriteAsync(FocasAddress address, FocasDataType type, object? value, CancellationToken ct) => + throw new NotSupportedException("bit writes are not supported on this area"); + } +} From 8ffa6178b577b7030e03a1e5892e4b2b68aa917e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:27:38 -0400 Subject: [PATCH 09/12] test(focas): integration case for re-initialise under live wire reads (R07:H13) Against the focas-mock fixture: the fixed-tree loop is running and reads are in flight while the driver re-initialises five times. No reader throws and reads keep returning Good. After shutdown, a fresh driver still connects and reads. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Series/WireBackendCoverageTests.cs | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs index 4cbf460e..ae0055b3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendCoverageTests.cs @@ -141,6 +141,86 @@ public sealed class WireBackendCoverageTests } } + /// + /// #740 / R07:H13 — re-initialising a live driver (the fixed-tree loop running, reads in flight over the real + /// wire) swaps the device table instead of clearing it under its readers: no reader throws, reads keep + /// returning Good, and shutdown leaves no session behind (a fresh driver can still connect afterwards). + /// + [Fact] + public async Task Reinitialise_under_live_reads_swaps_the_device_table() + { + if (_fx.SkipReason is not null) Assert.Skip(_fx.SkipReason); + var ct = TestContext.Current.CancellationToken; + + await _fx.LoadProfileAsync("FWLIB64", ct); + await _fx.PatchStateAsync(new + { + macros = new Dictionary { ["500"] = new { value = 42000, @decimal = 3 } }, + }, ct); + + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(DeviceHost)], + RawTags = FocasRawTags.Entries( + new FocasTagDefinition("Macro500", DeviceHost, "MACRO:500", FocasDataType.Float64, Writable: false)), + Probe = new FocasProbeOptions { Enabled = false }, + FixedTree = new FocasFixedTreeOptions + { + Enabled = true, + PollInterval = TimeSpan.FromMilliseconds(50), + ProgramPollInterval = TimeSpan.Zero, + TimerPollInterval = TimeSpan.Zero, + }, + }, driverInstanceId: "wire-reinit-swap", clientFactory: new WireFocasClientFactory()); + + await using (drv) + { + await drv.InitializeAsync("{}", ct); + using var stop = CancellationTokenSource.CreateLinkedTokenSource(ct); + var errors = new ConcurrentQueue(); + var good = 0; + var reader = Task.Run(async () => + { + while (!stop.IsCancellationRequested) + { + try + { + _ = drv.GetHostStatuses(); + var s = await drv.ReadAsync(["Macro500", $"{DeviceHost}/Identity/SeriesNumber"], CancellationToken.None); + if (s[0].StatusCode == FocasStatusMapper.Good) Interlocked.Increment(ref good); + } + catch (Exception ex) { errors.Enqueue(ex); } + } + }, CancellationToken.None); + + for (var i = 0; i < 5; i++) + { + await Task.Delay(150, ct); + await drv.InitializeAsync("{}", ct); + } + await Task.Delay(300, ct); + stop.Cancel(); + await reader; + + errors.ShouldBeEmpty(); + good.ShouldBeGreaterThan(0, "positive control: reads went over the wire between re-initialises"); + } + + // A fresh driver still connects and reads Good after the re-initialise churn and the shutdown. + var after = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(DeviceHost)], + RawTags = FocasRawTags.Entries( + new FocasTagDefinition("Macro500", DeviceHost, "MACRO:500", FocasDataType.Float64, Writable: false)), + Probe = new FocasProbeOptions { Enabled = false }, + }, driverInstanceId: "wire-reinit-after", clientFactory: new WireFocasClientFactory()); + await using (after) + { + await after.InitializeAsync("{}", ct); + (await after.ReadAsync(["Macro500"], ct))[0].StatusCode.ShouldBe(FocasStatusMapper.Good); + } + } + /// Verifies that discover emits device folder and tag variables. [Fact] public async Task Discover_emits_device_folder_and_tag_variables() From fd64fed21784253cd3de0c1851e6905894a373be Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:44:42 -0400 Subject: [PATCH 10/12] fix(focas): a stalled device fails fast for the rest of its batch; generation-stamp health (#740 review round 1) I1: Task.WhenAll waits for the slowest device, so concurrency alone still let a device that accepts TCP and then stalls cost (its tag count x call timeout) per batch. After the first timeout, transport failure or comms-class status on a device, its remaining tags in the batch now fail BadCommunicationError immediately, the same as the coalescer's FailMembers, which now also stops after a failed range. FIXED: tags are still served from the cache. A malformed address is BadNodeIdUnknown and does not count as a link failure. Test: a 10-tag stalled device beside a healthy one completes in under 3 call timeouts with the healthy value Good. The P9 test now also asserts per-client read counts and that two reads overlapped across devices. Minors: - 1: a superseded init releases its table only if it was never published (flag). - 2: a fixed-tree refusal credits LastSuccessfulRead; a value-path refusal after a comms fault restores Healthy. - 5: alarm/fixed/refusal health is reset inside the publish lock, after the generation check. - 6: FocasFixedTreeHealthTests wait on served-failure/refusal counters, not wall-clock. - 8: data-path health writes (reads, coalescer, writes, poll errors, refusals) carry the generation they started under and are dropped once a lifecycle call has moved on. Each write is a check-and-set under the lifecycle lock. Also adds a unit case where 20 vendor records with type 0 and empty messages stay vendor. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 203 ++++++++++++------ .../FocasAlarmLayoutTests.cs | 18 ++ .../FocasConcurrentDeviceReadTests.cs | 75 ++++++- .../FocasFixedTreeHealthTests.cs | 53 +++-- .../FocasReadHealthContractTests.cs | 72 +++++++ 5 files changed, 335 insertions(+), 86 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index c5bb0dda..5177119b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -200,10 +200,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, internal void HandlePollError(Exception ex) { _logger.LogWarning(ex, "FOCAS poll reader failed. Driver={DriverInstanceId}", _driverInstanceId); - var current = Volatile.Read(ref _health); - if (current.State != DriverState.Faulted) - Volatile.Write(ref _health, - new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, ex.Message)); + DegradeHealth(CurrentGeneration, ex.Message); } /// @@ -234,10 +231,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // shutdown) is superseded by that shutdown's bump, and checks for it before publishing anything. var generation = Interlocked.Increment(ref _lifecycleGeneration); await ReleaseDeviceStatesAsync().ConfigureAwait(false); - ResetAlarmHealth(); - _refusalWarned.Clear(); WriteHealthIfCurrent(generation, new DriverHealth(DriverState.Initializing, null, null)); var devices = new Dictionary(StringComparer.OrdinalIgnoreCase); + // Set once this attempt's table is live: from then on it is the NEXT lifecycle call's to release, never + // this attempt's (review minor 1 — a reference compare could release a table twice). + var published = false; try { // adopt a changed config. Options and the backend client factory are re-derived by ONE @@ -361,6 +359,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // leaves the previous snapshot standing rather than a truncated one that would read as // "we now skip fewer tags". _skippedTags.Commit(); + // The per-path health of the previous table, reset only now that this attempt is known to be the + // live one (review minor 5): a superseded attempt must not wipe the health of the call that + // superseded it. + ResetAlarmHealth(); + _refusalWarned.Clear(); Volatile.Write(ref _parsedAddressesByTagName, parsedAddresses); Volatile.Write(ref _tagsByRawPath, tagsByRawPath); Volatile.Write(ref _alarmTags, alarmTags); @@ -370,6 +373,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, _fixedTreeActive = _options.FixedTree.Enabled || parsedAddresses.Values.Any(a => a.Kind == FocasAreaKind.Fixed); Volatile.Write(ref _devices, devices); + published = true; StartDeviceLoops(devices.Values); } @@ -390,7 +394,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { // Superseded: whatever this attempt built but never published is released here (a published table // is the superseding call's to release); health belongs to that call too. - if (!ReferenceEquals(Devices, devices)) await ReleaseTableAsync(devices.Values).ConfigureAwait(false); + if (!published) await ReleaseTableAsync(devices.Values).ConfigureAwait(false); throw; } catch (Exception ex) @@ -647,7 +651,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, ArgumentNullException.ThrowIfNull(fullReferences); var now = DateTime.UtcNow; var results = new DataValueSnapshot[fullReferences.Count]; - // One snapshot of each swapped table for the whole call (see _devices). + // The generation FIRST, then one snapshot of each swapped table for the whole call (see _devices): health + // written by this batch is dropped if a lifecycle call has moved on meanwhile. + var generation = CurrentGeneration; var devices = Devices; var alarmTags = Volatile.Read(ref _alarmTags); var fixedTreeActive = _fixedTreeActive; @@ -693,12 +699,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (byDevice.Count == 1) { foreach (var (device, indices) in byDevice) - await ReadDeviceAsync(device, indices, fullReferences, results, now, cancellationToken).ConfigureAwait(false); + await ReadDeviceAsync(device, indices, fullReferences, results, now, generation, cancellationToken).ConfigureAwait(false); } else { await Task.WhenAll(byDevice.Select(kv => - ReadDeviceAsync(kv.Key, kv.Value, fullReferences, results, now, cancellationToken))).ConfigureAwait(false); + ReadDeviceAsync(kv.Key, kv.Value, fullReferences, results, now, generation, cancellationToken))).ConfigureAwait(false); } return results; } @@ -708,16 +714,25 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// index it did not serve through the per-tag path. Each result lands at its own index, so concurrent /// device reads never touch the same slot. /// + /// + /// One link failure ends the device's batch (#740 review I1). Concurrency alone does not bound a + /// batch: Task.WhenAll waits for the slowest device, and a CNC that accepts TCP and then stalls costs + /// the full call timeout on EVERY tag — so a 10-tag stalled device held the healthy devices' values back by + /// ten timeouts. After the first timeout / transport failure (or a comms-class status) on this device, its + /// remaining tags in the batch fail fast with BadCommunicationError, exactly as the coalescer's + /// FailMembers does for a merged range. Fixed-tree tags are still served from the cache: they need no + /// link. A CNC REFUSAL does not end the batch — the link answered. + /// private async Task ReadDeviceAsync( DeviceState device, List indices, IReadOnlyList fullReferences, - DataValueSnapshot[] results, DateTime now, CancellationToken cancellationToken) + DataValueSnapshot[] results, DateTime now, int generation, CancellationToken cancellationToken) { // Merge adjacent / near PMC reads of the device into fewer pmc_rdpmcrng calls. Indices it serves are // marked here and skipped by the per-tag loop below; everything else (including every single-tag range // and any range the CNC rejected) takes the unchanged per-tag path. - var served = _options.PmcCoalesce.Enabled - ? await CoalescePmcReadsAsync(device, indices, fullReferences, results, now, cancellationToken).ConfigureAwait(false) - : null; + var (served, linkDown) = _options.PmcCoalesce.Enabled + ? await CoalescePmcReadsAsync(device, indices, fullReferences, results, now, generation, cancellationToken).ConfigureAwait(false) + : (null, false); foreach (var i in indices) { @@ -729,29 +744,43 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, continue; } + FocasAddress parsed; + try { parsed = ResolveParsedAddress(def); } + catch (InvalidOperationException) + { + // A malformed address is a config fault of this tag, not of the link. + results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadNodeIdUnknown, null, now); + continue; + } + + // A FIXED: tag names a field of this device's fixed-tree snapshot, which the + // fixed-tree loop already maintains — there is no area read to issue, and no + // connection to force. A null here means the path resolves to nothing on THIS + // device (an unsupported capability, a renamed axis, a typo), which is a genuine + // BadNodeIdUnknown; a device that has not answered yet reports a comms fault + // instead, because the address may well be fine. + if (parsed.Kind == FocasAreaKind.Fixed) + { + results[i] = TryReadFixedTreeOnDevice(device, parsed.FixedPath!, now) + ?? new DataValueSnapshot( + null, + device.LastFixedPollUtc is null + ? FocasStatusMapper.BadCommunicationError + : FocasStatusMapper.BadNodeIdUnknown, + null, + now); + continue; + } + + if (linkDown) + { + // Already failed (and health already degraded) this batch — don't pay another timeout. + results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadCommunicationError, null, now); + continue; + } + try { - var parsed = ResolveParsedAddress(def); - - // A FIXED: tag names a field of this device's fixed-tree snapshot, which the - // fixed-tree loop already maintains — there is no area read to issue, and no - // connection to force. A null here means the path resolves to nothing on THIS - // device (an unsupported capability, a renamed axis, a typo), which is a genuine - // BadNodeIdUnknown; a device that has not answered yet reports a comms fault - // instead, because the address may well be fine. - if (parsed.Kind == FocasAreaKind.Fixed) - { - results[i] = TryReadFixedTreeOnDevice(device, parsed.FixedPath!, now) - ?? new DataValueSnapshot( - null, - device.LastFixedPollUtc is null - ? FocasStatusMapper.BadCommunicationError - : FocasStatusMapper.BadNodeIdUnknown, - null, - now); - continue; - } - var client = await EnsureConnectedAsync(device, cancellationToken).ConfigureAwait(false); var (value, status) = await client.ReadAsync(parsed, def.DataType, cancellationToken).ConfigureAwait(false); @@ -759,17 +788,16 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (status == FocasStatusMapper.Good) { _refusalWarned.TryRemove(reference, out _); - Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null)); + CreditHealth(generation, now); } else if (IsCommsStatus(status)) { - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, - $"FOCAS status 0x{status:X8} reading {reference}")); + DegradeHealth(generation, $"FOCAS status 0x{status:X8} reading {reference}"); + linkDown = true; } else { - NoteRefusal(reference, def.Address, status, now); + NoteRefusal(reference, def.Address, status, now, generation); } } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } @@ -779,18 +807,48 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Timeout budget. Surface a recoverable comm error so the BadWaitingForInitialData // seed is overwritten and health degrades, instead of the read hanging forever. results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadCommunicationError, null, now); - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, $"FOCAS read timed out for {reference}")); + DegradeHealth(generation, $"FOCAS read timed out for {reference}"); + linkDown = true; } catch (Exception ex) { results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadCommunicationError, null, now); - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, ex.Message)); + DegradeHealth(generation, ex.Message); + linkDown = true; } } } + /// + /// Applies a data-path health transition — only while is still the live + /// lifecycle generation (#740 review minor 8): a read that started against the previous table must not + /// write its Degraded onto the next initialise's health. Taken under the lifecycle lock, so the check and + /// the write cannot straddle a publish; the lock is uncontended outside a lifecycle transition. + /// + /// The generation the caller's operation started under. + /// The transition; returns its input unchanged to leave health alone. + private void UpdateHealth(int generation, Func next) + { + lock (_lifecycleLock) + { + if (generation != Volatile.Read(ref _lifecycleGeneration)) return; + Volatile.Write(ref _health, next(Volatile.Read(ref _health))); + } + } + + /// A comms fault: Degraded with LastSuccessfulRead preserved; never downgrades Faulted. + private void DegradeHealth(int generation, string reason) => + UpdateHealth(generation, h => h.State == DriverState.Faulted + ? h + : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, reason)); + + /// An answered read: Healthy with LastSuccessfulRead advanced. + private void CreditHealth(int generation, DateTime now) => + UpdateHealth(generation, _ => new DriverHealth(DriverState.Healthy, now, null)); + + /// The live lifecycle generation — captured by a data-path operation when it starts. + private int CurrentGeneration => Volatile.Read(ref _lifecycleGeneration); + /// /// RawPaths whose CNC refusal has already been logged — a refusal warns ONCE per tag, not once per poll, /// and re-arms when the tag next reads Good. Cleared at initialise. @@ -811,17 +869,14 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// state change — but the CNC DID answer, so LastSuccessfulRead advances (else a reachable control whose /// only tags are refused would look silent to the comms-loss watchdog). /// - private void NoteRefusal(string reference, string address, uint status, DateTime now) + private void NoteRefusal(string reference, string address, uint status, DateTime now, int generation) { - // CAS, not a plain write: devices are read concurrently, and a read-modify-write could otherwise put back - // the state another device's comms fault had just replaced. - while (true) - { - var current = Volatile.Read(ref _health); - if (current.LastSuccessfulRead is { } last && last >= now) break; - if (ReferenceEquals(Interlocked.CompareExchange(ref _health, current with { LastSuccessfulRead = now }, current), current)) - break; - } + // The link answered, so a value-path Degraded left by an earlier comms fault is over (review minor 2): a + // device that recovers and then only refuses must not stay Degraded forever. Under the lifecycle lock + // (UpdateHealth), so concurrent device reads cannot interleave a read-modify-write. + UpdateHealth(generation, h => h.State is DriverState.Healthy or DriverState.Degraded + ? new DriverHealth(DriverState.Healthy, h.LastSuccessfulRead is { } l && l > now ? l : now, null) + : h); if (_refusalWarned.TryAdd(reference, 0)) _logger.LogWarning( "FOCAS CNC refused {RawPath} ({Address}) with 0x{Status:X8}; the tag reads Bad until it is fixed (warned once). Driver={DriverInstanceId}", @@ -844,9 +899,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// /// Returns the served indices, or null when nothing was served. /// - private async Task?> CoalescePmcReadsAsync( + private async Task<(HashSet? Served, bool LinkDown)> CoalescePmcReadsAsync( DeviceState device, List indices, IReadOnlyList references, DataValueSnapshot[] results, - DateTime now, CancellationToken ct) + DateTime now, int generation, CancellationToken ct) { List? items = null; foreach (var i in indices) @@ -863,12 +918,13 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /* malformed / unresolvable — the per-tag loop reports it with its usual status */ } } - if (items is null || items.Count < 2) return null; + if (items is null || items.Count < 2) return (null, false); var plans = FocasPmcCoalescer.Plan(items, _options.PmcCoalesce.MaxSpanBytes, _options.PmcCoalesce.MaxGapBytes); - if (!plans.Any(p => p.Members.Count >= 2 && !device.CoalesceRejected.ContainsKey(p.Key))) return null; + if (!plans.Any(p => p.Members.Count >= 2 && !device.CoalesceRejected.ContainsKey(p.Key))) return (null, false); HashSet? served = null; + var linkDown = false; // One connect attempt per device per batch: if it fails, every PMC tag of the device fails with // that status here instead of paying a connect attempt per plan and again per tag. IFocasClient client; @@ -878,13 +934,20 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { FailMembers(items, FocasStatusMapper.BadCommunicationError, $"FOCAS connect failed reading PMC tags of {device.Options.HostAddress}"); - return served; + return (served, true); } foreach (var plan in plans) { if (plan.Members.Count < 2) continue; if (device.CoalesceRejected.ContainsKey(plan.Key)) continue; + if (linkDown) + { + // The link failed on an earlier range this batch: fail the rest without another timeout. + FailMembers(plan.Members, FocasStatusMapper.BadCommunicationError, + $"FOCAS transport failure on {device.Options.HostAddress} earlier in this batch"); + continue; + } var members = plan.Members; try @@ -903,7 +966,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, : new DataValueSnapshot(null, FocasStatusMapper.BadOutOfRange, now, now); (served ??= []).Add(m.Index); } - Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null)); + CreditHealth(generation, now); } else { @@ -924,9 +987,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Transport failure / timeout: the link is suspect, so fail the members without per-tag retries. FailMembers(members, FocasStatusMapper.BadCommunicationError, $"FOCAS transport failure reading PMC range {plan.Key}"); + linkDown = true; } } - return served; + return (served, linkDown); void FailMembers(IEnumerable failed, uint status, string reason) { @@ -935,9 +999,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[m.Index] = new DataValueSnapshot(null, status, null, now); (served ??= []).Add(m.Index); } - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, - $"{reason} (0x{status:X8})")); + DegradeHealth(generation, $"{reason} (0x{status:X8})"); } } @@ -948,6 +1010,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, IReadOnlyList writes, CancellationToken cancellationToken) { ArgumentNullException.ThrowIfNull(writes); + var generation = CurrentGeneration; var results = new WriteResult[writes.Count]; for (var i = 0; i < writes.Count; i++) @@ -998,8 +1061,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Per-call timeout (not external cancellation) — the write stalled past the device // Timeout budget. Surface a recoverable comm error rather than aborting the batch. results[i] = new WriteResult(FocasStatusMapper.BadCommunicationError); - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, $"FOCAS write timed out for {w.FullReference}")); + DegradeHealth(generation, $"FOCAS write timed out for {w.FullReference}"); } catch (NotSupportedException nse) { @@ -1019,8 +1081,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, catch (Exception ex) { results[i] = new WriteResult(FocasStatusMapper.BadCommunicationError); - Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, - Volatile.Read(ref _health).LastSuccessfulRead, ex.Message)); + DegradeHealth(generation, ex.Message); } } @@ -1992,6 +2053,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// An answered fixed-tree pass of : clears its failure, advances LastSuccessfulRead. private void NoteFixedTreeAnswered(DeviceState state) { + if (state.IsRetired) return; _fixedTreeFailures.TryRemove(state.Options.HostAddress, out _); Interlocked.Exchange(ref _fixedTreeLastSuccessTicks, DateTime.UtcNow.Ticks); } @@ -2004,9 +2066,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// private void NoteFixedTreeFailed(DeviceState state, Exception error) { + if (state.IsRetired) return; // a released device's late tick says nothing about the live table if (FocasAlarmProjection.IsCncRefusal(error)) { - _fixedTreeFailures.TryRemove(state.Options.HostAddress, out _); + // The CNC answered — evidence of life (review minor 2): clear the failure AND credit the answer, so a + // control that only refuses after an outage does not look silent to the comms-loss watchdog. + NoteFixedTreeAnswered(state); return; } var since = state.LastFixedPollUtc ?? state.FixedTreeStartedUtc; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs index bc1bc1b2..467e72c3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs @@ -84,6 +84,24 @@ public sealed class FocasAlarmLayoutTests alarms[19].Message.ShouldBe("OVERTRAVEL +19"); } + /// + /// Review I2 edge: vendor records with type 0 and EMPTY messages also satisfy the legacy invariants (the + /// int32 read of type<<16 | axis is just the axis, and the "length" int32 is the empty message's + /// zero bytes) — both readings are plausible, so the vendor default must hold and the numbers decode. + /// + [Fact] + public void Twenty_vendor_records_with_type_zero_and_empty_messages_stay_vendor() + { + var payload = Vendor([.. Enumerable.Range(0, 20).Select(i => (2000 + i, (short)0, (short)(i % 3), ""))]); + payload.Length.ShouldBe(1520); + + var alarms = FocasWireClient.ParseAlarms(payload, 32); + + alarms.Count.ShouldBe(20); + alarms.Select(a => a.AlarmNumber).ShouldBe(Enumerable.Range(2000, 20)); + alarms.ShouldAllBe(a => a.Type == 0 && a.Message == ""); + } + /// The unambiguous lengths are unchanged. [Fact] public void Unambiguous_lengths_are_unchanged() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs index 9ca35cf7..23e0e4e3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs @@ -20,11 +20,12 @@ public sealed class FocasConcurrentDeviceReadTests /// STARTED — so a sequential walk (A first) can never finish A, while a concurrent one does. Also counts /// the peak number of reads in flight on any one client. /// - private sealed class OrderingClient(TaskCompletionSource bStarted) : FakeFocasClient + private sealed class OrderingClient(TaskCompletionSource bStarted, int[] global) : FakeFocasClient { private string _host = ""; private int _inFlight; public int PeakInFlight; + public int Reads; public override Task ConnectAsync(FocasHostAddress address, TimeSpan timeout, CancellationToken ct) { @@ -37,6 +38,9 @@ public sealed class FocasConcurrentDeviceReadTests { var now = Interlocked.Increment(ref _inFlight); InterlockedMax(ref PeakInFlight, now); + Interlocked.Increment(ref Reads); + // global[0] = reads in flight across EVERY client, global[1] = its peak. + InterlockedMax(ref global[1], Interlocked.Increment(ref global[0])); try { if (_host.Contains("10.0.0.6", StringComparison.Ordinal)) bStarted.TrySetResult(); @@ -44,7 +48,11 @@ public sealed class FocasConcurrentDeviceReadTests await Task.Delay(20, ct).ConfigureAwait(false); return ((sbyte)1, FocasStatusMapper.Good); } - finally { Interlocked.Decrement(ref _inFlight); } + finally + { + Interlocked.Decrement(ref _inFlight); + Interlocked.Decrement(ref global[0]); + } } private static void InterlockedMax(ref int target, int value) @@ -60,7 +68,8 @@ public sealed class FocasConcurrentDeviceReadTests public async Task Devices_are_read_concurrently_and_each_stays_serialised() { var bStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var factory = new FakeFocasClientFactory { Customise = () => new OrderingClient(bStarted) }; + var global = new int[2]; + var factory = new FakeFocasClientFactory { Customise = () => new OrderingClient(bStarted, global) }; var drv = new FocasDriver(new FocasDriverOptions { Devices = [new FocasDeviceOptions(HostA, DeviceName: "A"), new FocasDeviceOptions(HostB, DeviceName: "B")], @@ -81,8 +90,66 @@ public sealed class FocasConcurrentDeviceReadTests snaps.ShouldAllBe(s => s.StatusCode == FocasStatusMapper.Good, "device A's read can only finish once device B's has started — a sequential walk times A out"); factory.Clients.Count.ShouldBe(2); - factory.Clients.Cast().ShouldAllBe(c => c.PeakInFlight == 1, + var clients = factory.Clients.Cast().ToList(); + // Every client really served reads (2 on A, 1 on B), so "peak 1" is measured, not vacuous … + clients.Select(c => c.Reads).OrderBy(n => n).ShouldBe([1, 2]); + clients.ShouldAllBe(c => c.PeakInFlight == 1, "one device's socket is still request→response: never two reads in flight on it"); + // … and the two devices overlapped: two reads were in flight at once across the clients. + global[1].ShouldBe(2, "the devices must be read at the same time, not one after the other"); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// A device that accepted TCP and then stopped answering: every read hangs until the call timeout. + private sealed class StalledClient : FakeFocasClient + { + public int Reads; + + public override async Task<(object? value, uint status)> ReadAsync( + FocasAddress address, FocasDataType type, CancellationToken ct) + { + Interlocked.Increment(ref Reads); + await Task.Delay(Timeout.Infinite, ct).ConfigureAwait(false); + return (null, FocasStatusMapper.Good); + } + } + + /// + /// #740 review I1 — Task.WhenAll waits for the slowest device, so concurrency alone still made a + /// stalled device cost (its tag count × the call timeout) per batch, and the healthy device's values waited + /// that long. After the first timeout on a device its remaining tags in the batch fail fast: the batch is + /// bounded by about ONE call timeout and the healthy device's values are Good. + /// + [Fact] + public async Task A_stalled_device_costs_one_call_timeout_per_batch_not_one_per_tag() + { + var callTimeout = TimeSpan.FromMilliseconds(200); + var factory = new FakeFocasClientFactory(); + factory.Customise = () => factory.Clients.Count == 0 ? new StalledClient() : new FakeFocasClient { Values = { ["R100"] = (sbyte)9 } }; + var stalledTags = Enumerable.Range(0, 10) + .Select(i => FocasRawTags.Entry(new FocasTagDefinition($"raw/A{i}", "A", $"R{100 + i}", FocasDataType.Byte))); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(HostA, DeviceName: "A"), new FocasDeviceOptions(HostB, DeviceName: "B")], + RawTags = [.. stalledTags, FocasRawTags.Entry(new FocasTagDefinition("raw/B1", "B", "R100", FocasDataType.Byte))], + Probe = new FocasProbeOptions { Enabled = false }, + PmcCoalesce = new FocasPmcCoalesceOptions { Enabled = false }, + Timeout = callTimeout, + }, "focas-stall", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + // Connect device A first so it gets the stalled client (connect order = client order). + await drv.ReadAsync(["raw/A0"], CancellationToken.None); + + var refs = Enumerable.Range(0, 10).Select(i => $"raw/A{i}").Append("raw/B1").ToArray(); + var sw = System.Diagnostics.Stopwatch.StartNew(); + var snaps = await drv.ReadAsync(refs, CancellationToken.None); + sw.Stop(); + + sw.Elapsed.ShouldBeLessThan(callTimeout * 3, "one stalled device must cost ~one call timeout, not ten"); + snaps[10].StatusCode.ShouldBe(FocasStatusMapper.Good); + snaps[10].Value.ShouldBe((sbyte)9); + snaps.Take(10).ShouldAllBe(s => s.StatusCode == FocasStatusMapper.BadCommunicationError); + ((StalledClient)factory.Clients[0]).Reads.ShouldBe(2, "one read in the warm-up batch, one in the measured batch"); await drv.ShutdownAsync(CancellationToken.None); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs index b83898e8..fae11d58 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs @@ -24,6 +24,10 @@ public sealed class FocasFixedTreeHealthTests { public volatile bool Down; public volatile bool Refuse; + /// Transport failures served so far — what the tests wait on instead of wall-clock delays. + public int Failures; + /// Refusals served so far. + public int Refusals; } private sealed class SwitchableCnc(CncSwitch sw) : FakeFocasClient @@ -31,13 +35,25 @@ public sealed class FocasFixedTreeHealthTests private bool Down => sw.Down; private bool Refuse => sw.Refuse; - public override Task GetSysInfoAsync(CancellationToken ct) => - Down ? throw new IOException("socket closed") : base.GetSysInfoAsync(ct); + public override Task GetSysInfoAsync(CancellationToken ct) + { + if (!Down) return base.GetSysInfoAsync(ct); + Interlocked.Increment(ref sw.Failures); + throw new IOException("socket closed"); + } public override Task ReadDynamicAsync(int axisIndex, CancellationToken ct) { - if (Down) throw new IOException("socket closed"); - if (Refuse) throw new FocasWireException("cnc_rddynamic2 refused", rc: 6); + if (Down) + { + Interlocked.Increment(ref sw.Failures); + throw new IOException("socket closed"); + } + if (Refuse) + { + Interlocked.Increment(ref sw.Refusals); + throw new FocasWireException("cnc_rddynamic2 refused", rc: 6); + } return base.ReadDynamicAsync(axisIndex, ct); } } @@ -71,18 +87,22 @@ public sealed class FocasFixedTreeHealthTests await drv.InitializeAsync("{}", CancellationToken.None); await WaitUntil(() => drv.GetDeviceState(Host)?.LastFixedPollUtc is not null); - var first = drv.GetHealth().LastSuccessfulRead; - await Task.Delay(100); - var healthy = drv.GetHealth(); - healthy.State.ShouldBe(DriverState.Healthy); - healthy.LastSuccessfulRead!.Value.ShouldBeGreaterThan(first!.Value, "an answered axis pass is a successful read"); + var first = drv.GetHealth().LastSuccessfulRead!.Value; + // An answered axis pass is a successful read: wait for it to move rather than sleeping. + await WaitUntil(() => drv.GetHealth().LastSuccessfulRead > first); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); sw.Down = true; - await Task.Delay(100); - drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a blip inside the staleness budget does not degrade"); + var lastAnswer = drv.GetDeviceState(Host)!.LastFixedPollUtc!.Value; + // Two failed ticks have demonstrably happened (positive evidence for the absence check below) … + await WaitUntil(() => Volatile.Read(ref sw.Failures) >= 2); + // … and, if they landed inside the budget, the driver is still Healthy. + if (DateTime.UtcNow - lastAnswer < Stale) + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a blip inside the staleness budget does not degrade"); await WaitUntil(() => drv.GetHealth().State == DriverState.Degraded); var frozen = drv.GetHealth().LastSuccessfulRead; - await Task.Delay(150); + var failuresAtDegrade = Volatile.Read(ref sw.Failures); + await WaitUntil(() => Volatile.Read(ref sw.Failures) >= failuresAtDegrade + 5); drv.GetHealth().State.ShouldBe(DriverState.Degraded); drv.GetHealth().LastSuccessfulRead.ShouldBe(frozen, "LastSuccessfulRead must stay frozen while the CNC is silent"); @@ -116,8 +136,15 @@ public sealed class FocasFixedTreeHealthTests await WaitUntil(() => drv.GetDeviceState(Host)?.LastFixedPollUtc is not null); sw.Refuse = true; - await Task.Delay(Stale * 3); + var started = DateTime.UtcNow; + await WaitUntil(() => Volatile.Read(ref sw.Refusals) >= 1); + var atFirstRefusal = drv.GetHealth().LastSuccessfulRead!.Value; + // Refused ticks really happened, for longer than the staleness budget … + await WaitUntil(() => Volatile.Read(ref sw.Refusals) >= 5 && DateTime.UtcNow - started > Stale * 2); + // … and the driver stayed Healthy, with each refusal crediting the answer (review minor 2). drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(atFirstRefusal, + "a refusal is an answer from the CNC — LastSuccessfulRead advances"); sw.Refuse = false; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs index 88e7f206..923ec354 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReadHealthContractTests.cs @@ -112,6 +112,78 @@ public sealed class FocasReadHealthContractTests await drv.ShutdownAsync(CancellationToken.None); } + /// + /// Review minor 2 — a device that recovers from a comms fault and then only REFUSES (every tag it serves is + /// refused) is answering, so it is restored to Healthy instead of staying Degraded forever. + /// + [Fact] + public async Task Refusal_after_a_comms_fault_restores_healthy() + { + var (drv, client, _) = NewDriver(FocasStatusMapper.BadCommunicationError); + await drv.InitializeAsync("{}", CancellationToken.None); + await drv.ReadAsync(["raw/R100"], CancellationToken.None); + drv.GetHealth().State.ShouldBe(DriverState.Degraded, "positive control: the comms fault degraded it"); + + client.ReadStatuses["R100"] = FocasStatusMapper.BadOutOfRange; + await drv.ReadAsync(["raw/R100"], CancellationToken.None); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// A read whose CNC answer is held until released, then fails at the transport. + private sealed class HeldReadClient(TaskCompletionSource entered, Task release) : FakeFocasClient + { + public volatile bool Hold = true; + + public override async Task<(object? value, uint status)> ReadAsync( + FocasAddress address, FocasDataType type, CancellationToken ct) + { + if (!Hold) return await base.ReadAsync(address, type, ct).ConfigureAwait(false); + entered.TrySetResult(); + await release.ConfigureAwait(false); + throw new IOException("socket reset"); + } + } + + /// + /// Review minor 8 — a read that started against the PREVIOUS table and fails after a re-initialise must not + /// write its Degraded onto the new generation's health. Positive control: the same failure under the live + /// generation does degrade. + /// + [Fact] + public async Task A_read_from_the_previous_generation_cannot_degrade_the_new_one() + { + var entered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var clients = new List(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host)], + RawTags = [FocasRawTags.Entry(new FocasTagDefinition("raw/R100", Host, "R100", FocasDataType.Byte))], + Probe = new FocasProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(10), + }, "focas-gen", new FakeFocasClientFactory + { + Customise = () => { var c = new HeldReadClient(entered, release.Task); lock (clients) clients.Add(c); return c; }, + }); + await drv.InitializeAsync("{}", CancellationToken.None); + + var stale = drv.ReadAsync(["raw/R100"], CancellationToken.None); + await entered.Task.WaitAsync(TimeSpan.FromSeconds(5)); + await drv.InitializeAsync("{}", CancellationToken.None); // a new generation, Healthy + release.SetResult(); + (await stale.WaitAsync(TimeSpan.FromSeconds(5)))[0].StatusCode.ShouldBe(FocasStatusMapper.BadCommunicationError); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "the old generation's failure must not land on the new one"); + + // Positive control: a failure under the live generation degrades. + var live = drv.ReadAsync(["raw/R100"], CancellationToken.None); + (await live.WaitAsync(TimeSpan.FromSeconds(5)))[0].StatusCode.ShouldBe(FocasStatusMapper.BadCommunicationError); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + await drv.ShutdownAsync(CancellationToken.None); + } + /// A write the driver refuses (NotSupportedException) is not a comms fault either. [Fact] public async Task Write_refusal_does_not_degrade() From de54b23b1a71a6301b35588ecf299ac64d56eb41 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:44:42 -0400 Subject: [PATCH 11/12] test(focas): wire-level cases for fixed-tree health and the legacy alarm layout (#740 review I2) Against focas-mock: - H15: a local TCP relay in front of the shared mock takes "the CNC" away and brings it back without stopping the fixture. A FIXED-only driver goes Degraded with LastSuccessfulRead frozen and its value Bad, then returns to Healthy with Good values (the positive control). - H20: the mock answers cnc_rdalmmsg2 in the 80-byte legacy layout. Nineteen alarms (1,520 B = 20 x 76) now decode as the 19 seeded alarms; the old length test gives 20, verified red. A 3-alarm reply is the control. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Series/WireBackendHealthAndLayoutTests.cs | 206 ++++++++++++++++++ 1 file changed, 206 insertions(+) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs new file mode 100644 index 00000000..30d39394 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs @@ -0,0 +1,206 @@ +using System.Net; +using System.Net.Sockets; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests.Series; + +/// +/// #740 fix round 1 (review I2) — wire-level cases for H15 (fixed-tree health) and H20 (alarm record layout) +/// against the focas-mock fixture. +/// +[Collection(FocasSimCollection.Name)] +public sealed class WireBackendHealthAndLayoutTests +{ + private readonly FocasSimFixture _fx; + + /// Initializes a new instance of the test class with the FOCAS simulation fixture. + /// The FOCAS simulation fixture. + public WireBackendHealthAndLayoutTests(FocasSimFixture fx) => _fx = fx; + + /// + /// A local TCP relay in front of the shared mock, so ONE test can take "the CNC" away and bring it back + /// without stopping a fixture other suites use: closes every relayed session and stops + /// listening (new connects are refused); listens again on the same port. + /// + private sealed class CncRelay : IAsyncDisposable + { + private readonly string _targetHost; + private readonly int _targetPort; + private readonly List _live = []; + private TcpListener? _listener; + private CancellationTokenSource? _cts; + + public CncRelay(string targetHost, int targetPort) + { + _targetHost = targetHost; + _targetPort = targetPort; + Port = Listen(0); + } + + public int Port { get; private set; } + + private int Listen(int port) + { + _cts = new CancellationTokenSource(); + _listener = new TcpListener(IPAddress.Loopback, port); + _listener.Server.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); + _listener.Start(); + var ct = _cts.Token; + var listener = _listener; + _ = Task.Run(() => AcceptLoopAsync(listener, ct), CancellationToken.None); + return ((IPEndPoint)_listener.LocalEndpoint).Port; + } + + private async Task AcceptLoopAsync(TcpListener listener, CancellationToken ct) + { + while (!ct.IsCancellationRequested) + { + TcpClient inbound; + try { inbound = await listener.AcceptTcpClientAsync(ct); } + catch { return; } + var outbound = new TcpClient(); + try { await outbound.ConnectAsync(_targetHost, _targetPort, ct); } + catch { inbound.Dispose(); outbound.Dispose(); continue; } + lock (_live) { _live.Add(inbound); _live.Add(outbound); } + _ = Pump(inbound, outbound, ct); + _ = Pump(outbound, inbound, ct); + } + } + + private static async Task Pump(TcpClient from, TcpClient to, CancellationToken ct) + { + try { await from.GetStream().CopyToAsync(to.GetStream(), ct); } + catch { /* the relay was taken down, or a side closed */ } + finally { from.Dispose(); to.Dispose(); } + } + + public void Down() + { + _cts?.Cancel(); + _listener?.Stop(); + lock (_live) + { + foreach (var c in _live) c.Dispose(); + _live.Clear(); + } + } + + public void Up() => Listen(Port); + + public ValueTask DisposeAsync() + { + Down(); + return ValueTask.CompletedTask; + } + } + + /// + /// H15 over the wire: a driver serving only FIXED: tags is Healthy with a moving LastSuccessfulRead + /// while its CNC answers; when the CNC disappears it degrades with LastSuccessfulRead frozen (the #527 + /// watchdog's conjunction); and it recovers to Healthy when the CNC returns (positive control). + /// + [Fact] + public async Task Fixed_only_driver_degrades_on_cnc_loss_and_recovers_over_the_wire() + { + if (_fx.SkipReason is not null) Assert.Skip(_fx.SkipReason); + var ct = TestContext.Current.CancellationToken; + await _fx.LoadProfileAsync("FWLIB64", ct); + + await using var relay = new CncRelay(_fx.Host, _fx.Port); + var host = $"focas://127.0.0.1:{relay.Port}"; + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(host)], + RawTags = FocasRawTags.Entries( + new FocasTagDefinition("Series", host, "FIXED:Identity/SeriesNumber", FocasDataType.String, Writable: false)), + Probe = new FocasProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(1), + FixedTree = new FocasFixedTreeOptions + { + PollInterval = TimeSpan.FromMilliseconds(50), + ProgramPollInterval = TimeSpan.Zero, + TimerPollInterval = TimeSpan.Zero, + }, + }, driverInstanceId: "wire-fixed-health", clientFactory: new WireFocasClientFactory()) + { + FixedTreeStaleBudgetOverride = TimeSpan.FromMilliseconds(500), + }; + + await using (drv) + { + await drv.InitializeAsync("{}", ct); + await WaitFor(() => drv.GetDeviceState(host)?.LastFixedPollUtc is not null, TimeSpan.FromSeconds(10)); + var first = drv.GetHealth().LastSuccessfulRead!.Value; + await WaitFor(() => drv.GetHealth().LastSuccessfulRead > first, TimeSpan.FromSeconds(5)); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + + relay.Down(); + await WaitFor(() => drv.GetHealth().State == DriverState.Degraded, TimeSpan.FromSeconds(15)); + var frozen = drv.GetHealth().LastSuccessfulRead; + await Task.Delay(TimeSpan.FromSeconds(1), ct); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldBe(frozen, "nothing answered, so LastSuccessfulRead stays frozen"); + (await drv.ReadAsync(["Series"], ct))[0].StatusCode.ShouldBe(FocasStatusMapper.BadCommunicationError, + "the stale fixed-tree value reads Bad on the same budget that degrades the driver"); + + // Positive control: the CNC returns ⇒ Healthy again, LastSuccessfulRead moves. The connect backoff + // (1 s doubling) bounds how soon the loop re-dials. + relay.Up(); + await WaitFor(() => drv.GetHealth().State == DriverState.Healthy, TimeSpan.FromSeconds(40)); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(frozen!.Value); + (await drv.ReadAsync(["Series"], ct))[0].StatusCode.ShouldBe(FocasStatusMapper.Good); + } + } + + /// + /// H20 over the wire: the mock answers cnc_rdalmmsg2 in the 80-byte LEGACY layout. Nineteen active + /// alarms make a 1,520-byte payload — also exactly 20 × 76 — which the old length test decoded as twenty + /// garbage vendor records. Positive control: the unambiguous 3-alarm reply decodes the same alarms. + /// + [Fact] + public async Task Nineteen_legacy_alarms_decode_as_nineteen_over_the_wire() + { + if (_fx.SkipReason is not null) Assert.Skip(_fx.SkipReason); + var ct = TestContext.Current.CancellationToken; + await _fx.LoadProfileAsync("FWLIB64", ct); + + var client = new WireFocasClientFactory().Create(); + try + { + await client.ConnectAsync(FocasHostAddress.TryParse(_fx.DeviceUri)!, TimeSpan.FromSeconds(2), ct); + + await _fx.PatchStateAsync(new { alarms = Alarms(3) }, ct); + var three = await client.ReadAlarmsAsync(ct); + three.Count.ShouldBe(3); + three[2].AlarmNumber.ShouldBe(402); + + await _fx.PatchStateAsync(new { alarms = Alarms(19) }, ct); + var nineteen = await client.ReadAlarmsAsync(ct); + nineteen.Count.ShouldBe(19, "a 1,520-byte legacy reply is 19 records, not 20 vendor ones"); + nineteen.Select(a => a.AlarmNumber).ShouldBe(Enumerable.Range(400, 19)); + nineteen[18].Message.ShouldBe("SERVO ALARM 418"); + nineteen[18].Axis.ShouldBe((short)(1 + (18 % 4))); + } + finally + { + client.Dispose(); + await _fx.PatchStateAsync(new { alarms = Array.Empty() }, ct); + } + + static object[] Alarms(int n) => [.. Enumerable.Range(0, n) + .Select(i => (object)new { alm_no = 400 + i, type = 2 + (i % 3), axis = 1 + (i % 4), msg = $"SERVO ALARM {400 + i}" })]; + } + + private static async Task WaitFor(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("condition not reached"); + await Task.Delay(25); + } + } +} From 09e0fe18418e2cfb8c7909e66d04fce11e4214be Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:55:37 -0400 Subject: [PATCH 12/12] fix(focas): a returned status never ends a device's batch; round-2 minors (#740 review round 2) N1: linkDown is set only when NO reply arrived, i.e. a timeout, a transport exception or a connect failure. A status the CNC returned proves it answered. EW_REJECT and every unlisted rc map to BadCommunicationError, so a deterministic rc on one tag used to fail every later tag of a reachable control on every poll. The comms-classed status still degrades health. The coalescer already treated a returned refusal this way; it is unchanged. Tests: a returned EW_REJECT on tag 1 leaves tags 2 and 3 read and Good (red before). A timeout still fails the rest without reading them (positive control). Minors: - m1: the _lifecycleLock doc now covers the health transitions it also guards. - m2: the NoteFixedTreeFailed summary says a refusal credits the answer. - m3: ReadAsync stamps a failed batch's exception with the generation the batch started under, and HandlePollError degrades that generation only. - m4: the fixed-tree "blip" absence check runs unconditionally under a wide budget, and asserts that both ticks landed inside it. - m5: direct unit cases for the coalescer link-down paths. A transport fault on the first range fails the second range and the per-tag tag without reading them. A connect failure costs one connect per batch. CncRelay.Up() retries the re-bind. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../FocasDriver.cs | 53 ++++++--- .../Series/WireBackendHealthAndLayoutTests.cs | 14 ++- .../FocasConcurrentDeviceReadTests.cs | 102 ++++++++++++++++++ .../FocasFixedTreeHealthTests.cs | 15 +-- .../FocasPollErrorHealthTests.cs | 28 +++++ 5 files changed, 190 insertions(+), 22 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 5177119b..bebe023a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -59,8 +59,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, private IReadOnlyDictionary Devices => Volatile.Read(ref _devices); /// - /// Guards the publish/retire of the device table against an overlapping lifecycle call. Held only for - /// synchronous swaps — never across an await, and nothing a loop runs ever takes it. + /// Guards the publish/retire of the device table against an overlapping lifecycle call, and makes each + /// generation-checked health transition atomic (). Held only for synchronous swaps + /// and single health transitions — never across an await, and nothing is called under it. /// private readonly object _lifecycleLock = new(); @@ -200,7 +201,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, internal void HandlePollError(Exception ex) { _logger.LogWarning(ex, "FOCAS poll reader failed. Driver={DriverInstanceId}", _driverInstanceId); - DegradeHealth(CurrentGeneration, ex.Message); + // The generation the failed batch started under, when ReadAsync stamped it; otherwise the live one. + var generation = ex.Data.Contains(BatchGenerationKey) && ex.Data[BatchGenerationKey] is int g ? g : CurrentGeneration; + DegradeHealth(generation, ex.Message); } /// @@ -696,19 +699,35 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } if (byDevice is null) return results; - if (byDevice.Count == 1) + try { - foreach (var (device, indices) in byDevice) - await ReadDeviceAsync(device, indices, fullReferences, results, now, generation, cancellationToken).ConfigureAwait(false); + if (byDevice.Count == 1) + { + foreach (var (device, indices) in byDevice) + await ReadDeviceAsync(device, indices, fullReferences, results, now, generation, cancellationToken).ConfigureAwait(false); + } + else + { + await Task.WhenAll(byDevice.Select(kv => + ReadDeviceAsync(kv.Key, kv.Value, fullReferences, results, now, generation, cancellationToken))).ConfigureAwait(false); + } } - else + catch (Exception ex) when (ex is not OperationCanceledException) { - await Task.WhenAll(byDevice.Select(kv => - ReadDeviceAsync(kv.Key, kv.Value, fullReferences, results, now, generation, cancellationToken))).ConfigureAwait(false); + // Stamp the generation this batch STARTED under (review round 2, m3), so the poll engine's onError + // (HandlePollError) cannot degrade a generation that re-initialised after the batch began. + ex.Data[BatchGenerationKey] = generation; + throw; } return results; } + /// key carrying the lifecycle generation a failed batch started under. + internal const string BatchGenerationKey = "FocasDriver.BatchGeneration"; + + /// Test seam — the live lifecycle generation. + internal int LifecycleGeneration => CurrentGeneration; + /// /// Reads one device's share of a batch, in request order: the PMC coalescing pre-pass first, then every /// index it did not serve through the per-tag path. Each result lands at its own index, so concurrent @@ -718,10 +737,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// One link failure ends the device's batch (#740 review I1). Concurrency alone does not bound a /// batch: Task.WhenAll waits for the slowest device, and a CNC that accepts TCP and then stalls costs /// the full call timeout on EVERY tag — so a 10-tag stalled device held the healthy devices' values back by - /// ten timeouts. After the first timeout / transport failure (or a comms-class status) on this device, its - /// remaining tags in the batch fail fast with BadCommunicationError, exactly as the coalescer's + /// ten timeouts. After the first timeout / transport failure / connect failure on this device — i.e. NO + /// reply arrived — its remaining tags in the batch fail fast with BadCommunicationError, exactly as the coalescer's /// FailMembers does for a merged range. Fixed-tree tags are still served from the cache: they need no - /// link. A CNC REFUSAL does not end the batch — the link answered. + /// link. Any status the CNC RETURNED — a refusal, or a comms-classed rc such as EW_REJECT — does not end the + /// batch: a reply proves the link answered. /// private async Task ReadDeviceAsync( DeviceState device, List indices, IReadOnlyList fullReferences, @@ -792,8 +812,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } else if (IsCommsStatus(status)) { + // Degrade, but do NOT end the batch (#740 review round 2, N1): this status came back in a reply, + // so the CNC answered — EW_REJECT and every unlisted rc map to BadCommunicationError. Ending the + // batch here starved every later tag of a reachable control forever (authored order is fixed). + // A session that is really dead throws or times out on the next tag, which does end it. DegradeHealth(generation, $"FOCAS status 0x{status:X8} reading {reference}"); - linkDown = true; } else { @@ -2059,8 +2082,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// - /// A failed fixed-tree tick. A CNC REFUSAL (non-OK rc, transport fine) is not a comms loss and clears any - /// failure without crediting a read; a transport failure degrades the driver only once the device has been + /// A failed fixed-tree tick. A CNC REFUSAL (non-OK rc, transport fine) is not a comms loss: it clears any + /// failure AND credits the answer to LastSuccessfulRead (the CNC replied); a transport failure degrades the driver only once the device has been /// silent past (measured from its last answer, or from loop start when /// it never answered), so one dropped tick on a slow control does not flap /hosts. /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs index 30d39394..c704b21f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendHealthAndLayoutTests.cs @@ -88,7 +88,19 @@ public sealed class WireBackendHealthAndLayoutTests } } - public void Up() => Listen(Port); + /// + /// Listens again on the SAME port (the driver's host string names it). The re-bind is retried briefly: the + /// OS can hold the port for a moment after , or hand it to another process — the latter + /// surfaces as a clear failure after the retries rather than a random flake. + /// + public void Up() + { + for (var attempt = 1; ; attempt++) + { + try { Listen(Port); return; } + catch (SocketException) when (attempt < 20) { Thread.Sleep(100); } + } + } public ValueTask DisposeAsync() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs index 23e0e4e3..f4fa7d11 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasConcurrentDeviceReadTests.cs @@ -153,6 +153,108 @@ public sealed class FocasConcurrentDeviceReadTests await drv.ShutdownAsync(CancellationToken.None); } + /// Counts per-tag reads; one address can be made to time out (throw) or answer a status. + private sealed class CountingClient : FakeFocasClient + { + public int Reads; + public string? TimeoutAt; + + public override async Task<(object? value, uint status)> ReadAsync( + FocasAddress address, FocasDataType type, CancellationToken ct) + { + Interlocked.Increment(ref Reads); + if (address.Canonical == TimeoutAt) + { + await Task.Delay(Timeout.Infinite, ct).ConfigureAwait(false); + } + return await base.ReadAsync(address, type, ct).ConfigureAwait(false); + } + } + + private static FocasDriver OneDevice(FakeFocasClientFactory factory, bool coalesce, params string[] addresses) => new( + new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(HostA, DeviceName: "A")], + RawTags = [.. addresses.Select(a => FocasRawTags.Entry(new FocasTagDefinition($"raw/{a}", "A", a, FocasDataType.Byte)))], + Probe = new FocasProbeOptions { Enabled = false }, + PmcCoalesce = new FocasPmcCoalesceOptions { Enabled = coalesce }, + Timeout = TimeSpan.FromMilliseconds(200), + }, "focas-linkdown", factory); + + /// + /// #740 review round 2 N1 — a status the CNC RETURNED (EW_REJECT and every unlisted rc map to + /// BadCommunicationError) proves the device answered, so it must not end the device's batch: the tags after it + /// still read. Before the fix, a deterministic rc on the first tag starved every later tag forever. + /// + [Fact] + public async Task A_returned_comms_status_does_not_fail_the_rest_of_the_batch() + { + var client = new CountingClient { Values = { ["R101"] = (sbyte)1, ["R102"] = (sbyte)2 } }; + client.ReadStatuses["R100"] = FocasStatusMapper.MapFocasReturn(13); // EW_REJECT ⇒ BadCommunicationError + var drv = OneDevice(new FakeFocasClientFactory { Customise = () => client }, coalesce: false, "R100", "R101", "R102"); + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/R100", "raw/R101", "raw/R102"], CancellationToken.None); + + snaps[0].StatusCode.ShouldBe(FocasStatusMapper.BadCommunicationError); + snaps[1].StatusCode.ShouldBe(FocasStatusMapper.Good); + snaps[2].Value.ShouldBe((sbyte)2); + client.Reads.ShouldBe(3, "every tag was really read"); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// Positive control for N1: a TIMEOUT (no reply at all) still fails the rest of the batch without reading it. + [Fact] + public async Task A_timeout_still_fails_the_rest_of_the_batch_fast() + { + var client = new CountingClient { TimeoutAt = "R100", Values = { ["R101"] = (sbyte)1 } }; + var drv = OneDevice(new FakeFocasClientFactory { Customise = () => client }, coalesce: false, "R100", "R101", "R102"); + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/R100", "raw/R101", "raw/R102"], CancellationToken.None); + + snaps.ShouldAllBe(s => s.StatusCode == FocasStatusMapper.BadCommunicationError); + client.Reads.ShouldBe(1, "only the tag that timed out touched the wire"); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// + /// Review round 2 m5 — the coalescer's link-down paths: a transport fault on the first merged range fails the + /// second range WITHOUT a second range read, and the per-tag tags of the same device without a per-tag read. + /// + [Fact] + public async Task Coalescer_transport_fault_fails_later_ranges_and_per_tag_tags_without_reading_them() + { + var client = new CountingClient { ThrowOnPmcRange = true }; + // Two ranges far apart (gap > MaxGapBytes) + one lone tag that takes the per-tag path. + var drv = OneDevice(new FakeFocasClientFactory { Customise = () => client }, coalesce: true, + "R100", "R101", "R500", "R501", "D900"); + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/R100", "raw/R101", "raw/R500", "raw/R501", "raw/D900"], CancellationToken.None); + + snaps.ShouldAllBe(s => s.StatusCode == FocasStatusMapper.BadCommunicationError); + client.PmcRangeReads.Count.ShouldBe(1, "the second range must not pay another transport attempt"); + client.Reads.ShouldBe(0, "the per-tag tag must not pay one either"); + await drv.ShutdownAsync(CancellationToken.None); + } + + /// Review round 2 m5 — a coalescer CONNECT failure fails the per-tag tags of the device without a second connect. + [Fact] + public async Task Coalescer_connect_failure_fails_per_tag_tags_without_reconnecting() + { + var factory = new FakeFocasClientFactory { Customise = () => new CountingClient { ThrowOnConnect = true } }; + var drv = OneDevice(factory, coalesce: true, "R100", "R101", "D900"); + await drv.InitializeAsync("{}", CancellationToken.None); + + var snaps = await drv.ReadAsync(["raw/R100", "raw/R101", "raw/D900"], CancellationToken.None); + + snaps.ShouldAllBe(s => s.StatusCode == FocasStatusMapper.BadCommunicationError); + factory.Clients.Count.ShouldBe(1, "one connect attempt per device per batch"); + factory.Clients.Sum(c => c.ConnectCount).ShouldBe(1); + await drv.ShutdownAsync(CancellationToken.None); + } + /// Results land at their own indices whatever order the devices finish in. [Fact] public async Task Results_keep_their_request_order_across_devices() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs index fae11d58..ff5aef11 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFixedTreeHealthTests.cs @@ -18,6 +18,7 @@ public sealed class FocasFixedTreeHealthTests { private const string Host = "focas://10.0.0.5:8193"; private static readonly TimeSpan Stale = TimeSpan.FromMilliseconds(300); + private static readonly TimeSpan BlipBudget = TimeSpan.FromMilliseconds(1500); /// A CNC that can be switched off (transport failure) or made to refuse the dynamic read. private sealed class CncSwitch @@ -58,7 +59,7 @@ public sealed class FocasFixedTreeHealthTests } } - private static FocasDriver FixedOnlyDriver(CncSwitch sw) => new(new FocasDriverOptions + private static FocasDriver FixedOnlyDriver(CncSwitch sw, TimeSpan? budget = null) => new(new FocasDriverOptions { Devices = [new FocasDeviceOptions(Host)], RawTags = [FocasRawTags.Entry(new FocasTagDefinition("raw/X", Host, "FIXED:Axes/X/AbsolutePosition", FocasDataType.Float64))], @@ -71,7 +72,7 @@ public sealed class FocasFixedTreeHealthTests }, }, "focas-fixed-health", new FakeFocasClientFactory { Customise = () => new SwitchableCnc(sw) }) { - FixedTreeStaleBudgetOverride = Stale, + FixedTreeStaleBudgetOverride = budget ?? Stale, }; /// @@ -83,7 +84,9 @@ public sealed class FocasFixedTreeHealthTests public async Task Fixed_only_driver_degrades_when_its_cnc_disappears_and_recovers_when_it_returns() { var sw = new CncSwitch(); - await using var drv = FixedOnlyDriver(sw); + // A wide budget (review round 2 m4): two 20 ms ticks always land well inside it, so the "blip does not + // degrade" absence check below runs unconditionally instead of being skipped on a slow agent. + await using var drv = FixedOnlyDriver(sw, BlipBudget); await drv.InitializeAsync("{}", CancellationToken.None); await WaitUntil(() => drv.GetDeviceState(Host)?.LastFixedPollUtc is not null); @@ -96,9 +99,9 @@ public sealed class FocasFixedTreeHealthTests var lastAnswer = drv.GetDeviceState(Host)!.LastFixedPollUtc!.Value; // Two failed ticks have demonstrably happened (positive evidence for the absence check below) … await WaitUntil(() => Volatile.Read(ref sw.Failures) >= 2); - // … and, if they landed inside the budget, the driver is still Healthy. - if (DateTime.UtcNow - lastAnswer < Stale) - drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a blip inside the staleness budget does not degrade"); + // … inside the budget (asserted, not assumed), and the driver is still Healthy. + (DateTime.UtcNow - lastAnswer).ShouldBeLessThan(BlipBudget, "the two failed ticks must land inside the budget"); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a blip inside the staleness budget does not degrade"); await WaitUntil(() => drv.GetHealth().State == DriverState.Degraded); var frozen = drv.GetHealth().LastSuccessfulRead; var failuresAtDegrade = Volatile.Read(ref sw.Failures); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasPollErrorHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasPollErrorHealthTests.cs index f738c275..3e89a3ff 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasPollErrorHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasPollErrorHealthTests.cs @@ -57,4 +57,32 @@ public sealed class FocasPollErrorHealthTests drv.GetHealth().State.ShouldBe(DriverState.Faulted); } + + /// + /// #740 review round 2 m3 — a poll failure carries the generation its batch STARTED under, so a batch that + /// began before a re-initialise cannot degrade the new generation. Positive control: a failure stamped with + /// the live generation (or unstamped) does degrade. + /// + [Fact] + public async Task PollReaderFailure_from_a_previous_generation_does_not_degrade_the_new_one() + { + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")], + Probe = new FocasProbeOptions { Enabled = false }, + }, "focas-1", new FakeFocasClientFactory()); + await drv.InitializeAsync("{}", CancellationToken.None); + var oldGeneration = drv.LifecycleGeneration; + await drv.InitializeAsync("{}", CancellationToken.None); + + var stale = new InvalidOperationException("old batch boom"); + stale.Data[FocasDriver.BatchGenerationKey] = oldGeneration; + drv.HandlePollError(stale); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + + var live = new InvalidOperationException("live batch boom"); + live.Data[FocasDriver.BatchGenerationKey] = drv.LifecycleGeneration; + drv.HandlePollError(live); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } }