From 20cb2a52d644322755d42b1726f533f9b803504e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:02:00 -0400 Subject: [PATCH 01/15] fix(abcip): build-and-swap device tables, volatile health, async-only dispose H13: _devices/_devicesByRoutingKey are published by reference swap under a lifecycle lock and never mutated after publish, so the retained poll engine, discovery and the actor's GetHostStatuses pull never read a table mid-Clear. Every retire (init's own release, shutdown, dispose) takes the table it replaces, so ShutdownAsync racing DisposeAsync tears each state down once, and a generation check makes an InitializeAsync overtaken by a shutdown publish nothing (it throws OperationCanceledException) instead of resurrecting probe loops behind the shutdown. Probe join is one bounded 5 s wait independent of the caller token. H21: _health is volatile and read-modify-write updates go through a CAS. F10: the sync Dispose() (sync-over-async) is removed; DisposeAsync is the complete, idempotent teardown the runtime calls after ShutdownAsync. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 260 +++++++++++------- .../AbCipLifecycleConcurrencyTests.cs | 172 ++++++++++++ 2 files changed, 333 insertions(+), 99 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 9f1d4fec..02c5f055 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -27,7 +27,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// handles GC-collected tags. /// public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, IBatchSubscribable, - IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, ISkippedTagReporter, IDisposable, IAsyncDisposable + IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, ISkippedTagReporter, IAsyncDisposable { /// Authored tags this driver could not build into its poll table. private readonly SkippedTagLedger _skippedTags = new(); @@ -98,12 +98,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, private readonly PollGroupEngine _poll; // Devices keyed by host address — the primary registry (GetDeviceState, discovery, probe, diagnostics). - private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); + // BUILD-AND-SWAP, never mutated after publish: the retained poll engine, the alarm projection, discovery and + // the actor's GetHostStatuses pull all read it while a re-init / shutdown runs. Each teardown takes the table + // it replaces, so concurrent teardowns (ShutdownAsync racing DisposeAsync) own disjoint device states. + private volatile Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); // Tag-routing index: a tag's device routing key (DeviceName ?? HostAddress, plus the HostAddress alias) // → its DeviceState. A tag resolves to its device by DeviceName-or-host so multi-device routing keys on // the RawPath's device segment. The HostAddress alias is deliberate (Client CLI + tests route by host); - // _options.Devices is already the merged per-device DeviceConfig. - private readonly Dictionary _devicesByRoutingKey = new(StringComparer.OrdinalIgnoreCase); + // _options.Devices is already the merged per-device DeviceConfig. Swapped with _devices, same rule. + private volatile Dictionary _devicesByRoutingKey = new(StringComparer.OrdinalIgnoreCase); + + // Serialises table publish/retire. _lifecycleGeneration is bumped by every retire (an init's own release, a + // shutdown, a dispose); an init publishes only if no retire happened after its own, so an abandoned init that a + // shutdown overtook can never resurrect devices or probe loops after that shutdown returned. + private readonly object _lifecycleLock = new(); + private int _lifecycleGeneration; // v3 authored RawPath → definition table (parents + fanned-out UDT members), keyed case-sensitive // ordinal. Built in InitializeAsync from _options.RawTags via the pure mapper. private volatile Dictionary _tagsByRawPath = new(StringComparer.Ordinal); @@ -133,7 +142,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// when a definition exists for . internal bool TryGetTagDefinition(string rawPath, out AbCipTagDefinition definition) => _tagsByRawPath.TryGetValue(rawPath, out definition!); - private DriverHealth _health = new(DriverState.Unknown, null, null); + // volatile: written from the read batch, the write path, the poll engine's error hook and the lifecycle, and + // read from the actor thread (GetHealth). Read-modify-write updates go through UpdateHealth (CAS) so a + // concurrent update is never lost. + private volatile DriverHealth _health = new(DriverState.Unknown, null, null); + + /// Atomically replaces with applied to its current value. + /// Computes the new health from the current one. + private void UpdateHealth(Func update) + { + while (true) + { + var current = _health; + var next = update(current); +#pragma warning disable CS0420 // a volatile field passed by ref to Interlocked keeps its full-fence semantics + if (ReferenceEquals(Interlocked.CompareExchange(ref _health, next, current), current)) return; +#pragma warning restore CS0420 + } + } /// Occurs when a subscribed tag's value changes. public event EventHandler? OnDataChange; @@ -270,9 +296,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, internal void HandlePollError(Exception ex) { _logger.LogWarning(ex, "AbCip poll reader failed. Driver={DriverInstanceId}", _driverInstanceId); - var current = _health; - if (current.State != DriverState.Faulted) - _health = new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, ex.Message); + UpdateHealth(current => current.State == DriverState.Faulted + ? current + : new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, ex.Message)); } /// @@ -364,48 +390,58 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { // InitializeAsync is reachable on a live driver with no teardown first (the IDriver contract - // allows it, and a reconnect retry can overlap a running attempt). Release the previous device states' probe loops + handles before - // the overwrite below. Deliberately NOT ShutdownAsync: that also drops _poll and the subscriptions. - await ReleaseDeviceStatesAsync(cancellationToken).ConfigureAwait(false); - _health = new DriverHealth(DriverState.Initializing, null, null); + // allows it, and a reconnect retry can overlap a running attempt). Retire the previous device states' probe + // loops + handles before building new ones. Deliberately NOT ShutdownAsync: that also drops _poll and the + // subscriptions. The generation this release produced is what the publish below is checked against. + var generation = await ReleaseDeviceStatesAsync().ConfigureAwait(false); + lock (_lifecycleLock) + { + if (generation == _lifecycleGeneration) + _health = new DriverHealth(DriverState.Initializing, null, null); + } _logger.LogInformation( "AbCip driver {DriverInstanceId} initializing: transport={Transport} tagFactory={TagFactory}", _driverInstanceId, _transportDescription, _tagFactory.GetType().Name); + Dictionary? builtDevices = null; try { + var options = _options; + var adoptedConfig = false; if (!string.IsNullOrWhiteSpace(driverConfigJson)) { var parsed = AbCipDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); if (parsed.Devices.Count > 0 || parsed.RawTags.Count > 0) { - _options = parsed; - _alarmProjection = new AbCipAlarmProjection(this, _options.AlarmPollInterval, _logger); + options = parsed; + adoptedConfig = true; } } - foreach (var device in _options.Devices) + builtDevices = new Dictionary(StringComparer.OrdinalIgnoreCase); + var builtRoutes = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var device in options.Devices) { var addr = AbCipHostAddress.TryParse(device.HostAddress) ?? throw new InvalidOperationException( $"AbCip device has invalid HostAddress '{device.HostAddress}' — expected 'ab://gateway[:port]/cip-path'."); var profile = AbCipPlcFamilyProfile.ForFamily(device.PlcFamily); var state = new DeviceState(addr, device, profile); - _devices[device.HostAddress] = state; + builtDevices[device.HostAddress] = state; // Route index: a tag reaches this device by matching its DeviceName routing key OR its // host address. Devices with no explicit DeviceName are identified purely by host today. - _devicesByRoutingKey[device.HostAddress] = state; + builtRoutes[device.HostAddress] = state; if (!string.IsNullOrEmpty(device.DeviceName)) - _devicesByRoutingKey[device.DeviceName] = state; + builtRoutes[device.DeviceName] = state; } // Build the RawPath → definition table from the authored raw tags. Each entry's TagConfig blob // is mapped by the pure factory; the entry's DeviceName (routing key) + WriteIdempotent flag are // threaded onto the def (they live on the RawTagEntry, not inside the blob). A mapper miss is // skipped (logged), never thrown — a bad tag must not fail the whole driver init. - _skippedTags.BeginRebuild(_options.RawTags.Count); + _skippedTags.BeginRebuild(options.RawTags.Count); var builtTagsByRawPath = new Dictionary(StringComparer.Ordinal); var builtDeclaredTags = new List(); - foreach (var entry in _options.RawTags) + foreach (var entry in options.RawTags) { if (!AbCipTagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var mapped)) { @@ -457,24 +493,43 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } } - // Publish the finished tables by reference swap — readers see the old or the new, never a half-built one. - _tagsByRawPath = builtTagsByRawPath; - _declaredTags = builtDeclaredTags; - _skippedTags.Commit(); - // Probe loops — one per device when enabled + a ProbeTagPath is configured. - if (_options.Probe.Enabled && !string.IsNullOrWhiteSpace(_options.Probe.ProbeTagPath)) + // Publish everything under the lifecycle lock, and only if no shutdown / other init retired the tables + // after this init's own release — readers see the old tables or the new ones, never a half-built one, + // and an overtaken init leaves nothing running behind the shutdown that overtook it. + lock (_lifecycleLock) { - foreach (var state in _devices.Values) + if (generation != _lifecycleGeneration) + throw new OperationCanceledException( + $"AbCip driver {_driverInstanceId} initialization was superseded by a shutdown or a newer initialization."); + + _options = options; + if (adoptedConfig) + _alarmProjection = new AbCipAlarmProjection(this, options.AlarmPollInterval, _logger); + _devices = builtDevices; + _devicesByRoutingKey = builtRoutes; + _tagsByRawPath = builtTagsByRawPath; + _declaredTags = builtDeclaredTags; + _skippedTags.Commit(); + + // Probe loops — one per device when enabled + a ProbeTagPath is configured. Started inside the lock so + // a retire that follows this publish always sees (and joins) them. + if (options.Probe.Enabled && !string.IsNullOrWhiteSpace(options.Probe.ProbeTagPath)) { - state.ProbeCts = new CancellationTokenSource(); - var ct = state.ProbeCts.Token; - // Keep the loop Task so ShutdownAsync can await its clean exit before - // disposing the CTS / handles the loop is still using. - state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); + foreach (var state in builtDevices.Values) + { + state.ProbeCts = new CancellationTokenSource(); + var ct = state.ProbeCts.Token; + // Keep the loop Task so a teardown can await its clean exit before + // disposing the CTS / handles the loop is still using. + state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); + } } + _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); } - else if (_options.Probe.Enabled && _devices.Count > 0) + builtDevices = null; // published: owned by the next retire now + + if (options.Probe.Enabled && string.IsNullOrWhiteSpace(options.Probe.ProbeTagPath) && options.Devices.Count > 0) { // Probe is Enabled but no ProbeTagPath is configured. Without a // tag path the loop has nothing to read, so HostState would stay Unknown forever @@ -486,51 +541,84 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "as Unknown until a ProbeTagPath is set or Probe.Enabled is set to false.", _driverInstanceId); } - _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); } catch (Exception ex) { - _health = new DriverHealth(DriverState.Faulted, null, ex.Message); + // Never-published states hold no probe loop and no runtime yet; dispose for symmetry. + if (builtDevices is not null) + foreach (var state in builtDevices.Values) state.DisposeHandles(); + if (ex is OperationCanceledException) + { + _logger.LogInformation("AbCip driver {DriverInstanceId} initialization abandoned: {Reason}", + _driverInstanceId, ex.Message); + throw; + } + lock (_lifecycleLock) + { + // A superseded init must not overwrite the health of whatever superseded it. + if (generation == _lifecycleGeneration) + _health = new DriverHealth(DriverState.Faulted, null, ex.Message); + } _logger.LogError(ex, "AbCip driver {DriverInstanceId} failed to initialize", _driverInstanceId); throw; } } + /// Bound on joining the retired probe loops — matches the fleet's "≤ 5 s join" teardown rule. + private static readonly TimeSpan ProbeJoinTimeout = TimeSpan.FromSeconds(5); + /// - /// Cancels, joins (bounded) and disposes every existing device state's probe loop + handles, then - /// clears the device/route tables. Device part of only. + /// Retires the published device states: swaps empty device/route/tag tables in under the lifecycle lock + /// (bumping the generation), then cancels, joins (bounded, independent of any caller token — a cancelled + /// caller must not dispose handles under a probe loop that is still running) and disposes the states it took. + /// Concurrent callers each take a disjoint table, so every state is torn down exactly once. The device part of + /// and the first step of . /// - private async Task ReleaseDeviceStatesAsync(CancellationToken cancellationToken) + /// The lifecycle generation this retire produced. + private async Task ReleaseDeviceStatesAsync() { - // The projection is replaced (not reused) when a config body is adopted; dispose the old one so its - // poll loop does not outlive it and raise duplicate alarm events through this driver. - await _alarmProjection.DisposeAsync().ConfigureAwait(false); - foreach (var state in _devices.Values) + Dictionary retired; + AbCipAlarmProjection projection; + int generation; + lock (_lifecycleLock) + { + generation = ++_lifecycleGeneration; + retired = _devices; + _devices = new Dictionary(StringComparer.OrdinalIgnoreCase); + _devicesByRoutingKey = new Dictionary(StringComparer.OrdinalIgnoreCase); + // Rebuilt wholesale by Initialize; swap, never Clear(): the retained poll engine / discovery may be enumerating. + _tagsByRawPath = new(StringComparer.Ordinal); + _declaredTags = new(); + projection = _alarmProjection; + } + + // The projection is replaced (not reused) when a config body is adopted; dispose it so its poll loop does + // not outlive it and raise duplicate alarm events through this driver. Idempotent, and reusable afterwards. + await projection.DisposeAsync().ConfigureAwait(false); + + foreach (var state in retired.Values) { try { state.ProbeCts?.Cancel(); } catch (ObjectDisposedException) { } } - foreach (var state in _devices.Values) + var loops = retired.Values.Select(s => s.ProbeTask).OfType().ToArray(); + if (loops.Length > 0) { - var probeTask = state.ProbeTask; - if (probeTask is null) continue; - // Own bounded timeout, independent of the caller token: cancelling init must not abort the join - // and dispose handles under a probe loop that is still running. - try { await probeTask.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); } - catch (TimeoutException) { } - catch (OperationCanceledException) { } + try { await Task.WhenAll(loops).WaitAsync(ProbeJoinTimeout).ConfigureAwait(false); } + catch (TimeoutException) + { + _logger.LogWarning("AbCip driver {DriverInstanceId}: a probe loop did not exit within {Timeout}; disposing anyway", + _driverInstanceId, ProbeJoinTimeout); + } + catch (Exception) { /* a loop never faults by design; nothing to add here */ } } - foreach (var state in _devices.Values) + foreach (var state in retired.Values) { state.ProbeCts?.Dispose(); state.ProbeCts = null; state.ProbeTask = null; state.DisposeHandles(); } - _devices.Clear(); - _devicesByRoutingKey.Clear(); - // Rebuilt wholesale by Initialize; stale entries would trip its member fan-out collision check. - _tagsByRawPath = new(StringComparer.Ordinal); // swap, never Clear(): the retained poll engine / discovery may be enumerating - _declaredTags = new(); + return generation; } /// @@ -541,46 +629,17 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// + /// + /// A complete teardown: the poll engine (and every subscription), the alarm projection, every probe loop + /// (joined ≤ 5 s) and every runtime handle. Safe to run concurrently with and with + /// an it overtakes (that init then publishes nothing). + /// public async Task ShutdownAsync(CancellationToken cancellationToken) { - await _alarmProjection.DisposeAsync().ConfigureAwait(false); await _poll.DisposeAsync().ConfigureAwait(false); - - // Phase 1: signal every probe loop to stop. - foreach (var state in _devices.Values) - { - try { state.ProbeCts?.Cancel(); } catch (ObjectDisposedException) { } - } - - // Phase 2: wait for each probe loop to observe cancellation and exit. The loop never - // throws on cancellation (it catches OperationCanceledException internally), but guard - // anyway so one slow device can't wedge the whole shutdown. - foreach (var state in _devices.Values) - { - var probeTask = state.ProbeTask; - if (probeTask is null) continue; - try - { - await probeTask.WaitAsync(TimeSpan.FromSeconds(10), cancellationToken).ConfigureAwait(false); - } - catch (TimeoutException) { } - catch (OperationCanceledException) { } - } - - // Phase 3: now the loops are gone, dispose the CTS + native handles with no live reader. - foreach (var state in _devices.Values) - { - state.ProbeCts?.Dispose(); - state.ProbeCts = null; - state.ProbeTask = null; - state.DisposeHandles(); - } - _devices.Clear(); - _devicesByRoutingKey.Clear(); - _tagsByRawPath = new(StringComparer.Ordinal); - _declaredTags = new(); + await ReleaseDeviceStatesAsync().ConfigureAwait(false); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + UpdateHealth(h => new DriverHealth(DriverState.Unknown, h.LastSuccessfulRead, null)); } // ---- ISubscribable (polling overlay via shared engine) ---- @@ -875,10 +934,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // exactly as the per-item writes did before. if (worst is not { } worstState) return; - _health = new DriverHealth( + UpdateHealth(current => new DriverHealth( worstState, - anySuccess ? now : _health.LastSuccessfulRead, - worstState == DriverState.Healthy ? null : worstError); + anySuccess ? now : current.LastSuccessfulRead, + worstState == DriverState.Healthy ? null : worstError)); } private async Task ReadSingleAsync( @@ -1896,10 +1955,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, internal DeviceState? GetDeviceState(string hostAddress) => _devices.TryGetValue(hostAddress, out var s) ? s : null; - /// Releases all resources used by the driver. - public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); - - /// Asynchronously releases all resources used by the driver. + /// + /// Asynchronously releases all resources used by the driver — the same complete teardown as + /// . Idempotent, and safe CONCURRENTLY with a still-running + /// (the runtime's final stop abandons a shutdown that overruns its budget and + /// disposes anyway): each teardown retires a disjoint device table. There is deliberately no synchronous + /// Dispose() — it could only block on this teardown (sync-over-async). + /// /// A value task representing the asynchronous disposal. public async ValueTask DisposeAsync() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs new file mode 100644 index 00000000..00cb921a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs @@ -0,0 +1,172 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Lifecycle overlap the runtime can produce (review 07 H13/F10, Lane A): the final stop runs +/// ShutdownAsync then DisposeAsync under one bounded budget, so a slow shutdown is abandoned and +/// the dispose runs CONCURRENTLY with it; an abandoned InitializeAsync can overlap the next attempt's +/// shutdown. Device tables are build-and-swapped, so each teardown owns a disjoint set of device states. +/// +[Trait("Category", "Unit")] +public sealed class AbCipLifecycleConcurrencyTests +{ + private const string Host = "ab://10.0.0.5/1,0"; + + /// A fake whose reads ignore cancellation for a while, so a teardown waits on the probe loop. + private sealed class SlowTag(AbCipTagCreateParams p, TimeSpan readDelay) : FakeAbCipTag(p) + { + private int _disposeCount; + + public int DisposeCount => Volatile.Read(ref _disposeCount); + + public override async Task ReadAsync(CancellationToken cancellationToken) + { + await base.ReadAsync(cancellationToken); + if (readDelay > TimeSpan.Zero) await Task.Delay(readDelay, CancellationToken.None); + } + + public override void Dispose() + { + Interlocked.Increment(ref _disposeCount); + base.Dispose(); + } + } + + private static (AbCipDriver Driver, FakeAbCipTagFactory Factory) Build(TimeSpan probeReadDelay) + { + var factory = new FakeAbCipTagFactory + { + Customise = p => new SlowTag(p, p.TagName == "ProbeTag" ? probeReadDelay : TimeSpan.Zero) { Value = 1 }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Speed", Host, "Motor1.Speed", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions + { + Enabled = true, + ProbeTagPath = "ProbeTag", + Interval = TimeSpan.FromMilliseconds(10), + Timeout = TimeSpan.FromMilliseconds(200), + }, + }, "abcip-lifecycle", factory); + return (drv, factory); + } + + private static async Task WaitUntilAsync(Func condition, string what) + { + var deadline = DateTime.UtcNow.AddSeconds(5); + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException(what); + await Task.Delay(5); + } + } + + /// + /// Shutdown and Dispose racing each other tear the device down exactly once: the data runtime is + /// disposed once (not once per teardown), neither call throws, and the probe loop has exited. + /// + [Fact] + public async Task Dispose_concurrent_with_a_running_Shutdown_tears_each_state_down_exactly_once() + { + var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(300)); + await drv.InitializeAsync("{}", CancellationToken.None); + (await drv.ReadAsync(["Speed"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); + var state = drv.GetDeviceState(Host)!; + var probeLoop = state.ProbeTask!; + var data = (SlowTag)factory.Tags["Motor1.Speed"]; + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + var dispose = drv.DisposeAsync().AsTask(); + await Task.WhenAll(shutdown, dispose); + + data.DisposeCount.ShouldBe(1); + probeLoop.IsCompleted.ShouldBeTrue(); + drv.GetDeviceState(Host).ShouldBeNull(); + } + + /// + /// An InitializeAsync that a ShutdownAsync overtakes publishes nothing: no device state and no probe + /// loop survive the shutdown. Positive control: an init that nothing overtakes still publishes. + /// + [Fact] + public async Task An_init_overtaken_by_Shutdown_publishes_no_device_and_starts_no_probe() + { + var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(300)); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); + var firstProbe = factory.Tags["ProbeTag"]; + + // The second init's own release waits on the slow first probe loop; the shutdown overtakes it there. + var init = drv.InitializeAsync("{}", CancellationToken.None); + await Task.Delay(50); + await drv.ShutdownAsync(CancellationToken.None); + await Should.ThrowAsync(() => init); + + drv.GetDeviceState(Host).ShouldBeNull(); + drv.GetHostStatuses().ShouldBeEmpty(); + await Task.Delay(100); + factory.Tags["ProbeTag"].ShouldBeSameAs(firstProbe); // no second probe loop ever created a runtime + drv.GetHealth().State.ShouldNotBe(DriverState.Healthy); + + // Positive control: a later init that nothing overtakes publishes the device again. + await drv.InitializeAsync("{}", CancellationToken.None); + drv.GetDeviceState(Host).ShouldNotBeNull(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.DisposeAsync(); + } + + /// + /// Readers of the device/routing tables (the retained poll engine, the actor's host-status pull) never + /// see a table mid-mutation while the driver re-initialises underneath them. + /// + [Fact] + public async Task Readers_survive_repeated_reinitialisation() + { + var (drv, _) = Build(probeReadDelay: TimeSpan.Zero); + await drv.InitializeAsync("{}", CancellationToken.None); + using var stop = new CancellationTokenSource(); + var reader = Task.Run(async () => + { + var reads = 0; + while (!stop.IsCancellationRequested) + { + _ = drv.GetHostStatuses(); + _ = drv.ResolveHost("Speed"); + await drv.ReadAsync(["Speed"], CancellationToken.None); + reads++; + } + return reads; + }); + + for (var i = 0; i < 50; i++) + await drv.InitializeAsync("{}", CancellationToken.None); + stop.Cancel(); + + (await reader).ShouldBeGreaterThan(0); // positive control: the reader actually ran + await drv.DisposeAsync(); + } + + /// F10: the driver exposes only an async dispose; a sync Dispose() that blocked on the async + /// teardown (sync-over-async) is gone, and the runtime takes the path. + [Fact] + public async Task Driver_is_async_disposable_only_and_DisposeAsync_stops_the_probe() + { + typeof(IDisposable).IsAssignableFrom(typeof(AbCipDriver)).ShouldBeFalse(); + var (drv, factory) = Build(probeReadDelay: TimeSpan.Zero); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); + var loop = drv.GetDeviceState(Host)!.ProbeTask!; + + await drv.DisposeAsync(); + await drv.DisposeAsync(); // idempotent + + loop.IsCompleted.ShouldBeTrue(); + factory.Tags["ProbeTag"].Disposed.ShouldBeTrue(); + } +} From f087f01d2bdfa1a04428926f5ea20a7292fb464e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:07:50 -0400 Subject: [PATCH 02/15] fix(abcip): classify every controller refusal as per-tag Bad, keep the runtime H1/L4: one classification (AbCipRefusals.IsDeviceRefusal) of the statuses that prove the controller answered - NotFound, OutOfBounds, NotAllowed, TooLarge, TooSmall, TypeMismatch, Unsupported - shared by the data path, the Test-Connect probe (IsReachableStatus widened) and the native runtime. - A refused read/write (thrown or as a status word) is per-tag Bad with a once-per-tag Warning, keeps the runtime (no evict, so no lease/Forward-Open churn for a sole lease holder), writes no Degraded and, since the device answered, advances LastSuccessfulRead without touching the state. - A refused cold create records the backoff as reachable, so siblings are no longer throttled for up to 30 s by one Arr[999]. - The native runtime completes InitializeAsync on any refusal (it used to on TypeMismatch/Unsupported only) and reports it on every later read. - A refused READ of NotAllowed reads BadNotReadable, not BadNotWritable. - Test Connect names the refusal ("probe tag refused: OutOfBounds"). 0x1C/0x20/0xFF-0x2104 still fold into BadReply inside Driver.Ab.Transport (Lane K follow-up); they remain comms faults until mapped there. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 93 +++++++-- .../AbCipDriverProbe.cs | 28 ++- .../AbCipRefusals.cs | 40 ++++ .../AbCipStatusMapper.cs | 11 ++ .../NativeAbCipTagRuntime.cs | 17 +- .../AbCipAlmaProjectionTests.cs | 9 +- .../AbCipDeviceRefusalTests.cs | 181 ++++++++++++++++++ .../AbCipTypeMismatchTests.cs | 23 ++- .../NativeAbCipTagRuntimeTests.cs | 31 ++- 9 files changed, 386 insertions(+), 47 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 02c5f055..3ddd7e82 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -266,6 +266,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, _logger.LogWarning(message, args); } + // Tags whose device-refusal Warning has already been logged, per operation ("read"/"write"/…): a refused tag is + // polled (or retried) forever, so the Warning is once per tag per operation per driver instance. + private readonly System.Collections.Concurrent.ConcurrentDictionary _refusalWarned = + new(StringComparer.OrdinalIgnoreCase); + + /// Logs a device-refusal Warning the first time is refused for . + private void WarnRefusalOnce(string tagName, string operation, string reference, string device, string reason) + { + if (_refusalWarned.TryAdd($"{operation}\0{tagName}", 0)) + _logger.LogWarning( + "AbCip {Operation} of {Tag} on device {Device} refused by the controller: {Reason}; reporting it Bad " + + "for this tag only — the device and its other tags are unaffected (logged once per tag)", + operation, reference, device, reason); + } + // Tags whose malformed-name Warning has already been logged — polled forever, warned once. private readonly System.Collections.Concurrent.ConcurrentDictionary _configErrorWarned = new(StringComparer.OrdinalIgnoreCase); @@ -884,11 +899,17 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// A null means "no health signal" — a config-level miss such as an /// unknown reference or unknown device, which never touched the wire and never touched health. /// - private readonly record struct ReadOutcome(DriverState? State, string? Error) + private readonly record struct ReadOutcome(DriverState? State, string? Error, bool Answered = false) { /// The item produced no health signal at all. public static ReadOutcome None => new(null, null); + /// + /// The controller answered with a deterministic refusal: no health STATE signal (a refusal is not a comms + /// fault), but proof the device is reachable, so LastSuccessfulRead advances. + /// + public static ReadOutcome Refused => new(null, null, Answered: true); + /// The item read successfully. public static ReadOutcome Success => new(DriverState.Healthy, null); @@ -917,10 +938,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, private void ApplyBatchHealth(ReadOnlySpan outcomes, DateTime now) { var anySuccess = false; + var anyAnswered = false; DriverState? worst = null; string? worstError = null; foreach (var outcome in outcomes) { + if (outcome.Answered) anyAnswered = true; if (outcome.State is not { } state) continue; if (state == DriverState.Healthy) anySuccess = true; if (worst is null || HealthRank(state) > HealthRank(worst.Value)) @@ -930,13 +953,19 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } - // Nothing reached the wire (every reference was unknown / unroutable) — leave health alone, - // exactly as the per-item writes did before. - if (worst is not { } worstState) return; + if (worst is not { } worstState) + { + // No state signal. If a controller answered (a refusal), that still proves it reachable: advance + // LastSuccessfulRead and leave the state alone. Otherwise nothing reached the wire (every reference was + // unknown / unroutable / misconfigured) — leave health alone entirely. + if (anyAnswered) + UpdateHealth(current => current with { LastSuccessfulRead = now }); + return; + } UpdateHealth(current => new DriverHealth( worstState, - anySuccess ? now : current.LastSuccessfulRead, + anySuccess || anyAnswered ? now : current.LastSuccessfulRead, worstState == DriverState.Healthy ? null : worstError)); } @@ -987,7 +1016,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip read of {Tag} on device {Device}: the controller's type does not match the authored " + "{DataType}; reporting BadTypeMismatch until the tag's dataType is corrected (logged once per tag)", reference, def.DeviceHostAddress, def.DataType); - return ReadOutcome.None; + return ReadOutcome.Refused; + } + if (AbCipRefusals.IsDeviceRefusal(status)) + { + // The controller answered with a deterministic refusal: per-tag Bad, keep the handle. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.MapReadStatus((AbStatus)status), null, now); + WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ((AbStatus)status).ToString()); + return ReadOutcome.Refused; } if (status != 0) { @@ -1022,7 +1058,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, WarnTypeMismatchOnce(def.Name, "AbCip read of {Tag} on device {Device} returned {Size} bytes but its authored type {DataType} is {Width} bytes wide; check the tag's dataType (logged once per tag)", reference, def.DeviceHostAddress, size, def.DataType, width); - return ReadOutcome.None; + return ReadOutcome.Refused; } // Review I-1 — an array tag (the EXPLICIT IsArray flag) decodes the whole buffer into an // element-typed CLR array (int[]/float[]/bool[]/string[]…), INCLUDING a 1-element array @@ -1063,7 +1099,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); WarnTypeMismatchOnce(def.Name, "AbCip read of {Tag} on device {Device} refused: {Reason} (logged once per tag)", reference, def.DeviceHostAddress, ate.Message); - return ReadOutcome.None; + return ReadOutcome.Refused; + } + catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate.Status)) + { + // H1/L4: the controller answered "no" (renamed tag, Arr[999], read-protected …) — a per-tag config + // error, not a comms fault. Keep the handle (no evict → no lease / Forward-Open churn), no Degraded, + // no backoff (EnsureTagRuntimeAsync already recorded the answer as reachable), warn once per tag. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.MapReadStatus(ate.Status), null, now); + WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ate.Message); + return ReadOutcome.Refused; } catch (Exception ex) { @@ -1119,7 +1164,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip whole-UDT read of {Parent} on device {Device}: the controller's type does not match " + "the authored type; {MemberCount} members report BadTypeMismatch (logged once per tag)", group.ParentName, parent.DeviceHostAddress, group.Members.Count); - return ReadOutcome.None; + return ReadOutcome.Refused; + } + if (AbCipRefusals.IsDeviceRefusal(status)) + { + StampGroupStatus(group, results, now, AbCipStatusMapper.MapReadStatus((AbStatus)status)); + WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, ((AbStatus)status).ToString()); + return ReadOutcome.Refused; } if (status != 0) { @@ -1167,7 +1218,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, WarnTypeMismatchOnce(parent.Name, "AbCip whole-UDT read of {Parent} on device {Device} refused: {Reason} (logged once per tag)", group.ParentName, parent.DeviceHostAddress, ate.Message); - return ReadOutcome.None; + return ReadOutcome.Refused; + } + catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate.Status)) + { + StampGroupStatus(group, results, now, AbCipStatusMapper.MapReadStatus(ate.Status)); + WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, ate.Message); + return ReadOutcome.Refused; } catch (Exception ex) { @@ -1298,6 +1355,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write to {Tag} on device {Device} refused: the controller's type does not match the authored {DataType} (logged once per tag)", w.FullReference, def.DeviceHostAddress, def.DataType); } + else if (AbCipRefusals.IsDeviceRefusal(status)) + { + // The controller refused this write (read-only, out of bounds …): keep the handle, no health. + results[i] = new WriteResult(AbCipStatusMapper.MapStatus(status)); + WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, ((AbStatus)status).ToString()); + } else if (status != 0) { EvictRuntime(device, def.Name); @@ -1378,6 +1441,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write to {Tag} on device {Device} refused: {Reason} (logged once per tag)", w.FullReference, def.DeviceHostAddress, ate.Message); } + catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate.Status)) + { + // H1: the controller answered "no" to this write — keep the handle, leave health alone, warn once. + results[i] = new WriteResult(AbCipStatusMapper.MapStatus(ate.Status)); + WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, ate.Message); + } catch (Exception ex) { // Transport / wire error — evict so the next write creates a fresh handle. @@ -1538,9 +1607,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // not answer. "Tag not found" means the controller replied over a working session; it // is a config error on this one tag, and counting it as a failed Forward-Open throttled // every sibling tag on the device (seen live: one typo blacked out the whole device). - if (ex is AbTransportException ate - && ate.Status != AbStatus.Ok - && AbCipDriverProbe.IsReachableStatus(ate.Status)) + if (AbCipRefusals.IsDeviceRefusal(ex)) device.Backoff.RecordSuccess(); else device.Backoff.RecordFailure(DateTime.UtcNow); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs index e9bedb8f..52bb0f46 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs @@ -18,10 +18,11 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// non-CIP server) returns Ok = false with a "handshake failed" message instead of /// a false-positive green tick. /// -/// Status-code discrimination: AbStatus.Ok and AbStatus.NotFound mean the controller -/// answered the CIP session (a non-existent probe tag name still proves CIP connectivity) and are -/// treated as "reachable - Ok = true". Every other status (timeout, bad connection, bad reply, -/// partial, abort, ...) is a transport or session failure: "handshake failed - Ok = false". +/// Status-code discrimination: AbStatus.Ok and every device refusal (not found, out of bounds, +/// not allowed, too large/small, type mismatch, unsupported) mean the controller answered the CIP session +/// (a non-existent probe tag name still proves CIP connectivity) and are treated as "reachable - Ok = true". +/// Every other status (timeout, bad connection, bad reply, partial, abort, ...) is a transport or session +/// failure: "handshake failed - Ok = false". /// /// public sealed class AbCipDriverProbe : IDriverProbe @@ -147,7 +148,7 @@ public sealed class AbCipDriverProbe : IDriverProbe { var msg = statusCode == AbStatus.Ok ? "CIP session OK" - : "CIP session OK (controller reachable; probe tag not found)"; + : RefusedProbeMessage(statusCode); return new(true, msg, sw.Elapsed); } @@ -164,7 +165,7 @@ public sealed class AbCipDriverProbe : IDriverProbe if (IsReachableException(ex)) { sw.Stop(); - return new(true, "CIP session OK (controller reachable; probe tag not found)", sw.Elapsed); + return new(true, RefusedProbeMessage(ex.Status), sw.Elapsed); } return new(false, $"Reachable at {host}:{port} but CIP handshake failed: {ex.Message}", null); @@ -180,13 +181,20 @@ public sealed class AbCipDriverProbe : IDriverProbe } /// - /// Returns true for transport statuses that indicate the CIP controller answered - /// but the probe tag name was not found or did not match — sufficient to confirm CIP - /// reachability. Returns true for Ok (tag was found). + /// Returns true for transport statuses that indicate the CIP controller answered — Ok, or any + /// device refusal (: not found, out of bounds, not + /// allowed, too large/small, type mismatch, unsupported) — sufficient to confirm CIP reachability. /// /// The transport status to classify. /// if the status confirms CIP reachability. - internal static bool IsReachableStatus(AbStatus s) => s is AbStatus.Ok or AbStatus.NotFound; + internal static bool IsReachableStatus(AbStatus s) => s == AbStatus.Ok || AbCipRefusals.IsDeviceRefusal(s); + + /// The Test-Connect message for a controller that answered but refused the probe tag. + /// The refusal. + /// The message. + private static string RefusedProbeMessage(AbStatus status) => status == AbStatus.NotFound + ? "CIP session OK (controller reachable; probe tag not found)" + : $"CIP session OK (controller reachable; probe tag refused: {status})"; /// /// Decides whether an is a "controller answered CIP but the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs new file mode 100644 index 00000000..fa82e31b --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs @@ -0,0 +1,40 @@ +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +/// +/// The ONE classification of a transport status into "the controller answered" vs "the wire failed", shared by +/// the data path, the probe loop, the Test-Connect probe and the native runtime's initialisation. +/// +/// +/// A device refusal is deterministic: the session works and the controller said no to this one tag (a renamed +/// tag, Arr[999], a read-protected tag, a type the authored config does not match). It is per-tag Bad, +/// keeps the tag runtime (evicting re-creates it — and, for a sole lease holder, re-Forward-Opens — on every poll), +/// never writes Degraded, never opens the device-wide connect backoff, and warns once per tag. Everything +/// else (timeout, bad connection, a reply that failed to parse, partial, abort, …) is a comms fault. +/// +/// The native transport currently folds CIP general statuses 0x1C, 0x20 and 0xFF/0x2104 into +/// (AbStatusCodes.FromCip, owned by Driver.Ab.Transport), which +/// this classification cannot tell from a garbled reply; until the transport maps them explicitly they stay +/// comms faults. +/// +/// +internal static class AbCipRefusals +{ + /// True when proves the controller answered but refused this tag. + /// The transport status. + /// for a deterministic device refusal. + public static bool IsDeviceRefusal(AbStatus status) => status is AbStatus.NotFound or AbStatus.OutOfBounds + or AbStatus.NotAllowed or AbStatus.TooLarge or AbStatus.TooSmall or AbStatus.TypeMismatch + or AbStatus.Unsupported; + + /// True when (an value) is a device refusal. + /// The runtime status word. + /// for a deterministic device refusal. + public static bool IsDeviceRefusal(int status) => IsDeviceRefusal((AbStatus)status); + + /// True when is an carrying a device refusal. + /// The exception. + /// for a deterministic device refusal. + public static bool IsDeviceRefusal(Exception ex) => ex is AbTransportException ate && IsDeviceRefusal(ate.Status); +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs index 18296c88..2988a685 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs @@ -15,6 +15,7 @@ public static class AbCipStatusMapper public const uint GoodMoreData = 0x00A60000u; public const uint BadInternalError = 0x80020000u; public const uint BadNodeIdUnknown = 0x80340000u; + public const uint BadNotReadable = 0x803A0000u; public const uint BadNotWritable = 0x803B0000u; public const uint BadOutOfRange = 0x803C0000u; public const uint BadNotSupported = 0x803D0000u; @@ -51,4 +52,14 @@ public static class AbCipStatusMapper /// The OPC UA status code. public static uint MapException(Exception ex) => AbOpcUaStatusMap.MapException(ex); + + /// + /// Status for a READ the controller refused: as , except that a + /// answer to a read is (the shared table's + /// BadNotWritable describes a refused write). + /// + /// The refusal status. + /// The OPC UA status code. + public static uint MapReadStatus(AbStatus status) => + status == AbStatus.NotAllowed ? BadNotReadable : MapStatus(status); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index af1ce7e6..8bf65cca 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -94,16 +94,15 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime /// The trailing bit selector split off the tag name (Tag.5), if any. internal int? ImplicitBit => _implicitBit; - // Performs one read to prove the tag exists: libplctag's InitializeAsync errors for an unknown - // tag, and the driver's backoff classification depends on create throwing AbStatus.NotFound. + // Performs one read to prove the session works and learn the tag's type. // // A name the encoder refuses throws AbCipTagConfigurationException before any lease: it is this // tag's configuration, not the device, and must not black out the device's other tags. A read the - // controller answers with a deterministic refusal (AbStatus.TypeMismatch / AbStatus.Unsupported) - // proves the tag exists, so it completes the initialization with that status rather than throwing: - // a throw here makes the driver dispose the runtime and re-create it - and, for a sole lease - // holder, re-Forward-Open - on every poll (mistype churn). The driver's own read reports the - // refusal. + // controller answers with a deterministic refusal (AbCipRefusals.IsDeviceRefusal: NotFound, OutOfBounds, + // NotAllowed, TooLarge/TooSmall, TypeMismatch, Unsupported) proves the session works, so it completes the + // initialization with that status rather than throwing: a throw here makes the driver dispose the runtime + // and re-create it - and, for a sole lease holder, re-Forward-Open - on every poll (a renamed tag or a + // mistype churned the lease). The driver's own read reports the refusal, per tag, on the kept runtime. /// public async Task InitializeAsync(CancellationToken cancellationToken) { @@ -137,9 +136,9 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime { await ReadAsync(cancellationToken).ConfigureAwait(false); } - catch (AbTransportException ex) when (ex.Status is AbStatus.TypeMismatch or AbStatus.Unsupported) + catch (AbTransportException ex) when (AbCipRefusals.IsDeviceRefusal(ex.Status)) { - // The controller answered — the tag exists. _status already holds the refusal. + // The controller answered. _status already holds the refusal. } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipAlmaProjectionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipAlmaProjectionTests.cs index c6012a88..d93616e8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipAlmaProjectionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipAlmaProjectionTests.cs @@ -311,8 +311,9 @@ public sealed class AbCipAlmaProjectionTests // A non-zero libplctag status ⇒ Bad on every member read; the driver evicts the handle and re-creates it, so // every re-created runtime must fail too (a fresh fake would read as "all clear"). var recreations = 0; - r.Factory.Customise = p => { Interlocked.Increment(ref recreations); return new FakeAbCipTag(p) { Status = (int)AbStatus.NotFound }; }; - foreach (var t in r.Factory.Tags.Values.ToList()) t.Status = (int)AbStatus.NotFound; + // A comms status (not a device refusal, which keeps the handle since review 07 H1) so the handle is evicted. + r.Factory.Customise = p => { Interlocked.Increment(ref recreations); return new FakeAbCipTag(p) { Status = (int)AbStatus.BadConnection }; }; + foreach (var t in r.Factory.Tags.Values.ToList()) t.Status = (int)AbStatus.BadConnection; await WaitFor(() => Volatile.Read(ref recreations) >= 6); await r.StopAsync(); Snapshot(r).Any(x => x.Kind == AlarmTransitionKind.Clear).ShouldBeFalse(); @@ -334,9 +335,9 @@ public sealed class AbCipAlmaProjectionTests { if (!p.TagName.EndsWith("HAcked", StringComparison.Ordinal)) return null!; Interlocked.Increment(ref bad); - return new FakeAbCipTag(p) { Status = (int)AbStatus.NotFound }; + return new FakeAbCipTag(p) { Status = (int)AbStatus.BadConnection }; // comms ⇒ evicted + re-created }; - r.Factory.Tags[$"{Tag}.HAcked"].Status = (int)AbStatus.NotFound; + r.Factory.Tags[$"{Tag}.HAcked"].Status = (int)AbStatus.BadConnection; await WaitFor(() => Volatile.Read(ref bad) >= 4); // The link recovers and HAcked reads true again. r.Factory.Customise = p => new FakeAbCipTag(p) { Value = true }; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs new file mode 100644 index 00000000..2cb690f5 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs @@ -0,0 +1,181 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 H1 / L4: a status that proves the controller ANSWERED (NotFound, OutOfBounds, NotAllowed, +/// TooLarge, TooSmall, TypeMismatch, Unsupported) is a deterministic per-tag refusal, not a comms fault. It +/// keeps the tag runtime (no evict → no lease / Forward-Open churn), writes no Degraded, opens no device-wide +/// backoff window for siblings, warns once, and — since the device answered — advances LastSuccessfulRead. +/// +[Trait("Category", "Unit")] +public sealed class AbCipDeviceRefusalTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + /// A fake whose reads/writes throw a chosen transport status (init succeeds unless told otherwise). + private sealed class RefusingTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public AbStatus? ReadThrows { get; set; } + public AbStatus? WriteThrows { get; set; } + + public override async Task ReadAsync(CancellationToken cancellationToken) + { + await base.ReadAsync(cancellationToken); + if (ReadThrows is { } s) throw new AbTransportException(s, s.ToString()); + } + + public override async Task WriteAsync(CancellationToken cancellationToken) + { + await base.WriteAsync(cancellationToken); + if (WriteThrows is { } s) throw new AbTransportException(s, s.ToString()); + } + } + + private static (AbCipDriver Driver, FakeAbCipTagFactory Factory) Build( + Func make) + { + var factory = new FakeAbCipTagFactory { Customise = make }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Bad", Device, "Arr[999]", AbCipDataType.DInt), + new AbCipTagDefinition("Good", Device, "Real1", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + MaxConcurrentReads = 1, + }, "abcip-refusal", factory); + return (drv, factory); + } + + /// The classification itself: every answered refusal is reachable; transport failures are not. + /// The transport status. + /// Whether it is a device refusal. + [Theory] + [InlineData(AbStatus.NotFound, true)] + [InlineData(AbStatus.OutOfBounds, true)] + [InlineData(AbStatus.NotAllowed, true)] + [InlineData(AbStatus.TooLarge, true)] + [InlineData(AbStatus.TooSmall, true)] + [InlineData(AbStatus.TypeMismatch, true)] + [InlineData(AbStatus.Unsupported, true)] + [InlineData(AbStatus.Ok, false)] + [InlineData(AbStatus.Timeout, false)] + [InlineData(AbStatus.BadConnection, false)] + [InlineData(AbStatus.BadReply, false)] + [InlineData(AbStatus.Partial, false)] + [InlineData(AbStatus.Abort, false)] + public void Answered_statuses_are_device_refusals(AbStatus status, bool refusal) + { + AbCipRefusals.IsDeviceRefusal(status).ShouldBe(refusal); + AbCipDriverProbe.IsReachableStatus(status).ShouldBe(refusal || status == AbStatus.Ok); + } + + /// A read the controller refuses keeps its runtime, stays Healthy, and advances LastSuccessfulRead. + /// The refusal the controller answers with. + /// The OPC UA status the tag reads. + [Theory] + [InlineData(AbStatus.NotFound, AbCipStatusMapper.BadNodeIdUnknown)] + [InlineData(AbStatus.OutOfBounds, AbCipStatusMapper.BadOutOfRange)] + [InlineData(AbStatus.TooLarge, AbCipStatusMapper.BadOutOfRange)] + [InlineData(AbStatus.TooSmall, AbCipStatusMapper.BadOutOfRange)] + [InlineData(AbStatus.NotAllowed, AbCipStatusMapper.BadNotReadable)] + public async Task A_refused_read_keeps_the_runtime_and_writes_no_Degraded(AbStatus status, uint expected) + { + var (drv, factory) = Build(p => new RefusingTag(p) { ReadThrows = p.TagName == "Arr[999]" ? status : null, Value = 1 }); + await drv.InitializeAsync("{}", CancellationToken.None); + var before = drv.GetHealth().LastSuccessfulRead!.Value; + await Task.Delay(20); + + for (var i = 0; i < 3; i++) + { + var r = await drv.ReadAsync(["Bad"], CancellationToken.None); + r[0].StatusCode.ShouldBe(expected); + } + + var bad = factory.Tags["Arr[999]"]; + bad.InitializeCount.ShouldBe(1); // never evicted → never re-created + bad.Disposed.ShouldBeFalse(); + bad.ReadCount.ShouldBe(3); + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before); // the device answered + } + + /// Positive control: a genuine transport failure on the same tag still evicts and degrades. + [Fact] + public async Task A_transport_failure_still_evicts_and_degrades() + { + var (drv, factory) = Build(p => new RefusingTag(p) { ReadThrows = p.TagName == "Arr[999]" ? AbStatus.Timeout : null }); + await drv.InitializeAsync("{}", CancellationToken.None); + + var r = await drv.ReadAsync(["Bad"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTimeout); + factory.Tags["Arr[999]"].Disposed.ShouldBeTrue(); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// A refusal returned as a status word (no throw) is classified the same way. + [Fact] + public async Task A_refusal_status_word_keeps_the_runtime() + { + var (drv, factory) = Build(p => new FakeAbCipTag(p) { Status = p.TagName == "Arr[999]" ? (int)AbStatus.OutOfBounds : 0, Value = 1 }); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.ReadAsync(["Bad"], CancellationToken.None); + var r = await drv.ReadAsync(["Bad", "Good"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); + factory.Tags["Arr[999]"].InitializeCount.ShouldBe(1); + factory.Tags["Arr[999]"].Disposed.ShouldBeFalse(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// A cold create the controller refuses with OutOfBounds proves the session works: it must not open the + /// device-wide backoff window, so a sibling cold tag still connects and reads Good. + /// + [Fact] + public async Task A_refused_create_does_not_throttle_sibling_cold_creates() + { + var (drv, _) = Build(p => p.TagName == "Arr[999]" + ? new FakeAbCipTag(p) { ThrowOnInitialize = true, Exception = new AbTransportException(AbStatus.OutOfBounds, "OutOfBounds") } + : new FakeAbCipTag(p) { Value = 42 }); + await drv.InitializeAsync("{}", CancellationToken.None); + + var r = await drv.ReadAsync(["Bad", "Good"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); + r[1].Value.ShouldBe(42); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A write the controller refuses keeps the runtime and writes no health. + [Fact] + public async Task A_refused_write_keeps_the_runtime_and_writes_no_Degraded() + { + var factory = new FakeAbCipTagFactory { Customise = p => new RefusingTag(p) { WriteThrows = AbStatus.NotAllowed } }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Ro", Device, "ReadOnlyTag", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-refusal", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var w1 = await drv.WriteAsync([new WriteRequest("Ro", 5)], CancellationToken.None); + var w2 = await drv.WriteAsync([new WriteRequest("Ro", 6)], CancellationToken.None); + + w1[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotWritable); + w2[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotWritable); + factory.Tags["ReadOnlyTag"].InitializeCount.ShouldBe(1); + factory.Tags["ReadOnlyTag"].Disposed.ShouldBeFalse(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs index 1bd4cbe5..65171947 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs @@ -73,7 +73,7 @@ public sealed class AbCipTypeMismatchTests creates().ShouldBe(1); factory.Tags["T"].Disposed.ShouldBeFalse(); factory.Tags["T"].ReadCount.ShouldBe(Polls); // positive control: every poll did reach the runtime - drv.GetHealth().ShouldBe(health); + AssertStateUnchanged(drv.GetHealth(), health); MismatchWarnings(log).ShouldBe(1); } @@ -104,7 +104,7 @@ public sealed class AbCipTypeMismatchTests Volatile.Read(ref creates).ShouldBe(1); factory.Tags["T"].Disposed.ShouldBeFalse(); factory.Tags["T"].ReadCount.ShouldBe(Polls); // positive control: every poll did reach the runtime - drv.GetHealth().ShouldBe(health); + AssertStateUnchanged(drv.GetHealth(), health); drv.GetHealth().State.ShouldNotBe(DriverState.Degraded); MismatchWarnings(log).ShouldBe(1); } @@ -121,7 +121,7 @@ public sealed class AbCipTypeMismatchTests creates().ShouldBe(1); factory.Tags["T"].Disposed.ShouldBeFalse(); - drv.GetHealth().ShouldBe(health); + AssertStateUnchanged(drv.GetHealth(), health); MismatchWarnings(log).ShouldBe(1); } @@ -179,7 +179,7 @@ public sealed class AbCipTypeMismatchTests target.ForwardOpens.ShouldBe(1); target.ForwardCloses.ShouldBe(0); target.ConnectedRequests.ShouldBeGreaterThanOrEqualTo(Polls); // positive control: polls hit the wire - drv.GetHealth().ShouldBe(health); + AssertStateUnchanged(drv.GetHealth(), health); MismatchWarnings(log).ShouldBe(1); } finally @@ -226,7 +226,7 @@ public sealed class AbCipTypeMismatchTests target.ForwardOpens.ShouldBe(1); target.ForwardCloses.ShouldBe(0); target.ConnectedRequests.ShouldBeGreaterThanOrEqualTo(Polls); // positive control: polls hit the wire - drv.GetHealth().ShouldBe(health); + AssertStateUnchanged(drv.GetHealth(), health); } finally { @@ -267,12 +267,23 @@ public sealed class AbCipTypeMismatchTests Volatile.Read(ref creates).ShouldBe(1); factory.Tags["T"].Disposed.ShouldBeFalse(); factory.Tags["T"].ReadCount.ShouldBe(Polls - 1); // positive control: later polls reached the kept runtime - drv.GetHealth().ShouldBe(health); + AssertStateUnchanged(drv.GetHealth(), health); } private sealed class CountingFactory(Func create) : IAbCipTagFactory { public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams); } + /// + /// A refusal leaves the health STATE and error alone; since the controller answered, LastSuccessfulRead may + /// only move forward (review 07 H1 — any data proving the device answered advances it). + /// + private static void AssertStateUnchanged(DriverHealth actual, DriverHealth before) + { + actual.State.ShouldBe(before.State); + actual.LastError.ShouldBe(before.LastError); + (actual.LastSuccessfulRead ?? DateTime.MinValue).ShouldBeGreaterThanOrEqualTo(before.LastSuccessfulRead ?? DateTime.MinValue); + } + #pragma warning restore OTOPCUA0001 } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 91a1a428..35f42bd3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -129,14 +129,35 @@ public sealed class NativeAbCipTagRuntimeTests rt.GetStatus().ShouldBe((int)AbStatus.Ok); } - [Fact] - public async Task Unknown_tag_fails_initialize_with_NotFound() + /// + /// Review 07 L4: a controller refusal on the existence read completes the initialization (the session works — + /// throwing made the driver dispose + re-create the runtime, re-leasing the connection, every poll); the + /// refusal stays readable and every later read reports it again. + /// + [Theory] + [InlineData("D2000400", AbStatus.NotFound)] + [InlineData("D2000500", AbStatus.NotFound)] + [InlineData("D2000A00", AbStatus.OutOfBounds)] + [InlineData("D2000F00", AbStatus.NotAllowed)] + [InlineData("D200FF010521", AbStatus.OutOfBounds)] + public async Task A_refused_tag_completes_initialize_and_reports_the_refusal_on_read(string reply, AbStatus expected) { - var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D2000400")) }; + var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString(reply)) }; + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Nope", AbCipDataType.DInt)); + await rt.InitializeAsync(CancellationToken.None); + rt.GetStatus().ShouldBe((int)expected); + (await Should.ThrowAsync(() => rt.ReadAsync(CancellationToken.None))) + .Status.ShouldBe(expected); + } + + /// Positive control: a reply the transport cannot classify as a refusal still fails initialize. + [Fact] + public async Task An_unclassified_reply_still_fails_initialize() + { + var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D2000100")) }; using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Nope", AbCipDataType.DInt)); (await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None))) - .Status.ShouldBe(AbStatus.NotFound); - rt.GetStatus().ShouldBe((int)AbStatus.NotFound); + .Status.ShouldBe(AbStatus.BadReply); } [Fact] From 22d4680e7be3eba1287ade6d163e3ffb0f9335b4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:17:03 -0400 Subject: [PATCH 03/15] fix(abcip): a refused probe tag proves the device reachable (no blackout) H10: the probe loop recorded EVERY exception - including the controller answering NotFound for a typo'd or download-deleted Probe.ProbeTagPath - as a Forward-Open failure. At a 5 s tick against a 30 s backoff cap ShouldAttempt was never true again, so every cold or evicted data tag on a reachable device read BadCommunicationError indefinitely and the host showed Stopped. The probe now decides the backoff verdict and the host state separately: - a device refusal (thrown or as a status word) is backoff success + host Running + one Warning per device naming the probe tag; an initialised probe runtime is kept so its lease is not re-opened every tick; - a malformed probe tag name (AbCipTagConfigurationException) never reached the device: no backoff signal, no host transition, one Warning; - a genuine transport failure is unchanged (backoff failure, host Stopped). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 70 ++++++++++++--- .../AbCipProbeRefusalTests.cs | 86 +++++++++++++++++++ 2 files changed, 145 insertions(+), 11 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProbeRefusalTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 3ddd7e82..c9c2f669 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -712,12 +712,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, IAbCipTagRuntime? probeRuntime = null; while (!ct.IsCancellationRequested) { - var success = false; - // The probe's backoff verdict for this tick: true = record success, false = record - // failure, null = no signal (a read that returned a non-zero status without throwing - // never recorded either, and still doesn't). Recorded AFTER the try/catch so the - // per-device connect gate is taken outside a catch block — see below. + // The probe's verdicts for this tick, decided SEPARATELY (review 07 H10): + // backoffVerdict — true = record success, false = record failure, null = no signal; + // hostState — the host transition, null = no evidence either way (leave it). + // Recorded AFTER the try/catch so the per-device connect gate is taken outside a catch block. bool? backoffVerdict = null; + HostState? hostState = HostState.Stopped; try { probeRuntime ??= _tagFactory.Create(probeParams); @@ -729,14 +729,51 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, state.ProbeInitialized = true; } await probeRuntime.ReadAsync(ct).ConfigureAwait(false); - success = probeRuntime.GetStatus() == 0; - if (success) + var status = probeRuntime.GetStatus(); + if (status == 0) + { backoffVerdict = true; // Device reachable — reset the data-path window + hostState = HostState.Running; + } + else if (AbCipRefusals.IsDeviceRefusal(status)) + { + // The controller answered, refusing the probe tag: reachable. + backoffVerdict = true; + hostState = HostState.Running; + WarnProbeRefusedOnce(state, ((AbStatus)status).ToString()); + } } catch (OperationCanceledException) when (ct.IsCancellationRequested) { break; } + catch (Exception ex) when (AbCipRefusals.IsDeviceRefusal(ex)) + { + // H10: a missing / typo'd / download-deleted probe tag. The controller ANSWERED over a working session, + // so this is a config error on the probe tag — not a Forward-Open failure. Recording a failure here + // every tick saturated the backoff (5 s ticks vs a 30 s cap ⇒ ShouldAttempt never true) and blacked out + // every cold / evicted data tag on a reachable device, indefinitely. + backoffVerdict = true; + hostState = HostState.Running; + WarnProbeRefusedOnce(state, ex.Message); + // Keep an initialised runtime (its lease stays up — no Forward Open/Close per tick); one whose + // initialisation threw is re-created next tick, as before. + if (!state.ProbeInitialized) + { + try { probeRuntime?.Dispose(); } catch { } + probeRuntime = null; + } + } + catch (AbCipTagConfigurationException ace) + { + // The probe tag's own name cannot become a request — it never reached the device, so it is no evidence + // either way: no backoff signal, no host transition, one Warning. + try { probeRuntime?.Dispose(); } catch { } + probeRuntime = null; + state.ProbeInitialized = false; + hostState = null; + WarnProbeRefusedOnce(state, ace.Message); + } catch (Exception ex) { // Wire / init error — tear down the probe runtime so the next tick re-creates it. @@ -748,9 +785,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, try { probeRuntime?.Dispose(); } catch { } probeRuntime = null; state.ProbeInitialized = false; - // Keep the data-path window open — unless the probe tag's own name is malformed - // — it never reached the device, so it is no evidence either way. - backoffVerdict = ex is AbCipTagConfigurationException ? null : false; + backoffVerdict = false; // the device did not answer — keep the data-path window open } // ConnectionBackoff is caller-serialized, and the data path now records under the @@ -780,7 +815,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } - TransitionDeviceState(state, success ? HostState.Running : HostState.Stopped); + if (hostState is { } host) TransitionDeviceState(state, host); try { await Task.Delay(_options.Probe.Interval, ct).ConfigureAwait(false); } catch (OperationCanceledException) { break; } @@ -789,6 +824,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, try { probeRuntime?.Dispose(); } catch { } } + /// Logs, once per device state, that the controller answered but refused the configured probe tag. + private void WarnProbeRefusedOnce(DeviceState state, string reason) + { + if (Interlocked.Exchange(ref state.ProbeRefusalWarned, 1) != 0) return; + _logger.LogWarning( + "AbCip probe tag {ProbeTag} on device {Device} cannot be read: {Reason}. The controller answered, so the device " + + "is treated as reachable; fix Probe.ProbeTagPath for driver {DriverInstanceId} (logged once per device)", + _options.Probe.ProbeTagPath, state.Options.HostAddress, reason, _driverInstanceId); + } + private void TransitionDeviceState(DeviceState state, HostState newState) { HostState old; @@ -2096,6 +2141,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// Gets or sets whether the probe has been initialized. public bool ProbeInitialized { get; set; } + /// 1 once the probe-tag refusal Warning has been logged for this device state. + public int ProbeRefusalWarned; + /// /// The fire-and-forget probe loop's . Stored so /// can await the loop's clean exit after diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProbeRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProbeRefusalTests.cs new file mode 100644 index 00000000..c0693b8b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipProbeRefusalTests.cs @@ -0,0 +1,86 @@ +using Microsoft.Extensions.Logging; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using CapturingLogger = ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.AbCipLoggingTests.CapturingLogger; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 H10: a typo'd (or download-deleted) Probe.ProbeTagPath used to record a Forward-Open +/// FAILURE every probe tick, so the device's backoff window never closed and every cold / evicted data tag read +/// BadCommunicationError forever — a permanent device blackout that a restart did not cure. The controller +/// ANSWERED, so the probe now counts it as reachable: backoff success, host Running, one config Warning. +/// +[Trait("Category", "Unit")] +public sealed class AbCipProbeRefusalTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private static (AbCipDriver Driver, CapturingLogger Logger) Build(Exception probeFault) + { + var logger = new CapturingLogger(); + var factory = new FakeAbCipTagFactory + { + Customise = p => p.TagName == "ProbeTagTypo" + ? new FakeAbCipTag(p) { ThrowOnInitialize = true, Exception = probeFault } + : new FakeAbCipTag(p) { Value = 7 }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Speed", Device, "Motor1.Speed", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions + { + Enabled = true, + ProbeTagPath = "ProbeTagTypo", + Interval = TimeSpan.FromMilliseconds(10), + Timeout = TimeSpan.FromMilliseconds(100), + }, + }, "abcip-probe-refusal", factory, logger: logger); + return (drv, logger); + } + + private static async Task WaitUntilAsync(Func condition) + { + var deadline = DateTime.UtcNow.AddSeconds(5); + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("condition never met"); + await Task.Delay(5); + } + } + + /// A NotFound probe tag leaves the device reachable: data tags still connect, host is Running, warned once. + [Fact] + public async Task A_missing_probe_tag_does_not_black_out_the_device() + { + var (drv, logger) = Build(new AbTransportException(AbStatus.NotFound, "NotFound")); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntilAsync(() => drv.GetHostStatuses().Single().State == HostState.Running); + await Task.Delay(100); // many probe ticks + + var r = await drv.ReadAsync(["Speed"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); // a cold create, NOT throttled + r[0].Value.ShouldBe(7); + drv.GetHostStatuses().Single().State.ShouldBe(HostState.Running); + logger.Entries.Count(e => e.Level == LogLevel.Warning && e.Message.Contains("ProbeTagTypo")).ShouldBe(1); + await drv.DisposeAsync(); + } + + /// Positive control: a probe that genuinely cannot connect still stops the host and throttles cold creates. + [Fact] + public async Task An_unreachable_device_still_stops_the_host_and_throttles() + { + var (drv, _) = Build(new AbTransportException(AbStatus.Timeout, "Timeout")); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntilAsync(() => drv.GetHostStatuses().Single().State == HostState.Stopped); + + var r = await drv.ReadAsync(["Speed"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError); // throttled by the open window + await drv.DisposeAsync(); + } +} From 48b0173ae95d5a0094cd1fedc79d3088ff244fa8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:17:16 -0400 Subject: [PATCH 04/15] fix(abcip): a reader racing a re-init neither leaks a runtime nor degrades Found by the H13 stress test (200 re-inits under a concurrent reader): a read that resolved the device just before the table swap could cold-create a runtime into the retired state after its teardown had disposed it (leaked native handle / lease), and could hit a per-tag SemaphoreSlim the teardown had disposed, which the generic catch reported as a comms fault (Degraded). - DeviceState.Retire marks the state retired before disposing handles; each handle is removed-then-disposed exactly once. A creator re-checks the flag after its TryAdd (fenced, Dekker-paired with Retire) and disposes the orphan. - A failure on a retired device is the teardown's doing: BadCommunicationError for that one call, no evict, no health signal. - Per-tag/RMW gates are no longer disposed (async-only waits hold no handle), matching the ConnectGate rationale. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 105 ++++++++++++++---- .../AbCipLifecycleConcurrencyTests.cs | 31 +++++- 2 files changed, 111 insertions(+), 25 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index c9c2f669..e4b6f9bb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -561,7 +561,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { // Never-published states hold no probe loop and no runtime yet; dispose for symmetry. if (builtDevices is not null) - foreach (var state in builtDevices.Values) state.DisposeHandles(); + foreach (var state in builtDevices.Values) state.Retire(); if (ex is OperationCanceledException) { _logger.LogInformation("AbCip driver {DriverInstanceId} initialization abandoned: {Reason}", @@ -631,7 +631,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, state.ProbeCts?.Dispose(); state.ProbeCts = null; state.ProbeTask = null; - state.DisposeHandles(); + state.Retire(); } return generation; } @@ -1155,6 +1155,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ate.Message); return ReadOutcome.Refused; } + catch (Exception) when (device.Retired) + { + // A re-init / shutdown retired this device under the read: its handles were disposed by the teardown, not + // lost on the wire — no evict (the teardown owns them), no health signal. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadCommunicationError, null, now); + return ReadOutcome.None; + } catch (Exception ex) { // Transport exception — evict so the next read creates a fresh handle. An AbTransportException @@ -1271,6 +1278,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, ate.Message); return ReadOutcome.Refused; } + catch (Exception) when (device.Retired) + { + StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError); + return ReadOutcome.None; + } catch (Exception ex) { EvictRuntime(device, parent.Name); @@ -1492,6 +1504,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[i] = new WriteResult(AbCipStatusMapper.MapStatus(ate.Status)); WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, ate.Message); } + catch (Exception) when (device.Retired) + { + // The device was retired under this write by a re-init / shutdown — not a comms fault. + results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); + } catch (Exception ex) { // Transport / wire error — evict so the next write creates a fresh handle. @@ -1554,6 +1571,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, DeviceState device, string parentTagName, CancellationToken ct) { if (device.ParentRuntimes.TryGetValue(parentTagName, out var existing)) return existing; + ThrowIfRetired(device); var runtime = _tagFactory.Create(device.BuildCreateParams(parentTagName, _options.Timeout)); try @@ -1567,10 +1585,25 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } // Two concurrent callers can both miss the cache + both initialize a runtime; only the // first TryAdd wins. Dispose the loser so it doesn't leak a native tag handle. - if (device.ParentRuntimes.TryAdd(parentTagName, runtime)) - return runtime; - runtime.Dispose(); - return device.ParentRuntimes[parentTagName]; + if (!device.ParentRuntimes.TryAdd(parentTagName, runtime)) + { + runtime.Dispose(); + return device.ParentRuntimes[parentTagName]; + } + Interlocked.MemoryBarrier(); // pairs with DeviceState.Retire + if (device.Retired) + { + if (device.ParentRuntimes.TryRemove(parentTagName, out var orphan)) orphan.Dispose(); + ThrowIfRetired(device); + } + return runtime; + } + + /// Throws when a teardown has retired (a reader raced a re-init / shutdown). + private static void ThrowIfRetired(DeviceState device) + { + if (device.Retired) + throw new ObjectDisposedException(nameof(DeviceState), $"AbCip device {device.Options.HostAddress} was retired by a re-initialization or shutdown."); } /// @@ -1607,6 +1640,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Re-check under the gate: a sibling cold tag may have connected (or a concurrent write // may have materialised this very handle) while this caller waited. if (device.Runtimes.TryGetValue(def.Name, out var raced)) return raced; + ThrowIfRetired(device); // Inside an open backoff window a data-path caller fails fast — no runtime // create, no Forward-Open. The malformed-path config error above still surfaces (a permanent @@ -1662,10 +1696,20 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // The gate makes a cold create single-flight, but a concurrent EvictRuntime (a failed read // on this handle) can still land between the re-check and the add; only the first TryAdd // wins. Dispose the loser so it doesn't leak a native tag handle. - if (device.Runtimes.TryAdd(def.Name, runtime)) - return runtime; - runtime.Dispose(); - return device.Runtimes[def.Name]; + if (!device.Runtimes.TryAdd(def.Name, runtime)) + { + runtime.Dispose(); + return device.Runtimes[def.Name]; + } + // A teardown that retired this state while we created would never see this handle: take it back out + // (TryRemove — exactly one of us disposes it) rather than leak it into a dead table. + Interlocked.MemoryBarrier(); // pairs with DeviceState.Retire + if (device.Retired) + { + if (device.Runtimes.TryRemove(def.Name, out var orphan)) orphan.Dispose(); + ThrowIfRetired(device); + } + return runtime; } finally { @@ -2230,19 +2274,38 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, AuthoredType: authoredType, UseConnectedMessaging: Profile.SupportsConnectedMessaging); - /// Disposes all runtime tag handles and clears the caches. + private volatile bool _retired; + + /// + /// True once a teardown retired this state. A reader that resolved the device just before the swap can + /// still be running against it: its failures are the teardown's doing, not the device's (no Degraded), and a + /// runtime it cold-creates afterwards is disposed by the creator instead of leaking into a dead table. + /// + public bool Retired => _retired; + + /// Marks the state retired, then disposes every runtime handle (see ). + public void Retire() + { + _retired = true; + // Dekker pairing with the creator's post-add check: the flag is globally visible before the tables are + // enumerated, and the creator fences between its add and its read of the flag — so at least one of the two + // sees the other's write and the handle is disposed. + Interlocked.MemoryBarrier(); + DisposeHandles(); + } + + /// + /// Disposes every runtime tag handle, each exactly once (removed before it is disposed, so a racing + /// creator's own cleanup cannot dispose it a second time). The per-runtime / RMW gates are deliberately NOT + /// disposed, for the same reason as : an in-flight reader of a retired state may + /// still release one, and a only awaited asynchronously holds no wait handle. + /// public void DisposeHandles() { - foreach (var r in Runtimes.Values) r.Dispose(); - Runtimes.Clear(); - foreach (var r in ParentRuntimes.Values) r.Dispose(); - ParentRuntimes.Clear(); - // Dispose + clear the per-runtime/RMW gates so ReinitializeAsync cycles don't orphan - // their SemaphoreSlim instances (each leaks a wait handle once contended). - foreach (var sem in _runtimeLocks.Values) sem.Dispose(); - _runtimeLocks.Clear(); - foreach (var sem in _rmwLocks.Values) sem.Dispose(); - _rmwLocks.Clear(); + foreach (var key in Runtimes.Keys) + if (Runtimes.TryRemove(key, out var r)) r.Dispose(); + foreach (var key in ParentRuntimes.Keys) + if (ParentRuntimes.TryRemove(key, out var r)) r.Dispose(); } } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs index 00cb921a..eea409d2 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs @@ -123,14 +123,32 @@ public sealed class AbCipLifecycleConcurrencyTests /// /// Readers of the device/routing tables (the retained poll engine, the actor's host-status pull) never - /// see a table mid-mutation while the driver re-initialises underneath them. + /// see a table mid-mutation while the driver re-initialises underneath them, never trip over a per-tag lock a + /// retire disposed, and never leak a runtime they cold-created into a device state a retire already tore down. /// [Fact] - public async Task Readers_survive_repeated_reinitialisation() + public async Task Readers_survive_repeated_reinitialisation_and_leak_no_runtime() { - var (drv, _) = Build(probeReadDelay: TimeSpan.Zero); + var created = new System.Collections.Concurrent.ConcurrentBag(); + var factory = new FakeAbCipTagFactory + { + Customise = p => + { + var t = new FakeAbCipTag(p) { Value = 1 }; + created.Add(t); + return t; + }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Speed", Host, "Motor1.Speed", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-lifecycle", factory); await drv.InitializeAsync("{}", CancellationToken.None); using var stop = new CancellationTokenSource(); + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var badHealth = 0; var reader = Task.Run(async () => { var reads = 0; @@ -139,17 +157,22 @@ public sealed class AbCipLifecycleConcurrencyTests _ = drv.GetHostStatuses(); _ = drv.ResolveHost("Speed"); await drv.ReadAsync(["Speed"], CancellationToken.None); + if (drv.GetHealth().State == DriverState.Degraded) Interlocked.Increment(ref badHealth); reads++; + started.TrySetResult(); } return reads; }); + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); - for (var i = 0; i < 50; i++) + for (var i = 0; i < 200; i++) await drv.InitializeAsync("{}", CancellationToken.None); stop.Cancel(); (await reader).ShouldBeGreaterThan(0); // positive control: the reader actually ran + Volatile.Read(ref badHealth).ShouldBe(0, "a re-init is not a comms fault"); await drv.DisposeAsync(); + created.Where(t => !t.Disposed).ShouldBeEmpty("every runtime a reader created must be disposed by some teardown"); } /// F10: the driver exposes only an async dispose; a sync Dispose() that blocked on the async From 4e9bd9d9665f10866063c1df24cc1f14132dddad Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:19:38 -0400 Subject: [PATCH 05/15] perf(abcip): native read window >= 64; cold waiters hold no window slot P8: on the native transport the bounded-parallel read window is also the cap on services per Multiple Service Packet (the device's shared scheduler packs whatever is queued, within the connection's request/reply byte budget), so the default 8 cost a 1000-tag controller ~125 round trips per poll instead of ~10. The effective window (AbCipDriver.ReadWindow) is now max(64, option) when the native tag factory serves reads; libplctag and injected factories keep the option exactly. Option doc + AdminUI help text updated (Razor: Wave 4 live check). P7 addendum: a work item now takes its window slot only after its runtime exists. A cold create queues on its device's single-flight connect gate, and holding a slot there let one dead device's backoff-expiry attempt occupy the whole window for a full Timeout, stalling every other device's reads. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriverOptions.cs | 6 + .../AbCipDriver.cs | 271 ++++++++++-------- .../Drivers/Forms/AbCipDriverForm.razor | 2 +- .../AbCipReadWindowTests.cs | 96 +++++++ 4 files changed, 254 insertions(+), 121 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWindowTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs index 9fc101c3..9656a70e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs @@ -117,6 +117,12 @@ public sealed class AbCipDriverOptions /// device round trips (the whole point: a strictly sequential batch pays one full round /// trip per tag); lowering it to 1 restores strictly sequential reads. Values below /// 1 are clamped to 1 at the use site. + /// + /// On the transport the effective window is at least 64: there the + /// window caps how many services one Multiple Service Packet coalesces (the device's shared scheduler packs + /// what is queued, bounded by the connection's request/reply byte budget), so a small value only multiplies + /// round trips. A value above 64 is honoured; a value below it is raised. + /// /// public int MaxConcurrentReads { get; init; } = 8; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index e4b6f9bb..b48a619c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -895,7 +895,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // round trips overlap, which a strictly sequential await chain made impossible. Results are // index-addressed, so completion order is irrelevant. Per-item health verdicts are COLLECTED // and written once after the batch: an inline write to the shared _health field would race. - var maxConcurrent = Math.Max(1, _options.MaxConcurrentReads); + var maxConcurrent = ReadWindow; var outcomes = new ReadOutcome[itemCount]; using var window = new SemaphoreSlim(maxConcurrent, maxConcurrent); var tasks = new Task[itemCount]; @@ -910,32 +910,44 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, ApplyBatchHealth(outcomes, now); return results; + // The window slot is taken INSIDE each item, after its runtime exists (P7): a cold create queues on its + // device's single-flight connect gate, and holding a slot while queued there let one dead device's + // backoff-expiry attempt occupy the whole window for a full Timeout, stalling every other device's reads. async Task RunItemAsync(int slot) { - // Acquire OUTSIDE the try: a wait that throws never acquired, so it must not release. - await window.WaitAsync(cancellationToken).ConfigureAwait(false); - try + if (slot < groups.Count) { - if (slot < groups.Count) - { - outcomes[slot] = await ReadGroupAsync(groups[slot], results, now, cancellationToken) - .ConfigureAwait(false); - } - else - { - var fb = fallbacks[slot - groups.Count]; - outcomes[slot] = await ReadSingleAsync( - fb, fullReferences[fb.OriginalIndex], results, now, cancellationToken) - .ConfigureAwait(false); - } + outcomes[slot] = await ReadGroupAsync(groups[slot], results, now, window, cancellationToken) + .ConfigureAwait(false); } - finally + else { - window.Release(); + var fb = fallbacks[slot - groups.Count]; + outcomes[slot] = await ReadSingleAsync( + fb, fullReferences[fb.OriginalIndex], results, now, window, cancellationToken) + .ConfigureAwait(false); } } } + /// + /// Floor on the read window when the native transport serves this driver (P8). Its runtimes share one + /// request scheduler per device that packs whatever is queued when the pump wakes into one Multiple Service + /// Packet — bounded by the connection's request AND reply byte budget, so a wide window cannot over-pack. The + /// window is therefore the cap on services per round trip: at 8, a 1000-tag controller paid ~125 round trips + /// per poll instead of ~10. + /// + internal const int NativeMinReadWindow = 64; + + /// + /// The effective bounded-parallel read window: (clamped + /// to ≥ 1), raised to at least when the native tag factory serves the reads. + /// libplctag overlaps per-handle round trips only, so it (and any injected factory) keeps the option as is. + /// + internal int ReadWindow => _tagFactory is NativeAbCipTagFactory + ? Math.Max(NativeMinReadWindow, _options.MaxConcurrentReads) + : Math.Max(1, _options.MaxConcurrentReads); + /// /// One read work item's health verdict. Returned by / /// instead of written to the shared health field inline, because @@ -1015,7 +1027,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } private async Task ReadSingleAsync( - AbCipUdtReadFallback fb, string reference, DataValueSnapshot[] results, DateTime now, CancellationToken ct) + AbCipUdtReadFallback fb, string reference, DataValueSnapshot[] results, DateTime now, + SemaphoreSlim window, CancellationToken ct) { if (!_resolver.TryResolve(reference, out var def)) { @@ -1041,82 +1054,91 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { var runtime = await EnsureTagRuntimeAsync(device, def, ct).ConfigureAwait(false); + // Window slot only now that the runtime exists (see ReadAsync); a wait that throws never acquired. + await window.WaitAsync(ct).ConfigureAwait(false); // Serialise Read → GetStatus → Decode against the shared runtime — the cached libplctag // Tag handle is not concurrency-safe and GetStatus returns the LAST op's status. var opLock = device.GetRuntimeLock(def.Name); - await opLock.WaitAsync(ct).ConfigureAwait(false); try { - await runtime.ReadAsync(ct).ConfigureAwait(false); + await opLock.WaitAsync(ct).ConfigureAwait(false); + try + { + await runtime.ReadAsync(ct).ConfigureAwait(false); - var status = runtime.GetStatus(); - if (status == (int)AbStatus.TypeMismatch) - { - // Final review I4: the controller ANSWERED with a type that disagrees with the authored - // one — a deterministic config error, not a comms fault. Keep the handle (evicting would - // re-create it — and, for a sole lease holder, re-Forward-Open — on every poll), leave - // health alone, warn once per tag. - results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); - WarnTypeMismatchOnce(def.Name, - "AbCip read of {Tag} on device {Device}: the controller's type does not match the authored " + - "{DataType}; reporting BadTypeMismatch until the tag's dataType is corrected (logged once per tag)", - reference, def.DeviceHostAddress, def.DataType); - return ReadOutcome.Refused; - } - if (AbCipRefusals.IsDeviceRefusal(status)) - { - // The controller answered with a deterministic refusal: per-tag Bad, keep the handle. - results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.MapReadStatus((AbStatus)status), null, now); - WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ((AbStatus)status).ToString()); - return ReadOutcome.Refused; - } - if (status != 0) - { - // Evict the stale handle so the next call re-creates it. - // A non-zero status can mean the controller dropped the connection or the tag - // handle became permanently invalid (e.g. after a PLC download). Evicting - // mirrors the probe loop's recreate-on-failure behaviour. - EvictRuntime(device, def.Name); - results[fb.OriginalIndex] = new DataValueSnapshot(null, - AbCipStatusMapper.MapStatus(status), null, now); - _logger.LogWarning( - "AbCip read returned non-zero transport status {Status} for tag {Tag} on device {Device}; " + - "evicting cached runtime so next call re-creates it", - status, reference, def.DeviceHostAddress); - return ReadOutcome.Degraded($"transport status {status} reading {reference}"); - } + var status = runtime.GetStatus(); + if (status == (int)AbStatus.TypeMismatch) + { + // Final review I4: the controller ANSWERED with a type that disagrees with the authored + // one — a deterministic config error, not a comms fault. Keep the handle (evicting would + // re-create it — and, for a sole lease holder, re-Forward-Open — on every poll), leave + // health alone, warn once per tag. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); + WarnTypeMismatchOnce(def.Name, + "AbCip read of {Tag} on device {Device}: the controller's type does not match the authored " + + "{DataType}; reporting BadTypeMismatch until the tag's dataType is corrected (logged once per tag)", + reference, def.DeviceHostAddress, def.DataType); + return ReadOutcome.Refused; + } + if (AbCipRefusals.IsDeviceRefusal(status)) + { + // The controller answered with a deterministic refusal: per-tag Bad, keep the handle. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.MapReadStatus((AbStatus)status), null, now); + WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ((AbStatus)status).ToString()); + return ReadOutcome.Refused; + } + if (status != 0) + { + // Evict the stale handle so the next call re-creates it. + // A non-zero status can mean the controller dropped the connection or the tag + // handle became permanently invalid (e.g. after a PLC download). Evicting + // mirrors the probe loop's recreate-on-failure behaviour. + EvictRuntime(device, def.Name); + results[fb.OriginalIndex] = new DataValueSnapshot(null, + AbCipStatusMapper.MapStatus(status), null, now); + _logger.LogWarning( + "AbCip read returned non-zero transport status {Status} for tag {Tag} on device {Device}; " + + "evicting cached runtime so next call re-creates it", + status, reference, def.DeviceHostAddress); + return ReadOutcome.Degraded($"transport status {status} reading {reference}"); + } - var tagPath = AbCipTagPath.TryParse(def.TagPath); - var bitIndex = tagPath?.BitIndex; + var tagPath = AbCipTagPath.TryParse(def.TagPath); + var bitIndex = tagPath?.BitIndex; - // libplctag never checks the CIP type the controller returned; it decodes the bytes as - // the authored type. A scalar whose buffer is not that type's width is mis-typed config - // (seen live: a REAL authored as INT read Good -17346), so refuse it rather than publish - // garbage as Good. Same-width mismatches (DINT vs REAL) cannot be seen this way. - if (!IsArrayTag(def) && bitIndex is null - && ExpectedScalarWidth(def.DataType) is { } width - && runtime.GetSize() is var size && size >= 0 && size != width) - { - // Same class as the native TypeMismatch above (final review I4): deterministic, device - // answered — no Degraded, warn once per tag. - results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); - WarnTypeMismatchOnce(def.Name, - "AbCip read of {Tag} on device {Device} returned {Size} bytes but its authored type {DataType} is {Width} bytes wide; check the tag's dataType (logged once per tag)", - reference, def.DeviceHostAddress, size, def.DataType, width); - return ReadOutcome.Refused; + // libplctag never checks the CIP type the controller returned; it decodes the bytes as + // the authored type. A scalar whose buffer is not that type's width is mis-typed config + // (seen live: a REAL authored as INT read Good -17346), so refuse it rather than publish + // garbage as Good. Same-width mismatches (DINT vs REAL) cannot be seen this way. + if (!IsArrayTag(def) && bitIndex is null + && ExpectedScalarWidth(def.DataType) is { } width + && runtime.GetSize() is var size && size >= 0 && size != width) + { + // Same class as the native TypeMismatch above (final review I4): deterministic, device + // answered — no Degraded, warn once per tag. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadTypeMismatch, null, now); + WarnTypeMismatchOnce(def.Name, + "AbCip read of {Tag} on device {Device} returned {Size} bytes but its authored type {DataType} is {Width} bytes wide; check the tag's dataType (logged once per tag)", + reference, def.DeviceHostAddress, size, def.DataType, width); + return ReadOutcome.Refused; + } + // Review I-1 — an array tag (the EXPLICIT IsArray flag) decodes the whole buffer into an + // element-typed CLR array (int[]/float[]/bool[]/string[]…), INCLUDING a 1-element array + // (ElementCount 1). Scalar tags keep the single-value path. + var value = IsArrayTag(def) + ? runtime.DecodeArray(def.DataType, Math.Max(1, def.ElementCount)) + : runtime.DecodeValue(def.DataType, bitIndex); + results[fb.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); + return ReadOutcome.Success; + } + finally + { + opLock.Release(); } - // Review I-1 — an array tag (the EXPLICIT IsArray flag) decodes the whole buffer into an - // element-typed CLR array (int[]/float[]/bool[]/string[]…), INCLUDING a 1-element array - // (ElementCount 1). Scalar tags keep the single-value path. - var value = IsArrayTag(def) - ? runtime.DecodeArray(def.DataType, Math.Max(1, def.ElementCount)) - : runtime.DecodeValue(def.DataType, bitIndex); - results[fb.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); - return ReadOutcome.Success; } finally { - opLock.Release(); + window.Release(); } } catch (OperationCanceledException) @@ -1185,7 +1207,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// per-tag fallback list are unaffected. /// private async Task ReadGroupAsync( - AbCipUdtReadGroup group, DataValueSnapshot[] results, DateTime now, CancellationToken ct) + AbCipUdtReadGroup group, DataValueSnapshot[] results, DateTime now, SemaphoreSlim window, CancellationToken ct) { var parent = group.ParentDefinition; @@ -1199,53 +1221,62 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { var runtime = await EnsureTagRuntimeAsync(device, parent, ct).ConfigureAwait(false); + // Window slot only now that the runtime exists (see ReadAsync); a wait that throws never acquired. + await window.WaitAsync(ct).ConfigureAwait(false); // Serialise the whole-UDT Read → GetStatus → per-member DecodeValueAt against the shared // parent runtime — the members decode from that one handle's buffer. var opLock = device.GetRuntimeLock(parent.Name); - await opLock.WaitAsync(ct).ConfigureAwait(false); try { - await runtime.ReadAsync(ct).ConfigureAwait(false); + await opLock.WaitAsync(ct).ConfigureAwait(false); + try + { + await runtime.ReadAsync(ct).ConfigureAwait(false); - var status = runtime.GetStatus(); - if (status == (int)AbStatus.TypeMismatch) - { - // Final review I4: deterministic — keep the handle, no health signal, warn once. - StampGroupStatus(group, results, now, AbCipStatusMapper.BadTypeMismatch); - WarnTypeMismatchOnce(parent.Name, - "AbCip whole-UDT read of {Parent} on device {Device}: the controller's type does not match " + - "the authored type; {MemberCount} members report BadTypeMismatch (logged once per tag)", - group.ParentName, parent.DeviceHostAddress, group.Members.Count); - return ReadOutcome.Refused; - } - if (AbCipRefusals.IsDeviceRefusal(status)) - { - StampGroupStatus(group, results, now, AbCipStatusMapper.MapReadStatus((AbStatus)status)); - WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, ((AbStatus)status).ToString()); - return ReadOutcome.Refused; - } - if (status != 0) - { - EvictRuntime(device, parent.Name); - var mapped = AbCipStatusMapper.MapStatus(status); - StampGroupStatus(group, results, now, mapped); - _logger.LogWarning( - "AbCip whole-UDT read returned non-zero transport status {Status} for parent {Parent} " + - "on device {Device}; {MemberCount} member values stamped with mapped status", - status, group.ParentName, parent.DeviceHostAddress, group.Members.Count); - return ReadOutcome.Degraded($"transport status {status} reading UDT {group.ParentName}"); - } + var status = runtime.GetStatus(); + if (status == (int)AbStatus.TypeMismatch) + { + // Final review I4: deterministic — keep the handle, no health signal, warn once. + StampGroupStatus(group, results, now, AbCipStatusMapper.BadTypeMismatch); + WarnTypeMismatchOnce(parent.Name, + "AbCip whole-UDT read of {Parent} on device {Device}: the controller's type does not match " + + "the authored type; {MemberCount} members report BadTypeMismatch (logged once per tag)", + group.ParentName, parent.DeviceHostAddress, group.Members.Count); + return ReadOutcome.Refused; + } + if (AbCipRefusals.IsDeviceRefusal(status)) + { + StampGroupStatus(group, results, now, AbCipStatusMapper.MapReadStatus((AbStatus)status)); + WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, ((AbStatus)status).ToString()); + return ReadOutcome.Refused; + } + if (status != 0) + { + EvictRuntime(device, parent.Name); + var mapped = AbCipStatusMapper.MapStatus(status); + StampGroupStatus(group, results, now, mapped); + _logger.LogWarning( + "AbCip whole-UDT read returned non-zero transport status {Status} for parent {Parent} " + + "on device {Device}; {MemberCount} member values stamped with mapped status", + status, group.ParentName, parent.DeviceHostAddress, group.Members.Count); + return ReadOutcome.Degraded($"transport status {status} reading UDT {group.ParentName}"); + } - foreach (var member in group.Members) - { - var value = runtime.DecodeValueAt(member.Definition.DataType, member.Offset, bitIndex: null); - results[member.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); + foreach (var member in group.Members) + { + var value = runtime.DecodeValueAt(member.Definition.DataType, member.Offset, bitIndex: null); + results[member.OriginalIndex] = new DataValueSnapshot(value, AbCipStatusMapper.Good, now, now); + } + return ReadOutcome.Success; + } + finally + { + opLock.Release(); } - return ReadOutcome.Success; } finally { - opLock.Release(); + window.Release(); } } catch (OperationCanceledException) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor index 49ecfd06..496a2bed 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/AbCipDriverForm.razor @@ -21,7 +21,7 @@
-
Read work items in flight at once. Default 8; 1 = sequential.
+
Read work items in flight at once. Default 8; 1 = sequential (libplctag). The native transport always uses at least 64 — it packs concurrent reads into one request.
diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWindowTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWindowTests.cs new file mode 100644 index 00000000..45cd3489 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipReadWindowTests.cs @@ -0,0 +1,96 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 P8 / P7: the bounded-parallel read window. On the native transport every concurrent tag read of a +/// device rides ONE shared request scheduler that packs whatever is queued into a Multiple Service Packet, so a +/// window of 8 capped MSP coalescing at 8 services per round trip; it is widened to at least 64 there (the +/// scheduler's reply budget already prevents over-packing). And a cold create waiting on a device's connect gate +/// must not occupy a window slot, or one dead device's backoff-expiry attempt stalls every other device's reads. +/// +[Trait("Category", "Unit")] +public sealed class AbCipReadWindowTests +{ + private static AbCipDriverOptions Options(int maxConcurrentReads, AbCipTransport? transport = null) => new() + { + Devices = [new AbCipDeviceOptions("ab://10.0.0.5/1,0")], + Probe = new AbCipProbeOptions { Enabled = false }, + MaxConcurrentReads = maxConcurrentReads, + Transport = transport, + }; + + /// The window per transport: native ≥ 64; libplctag and injected factories honour the option exactly. + [Fact] + public void The_native_window_is_at_least_64_and_other_transports_keep_the_option() + { + new AbCipDriver(Options(8, AbCipTransport.Native), "a").ReadWindow.ShouldBe(AbCipDriver.NativeMinReadWindow); + AbCipDriver.NativeMinReadWindow.ShouldBeGreaterThanOrEqualTo(64); + new AbCipDriver(Options(1, AbCipTransport.Native), "a").ReadWindow.ShouldBe(AbCipDriver.NativeMinReadWindow); + new AbCipDriver(Options(200, AbCipTransport.Native), "a").ReadWindow.ShouldBe(200); + new AbCipDriver(Options(8), "a", defaultTransport: AbCipTransport.Native).ReadWindow.ShouldBe(AbCipDriver.NativeMinReadWindow); + + // Positive controls: libplctag (per-handle concurrency) and an injected test factory keep the option. + new AbCipDriver(Options(8, AbCipTransport.Libplctag), "a").ReadWindow.ShouldBe(8); + new AbCipDriver(Options(0, AbCipTransport.Libplctag), "a").ReadWindow.ShouldBe(1); + new AbCipDriver(Options(3, AbCipTransport.Native), "a", new FakeAbCipTagFactory()).ReadWindow.ShouldBe(3); + } + + /// A tag whose initialisation blocks until released (a cold connect to a slow / dead device). + private sealed class BlockingInitTag(AbCipTagCreateParams p, Task release) : FakeAbCipTag(p) + { + public override async Task InitializeAsync(CancellationToken cancellationToken) + { + await base.InitializeAsync(cancellationToken); + await release.WaitAsync(cancellationToken); + } + } + + /// + /// With a window of ONE, device B's blocked cold create (and its queued sibling) must not stop device A's tag + /// from being read: cold waiters sit on B's connect gate, not in the window. + /// + [Fact] + public async Task A_blocked_cold_create_on_one_device_does_not_hold_the_window_against_another() + { + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeAbCipTagFactory + { + Customise = p => p.Gateway == "10.0.0.6" + ? new BlockingInitTag(p, release.Task) { Value = 2 } + : new FakeAbCipTag(p) { Value = 1 }, + }; + const string a = "ab://10.0.0.5/1,0", b = "ab://10.0.0.6/1,0"; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(a), new AbCipDeviceOptions(b)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("B1", b, "B1", AbCipDataType.DInt), + new AbCipTagDefinition("B2", b, "B2", AbCipDataType.DInt), + new AbCipTagDefinition("A1", a, "A1", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + MaxConcurrentReads = 1, + }, "abcip-window", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var read = drv.ReadAsync(["B1", "B2", "A1"], CancellationToken.None); + var deadline = DateTime.UtcNow.AddSeconds(5); + while (!(factory.Tags.TryGetValue("A1", out var tag) && tag.ReadCount > 0)) + { + if (DateTime.UtcNow > deadline) + { + release.TrySetResult(); + await read; + throw new ShouldAssertException("device A's read waited behind device B's blocked cold create"); + } + await Task.Delay(5); + } + read.IsCompleted.ShouldBeFalse(); // B is still blocked — A was read regardless + + release.SetResult(); + var r = await read; + r.Select(x => x.Value).ShouldBe([2, 2, 1]); // positive control: B's reads complete once it connects + await drv.DisposeAsync(); + } +} From cb16ebc75e757cc1129f56b41900117c5b55dd93 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:21:43 -0400 Subject: [PATCH 06/15] fix(abcip): bit-in-word writes take the parent width from its read H17: the .n read-modify-write hard-coded a DINT parent. On an INT or SINT host (both legal in Logix, and reads of Flags.3 there already worked) the DINT decode of a 2-byte buffer threw OutOfBounds, the generic catch evicted the BIT tag's (non-existent) runtime and reported BadCommunicationError + Degraded. The parent width now comes from the parent read's size (1 SINT / 2 INT / 4 DINT / 8 LINT; unknown keeps the DINT assumption). Narrow hosts use unsigned arithmetic so a set top bit encodes without overflow; a bit past the host's width is BadOutOfRange and a non-integer width BadTypeMismatch, both refused before anything is written, warned once, no health write. A transport failure in the RMW now evicts the parent runtime it actually used. Native end-to-end: an INT host is written as ONE INT (type 0xC3, 2 bytes). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 73 ++++++++-- .../AbCipBitInWordWidthTests.cs | 127 ++++++++++++++++++ 2 files changed, 189 insertions(+), 11 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index b48a619c..89f06793 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1374,6 +1374,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, continue; } + string? rmwParent = null; try { var parsedPath = AbCipTagPath.TryParse(def.TagPath); @@ -1384,6 +1385,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // losing one another's update. if (def.DataType == AbCipDataType.Bool && parsedPath?.BitIndex is int bit) { + rmwParent = (parsedPath with { BitIndex = null }).ToLibplctagName(); results[i] = new WriteResult( await WriteBitInDIntAsync(device, parsedPath, bit, w.Value, cancellationToken) .ConfigureAwait(false)); @@ -1542,8 +1544,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } catch (Exception ex) { - // Transport / wire error — evict so the next write creates a fresh handle. - EvictRuntime(device, def.Name); + // Transport / wire error — evict so the next write creates a fresh handle (for a bit write, the + // parent runtime the read-modify-write used; the bit tag itself holds none). + if (rmwParent is not null) EvictParentRuntime(device, rmwParent); + else EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); _logger.LogWarning(ex, @@ -1556,10 +1560,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// - /// Read-modify-write one bit within a DINT parent. Creates / reuses a parallel - /// parent-DINT runtime (distinct from the bit-selector handle) + serialises concurrent - /// writers against the same parent via a per-parent . - /// Matches the Modbus BitInRegister + FOCAS PMC Bit pattern. + /// Read-modify-write one bit within an integer parent (SINT / INT / DINT / LINT — Logix allows .n on + /// each). Creates / reuses a parallel parent runtime (distinct from the bit-selector handle) + serialises + /// concurrent writers against the same parent via a per-parent . The parent's + /// width comes from the size its read returned (H17 — a hard-coded DINT decode threw OutOfBounds on an INT + /// host and was reported as a comms fault). Matches the Modbus BitInRegister + FOCAS PMC Bit pattern. /// private async Task WriteBitInDIntAsync( DeviceState device, AbCipTagPath bitPath, int bit, object? value, CancellationToken ct) @@ -1576,12 +1581,49 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var readStatus = parentRuntime.GetStatus(); if (readStatus != 0) return AbCipStatusMapper.MapStatus(readStatus); - var current = Convert.ToInt32(parentRuntime.DecodeValue(AbCipDataType.DInt, bitIndex: null) ?? 0); - var updated = Convert.ToBoolean(value) - ? current | (1 << bit) - : current & ~(1 << bit); + // Unsigned at the narrow widths so a set top bit (SINT bit 7, INT bit 15) encodes without overflowing the + // signed type; DINT keeps its signed int arithmetic (1 << 31 is int.MinValue, which a DINT encodes as is). + var size = parentRuntime.GetSize(); + AbCipDataType? hostType = size switch + { + 1 => AbCipDataType.USInt, + 2 => AbCipDataType.UInt, + 8 => AbCipDataType.LInt, + 4 or <= 0 => AbCipDataType.DInt, // <= 0: the runtime cannot say — the historical DINT assumption + _ => null, + }; + if (hostType is not { } type) + { + WarnTypeMismatchOnce(parentName, + "AbCip bit write to {Parent}.{Bit} on device {Device} refused: the parent is {Size} byte(s), not an integer host (logged once per tag)", + parentName, bit, device.Options.HostAddress, size); + return AbCipStatusMapper.BadTypeMismatch; + } + var widthBits = type switch { AbCipDataType.USInt => 8, AbCipDataType.UInt => 16, AbCipDataType.LInt => 64, _ => 32 }; + if (bit >= widthBits) + { + WarnTypeMismatchOnce(parentName, + "AbCip bit write to {Parent}.{Bit} on device {Device} refused: the parent holds only {Bits} bits (logged once per tag)", + parentName, bit, device.Options.HostAddress, widthBits); + return AbCipStatusMapper.BadOutOfRange; + } - parentRuntime.EncodeValue(AbCipDataType.DInt, bitIndex: null, updated); + var set = Convert.ToBoolean(value); + var decoded = parentRuntime.DecodeValue(type, bitIndex: null) ?? 0; + object updated; + if (type == AbCipDataType.LInt) + { + var current = Convert.ToInt64(decoded); + updated = set ? current | (1L << bit) : current & ~(1L << bit); + } + else + { + var mask = type switch { AbCipDataType.USInt => 0xFF, AbCipDataType.UInt => 0xFFFF, _ => -1 }; + var current = Convert.ToInt32(decoded) & mask; + updated = (set ? current | (1 << bit) : current & ~(1 << bit)) & mask; + } + + parentRuntime.EncodeValue(type, bitIndex: null, updated); await parentRuntime.WriteAsync(ct).ConfigureAwait(false); var writeStatus = parentRuntime.GetStatus(); return writeStatus == 0 @@ -1763,6 +1805,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } + /// Evicts + disposes a bit-write parent runtime so the next read-modify-write re-creates it. + private static void EvictParentRuntime(DeviceState device, string parentName) + { + if (device.ParentRuntimes.TryRemove(parentName, out var stale)) + { + try { stale.Dispose(); } catch { } + } + } + /// public DriverHealth GetHealth() => _health; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs new file mode 100644 index 00000000..11f7b736 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs @@ -0,0 +1,127 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 H17: Logix allows a .n bit selector on SINT / INT / DINT (and LINT) hosts, and reads of +/// Flags.3 on an INT worked — but the bit write's read-modify-write hard-coded a DINT parent, so its +/// DecodeValue(DInt) on a 2-byte buffer threw OutOfBounds and the write was reported as a comms fault +/// (BadCommunicationError + Degraded). The parent width now comes from the parent read's size. +/// +[Trait("Category", "Unit")] +public sealed class AbCipBitInWordWidthTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private static async Task<(AbCipDriver Driver, FakeAbCipTagFactory Factory)> BuildAsync(int parentSize, object parentValue, string bitPath) + { + var factory = new FakeAbCipTagFactory + { + Customise = p => new FakeAbCipTag(p) { Value = parentValue, Size = parentSize }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Bit", Device, bitPath, AbCipDataType.Bool)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-bit", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory); + } + + /// SINT / INT / DINT hosts: the bit is set, every other bit is preserved, nothing degrades. + /// The parent's wire width. + /// The parent's value before the write. + /// The bit written true. + /// The value the parent must be written back with. + [Theory] + [InlineData(1, 0x81, 3, 0x89)] // SINT 0x81 (-127): bit 7 set stays set, written as its unsigned byte + [InlineData(1, 0x00, 7, 0x80)] // SINT top bit: 128 would overflow an sbyte encode + [InlineData(2, 0x8001, 3, 0x8009)] // INT top bit preserved + [InlineData(2, 0x0000, 15, 0x8000)] // INT top bit: 32768 would overflow a short encode + [InlineData(4, 0x00000001, 3, 0x00000009)] + public async Task A_bit_write_uses_the_parent_width(int size, int before, int bit, int after) + { + var (drv, factory) = await BuildAsync(size, before, $"Motor.Flags.{bit}"); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + Convert.ToInt64(factory.Tags["Motor.Flags"].Value).ShouldBe(after); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A bit past the host's width is refused before anything is written, with no health write. + [Fact] + public async Task A_bit_past_the_host_width_is_BadOutOfRange_and_nothing_is_written() + { + var (drv, factory) = await BuildAsync(2, 0, "Motor.Flags.20"); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A host whose width is no integer's is a type mismatch, refused before writing, no health write. + [Fact] + public async Task A_host_of_no_integer_width_is_BadTypeMismatch() + { + var (drv, factory) = await BuildAsync(3, 0, "Motor.Flags.1"); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// End to end over the REAL native runtime against a fake CIP target whose Flags is an INT: the write + /// goes out as ONE INT (type 0xC3, two data bytes) with the bit set and the other bits preserved. + /// + [Fact] + public async Task A_native_bit_write_on_an_INT_host_writes_an_INT() + { + await using var target = new FakeCipTarget(); + // Read Tag Fragmented (0x52) → INT (0x00C3) 0x8001; Write Tag (0x4D) → success. + target.ConnectedHandler = req => Task.FromResult(req[0] switch + { + 0x52 => [0xD2, 0x00, 0x00, 0x00, 0xC3, 0x00, 0x01, 0x80], + 0x4D => [0xCD, 0x00, 0x00, 0x00], + _ => target.ServiceReply(req), + }); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Bit", host, "Flags.3", AbCipDataType.Bool)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "abcip-bit-native", new NativeAbCipTagFactory(new DeviceSessionPool())); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var write = target.ConnectedCips.Single(c => c[0] == 0x4D); + var typeAt = 2 + write[1] * 2; + write[typeAt].ShouldBe((byte)0xC3); // INT, not DINT + write.Length.ShouldBe(typeAt + 2 + 2 + 2); // type, element count, ONE 2-byte element + write[^2].ShouldBe((byte)0x09); // 0x8001 | bit 3 + write[^1].ShouldBe((byte)0x80); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await drv.DisposeAsync(); + } + } +} From 895cc28886993feb421268f792d7c85948dc3fea Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:24:06 -0400 Subject: [PATCH 07/15] fix(abcip): write-side health neither latches nor overwrites H19: Format/InvalidCast/Overflow on a client value never touched the wire but latched Degraded; a good write set Healthy and so undid a Degraded the poll loop had just recorded for other tags (the batch read path is worst-wins for exactly this reason). - The three conversion failures map to BadTypeMismatch/BadOutOfRange with a once-per-tag Warning and no health write (AbLegacy's rule). - A good write (scalar or bit RMW) advances LastSuccessfulRead only - the device answered - and leaves the state alone. - A write comms fault (thrown, or a non-refusal status word, which used to write no health at all) is Degraded with LastSuccessfulRead preserved and never downgrades Faulted. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 45 ++++--- .../AbCipWriteHealthTests.cs | 113 ++++++++++++++++++ 2 files changed, 142 insertions(+), 16 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 89f06793..f675234e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1390,7 +1390,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, await WriteBitInDIntAsync(device, parsedPath, bit, w.Value, cancellationToken) .ConfigureAwait(false)); if (results[i].StatusCode == AbCipStatusMapper.Good) - _health = new DriverHealth(DriverState.Healthy, now, null); + RecordWriteAnswered(now); continue; } @@ -1455,6 +1455,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.MapStatus(status)); + RecordWriteCommsFault($"transport status {status} writing {w.FullReference}"); _logger.LogWarning( "AbCip write returned non-zero transport status {Status} for tag {Tag} on device {Device}; " + "evicting cached runtime so next call re-creates it", @@ -1463,7 +1464,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, else { results[i] = new WriteResult(AbCipStatusMapper.Good); - _health = new DriverHealth(DriverState.Healthy, now, null); + RecordWriteAnswered(now); } } catch (OperationCanceledException) @@ -1481,30 +1482,29 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write not supported for tag {Tag} on device {Device}: {Reason} (logged once per tag)", w.FullReference, def.DeviceHostAddress, nse.Message); } + // H19: the three client-value conversion failures below never touched the wire — they say nothing about + // the device, so they write no health (a Degraded here latched until the next good read, and AbLegacy + // already maps the same three with no health write). Not a transport fault either: no evict. catch (FormatException fe) { - // Value conversion error — not a transport fault; don't evict. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, fe.Message); - _logger.LogWarning(fe, - "AbCip write value-conversion error for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device}: the value cannot be converted to {DataType}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType, fe.Message); } catch (InvalidCastException ice) { results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ice.Message); - _logger.LogWarning(ice, - "AbCip write type-cast error for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device}: the value cannot be converted to {DataType}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType, ice.Message); } catch (OverflowException oe) { results[i] = new WriteResult(AbCipStatusMapper.BadOutOfRange); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, oe.Message); - _logger.LogWarning(oe, - "AbCip write value out of range for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device}: the value is out of range for {DataType}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType, oe.Message); } catch (AbTransportException ate) when (ate.Status == AbStatus.Unsupported) { @@ -1549,7 +1549,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (rmwParent is not null) EvictParentRuntime(device, rmwParent); else EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + RecordWriteCommsFault(ex.Message); _logger.LogWarning(ex, "AbCip write transport exception for tag {Tag} on device {Device}", w.FullReference, def.DeviceHostAddress); @@ -1559,6 +1559,19 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, return results; } + /// + /// A good write proves the device answered: advance LastSuccessfulRead but leave the STATE alone. Writing + /// Healthy here (H19) overwrote a Degraded the poll loop had just recorded for other tags. + /// + private void RecordWriteAnswered(DateTime now) => + UpdateHealth(h => h with { LastSuccessfulRead = now }); + + /// A write-side comms fault: Degraded (never downgrading Faulted), LastSuccessfulRead preserved. + private void RecordWriteCommsFault(string error) => + UpdateHealth(h => h.State == DriverState.Faulted + ? h + : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, error)); + /// /// Read-modify-write one bit within an integer parent (SINT / INT / DINT / LINT — Logix allows .n on /// each). Creates / reuses a parallel parent runtime (distinct from the bit-selector handle) + serialises diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs new file mode 100644 index 00000000..4821a43a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs @@ -0,0 +1,113 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 H19: write-side health. A client value that cannot be converted never touched the wire, so it must +/// not latch Degraded; and a good write must not overwrite a Degraded the read path just recorded +/// with Healthy (the batch read path is worst-wins for exactly this reason). A good write proves the device +/// answered, so it advances LastSuccessfulRead without touching the state. +/// +[Trait("Category", "Unit")] +public sealed class AbCipWriteHealthTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private sealed class ThrowingWriteTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override Task WriteAsync(CancellationToken cancellationToken) => + throw new InvalidOperationException("socket reset"); + } + + private static async Task<(AbCipDriver Driver, FakeAbCipTagFactory Factory)> BuildAsync( + Func? make = null) + { + var factory = new FakeAbCipTagFactory + { + Customise = make ?? (p => p.TagName == "Broken" + ? new FakeAbCipTag(p) { ThrowOnRead = true } + : new RealEncodingTag(p) { Value = 0 }), + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("D", Device, "D", AbCipDataType.DInt), + new AbCipTagDefinition("I", Device, "I", AbCipDataType.Int), + new AbCipTagDefinition("Broken", Device, "Broken", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-write-health", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory); + } + + /// A fake whose encode converts like the real runtimes do (Convert.ToInt32 / ToInt16). + private sealed class RealEncodingTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override void EncodeValue(AbCipDataType type, int? bitIndex, object? value) => + Value = type switch + { + AbCipDataType.Int => Convert.ToInt16(value), + _ => Convert.ToInt32(value), + }; + } + + /// Unconvertible / out-of-range client values: refused per tag, health untouched, nothing sent. + [Fact] + public async Task A_client_conversion_error_writes_no_health() + { + var (drv, factory) = await BuildAsync(); + var before = drv.GetHealth(); + + var r = await drv.WriteAsync( + [ + new WriteRequest("D", "not a number"), // FormatException + new WriteRequest("D", new object()), // InvalidCastException + new WriteRequest("I", 1_000_000), // OverflowException + ], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + r[2].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + drv.GetHealth().ShouldBe(before); + factory.Tags["D"].WriteCount.ShouldBe(0); + factory.Tags["I"].WriteCount.ShouldBe(0); + } + + /// A good write keeps a Degraded the read path recorded, and advances LastSuccessfulRead. + [Fact] + public async Task A_good_write_does_not_overwrite_Degraded_with_Healthy() + { + var (drv, _) = await BuildAsync(); + await drv.ReadAsync(["Broken"], CancellationToken.None); + var degraded = drv.GetHealth(); + degraded.State.ShouldBe(DriverState.Degraded); + await Task.Delay(20); + + var r = await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var after = drv.GetHealth(); + after.State.ShouldBe(DriverState.Degraded); + after.LastError.ShouldBe(degraded.LastError); + after.LastSuccessfulRead!.Value.ShouldBeGreaterThan(degraded.LastSuccessfulRead!.Value); // the device answered + } + + /// Positive control: a genuine write transport fault still degrades and preserves LastSuccessfulRead. + [Fact] + public async Task A_write_transport_fault_still_degrades() + { + var (drv, factory) = await BuildAsync(p => new ThrowingWriteTag(p)); + var before = drv.GetHealth(); + + var r = await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError); + var after = drv.GetHealth(); + after.State.ShouldBe(DriverState.Degraded); + after.LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead); + factory.Tags["D"].Disposed.ShouldBeTrue(); + } +} From 82a485e42ee9b3c99872080c16452f239ceb8ef1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:27:34 -0400 Subject: [PATCH 08/15] fix(abcip): a BOOL-array element reads its own bit, not "low byte != 0" U11: a scalar BOOL authored over Bits[n] requested element n and decoded "any of bits 0-7 of the reply set", so Bits[n] read Good with the subscript ignored. A Logix BOOL array is stored as DWORDs (0xD3) and its element segment addresses them, so the native runtime now requests DWORD n / 32 and decodes bit n % 32 when the controller answers 0xD3 (a plain BOOL reply still decodes its byte). libplctag exposes no type code and so cannot say which DWORD it read: a 4-byte reply for such a tag is refused BadNotSupported (warned once) instead of publishing a wrong value as Good. Real-controller proof is a hardware gate. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 1 + .../LibplctagTagRuntime.cs | 16 ++++++- .../NativeAbCipTagRuntime.cs | 42 ++++++++++++++++++- .../NativeAbCipTagRuntimeTests.cs | 31 ++++++++++++++ 4 files changed, 87 insertions(+), 3 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index f675234e..0a7eb3b0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1150,6 +1150,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // A deterministic local refusal — not a comms fault: BadNotSupported, keep the handle, no // health signal. results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNotSupported, null, now); + WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ate.Message); return ReadOutcome.None; } catch (AbCipTagConfigurationException ace) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index e2ff94c9..2e6cd7fc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -29,8 +29,19 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime public LibplctagTagRuntime(AbCipTagCreateParams p) { _tag = BuildTag(p); + _boolArrayElement = !p.IsArray && p.AuthoredType == AbCipDataType.Bool + && NativeAbCipTagRuntime.TrySplitBoolArrayElement(p.TagName, out _, out _); } + /// + /// A scalar BOOL authored over a BOOL-array element (Bits[n]). libplctag reads it as whatever the + /// controller returns for that element segment — a packed DWORD — and exposes no type code, so which DWORD + /// came back (and so which bit is element n) cannot be checked here. Review 07 U11: such a read used to decode + /// "low byte != 0" as Good; it is refused (BadNotSupported) when a DWORD came back. The native transport + /// (the default) addresses the right DWORD and decodes the bit. + /// + private readonly bool _boolArrayElement; + /// Builds the (uninitialized) libplctag tag for . /// The tag creation parameters. /// The new, uninitialised libplctag tag. @@ -120,7 +131,10 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime { AbCipDataType.Bool => bitIndex is int bit ? _tag.GetBit(bit) - : _tag.GetInt8(offset) != 0, + : _boolArrayElement && offset == 0 && _tag.GetSize() == 4 + ? throw new AbTransportException(AbStatus.Unsupported, + $"'{_tag.Name}' is a BOOL-array element read as a packed DWORD; libplctag cannot say which DWORD it is — use the native transport.") + : _tag.GetInt8(offset) != 0, AbCipDataType.SInt => (int)(sbyte)_tag.GetInt8(offset), AbCipDataType.USInt => (int)_tag.GetUInt8(offset), AbCipDataType.Int => (int)_tag.GetInt16(offset), diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index 8bf65cca..917f205b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -41,6 +41,10 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private int? _implicitBit; private byte[]? _path; + // U11: a scalar BOOL authored over a BOOL-array element "Bits[n]" — the bit (n % 32) of the DWORD the request + // addresses (n / 32). Null for every other tag. + private int? _boolElementBit; + private ConnectionLease? _lease; private RequestScheduler? _privateScheduler; // ForTest only — production always uses the lease's private LogixTagClient? _client; @@ -115,7 +119,14 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime try { split = CipPath.SplitBitSelector(_p.TagName); - path = CipPath.EncodeSymbolic(split.Name); + var name = split.Name; + if (split.Bit is null && !_p.IsArray && _p.AuthoredType == AbCipDataType.Bool + && TrySplitBoolArrayElement(name, out var dwordName, out var bit)) + { + name = dwordName; + _boolElementBit = bit; + } + path = CipPath.EncodeSymbolic(name); } catch (Exception ex) when (ex is ArgumentException or FormatException or OverflowException) { @@ -275,7 +286,9 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime return type switch { - AbCipDataType.Bool => bitIndex is int bit ? GetBit(bit) : Span(offset, 1)[0] != 0, + AbCipDataType.Bool => bitIndex is int bit ? GetBit(bit) + : offset == 0 && _boolElementBit is int element && _typeCode == LogixTypeCode.BoolArray ? GetBit(element) + : Span(offset, 1)[0] != 0, AbCipDataType.SInt => (int)(sbyte)Span(offset, 1)[0], AbCipDataType.USInt => (int)Span(offset, 1)[0], AbCipDataType.Int => (int)BinaryPrimitives.ReadInt16LittleEndian(Span(offset, 2)), @@ -409,6 +422,31 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime catch (Exception) { /* ForTest only; Dispose must not throw */ } } + /// + /// Splits a BOOL-array element reference Name[n] (one non-negative subscript, last segment) into the + /// DWORD it lives in (Name[n / 32]) and its bit within that DWORD (n % 32). A Logix BOOL array + /// is stored as DWORDs (type 0xD3) and its element segment addresses those DWORDs. + /// + /// The tag name, bit selector already split off. + /// The name addressing the containing DWORD. + /// The element's bit within that DWORD. + /// when ends in a single subscript. + internal static bool TrySplitBoolArrayElement(string name, out string dwordName, out int bit) + { + dwordName = name; + bit = 0; + if (!name.EndsWith(']')) return false; + var open = name.LastIndexOf('['); + if (open <= 0) return false; + var inner = name[(open + 1)..^1]; + if (inner.Length == 0 || !inner.All(char.IsAsciiDigit) + || !int.TryParse(inner, System.Globalization.NumberStyles.None, System.Globalization.CultureInfo.InvariantCulture, out var n)) + return false; + dwordName = $"{name[..open]}[{n / 32}]"; + bit = n % 32; + return true; + } + private byte[] Path() => _path ?? throw new InvalidOperationException("InitializeAsync has not been called."); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 35f42bd3..9d013613 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -112,6 +112,37 @@ public sealed class NativeAbCipTagRuntimeTests rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(true); } + /// + /// Review 07 U11: a scalar BOOL authored over a BOOL-array element Bits[n] reads DWORD n / 32 + /// (a Logix BOOL array is stored as DWORDs and its CIP element index addresses them) and decodes bit + /// n % 32 of it — it used to request element n and report "any of bits 0-7 of the reply set". + /// + /// The authored BOOL-array element. + /// The DWORD the request must address. + /// The DWORD the controller returns (little-endian hex). + /// The element's value. + [Theory] + [InlineData(37, 1, "20000000", true)] // bit 5 of DWORD 1 + [InlineData(36, 1, "20000000", false)] // bit 4 of the same DWORD — the old "low byte != 0" read true + [InlineData(5, 0, "20000000", true)] + [InlineData(1, 0, "01000000", false)] // bit 1, not bit 0 + [InlineData(31, 0, "00000080", true)] // top bit of DWORD 0 — outside the low byte entirely + public async Task A_BOOL_array_element_decodes_its_bit_of_its_DWORD(int n, int dword, string reply, bool expected) + { + var ch = Replying("D300", reply); + using var rt = await ReadAsync(ch, Params($"Bits[{n}]", AbCipDataType.Bool)); + rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(expected); + ch.Sent[0].ShouldBe(Convert.FromHexString("5204" + "910442697473" + "28" + dword.ToString("X2") + "0100" + "00000000")); + } + + /// Positive control: a BOOL answered as a plain BOOL (0xC1) still decodes its byte. + [Fact] + public async Task A_BOOL_element_answered_as_BOOL_decodes_the_byte() + { + using var rt = await ReadAsync(Replying("C100", "01"), Params("Flag", AbCipDataType.Bool)); + rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(true); + } + [Fact] public async Task Real_returned_for_a_DInt_authored_tag_is_TypeMismatch() { From 7cc841531041a557156b5083d80752b0279e7ecd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:30:11 -0400 Subject: [PATCH 09/15] feat(abcip): array writes encode count elements at the element width U10 (user decision: implement array encode now): config, the AdminUI and discovery all advertised array tags Operate-writable, but both runtimes encoded a scalar - an int[] threw InvalidCastException (BadTypeMismatch AND Degraded), a scalar wrote element 0 natively, and libplctag re-sent its whole last-read buffer with element 0 patched (stale elements 1..n-1). - AbCipArrayCodec encodes count elements little-endian at the element width, CONVERTING each element (ScadaBridge writes int[] to INT arrays; overflow is BadOutOfRange, unconvertible BadTypeMismatch, both before anything is sent). - The driver checks the shape before any runtime exists: not a list -> BadTypeMismatch, length != elementCount -> BadOutOfRange, BOOL/STRING/ structure element types -> BadNotSupported; and, before sending, a controller buffer that is not width x count -> BadTypeMismatch. Refusals warn once and write no health. - Native: ONE Write Tag of count elements with the controller's type code (the client fragments a large one); libplctag: the codec's bytes copied into the elem_count-sized buffer, refused TooSmall if it does not fit. - IAbCipTagRuntime.EncodeArray (default: NotSupportedException). Real-controller proof is a hardware userGate (see the lane report recipe). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipArrayCodec.cs | 80 ++++++++ .../AbCipDriver.cs | 63 ++++++- .../IAbCipTagRuntime.cs | 11 ++ .../LibplctagTagRuntime.cs | 17 ++ .../NativeAbCipTagRuntime.cs | 22 ++- .../AbCipArrayWriteTests.cs | 178 ++++++++++++++++++ .../FakeAbCipTag.cs | 19 ++ 7 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipArrayCodec.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipArrayCodec.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipArrayCodec.cs new file mode 100644 index 00000000..bb6455ab --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipArrayCodec.cs @@ -0,0 +1,80 @@ +using System.Buffers.Binary; +using System.Collections; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +/// +/// Encodes a whole-array write (review 07 U10): count elements at the element type's wire width, +/// little-endian, exactly the layout a Logix atomic array holds and the read path decodes. Shared by both +/// runtimes so the bytes on the wire are transport-independent. +/// +/// +/// Element values CONVERT, they are not exact-matched. ScadaBridge (and any OPC UA client that +/// round-trips what it read) writes CLR lists such as int[] to an INT array — the read path itself +/// publishes INT/SINT elements as int — so each element goes through the same the +/// scalar path uses. A value that does not fit is an (BadOutOfRange) and one that +/// cannot convert a / (BadTypeMismatch), all +/// thrown before any byte is sent. +/// BOOL arrays (bit-packed DWORDs — a byte-per-element encode would clobber neighbour bits), STRING arrays +/// (structures) and structure arrays are not encodable here; the driver refuses them BadNotSupported +/// before a runtime exists. +/// +internal static class AbCipArrayCodec +{ + /// Wire width of one element of an encodable array type, or null when the type is not encodable. + /// The element type. + /// The element width in bytes, or null. + public static int? ElementWidth(AbCipDataType type) => type switch + { + AbCipDataType.SInt or AbCipDataType.USInt => 1, + AbCipDataType.Int or AbCipDataType.UInt => 2, + AbCipDataType.DInt or AbCipDataType.UDInt or AbCipDataType.Real or AbCipDataType.Dt => 4, + AbCipDataType.LInt or AbCipDataType.ULInt or AbCipDataType.LReal => 8, + _ => null, + }; + + /// + /// The elements of an array write value, or null when the value is not a list (a scalar, a string, null). + /// Any non-string is accepted — int[], short[], List<object> … + /// + /// The client's write value. + /// The elements, or null. + public static object?[]? Elements(object? value) => value switch + { + null or string => null, + IEnumerable list => [.. list.Cast()], + _ => null, + }; + + /// Encodes as elements. + /// The element type (must have an ). + /// The element values; their count is the element count. + /// The little-endian wire bytes, values.Count × ElementWidth(type) long. + /// The type is not encodable. + public static byte[] Encode(AbCipDataType type, IReadOnlyList values) + { + var width = ElementWidth(type) + ?? throw new NotSupportedException($"Array writes of {type} elements are not supported."); + var buffer = new byte[values.Count * width]; + for (var i = 0; i < values.Count; i++) + { + var v = values[i]; + var span = buffer.AsSpan(i * width, width); + switch (type) + { + case AbCipDataType.SInt: span[0] = unchecked((byte)Convert.ToSByte(v)); break; + case AbCipDataType.USInt: span[0] = Convert.ToByte(v); break; + case AbCipDataType.Int: BinaryPrimitives.WriteInt16LittleEndian(span, Convert.ToInt16(v)); break; + case AbCipDataType.UInt: BinaryPrimitives.WriteUInt16LittleEndian(span, Convert.ToUInt16(v)); break; + case AbCipDataType.DInt: + case AbCipDataType.Dt: BinaryPrimitives.WriteInt32LittleEndian(span, Convert.ToInt32(v)); break; + case AbCipDataType.UDInt: BinaryPrimitives.WriteUInt32LittleEndian(span, Convert.ToUInt32(v)); break; + case AbCipDataType.LInt: BinaryPrimitives.WriteInt64LittleEndian(span, Convert.ToInt64(v)); break; + case AbCipDataType.ULInt: BinaryPrimitives.WriteUInt64LittleEndian(span, Convert.ToUInt64(v)); break; + case AbCipDataType.Real: BinaryPrimitives.WriteSingleLittleEndian(span, Convert.ToSingle(v)); break; + case AbCipDataType.LReal: BinaryPrimitives.WriteDoubleLittleEndian(span, Convert.ToDouble(v)); break; + } + } + return buffer; + } +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 0a7eb3b0..43afe57f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1376,6 +1376,43 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } string? rmwParent = null; + // U10: an array tag writes the WHOLE array. Shape is checked before any runtime exists, so a refused + // write creates, reads and sends nothing; a refusal is not a fault — no health change. + object?[]? arrayElements = null; + if (IsArrayTag(def)) + { + var count = Math.Max(1, def.ElementCount); + uint? shapeRefusal = null; + string reason = ""; + if (AbCipArrayCodec.ElementWidth(def.DataType) is null) + { + shapeRefusal = AbCipStatusMapper.BadNotSupported; + reason = $"{def.DataType} arrays cannot be written element-wise (BOOL arrays are bit-packed DWORDs, STRING/structure elements are structures)"; + } + else if (AbCipArrayCodec.Elements(w.Value) is not { } elements) + { + shapeRefusal = AbCipStatusMapper.BadTypeMismatch; + reason = $"the value is not an array ({w.Value?.GetType().Name ?? "null"})"; + } + else if (elements.Length != count) + { + shapeRefusal = AbCipStatusMapper.BadOutOfRange; + reason = $"{elements.Length} element(s) written to a {count}-element array"; + } + else + { + arrayElements = elements; + } + if (shapeRefusal is { } refusedShape) + { + results[i] = new WriteResult(refusedShape); + WarnWriteRefusedOnce(def.Name, + "AbCip array write to {Tag} on device {Device} refused: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, reason); + continue; + } + } + try { var parsedPath = AbCipTagPath.TryParse(def.TagPath); @@ -1413,10 +1450,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // BOOL-array DWORD wrote all 32 bits. Discovered UDT members are where a mis-authored type // is most likely (browse-commit fills dataType from the template, an operator may retype). size = runtime.GetSize(); - refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size, runtimeTypesBool: runtime is NativeAbCipTagRuntime); + refusal = arrayElements is not null + ? ArrayWriteWidthRefusal(def, size) + : WriteWidthRefusal(def, parsedPath?.BitIndex, size, runtimeTypesBool: runtime is NativeAbCipTagRuntime); if (refusal is null) { - runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); + if (arrayElements is not null) runtime.EncodeArray(def.DataType, arrayElements); + else runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); status = runtime.GetStatus(); } @@ -1436,7 +1476,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[i] = new WriteResult(refused); WarnTypeMismatchOnce(def.Name, "AbCip write to {Tag} on device {Device} refused before sending: the controller holds {Size} byte(s) there but the authored {DataType} encodes {Width} (logged once per tag)", - w.FullReference, def.DeviceHostAddress, size, def.DataType, ExpectedScalarWriteWidth(def.DataType)); + w.FullReference, def.DeviceHostAddress, size, def.DataType, + arrayElements is not null + ? AbCipArrayCodec.ElementWidth(def.DataType) * arrayElements.Length + : ExpectedScalarWriteWidth(def.DataType)); } else if (status == (int)AbStatus.TypeMismatch) { @@ -2055,6 +2098,20 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, : AbCipStatusMapper.BadTypeMismatch; } + /// + /// The refusal for a whole-array write whose authored shape does not fit what the controller holds: the last + /// read returned bytes, which must be the element width × the element count. Unknown + /// size (<= 0) is not judged here (the native runtime re-checks against the controller's type code). + /// + /// The array tag being written. + /// The runtime's read-buffer size in bytes. + /// BadTypeMismatch, or null to write. + internal static uint? ArrayWriteWidthRefusal(AbCipTagDefinition def, int size) + { + if (size <= 0 || AbCipArrayCodec.ElementWidth(def.DataType) is not { } width) return null; + return size == width * Math.Max(1, def.ElementCount) ? null : AbCipStatusMapper.BadTypeMismatch; + } + /// /// Recursively fan a controller-discovered UDT shape out into Variable nodes — one per /// ATOMIC leaf — under the folder produced by . Mirrors the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs index 1cf3c2e5..ad8b00a2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs @@ -85,6 +85,17 @@ public interface IAbCipTagRuntime : IDisposable /// Bit index for BOOL-within-DINT insertion, or null. /// Value to encode. void EncodeValue(AbCipDataType type, int? bitIndex, object? value); + + /// + /// Encode a WHOLE-array write of (values.Count elements of + /// , at the element width — see ) into the local buffer. + /// Callers pair this with . A runtime that cannot write arrays throws + /// . + /// + /// The array element type. + /// The element values; already length-checked against the tag's element count. + void EncodeArray(AbCipDataType type, IReadOnlyList values) => + throw new NotSupportedException($"{GetType().Name} does not support array writes."); } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index 2e6cd7fc..07ac008e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -235,6 +235,23 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime _ => 4, }; + /// + /// + /// The codec's bytes are copied into the buffer the last read sized (elem_count elements), so the write + /// sends exactly the new array — never a stale tail. A buffer smaller than the encoded array is refused + /// () before anything is changed. + /// + public void EncodeArray(AbCipDataType type, IReadOnlyList values) => + LibplctagStatusTranslation.WrapSync(() => + { + var bytes = AbCipArrayCodec.Encode(type, values); + var size = _tag.GetSize(); + if (size < bytes.Length) + throw new AbTransportException(AbStatus.TooSmall, + $"Array write to '{_tag.Name}': {bytes.Length} byte(s) encoded but the tag buffer holds {size}."); + for (var i = 0; i < bytes.Length; i++) _tag.SetUInt8(i, bytes[i]); + }); + /// public void EncodeValue(AbCipDataType type, int? bitIndex, object? value) => LibplctagStatusTranslation.WrapSync(() => EncodeValueCore(type, bitIndex, value)); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index 917f205b..a77179f9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -55,6 +55,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private AbStatus _status = AbStatus.Pending; private byte[]? _writeBuffer; + private ushort _writeElements = 1; private string? _pendingString; private bool _disposed; @@ -233,17 +234,18 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime // Width is checked only for codes with a known width: DATE / DATE_AND_TIME (0xCD/0xCF) and // Micro800 BYTE/WORD/LWORD (0xD1/0xD2/0xD4) are outside LogixTypeCode's table and write as today. var width = LogixTypeCode.ByteWidth(type); - if (width > 0 && width != buffer.Length) + if (width > 0 && width * _writeElements != buffer.Length) { // An authored BOOL over a packed BOOL-array DWORD: writing the DWORD would clobber the // 31 sibling bools (libplctag does exactly that), so refuse it as a config error. - if (type == LogixTypeCode.BoolArray) + if (type == LogixTypeCode.BoolArray && _writeElements == 1) throw new NotSupportedException( $"Write to '{_p.TagName}': a BOOL-array element (type 0xD3 DWORD) cannot be written as a single BOOL."); throw new AbTransportException(AbStatus.TypeMismatch, - $"Write to '{_p.TagName}': encoded {buffer.Length} byte(s) but the controller's type 0x{type:X4} is {width} byte(s) wide."); + $"Write to '{_p.TagName}': encoded {buffer.Length} byte(s) for {_writeElements} element(s) but the controller's type 0x{type:X4} is {width} byte(s) wide."); } - await client.WriteAsync(path, type, null, 1, buffer, cancellationToken).ConfigureAwait(false); + // U10: an array write is ONE Write Tag of _writeElements elements (the client fragments it if large). + await client.WriteAsync(path, type, null, _writeElements, buffer, cancellationToken).ConfigureAwait(false); } else { @@ -327,10 +329,22 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime }; } + /// + public void EncodeArray(AbCipDataType type, IReadOnlyList values) + { + ArgumentNullException.ThrowIfNull(values); + _writeBuffer = null; + _pendingString = null; + var buffer = AbCipArrayCodec.Encode(type, values); // throws (nothing staged) before any state changes + _writeElements = (ushort)Math.Clamp(values.Count, 1, ushort.MaxValue); + _writeBuffer = buffer; + } + /// public void EncodeValue(AbCipDataType type, int? bitIndex, object? value) { _writeBuffer = null; + _writeElements = 1; _pendingString = null; switch (type) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs new file mode 100644 index 00000000..ea908d12 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs @@ -0,0 +1,178 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 U10: array writes. Config and the AdminUI allowed them and discovery advertised array nodes +/// Operate-writable, but both runtimes encoded a SCALAR: an int[] threw InvalidCastException (BadTypeMismatch +/// AND Degraded), a scalar wrote element 0 natively, and libplctag re-sent the whole last-read buffer with only +/// element 0 patched. The driver now encodes count elements at the element width, refuses a length mismatch +/// with BadOutOfRange, and converts element values (ScadaBridge writes int[] to INT arrays). +/// +[Trait("Category", "Unit")] +public sealed class AbCipArrayWriteTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private static async Task<(AbCipDriver Driver, FakeAbCipTagFactory Factory)> BuildAsync( + AbCipDataType type, int count, int size = -1) + { + var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Size = size } }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Arr", Device, "Arr", type, ElementCount: count, IsArray: true)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-array-write", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory); + } + + // ---- the codec ---- + + /// Each element is converted and laid out little-endian at the element width. + [Fact] + public void The_codec_converts_elements_and_lays_them_out_at_the_element_width() + { + AbCipArrayCodec.Encode(AbCipDataType.Int, [1, -2, 300]).ShouldBe(Convert.FromHexString("0100FEFF2C01")); + AbCipArrayCodec.Encode(AbCipDataType.SInt, [(short)-1, 5L]).ShouldBe(Convert.FromHexString("FF05")); + AbCipArrayCodec.Encode(AbCipDataType.DInt, [1.0, 2]).ShouldBe(Convert.FromHexString("0100000002000000")); + AbCipArrayCodec.Encode(AbCipDataType.Real, [1, 2.5]).ShouldBe(Convert.FromHexString("0000803F00002040")); + AbCipArrayCodec.Encode(AbCipDataType.ULInt, [ulong.MaxValue]).ShouldBe(Convert.FromHexString("FFFFFFFFFFFFFFFF")); + Should.Throw(() => AbCipArrayCodec.Encode(AbCipDataType.Int, [40000])); + Should.Throw(() => AbCipArrayCodec.Encode(AbCipDataType.Bool, [true])); + } + + // ---- the driver ---- + + /// ScadaBridge's wire shape: an int[] to an INT[3] encodes 3 two-byte elements and writes once, Good. + [Fact] + public async Task An_int_array_to_an_INT_array_encodes_every_element() + { + var (drv, factory) = await BuildAsync(AbCipDataType.Int, 3, size: 6); + + var r = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, 2, 3 })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var tag = factory.Tags["Arr"]; + tag.EncodedArrayType.ShouldBe(AbCipDataType.Int); + tag.EncodedArrayCount.ShouldBe(3); + tag.EncodedArrayBytes.ShouldBe(Convert.FromHexString("010002000300")); + tag.WriteCount.ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A list of a different length is BadOutOfRange; nothing is created, encoded or written. + /// The client's element count. + [Theory] + [InlineData(2)] + [InlineData(4)] + public async Task A_length_mismatch_is_BadOutOfRange_and_nothing_is_written(int length) + { + var (drv, factory) = await BuildAsync(AbCipDataType.DInt, 3); + + var r = await drv.WriteAsync([new WriteRequest("Arr", Enumerable.Range(0, length).ToArray())], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + factory.Tags.ShouldNotContainKey("Arr"); // refused before any runtime exists + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// A scalar (or a string) to an array tag is BadTypeMismatch — it used to write element 0 only. + [Fact] + public async Task A_scalar_to_an_array_tag_is_BadTypeMismatch() + { + var (drv, factory) = await BuildAsync(AbCipDataType.DInt, 3); + + var r = await drv.WriteAsync( + [new WriteRequest("Arr", 5), new WriteRequest("Arr", "1,2,3")], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + factory.Tags.ShouldNotContainKey("Arr"); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// An element that does not fit is BadOutOfRange with no health write and nothing sent. + [Fact] + public async Task An_element_overflow_is_BadOutOfRange_and_nothing_is_sent() + { + var (drv, factory) = await BuildAsync(AbCipDataType.Int, 2, size: 4); + + var r = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, 70000 })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + factory.Tags["Arr"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// BOOL / STRING arrays are refused BadNotSupported before any runtime exists. + /// The array element type. + [Theory] + [InlineData(AbCipDataType.Bool)] + [InlineData(AbCipDataType.String)] + public async Task Unencodable_element_types_are_BadNotSupported(AbCipDataType type) + { + var (drv, factory) = await BuildAsync(type, 2); + + var r = await drv.WriteAsync([new WriteRequest("Arr", new object[] { true, false })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + factory.Tags.ShouldNotContainKey("Arr"); + } + + /// The controller holding a different shape than authored (size ≠ width × count) is BadTypeMismatch, nothing sent. + [Fact] + public async Task A_controller_shape_mismatch_is_BadTypeMismatch_and_nothing_is_sent() + { + var (drv, factory) = await BuildAsync(AbCipDataType.Int, 3, size: 12); // DINT[3] on the controller + + var r = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, 2, 3 })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + factory.Tags["Arr"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// End to end over the REAL native runtime: an int[] to an INT[3] goes out as ONE Write Tag of type + /// 0xC3 with element count 3 and the three 2-byte elements. + /// + [Fact] + public async Task A_native_array_write_is_one_write_of_count_elements() + { + await using var target = new FakeCipTarget(); + target.ConnectedHandler = req => Task.FromResult(req[0] switch + { + 0x52 => [0xD2, 0x00, 0x00, 0x00, 0xC3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00], // INT[3] = 0,0,0 + 0x4D => [0xCD, 0x00, 0x00, 0x00], + _ => target.ServiceReply(req), + }); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Arr", host, "Arr", AbCipDataType.Int, ElementCount: 3, IsArray: true)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "abcip-array-native", new NativeAbCipTagFactory(new DeviceSessionPool())); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + var r = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, -2, 300 })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var write = target.ConnectedCips.Single(c => c[0] == 0x4D); + var typeAt = 2 + write[1] * 2; + write.AsSpan(typeAt).ToArray().ShouldBe(Convert.FromHexString("C300" + "0300" + "0100FEFF2C01")); + } + finally + { + await drv.DisposeAsync(); + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs index 740e0985..a95b8822 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/FakeAbCipTag.cs @@ -115,6 +115,25 @@ internal class FakeAbCipTag : IAbCipTagRuntime /// The value to encode. public virtual void EncodeValue(AbCipDataType type, int? bitIndex, object? value) => Value = value; + /// The element type of the last , if any. + public AbCipDataType? EncodedArrayType { get; private set; } + + /// The bytes the last produced (the shared codec, so conversions throw as in production). + public byte[]? EncodedArrayBytes { get; private set; } + + /// The element count of the last . + public int EncodedArrayCount { get; private set; } + + /// Encodes an array write through the production codec and records it. + /// The element type. + /// The element values. + public virtual void EncodeArray(AbCipDataType type, IReadOnlyList values) + { + EncodedArrayBytes = AbCipArrayCodec.Encode(type, values); + EncodedArrayType = type; + EncodedArrayCount = values.Count; + } + /// Marks the tag as disposed. public virtual void Dispose() => Disposed = true; } From fc94b876341dde05c1cff42de274fb4fd04d874c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:31:32 -0400 Subject: [PATCH 10/15] perf(abcip): cache the declaration-only UDT layout per member list P11: AbCipUdtReadPlanner called AbCipUdtMemberLayout.TryBuild(parent.Members) for every member reference on every poll - O(members^2) per parent per tick, a dictionary allocation each time. GetOrBuild caches the layout (including a "not groupable" verdict) in a ConditionalWeakTable keyed by the member list's identity, which is immutable for the life of the tag table; a re-init's new lists replace the entries and the old ones are collected with them. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipUdtMemberLayout.cs | 24 +++++++++++++++++++ .../AbCipUdtReadPlanner.cs | 2 +- .../AbCipUdtMemberLayoutTests.cs | 22 +++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs index b4b271f3..3c44f80f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs @@ -25,6 +25,30 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// public static class AbCipUdtMemberLayout { + /// Layouts already computed, keyed by the parent definition's member list (reference identity). + private static readonly System.Runtime.CompilerServices.ConditionalWeakTable, LayoutBox> Cache = new(); + + /// Holds a computed layout, including a "not groupable" (null) verdict. + private sealed class LayoutBox(IReadOnlyDictionary? offsets) + { + public IReadOnlyDictionary? Offsets { get; } = offsets; + } + + /// + /// , computed once per member list (review 07 P11: the read planner rebuilt the layout — + /// a dictionary allocation and an O(members) walk — for every member reference on every poll, O(members²) per + /// parent per tick). A parent definition's member list is immutable for the life of the tag table that holds + /// it, so caching by list identity is exact; a re-init builds new lists and the old entries are collected with + /// them (). + /// + /// The parent definition's declared members. + /// The member offsets, or null when the members cannot be laid out by declaration. + public static IReadOnlyDictionary? GetOrBuild(IReadOnlyList members) + { + ArgumentNullException.ThrowIfNull(members); + return Cache.GetValue(members, m => new LayoutBox(TryBuild(m))).Offsets; + } + /// /// Try to compute member offsets for the supplied declared members. Returns null /// if any member type is unsupported for declaration-only layout. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs index 28bc181f..338d905e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs @@ -67,7 +67,7 @@ public static class AbCipUdtReadPlanner continue; } - var offsets = AbCipUdtMemberLayout.TryBuild(parent.Members); + var offsets = AbCipUdtMemberLayout.GetOrBuild(parent.Members); if (offsets is null || !offsets.TryGetValue(memberName, out var offset)) { fallback.Add(new AbCipUdtReadFallback(i, name)); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs index b895b2cc..4841c4f6 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs @@ -99,4 +99,26 @@ public sealed class AbCipUdtMemberLayoutTests new AbCipStructureMember("Setpoints", AbCipDataType.Real, ElementCount: 4), }).ShouldBeNull(); } + + /// Review 07 P11: the layout is computed once per member list, not once per member per poll. + [Fact] + public void GetOrBuild_computes_a_layout_once_per_member_list() + { + IReadOnlyList members = + [ + new AbCipStructureMember("A", AbCipDataType.DInt), + new AbCipStructureMember("B", AbCipDataType.Real), + ]; + var first = AbCipUdtMemberLayout.GetOrBuild(members); + first.ShouldNotBeNull(); + first["B"].ShouldBe(4); + AbCipUdtMemberLayout.GetOrBuild(members).ShouldBeSameAs(first); + + // Positive control: a different (equal-content) list is its own entry, and a not-groupable verdict caches too. + IReadOnlyList copy = [.. members]; + AbCipUdtMemberLayout.GetOrBuild(copy).ShouldNotBeSameAs(first); + IReadOnlyList withBool = [new AbCipStructureMember("F", AbCipDataType.Bool)]; + AbCipUdtMemberLayout.GetOrBuild(withBool).ShouldBeNull(); + AbCipUdtMemberLayout.GetOrBuild(withBool).ShouldBeNull(); + } } From aabe027d10c963d3ddd60ed464c66c29bf6b38b2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:32:31 -0400 Subject: [PATCH 11/15] test(abcip): native end-to-end proof a NotFound tag causes no lease churn L4 evidence over the real native runtime and a fake CIP target: a renamed tag that is the device's only lease holder is created once and Forward-Opened once across five polls, with no Forward Close, every poll BadNodeIdUnknown and the driver Healthy. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDeviceRefusalTests.cs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs index 2cb690f5..6803d3ab 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs @@ -2,6 +2,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; @@ -178,4 +179,50 @@ public sealed class AbCipDeviceRefusalTests factory.Tags["ReadOnlyTag"].Disposed.ShouldBeFalse(); drv.GetHealth().State.ShouldBe(DriverState.Healthy); } + + /// + /// L4 end to end over the REAL native runtime + a private pool: a renamed tag (CIP 0x04) that is the device's + /// ONLY lease holder used to be evicted every poll — the last release, so Forward Close, then a throttled Forward + /// Open on the next poll. One runtime and one Forward Open across N polls and no Forward Close proves no churn. + /// + [Fact] + public async Task A_native_NotFound_tag_causes_no_runtime_or_connection_churn() + { + await using var target = new FakeCipTarget(); + target.ConnectedHandler = req => Task.FromResult( + req[0] == 0x52 ? [0xD2, 0x00, 0x04, 0x00] : target.ServiceReply(req)); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var creates = 0; + var native = new NativeAbCipTagFactory(new DeviceSessionPool()); + var factory = new CountingFactory(p => { Interlocked.Increment(ref creates); return native.Create(p); }); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Renamed", host, "Renamed", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "abcip-l4-native", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + for (var n = 0; n < 5; n++) + (await drv.ReadAsync(["Renamed"], CancellationToken.None))[0].StatusCode + .ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + + Volatile.Read(ref creates).ShouldBe(1); + target.ForwardOpens.ShouldBe(1); + target.ForwardCloses.ShouldBe(0); + target.ConnectedRequests.ShouldBeGreaterThanOrEqualTo(5); // positive control: every poll hit the wire + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await drv.DisposeAsync(); + } + } + + private sealed class CountingFactory(Func create) : IAbCipTagFactory + { + public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams); + } } From 8a941de4cd79e86957e00a44d73768d24dbaebfd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:34:53 -0400 Subject: [PATCH 12/15] fix(abcip): keep a sync Dispose, but one that never blocks on the teardown F10 follow-through: removing IDisposable outright broke a synchronous drv.Dispose() call site outside this lane (Runtime.Tests AbCipAlmaNativeAlarmEndToEndTests). The sync Dispose() is back, but it starts the same teardown as DisposeAsync and returns instead of parking the caller on DisposeAsync().AsTask().GetAwaiter().GetResult(); a background fault is logged. The runtime still takes the IAsyncDisposable path (preferred, never both). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 22 ++++++++++++-- .../AbCipLifecycleConcurrencyTests.cs | 29 ++++++++++++++++--- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 43afe57f..8e19daed 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -27,7 +27,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// handles GC-collected tags. /// public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, IBatchSubscribable, - IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, ISkippedTagReporter, IAsyncDisposable + IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, ISkippedTagReporter, IAsyncDisposable, IDisposable { /// Authored tags this driver could not build into its poll table. private readonly SkippedTagLedger _skippedTags = new(); @@ -2268,8 +2268,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// Asynchronously releases all resources used by the driver — the same complete teardown as /// . Idempotent, and safe CONCURRENTLY with a still-running /// (the runtime's final stop abandons a shutdown that overruns its budget and - /// disposes anyway): each teardown retires a disjoint device table. There is deliberately no synchronous - /// Dispose() — it could only block on this teardown (sync-over-async). + /// disposes anyway): each teardown retires a disjoint device table. This is the dispose the runtime calls + /// (it prefers ). /// /// A value task representing the asynchronous disposal. public async ValueTask DisposeAsync() @@ -2277,6 +2277,22 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); } + /// + /// Starts the same teardown as WITHOUT blocking the calling thread on it (review + /// 07 F10: the old DisposeAsync().AsTask().GetAwaiter().GetResult() parked the caller — a dispatcher + /// thread — for up to the probe join, with the usual sync-over-async deadlock exposure). The teardown runs to + /// completion in the background and any fault is logged. Callers that need completion use + /// ; kept only for synchronous using / Dispose() call sites. + /// + public void Dispose() + { + var teardown = DisposeAsync().AsTask(); + if (teardown.IsCompleted) { teardown.GetAwaiter().GetResult(); return; } + _ = teardown.ContinueWith( + t => _logger.LogWarning(t.Exception, "AbCip driver {DriverInstanceId}: background dispose failed", _driverInstanceId), + CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default); + } + /// /// Per-device runtime state. Holds the parsed host address, family profile, and the /// live libplctag.NET instances keyed by tag name. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs index eea409d2..2b009a7f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs @@ -175,12 +175,10 @@ public sealed class AbCipLifecycleConcurrencyTests created.Where(t => !t.Disposed).ShouldBeEmpty("every runtime a reader created must be disposed by some teardown"); } - /// F10: the driver exposes only an async dispose; a sync Dispose() that blocked on the async - /// teardown (sync-over-async) is gone, and the runtime takes the path. + /// F10: DisposeAsync is the complete, idempotent teardown (the runtime prefers it). [Fact] - public async Task Driver_is_async_disposable_only_and_DisposeAsync_stops_the_probe() + public async Task DisposeAsync_is_a_complete_idempotent_teardown() { - typeof(IDisposable).IsAssignableFrom(typeof(AbCipDriver)).ShouldBeFalse(); var (drv, factory) = Build(probeReadDelay: TimeSpan.Zero); await drv.InitializeAsync("{}", CancellationToken.None); await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); @@ -192,4 +190,27 @@ public sealed class AbCipLifecycleConcurrencyTests loop.IsCompleted.ShouldBeTrue(); factory.Tags["ProbeTag"].Disposed.ShouldBeTrue(); } + + /// + /// F10: the sync Dispose() no longer blocks the caller's thread on the async teardown (sync-over-async — + /// a dispatcher thread for up to the probe join). It starts the same teardown and returns; the teardown still + /// completes. + /// + [Fact] + public async Task Sync_Dispose_does_not_block_on_the_teardown_but_still_completes_it() + { + var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(500)); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); + var loop = drv.GetDeviceState(Host)!.ProbeTask!; + await Task.Delay(20); // the probe is inside its slow, cancellation-deaf read + + var sw = System.Diagnostics.Stopwatch.StartNew(); + ((IDisposable)drv).Dispose(); + sw.Stop(); + + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(250)); // did not wait out the 500 ms probe read + await WaitUntilAsync(() => loop.IsCompleted && factory.Tags["ProbeTag"].Disposed, "teardown never completed"); + drv.GetDeviceState(Host).ShouldBeNull(); + } } From 6b863cd524a0d778061be596e889539362ddef99 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:37:18 -0400 Subject: [PATCH 13/15] test(abcip): ab_server cases for the probe blackout, refusals, array writes Integration cases (both transports, skipped when the fixture is unreachable): - H10: a typo'd probe tag leaves the host Running and a cold data tag created after many probe ticks still reads Good; - H1/L4: a missing tag reads BadNodeIdUnknown every poll beside a Good sibling, the driver stays Healthy and LastSuccessfulRead advances even when only the refused tag is read; - U10 (AB_SERVER_ALLOW_WRITES=1): an int[16] written to TestArray (DINT[16]) reads back element for element through a fresh driver; a 15-element write is BadOutOfRange. Not run here: the 10.100.0.35:44818 fixture was down during this lane (Wave 4). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipRefusalLiveTests.cs | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipRefusalLiveTests.cs diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipRefusalLiveTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipRefusalLiveTests.cs new file mode 100644 index 00000000..c5aebf8d --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/AbCipRefusalLiveTests.cs @@ -0,0 +1,172 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests; + +/// +/// Review 07 (Gitea #737) against a running ab_server on both transports: a controller refusal is a per-tag +/// config error, not a comms fault (H10 probe, H1/L4 data path), and a whole-array write round-trips (U10). +/// Skipped when the fixture is unreachable (); the array write additionally +/// needs AB_SERVER_ALLOW_WRITES=1 (). +/// +[Trait("Category", "Integration")] +[Trait("Requires", "AbServer")] +public sealed class AbCipRefusalLiveTests +{ + private static string Device(AbServerFixture fixture) => $"ab://{fixture.Host}:{fixture.Port}/1,0"; + + private static async Task WaitUntilAsync(Func condition, TimeSpan timeout, string what) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException(what); + await Task.Delay(50); + } + } + + /// + /// H10: a typo'd probe tag on a reachable controller leaves the host Running, and a COLD data tag created + /// after many probe ticks still connects (the backoff window was never opened by the probe's NotFound). + /// + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_typod_probe_tag_does_not_black_out_a_reachable_device(AbCipTransport transport) + { + var device = Device(new AbServerFixture()); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(device, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Counter", device, "TestDINT", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions + { + Enabled = true, + ProbeTagPath = "NoSuchProbeTag_xyz", + Interval = TimeSpan.FromMilliseconds(200), + Timeout = TimeSpan.FromSeconds(2), + }, + Timeout = TimeSpan.FromSeconds(5), + Transport = transport, + }, $"drv-h10-{transport}"); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + await WaitUntilAsync(() => drv.GetHostStatuses().Single().State == HostState.Running, + TimeSpan.FromSeconds(10), "the host never went Running on a refused probe tag"); + await Task.Delay(1_000); // several probe ticks — each one used to record a Forward-Open failure + + var r = await drv.ReadAsync(["Counter"], CancellationToken.None); // a cold create + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + drv.GetHostStatuses().Single().State.ShouldBe(HostState.Running); + } + finally + { + await drv.DisposeAsync(); + } + } + + /// + /// H1/L4: a tag the controller does not have reads BadNodeIdUnknown every poll while a sibling reads Good, and + /// the driver stays Healthy with LastSuccessfulRead advancing (it used to latch Degraded and, with no other + /// tag, trip the comms-loss watchdog against a reachable controller). + /// + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_missing_tag_is_per_tag_Bad_and_never_degrades_the_driver(AbCipTransport transport) + { + var device = Device(new AbServerFixture()); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(device, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Typo", device, "NoSuchTag_xyz", AbCipDataType.DInt), + new AbCipTagDefinition("Counter", device, "TestDINT", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + Transport = transport, + }, $"drv-h1-{transport}"); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + DateTime? last = null; + for (var i = 0; i < 3; i++) + { + var r = await drv.ReadAsync(["Typo", "Counter"], CancellationToken.None); + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + if (last is { } previous) health.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(previous); + last = health.LastSuccessfulRead; + } + + // Only the refused tag: still Healthy, still answering (the watchdog conjunction can never trip). + var before = drv.GetHealth().LastSuccessfulRead!.Value; + await Task.Delay(20); + (await drv.ReadAsync(["Typo"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(before); + } + finally + { + await drv.DisposeAsync(); + } + } + + /// + /// U10: a whole int[] written to the 16-element TestArray (DINT[16]) reads back element for element + /// through a fresh driver; a wrong-length write is refused BadOutOfRange and changes nothing. + /// + /// The transport stack the driver runs on. + [AbServerTheory] + [MemberData(nameof(TransportMatrix.Transports), MemberType = typeof(TransportMatrix))] + public async Task A_whole_array_write_round_trips(AbCipTransport transport) + { + IntegrationWriteGate.SkipUnlessWritesAllowed(); + var device = Device(new AbServerFixture()); + var seed = Random.Shared.Next(1_000, 1_000_000); + var values = Enumerable.Range(0, 16).Select(i => seed + i * 7).ToArray(); + + AbCipDriver Make(string id) => new(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(device, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Arr", device, "TestArray", AbCipDataType.DInt, ElementCount: 16, IsArray: true)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + Transport = transport, + }, $"drv-u10-{id}-{transport}"); + + var writer = Make("writer"); + await writer.InitializeAsync("{}", CancellationToken.None); + try + { + var w = await writer.WriteAsync([new WriteRequest("Arr", values)], CancellationToken.None); + w[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var bad = await writer.WriteAsync([new WriteRequest("Arr", values.Take(15).ToArray())], CancellationToken.None); + bad[0].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + writer.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await writer.DisposeAsync(); + } + + var reader = Make("reader"); + await reader.InitializeAsync("{}", CancellationToken.None); + try + { + var r = await reader.ReadAsync(["Arr"], CancellationToken.None); + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + r[0].Value.ShouldBe(values); + } + finally + { + await reader.DisposeAsync(); + } + } +} From 459cbb444ee2f5387bc24595239f78c79cf82b5f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:53:06 -0400 Subject: [PATCH 14/15] fix(abcip): review fix round 1 - BOOL-array fallback, local refusals, minors I1: the U11 Bits[n] -> Bits[n/32] rewrite was unconditional. The native runtime now keeps the original element path until the first reply settles the layout: 0xD3 confirms DWORD addressing (bit n%32); any other reply type (e.g. a 0xC1 per-element BOOL array) or a refusal of the DWORD index falls back to Bits[n] and re-reads, and a write settles the layout by a read first, so it can never decode or write element n/32 of an element-addressed array. I2: libplctag ErrorNoMatch/ErrorBadDevice no longer fold into NotFound (they now read as BadConnection: comms, Degraded, LastSuccessfulRead preserved). ErrorNotImplemented - and the native runtime's own pre-send refusals - raise AbCipLocalRefusalException: BadNotSupported, no health, LastSuccessfulRead NOT advanced, no backoff record, not reachable for the probe or Test Connect. M1: the bit RMW's parent read/write status words follow the health contract (refusal: per-tag Bad + Warning; comms: evict the parent + Degraded). M2: device Unsupported read refusals return Refused (advance LastSuccessfulRead); device write refusals call RecordWriteAnswered (not the pre-send native type gate). M3: a good write clears a Degraded a write comms fault recorded (a read-side Degraded is still left for the read path). M4: a superseded init throws AbCipInitializationSupersededException (not an OCE); ShutdownAsync stamps Unknown only if no newer init published meanwhile. M5: the sync-Dispose timing assert is 2 s against a 4 s probe read; the unencodable-array test asserts health. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 126 ++++++++++++++---- .../AbCipDriverProbe.cs | 3 +- .../AbCipInitializationSupersededException.cs | 11 ++ .../AbCipLocalRefusalException.cs | 15 +++ .../AbCipRefusals.cs | 4 +- .../LibplctagStatusTranslation.cs | 28 +++- .../NativeAbCipTagRuntime.cs | 43 +++++- .../AbCipArrayWriteTests.cs | 1 + .../AbCipBitInWordWidthTests.cs | 34 +++++ .../AbCipDeviceRefusalTests.cs | 70 ++++++++++ .../AbCipLifecycleConcurrencyTests.cs | 37 ++++- .../AbCipLocalRefusalTests.cs | 7 +- .../AbCipStatusMapperTests.cs | 2 +- .../AbCipWriteHealthTests.cs | 18 +++ .../LibplctagStatusTranslationTests.cs | 24 +++- .../NativeAbCipTagRuntimeTests.cs | 24 ++++ 16 files changed, 395 insertions(+), 52 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipLocalRefusalException.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 8e19daed..10b7a813 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -515,7 +515,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, lock (_lifecycleLock) { if (generation != _lifecycleGeneration) - throw new OperationCanceledException( + throw new AbCipInitializationSupersededException( $"AbCip driver {_driverInstanceId} initialization was superseded by a shutdown or a newer initialization."); _options = options; @@ -562,7 +562,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Never-published states hold no probe loop and no runtime yet; dispose for symmetry. if (builtDevices is not null) foreach (var state in builtDevices.Values) state.Retire(); - if (ex is OperationCanceledException) + if (ex is AbCipInitializationSupersededException) { _logger.LogInformation("AbCip driver {DriverInstanceId} initialization abandoned: {Reason}", _driverInstanceId, ex.Message); @@ -652,9 +652,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, public async Task ShutdownAsync(CancellationToken cancellationToken) { await _poll.DisposeAsync().ConfigureAwait(false); - await ReleaseDeviceStatesAsync().ConfigureAwait(false); + var generation = await ReleaseDeviceStatesAsync().ConfigureAwait(false); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - UpdateHealth(h => new DriverHealth(DriverState.Unknown, h.LastSuccessfulRead, null)); + lock (_lifecycleLock) + { + // M4: a newer InitializeAsync that published while this shutdown was still joining its probe loops owns the + // health now — stamping Unknown over its Healthy would misreport a live driver. + if (generation == _lifecycleGeneration) + UpdateHealth(h => new DriverHealth(DriverState.Unknown, h.LastSuccessfulRead, null)); + } } // ---- ISubscribable (polling overlay via shared engine) ---- @@ -764,10 +770,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, probeRuntime = null; } } - catch (AbCipTagConfigurationException ace) + catch (Exception ace) when (ace is AbCipTagConfigurationException or AbCipLocalRefusalException) { - // The probe tag's own name cannot become a request — it never reached the device, so it is no evidence - // either way: no backoff signal, no host transition, one Warning. + // The probe tag's own name cannot become a request (or the library refused it locally) — it never + // reached the device, so it is no evidence either way: no backoff signal, no host transition, one Warning. try { probeRuntime?.Dispose(); } catch { } probeRuntime = null; state.ProbeInitialized = false; @@ -1145,13 +1151,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { throw; } + catch (AbCipLocalRefusalException local) + { + // #737 I2: refused by the library / driver itself — no I/O, so no proof the device answered: BadNotSupported, + // keep the handle, no health signal, LastSuccessfulRead untouched. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNotSupported, null, now); + WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, local.Message); + return ReadOutcome.None; + } catch (AbTransportException ate) when (ate.Status == AbStatus.Unsupported) { - // A deterministic local refusal — not a comms fault: BadNotSupported, keep the handle, no - // health signal. + // The DEVICE refused (CIP 0x08, or a decode refusal of data it returned) — not a comms fault: + // BadNotSupported, keep the handle, no state change; it answered, so LastSuccessfulRead advances (M2). results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNotSupported, null, now); WarnRefusalOnce(def.Name, "read", reference, def.DeviceHostAddress, ate.Message); - return ReadOutcome.None; + return ReadOutcome.Refused; } catch (AbCipTagConfigurationException ace) { @@ -1169,7 +1183,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, reference, def.DeviceHostAddress, ate.Message); return ReadOutcome.Refused; } - catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate.Status)) + catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate)) { // H1/L4: the controller answered "no" (renamed tag, Arr[999], read-protected …) — a per-tag config // error, not a comms fault. Keep the handle (no evict → no lease / Forward-Open churn), no Degraded, @@ -1284,10 +1298,17 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { throw; } + catch (AbCipLocalRefusalException local) + { + StampGroupStatus(group, results, now, AbCipStatusMapper.BadNotSupported); + WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, local.Message); + return ReadOutcome.None; + } catch (AbTransportException ate) when (ate.Status == AbStatus.Unsupported) { StampGroupStatus(group, results, now, AbCipStatusMapper.BadNotSupported); - return ReadOutcome.None; + WarnRefusalOnce(parent.Name, "whole-UDT read", group.ParentName, parent.DeviceHostAddress, ate.Message); + return ReadOutcome.Refused; } catch (AbCipTagConfigurationException ace) { @@ -1485,14 +1506,17 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { // Final review I4: deterministic — keep the handle, leave health, warn once per tag. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); + RecordWriteAnswered(now); WarnTypeMismatchOnce(def.Name, "AbCip write to {Tag} on device {Device} refused: the controller's type does not match the authored {DataType} (logged once per tag)", w.FullReference, def.DeviceHostAddress, def.DataType); } else if (AbCipRefusals.IsDeviceRefusal(status)) { - // The controller refused this write (read-only, out of bounds …): keep the handle, no health. + // The controller refused this write (read-only, out of bounds …): keep the handle, no state change; + // it answered, so LastSuccessfulRead advances (M2). results[i] = new WriteResult(AbCipStatusMapper.MapStatus(status)); + RecordWriteAnswered(now); WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, ((AbStatus)status).ToString()); } else if (status != 0) @@ -1550,14 +1574,20 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write to {Tag} on device {Device}: the value is out of range for {DataType}: {Reason} (logged once per tag)", w.FullReference, def.DeviceHostAddress, def.DataType, oe.Message); } + catch (AbCipLocalRefusalException local) + { + // Refused locally (e.g. the native runtime cannot type the write; libplctag NotImplemented) — nothing + // sent: BadNotSupported, keep the handle, leave health alone. + results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); + WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, local.Message); + } catch (AbTransportException ate) when (ate.Status == AbStatus.Unsupported) { - // A deterministic local refusal (e.g. the native runtime cannot type the write) — not a comms - // fault: BadNotSupported, keep the handle, leave health alone. + // The device refused the write as unsupported — BadNotSupported, keep the handle, no state change; + // it answered, so LastSuccessfulRead advances (M2). results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported); - _logger.LogWarning(ate, - "AbCip write refused (not supported) for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + RecordWriteAnswered(now); + WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, ate.Message); } catch (AbCipTagConfigurationException ace) { @@ -1570,15 +1600,18 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // Final review I4: the controller's type disagrees with the authored one (native type gate, or // CIP 0xFF/0x2107 from the controller) — deterministic: BadTypeMismatch, keep the handle, leave // health alone, warn once per tag. Retrying cannot succeed until the config is corrected. + // No LastSuccessfulRead advance: the native type gate throws this BEFORE sending. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); WarnTypeMismatchOnce(def.Name, "AbCip write to {Tag} on device {Device} refused: {Reason} (logged once per tag)", w.FullReference, def.DeviceHostAddress, ate.Message); } - catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate.Status)) + catch (AbTransportException ate) when (AbCipRefusals.IsDeviceRefusal(ate)) { - // H1: the controller answered "no" to this write — keep the handle, leave health alone, warn once. + // H1: the controller answered "no" to this write — keep the handle, no state change, warn once; it + // answered, so LastSuccessfulRead advances (M2). results[i] = new WriteResult(AbCipStatusMapper.MapStatus(ate.Status)); + RecordWriteAnswered(now); WarnRefusalOnce(def.Name, "write", w.FullReference, def.DeviceHostAddress, ate.Message); } catch (Exception) when (device.Retired) @@ -1607,14 +1640,29 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// A good write proves the device answered: advance LastSuccessfulRead but leave the STATE alone. Writing /// Healthy here (H19) overwrote a Degraded the poll loop had just recorded for other tags. /// + /// + /// M3: a Degraded that a WRITE comms fault recorded (still the current health — same error, no read has + /// replaced it since) is cleared back to Healthy: the device just accepted a write. A Degraded the read + /// path recorded is left for the read path to clear. + /// private void RecordWriteAnswered(DateTime now) => - UpdateHealth(h => h with { LastSuccessfulRead = now }); + UpdateHealth(h => h.State == DriverState.Degraded && h.LastError is { } err + && ReferenceEquals(err, Volatile.Read(ref _writeFaultError)) + ? new DriverHealth(DriverState.Healthy, now, null) + : h with { LastSuccessfulRead = now }); + + // The LastError string the most recent write-side comms fault wrote (reference identity marks "write-owned"). + private string? _writeFaultError; /// A write-side comms fault: Degraded (never downgrading Faulted), LastSuccessfulRead preserved. - private void RecordWriteCommsFault(string error) => + private void RecordWriteCommsFault(string error) + { + var owned = new string(error.AsSpan()); // a distinct instance, so reference identity cannot alias a read error + Volatile.Write(ref _writeFaultError, owned); UpdateHealth(h => h.State == DriverState.Faulted ? h - : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, error)); + : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, owned)); + } /// /// Read-modify-write one bit within an integer parent (SINT / INT / DINT / LINT — Logix allows .n on @@ -1636,7 +1684,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var parentRuntime = await EnsureParentRuntimeAsync(device, parentName, ct).ConfigureAwait(false); await parentRuntime.ReadAsync(ct).ConfigureAwait(false); var readStatus = parentRuntime.GetStatus(); - if (readStatus != 0) return AbCipStatusMapper.MapStatus(readStatus); + if (readStatus != 0) return BitWriteStatusWord(device, parentName, readStatus, "read"); // Unsigned at the narrow widths so a set top bit (SINT bit 7, INT bit 15) encodes without overflowing the // signed type; DINT keeps its signed int arithmetic (1 << 31 is int.MinValue, which a DINT encodes as is). @@ -1685,7 +1733,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, var writeStatus = parentRuntime.GetStatus(); return writeStatus == 0 ? AbCipStatusMapper.Good - : AbCipStatusMapper.MapStatus(writeStatus); + : BitWriteStatusWord(device, parentName, writeStatus, "write"); } finally { @@ -1693,6 +1741,28 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } + /// + /// M1: a non-zero status word on the bit write's parent read / write follows the health contract — a device + /// refusal is per-tag Bad with one Warning and no state change (it answered: LastSuccessfulRead advances); any + /// other status is a comms fault: evict the PARENT runtime and write Degraded (LastSuccessfulRead preserved). + /// + private uint BitWriteStatusWord(DeviceState device, string parentName, int status, string phase) + { + var mapped = AbCipStatusMapper.MapStatus(status); + if (AbCipRefusals.IsDeviceRefusal(status)) + { + RecordWriteAnswered(DateTime.UtcNow); + WarnRefusalOnce(parentName, $"bit-write parent {phase}", parentName, device.Options.HostAddress, ((AbStatus)status).ToString()); + return mapped; + } + EvictParentRuntime(device, parentName); + RecordWriteCommsFault($"transport status {status} on the bit-write parent {phase} of {parentName}"); + _logger.LogWarning( + "AbCip bit-write parent {Phase} of {Parent} on device {Device} returned transport status {Status}; evicting the parent runtime", + phase, parentName, device.Options.HostAddress, status); + return mapped; + } + /// /// Get or lazily create a parent-DINT runtime for a parent tag path, cached per-device /// so repeated bit writes against the same DINT share one handle. @@ -1802,10 +1872,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (!device.Runtimes.TryAdd(def.Name, runtime)) runtime.Dispose(); throw; } - catch (AbCipTagConfigurationException) + catch (Exception ex) when (ex is AbCipTagConfigurationException or AbCipLocalRefusalException) { - // The tag's own name cannot become a request — no I/O happened, so it says nothing - // about the device: leave the backoff window exactly as it was. + // The tag's own name cannot become a request, or the library refused the tag locally — no I/O happened, + // so it says nothing about the device: leave the backoff window exactly as it was. runtime.Dispose(); throw; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs index 52bb0f46..b927d758 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriverProbe.cs @@ -206,7 +206,8 @@ public sealed class AbCipDriverProbe : IDriverProbe internal static bool IsReachableException(AbTransportException ex) { ArgumentNullException.ThrowIfNull(ex); - return IsReachableStatus(ex.Status); + // A local refusal (the library / driver refused without any I/O) proves nothing about the controller. + return ex is not AbCipLocalRefusalException && IsReachableStatus(ex.Status); } private static (AbCipHostAddress? parsed, AbCipDeviceOptions? device) ExtractTarget(AbCipDriverOptions opts) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs new file mode 100644 index 00000000..eeb1afef --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs @@ -0,0 +1,11 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +/// +/// Thrown by when a shutdown, dispose or newer initialization retired the +/// driver's tables while this initialization was building, so it published nothing. Deliberately NOT an +/// : nobody cancelled this call, and a caller that treats cancellation as +/// "my own token fired" must not mistake a superseded attempt for one. The superseding operation owns the driver's +/// state; the caller (normally the runtime, which has already abandoned this attempt) can ignore the result. +/// +/// The detail. +public sealed class AbCipInitializationSupersededException(string message) : InvalidOperationException(message); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipLocalRefusalException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipLocalRefusalException.cs new file mode 100644 index 00000000..baa350e1 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipLocalRefusalException.cs @@ -0,0 +1,15 @@ +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +/// +/// A refusal raised LOCALLY — by the driver's own runtime or by libplctag (ErrorNotImplemented) — before or +/// without any reply from the controller. It carries (so it maps to +/// BadNotSupported) but, unlike a device refusal, proves nothing about the device: it keeps the runtime and +/// writes no health, does NOT advance LastSuccessfulRead, records nothing in the connect backoff, and is not +/// "reachable" for the probe or Test Connect (review 07 / Gitea #737 I2). +/// +/// The refusal detail. +/// The underlying exception, if any. +public sealed class AbCipLocalRefusalException(string message, Exception? inner = null) + : AbTransportException(AbStatus.Unsupported, message, inner); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs index fa82e31b..334568a1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs @@ -36,5 +36,7 @@ internal static class AbCipRefusals /// True when is an carrying a device refusal. /// The exception. /// for a deterministic device refusal. - public static bool IsDeviceRefusal(Exception ex) => ex is AbTransportException ate && IsDeviceRefusal(ate.Status); + /// An is never a device refusal: nothing reached the device. + public static bool IsDeviceRefusal(Exception ex) => + ex is AbTransportException ate and not AbCipLocalRefusalException && IsDeviceRefusal(ate.Status); } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs index 37614e2b..002e8c21 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs @@ -14,11 +14,17 @@ internal static class LibplctagStatusTranslation Status.Ok => AbStatus.Ok, Status.Pending => AbStatus.Pending, Status.ErrorTimeout => AbStatus.Timeout, - Status.ErrorNotFound or Status.ErrorNoMatch or Status.ErrorBadDevice => AbStatus.NotFound, + // #737 I2: only ErrorNotFound is the controller saying "no such tag". ErrorNoMatch / ErrorBadDevice describe a + // session or device-level failure (no proof the controller answered this tag), so they are comms-class: with + // the widened refusal classification, folding them into NotFound made a broken session read as "reachable". + Status.ErrorNotFound => AbStatus.NotFound, + Status.ErrorNoMatch or Status.ErrorBadDevice => AbStatus.BadConnection, Status.ErrorNotAllowed => AbStatus.NotAllowed, Status.ErrorOutOfBounds => AbStatus.OutOfBounds, Status.ErrorTooLarge => AbStatus.TooLarge, Status.ErrorTooSmall => AbStatus.TooSmall, + // ErrorNotImplemented is the LIBRARY refusing (no I/O); the exception paths raise it as an + // AbCipLocalRefusalException so it never counts as "the device answered" (see FromException). Status.ErrorUnsupported or Status.ErrorNotImplemented => AbStatus.Unsupported, Status.ErrorPartial => AbStatus.Partial, Status.ErrorAbort => AbStatus.Abort, @@ -70,9 +76,21 @@ internal static class LibplctagStatusTranslation public static async Task Wrap(Func op) { try { await op().ConfigureAwait(false); } - catch (LibPlcTagException ex) { throw new AbTransportException(ToAbStatus(ex), ex.Message, ex); } + catch (LibPlcTagException ex) { throw FromException(ex); } } + /// + /// The transport exception for a libplctag failure: ErrorNotImplemented becomes an + /// (a library refusal, no I/O); everything else an + /// carrying the translated status. + /// + /// The libplctag exception. + /// The exception to throw. + public static AbTransportException FromException(LibPlcTagException ex) => + Enum.TryParse((ex.Message ?? string.Empty).Trim(), ignoreCase: true, out var s) && s == Status.ErrorNotImplemented + ? new AbCipLocalRefusalException($"libplctag does not implement this operation: {ex.Message}", ex) + : new AbTransportException(ToAbStatus(ex), ex.Message ?? string.Empty, ex); + /// Runs , rethrowing any as . /// The result type. /// The libplctag operation. @@ -80,7 +98,7 @@ internal static class LibplctagStatusTranslation public static async Task Wrap(Func> op) { try { return await op().ConfigureAwait(false); } - catch (LibPlcTagException ex) { throw new AbTransportException(ToAbStatus(ex), ex.Message, ex); } + catch (LibPlcTagException ex) { throw FromException(ex); } } /// Runs a synchronous libplctag call, rethrowing as . @@ -90,7 +108,7 @@ internal static class LibplctagStatusTranslation public static T WrapSync(Func op) { try { return op(); } - catch (LibPlcTagException ex) { throw new AbTransportException(ToAbStatus(ex), ex.Message, ex); } + catch (LibPlcTagException ex) { throw FromException(ex); } } /// Runs a synchronous libplctag call, rethrowing as . @@ -98,6 +116,6 @@ internal static class LibplctagStatusTranslation public static void WrapSync(Action op) { try { op(); } - catch (LibPlcTagException ex) { throw new AbTransportException(ToAbStatus(ex), ex.Message, ex); } + catch (LibPlcTagException ex) { throw FromException(ex); } } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index a77179f9..33ce02d8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -42,9 +42,14 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private byte[]? _path; // U11: a scalar BOOL authored over a BOOL-array element "Bits[n]" — the bit (n % 32) of the DWORD the request - // addresses (n / 32). Null for every other tag. + // addresses (n / 32). Null for every other tag, and cleared (with _path restored to _elementPath) the moment the + // controller shows its BOOL array is element-addressed (#737 I1: a 0xC1 reply, or a refusal, to the DWORD request). private int? _boolElementBit; + // The ORIGINAL element-addressed path ("Bits[n]") while a DWORD-addressed _path is still unconfirmed; null once + // the layout is settled either way. + private byte[]? _elementPath; + private ConnectionLease? _lease; private RequestScheduler? _privateScheduler; // ForTest only — production always uses the lease's private LogixTagClient? _client; @@ -124,6 +129,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime if (split.Bit is null && !_p.IsArray && _p.AuthoredType == AbCipDataType.Bool && TrySplitBoolArrayElement(name, out var dwordName, out var bit)) { + _elementPath = CipPath.EncodeSymbolic(name); name = dwordName; _boolElementBit = bit; } @@ -148,7 +154,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime { await ReadAsync(cancellationToken).ConfigureAwait(false); } - catch (AbTransportException ex) when (AbCipRefusals.IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (AbCipRefusals.IsDeviceRefusal(ex)) { // The controller answered. _status already holds the refusal. } @@ -162,11 +168,25 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime try { var (type, handle, data) = await client.ReadAsync(path, _elements, cancellationToken, ExpectedElementBytes(_p.AuthoredType)).ConfigureAwait(false); + if (_elementPath is not null) + { + // First answer to the DWORD-addressed BOOL-array request settles the layout: 0xD3 confirms it; anything + // else means this family addresses BOOL-array ELEMENTS (e.g. a 0xC1 per-element array) — decoding or + // writing element n / 32 there would hit the wrong element, so go back to "Bits[n]" and read again. + if (type != LogixTypeCode.BoolArray) { FallBackToElementPath(); await ReadAsync(cancellationToken).ConfigureAwait(false); return; } + _elementPath = null; + } _data = data; _typeCode = type; _structHandle = handle; _status = IsTypeMismatch(type) ? AbStatus.TypeMismatch : AbStatus.Ok; } + catch (AbTransportException ex) when (_elementPath is not null && AbCipRefusals.IsDeviceRefusal(ex)) + { + // The DWORD index was refused (e.g. out of bounds on an element-addressed array): retry as "Bits[n]". + FallBackToElementPath(); + await ReadAsync(cancellationToken).ConfigureAwait(false); + } catch (AbTransportException ex) { // libplctag reports a failed operation by exception AND leaves the status readable. @@ -175,6 +195,14 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime } } + /// Abandons the DWORD addressing of a BOOL-array element: the original "Bits[n]" path, no bit extraction. + private void FallBackToElementPath() + { + _path = _elementPath; + _elementPath = null; + _boolElementBit = null; + } + /// /// Upper bound on one element's wire bytes for the scheduler's reply budget; 0 when the /// authored type is unknown or a structure, which sends the read alone rather than guess. @@ -207,6 +235,13 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime try { + // An unsettled BOOL-array element layout is settled by a read first, so a write never goes to the + // DWORD-addressed element of an element-addressed array (#737 I1). + if (_elementPath is not null) + { + await ReadAsync(cancellationToken).ConfigureAwait(false); + path = Path(); + } if (_pendingString is { } text) { // A STRING writes as the type the controller reported — a Logix STRING is a structure @@ -215,7 +250,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime if (_typeCode is null || (_typeCode == LogixTypeCode.Struct && _structHandle is null)) await ReadAsync(cancellationToken).ConfigureAwait(false); if (_typeCode == LogixTypeCode.Struct && _structHandle is null) - throw new AbTransportException(AbStatus.Unsupported, + throw new AbCipLocalRefusalException( $"STRING write to '{_p.TagName}': the controller returned no structure handle."); var payload = EncodeStringPayload(text); await client.WriteAsync(path, _typeCode!.Value, _structHandle, 1, payload, cancellationToken) @@ -224,7 +259,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime else if (_writeBuffer is { } buffer) { var type = _typeCode ?? AtomicCode(_p.AuthoredType) - ?? throw new AbTransportException(AbStatus.Unsupported, + ?? throw new AbCipLocalRefusalException( $"Write to '{_p.TagName}': type unknown — no prior read and no authored atomic type."); // The read rule decides first: a same-width mismatch (REAL under a DINT-authored tag) // would otherwise send the int bit pattern as a REAL and report Good. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs index ea908d12..5ebd5908 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipArrayWriteTests.cs @@ -123,6 +123,7 @@ public sealed class AbCipArrayWriteTests r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); factory.Tags.ShouldNotContainKey("Arr"); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); // M5: a refusal writes no health } /// The controller holding a different shape than authored (size ≠ width × count) is BadTypeMismatch, nothing sent. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs index 11f7b736..dce0c47d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipBitInWordWidthTests.cs @@ -124,4 +124,38 @@ public sealed class AbCipBitInWordWidthTests await drv.DisposeAsync(); } } + + /// + /// M1: a parent read that fails with a COMMS status evicts the parent runtime and degrades (LastSuccessfulRead + /// preserved); one the controller REFUSES is per-tag Bad, keeps the runtime and writes no health. + /// + [Fact] + public async Task A_failed_parent_read_follows_the_health_contract() + { + var (drv, factory) = await BuildAsync(4, 0, "Motor.Flags.3"); + factory.Customise = p => new FakeAbCipTag(p) { Status = (int)AbStatus.BadConnection }; + var before = drv.GetHealth(); + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError); + factory.Tags["Motor.Flags"].Disposed.ShouldBeTrue(); + factory.Tags["Motor.Flags"].WriteCount.ShouldBe(0); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead); + } + + /// M1 positive control: a refused parent read keeps the runtime, no health write. + [Fact] + public async Task A_refused_parent_read_keeps_the_runtime_and_health() + { + var (drv, factory) = await BuildAsync(4, 0, "Motor.Flags.3"); + factory.Customise = p => new FakeAbCipTag(p) { Status = (int)AbStatus.NotAllowed }; + + var r = await drv.WriteAsync([new WriteRequest("Bit", true)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotWritable); + factory.Tags["Motor.Flags"].Disposed.ShouldBeFalse(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs index 6803d3ab..e4e23d7c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDeviceRefusalTests.cs @@ -225,4 +225,74 @@ public sealed class AbCipDeviceRefusalTests { public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams); } + + /// + /// #737 I2: a LOCAL refusal (libplctag ErrorNotImplemented — no I/O) is per-tag BadNotSupported, keeps health + /// state, and does NOT advance LastSuccessfulRead (nothing proves the device answered). Positive control: the + /// device's own Unsupported refusal does advance it (M2). + /// + [Fact] + public async Task A_local_refusal_does_not_advance_LastSuccessfulRead_but_a_device_one_does() + { + var (drv, _) = Build(p => p.TagName == "Arr[999]" + ? new LocalRefusingTag(p) + : new RefusingTag(p) { ReadThrows = AbStatus.Unsupported }); + await drv.InitializeAsync("{}", CancellationToken.None); + var before = drv.GetHealth().LastSuccessfulRead!.Value; + await Task.Delay(20); + + (await drv.ReadAsync(["Bad"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + drv.GetHealth().LastSuccessfulRead.ShouldBe(before); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + + (await drv.ReadAsync(["Good"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(before); + } + + /// #737 I2: a local refusal at create opens no backoff — a sibling cold create still connects. + [Fact] + public async Task A_local_refusal_at_create_does_not_throttle_or_count_as_answered() + { + var (drv, _) = Build(p => p.TagName == "Arr[999]" + ? new FakeAbCipTag(p) { ThrowOnInitialize = true, Exception = new AbCipLocalRefusalException("ErrorNotImplemented") } + : new FakeAbCipTag(p) { Value = 3 }); + await drv.InitializeAsync("{}", CancellationToken.None); + + var r = await drv.ReadAsync(["Bad", "Good"], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNotSupported); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// M2: a write the device refuses proves it answered — LastSuccessfulRead advances. + [Fact] + public async Task A_refused_write_advances_LastSuccessfulRead() + { + var factory = new FakeAbCipTagFactory { Customise = p => new RefusingTag(p) { WriteThrows = AbStatus.NotAllowed } }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("Ro", Device, "ReadOnlyTag", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-refusal", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + var before = drv.GetHealth().LastSuccessfulRead!.Value; + await Task.Delay(20); + + (await drv.WriteAsync([new WriteRequest("Ro", 5)], CancellationToken.None))[0].StatusCode + .ShouldBe(AbCipStatusMapper.BadNotWritable); + + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(before); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + private sealed class LocalRefusingTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override async Task ReadAsync(CancellationToken cancellationToken) + { + await base.ReadAsync(cancellationToken); + throw new AbCipLocalRefusalException("ErrorNotImplemented"); + } + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs index 2b009a7f..6811afcc 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLifecycleConcurrencyTests.cs @@ -56,9 +56,9 @@ public sealed class AbCipLifecycleConcurrencyTests return (drv, factory); } - private static async Task WaitUntilAsync(Func condition, string what) + private static async Task WaitUntilAsync(Func condition, string what, int seconds = 5) { - var deadline = DateTime.UtcNow.AddSeconds(5); + var deadline = DateTime.UtcNow.AddSeconds(seconds); while (!condition()) { if (DateTime.UtcNow > deadline) throw new TimeoutException(what); @@ -106,7 +106,8 @@ public sealed class AbCipLifecycleConcurrencyTests var init = drv.InitializeAsync("{}", CancellationToken.None); await Task.Delay(50); await drv.ShutdownAsync(CancellationToken.None); - await Should.ThrowAsync(() => init); + var superseded = await Should.ThrowAsync(() => init); + superseded.ShouldNotBeAssignableTo(); // M4: distinct from a caller cancellation drv.GetDeviceState(Host).ShouldBeNull(); drv.GetHostStatuses().ShouldBeEmpty(); @@ -199,18 +200,40 @@ public sealed class AbCipLifecycleConcurrencyTests [Fact] public async Task Sync_Dispose_does_not_block_on_the_teardown_but_still_completes_it() { - var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(500)); + var (drv, factory) = Build(probeReadDelay: TimeSpan.FromSeconds(4)); await drv.InitializeAsync("{}", CancellationToken.None); await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); var loop = drv.GetDeviceState(Host)!.ProbeTask!; - await Task.Delay(20); // the probe is inside its slow, cancellation-deaf read + await Task.Delay(50); // the probe is inside its slow, cancellation-deaf read var sw = System.Diagnostics.Stopwatch.StartNew(); ((IDisposable)drv).Dispose(); sw.Stop(); - sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(250)); // did not wait out the 500 ms probe read - await WaitUntilAsync(() => loop.IsCompleted && factory.Tags["ProbeTag"].Disposed, "teardown never completed"); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(2)); // did not wait out the 4 s probe read (CI-tolerant margin) + await WaitUntilAsync(() => loop.IsCompleted && factory.Tags["ProbeTag"].Disposed, "teardown never completed", seconds: 15); drv.GetDeviceState(Host).ShouldBeNull(); } + + /// + /// M4: a ShutdownAsync overtaken by a NEWER InitializeAsync (its probe join is slow) must not stamp Unknown over + /// the newer init's Healthy when it finally finishes. + /// + [Fact] + public async Task A_slow_shutdown_does_not_overwrite_a_newer_inits_health() + { + var (drv, factory) = Build(probeReadDelay: TimeSpan.FromMilliseconds(400)); + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitUntilAsync(() => factory.Tags.ContainsKey("ProbeTag"), "probe never started"); + await Task.Delay(30); // inside the slow probe read + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + await Task.Delay(30); + await drv.InitializeAsync("{}", CancellationToken.None); // newer: publishes while the shutdown still joins + await shutdown; + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetDeviceState(Host).ShouldNotBeNull(); + await drv.DisposeAsync(); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLocalRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLocalRefusalTests.cs index f51bfe18..cb99093e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLocalRefusalTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipLocalRefusalTests.cs @@ -7,7 +7,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; /// /// Task 22 review r1: a runtime that refuses an operation with (the native -/// transport does this before the wire, e.g. a write it cannot type) reaches the client as BadNotSupported — +/// transport does this before the wire, e.g. a write it cannot type — an +/// since #737 I2) reaches the client as BadNotSupported — /// NOT as a comms fault. The handle is kept (one create, never disposed) and driver health is untouched. /// [Trait("Category", "Unit")] @@ -19,12 +20,12 @@ public sealed class AbCipLocalRefusalTests { public override Task ReadAsync(CancellationToken cancellationToken) => refuseRead - ? Task.FromException(new AbTransportException(AbStatus.Unsupported, "refused read")) + ? Task.FromException(new AbCipLocalRefusalException("refused read")) : base.ReadAsync(cancellationToken); public override Task WriteAsync(CancellationToken cancellationToken) => refuseWrite - ? Task.FromException(new AbTransportException(AbStatus.Unsupported, "refused write")) + ? Task.FromException(new AbCipLocalRefusalException("refused write")) : base.WriteAsync(cancellationToken); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipStatusMapperTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipStatusMapperTests.cs index 7c3c63df..778f2c42 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipStatusMapperTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipStatusMapperTests.cs @@ -77,7 +77,7 @@ public sealed class AbCipStatusMapperTests Status.Ok => AbCipStatusMapper.Good, Status.Pending => AbCipStatusMapper.GoodMoreData, Status.ErrorTimeout => AbCipStatusMapper.BadTimeout, - Status.ErrorNotFound or Status.ErrorNoMatch or Status.ErrorBadDevice => AbCipStatusMapper.BadNodeIdUnknown, + Status.ErrorNotFound => AbCipStatusMapper.BadNodeIdUnknown, // #737 I2: NoMatch/BadDevice are comms Status.ErrorNotAllowed => AbCipStatusMapper.BadNotWritable, Status.ErrorOutOfBounds or Status.ErrorTooLarge or Status.ErrorTooSmall => AbCipStatusMapper.BadOutOfRange, Status.ErrorUnsupported or Status.ErrorNotImplemented => AbCipStatusMapper.BadNotSupported, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs index 4821a43a..9a1458c4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs @@ -110,4 +110,22 @@ public sealed class AbCipWriteHealthTests after.LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead); factory.Tags["D"].Disposed.ShouldBeTrue(); } + + /// M3: a good write clears a Degraded that a WRITE comms fault recorded. + [Fact] + public async Task A_good_write_clears_a_write_side_Degraded() + { + var fail = true; + var (drv, _) = await BuildAsync(p => fail ? new ThrowingWriteTag(p) : new RealEncodingTag(p)); + (await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None))[0].StatusCode + .ShouldBe(AbCipStatusMapper.BadCommunicationError); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + fail = false; // the evicted runtime is re-created healthy + (await drv.WriteAsync([new WriteRequest("D", 6)], CancellationToken.None))[0].StatusCode + .ShouldBe(AbCipStatusMapper.Good); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetHealth().LastError.ShouldBeNull(); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagStatusTranslationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagStatusTranslationTests.cs index fba8f7ea..9e1fd14d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagStatusTranslationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagStatusTranslationTests.cs @@ -16,8 +16,8 @@ public sealed class LibplctagStatusTranslationTests [InlineData(Status.Pending, AbStatus.Pending)] [InlineData(Status.ErrorTimeout, AbStatus.Timeout)] [InlineData(Status.ErrorNotFound, AbStatus.NotFound)] - [InlineData(Status.ErrorNoMatch, AbStatus.NotFound)] - [InlineData(Status.ErrorBadDevice, AbStatus.NotFound)] + [InlineData(Status.ErrorNoMatch, AbStatus.BadConnection)] // #737 I2: not proof the controller answered + [InlineData(Status.ErrorBadDevice, AbStatus.BadConnection)] [InlineData(Status.ErrorNotAllowed, AbStatus.NotAllowed)] [InlineData(Status.ErrorOutOfBounds, AbStatus.OutOfBounds)] [InlineData(Status.ErrorTooLarge, AbStatus.TooLarge)] @@ -148,4 +148,24 @@ public sealed class LibplctagStatusTranslationTests LibplctagStatusTranslation.Wrap(() => throw new LibPlcTagException(Status.ErrorBadParam))); thrown.Status.ShouldBe(AbStatus.BadConnection); } + + /// + /// #737 I2: libplctag's ErrorNotImplemented is the LIBRARY refusing (no I/O): a local refusal, not a device + /// refusal — it must not count as "the controller answered". + /// + [Fact] + public async Task NotImplemented_is_a_local_refusal_not_a_device_refusal() + { + var ex = await Should.ThrowAsync(() => + LibplctagStatusTranslation.Wrap(() => throw new LibPlcTagException("ErrorNotImplemented"))); + ex.ShouldBeOfType(); + ex.Status.ShouldBe(AbStatus.Unsupported); + AbCipRefusals.IsDeviceRefusal(ex).ShouldBeFalse(); + AbCipDriverProbe.IsReachableException(ex).ShouldBeFalse(); + + // Positive control: the device's own Unsupported refusal still is one. + var device = await Should.ThrowAsync(() => + LibplctagStatusTranslation.Wrap(() => throw new LibPlcTagException("ErrorUnsupported"))); + AbCipRefusals.IsDeviceRefusal(device).ShouldBeTrue(); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 9d013613..1d514c91 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -135,6 +135,30 @@ public sealed class NativeAbCipTagRuntimeTests ch.Sent[0].ShouldBe(Convert.FromHexString("5204" + "910442697473" + "28" + dword.ToString("X2") + "0100" + "00000000")); } + /// + /// #737 I1: a family whose BOOL array is element-addressed (the controller answers the DWORD-index request with a + /// plain BOOL, 0xC1) falls back to the ORIGINAL element path Bits[n] — it must neither decode nor write + /// element n / 32. Positive control: the 0xD3 theory above still decodes bit n % 32. + /// + [Fact] + public async Task A_per_element_BOOL_array_falls_back_to_the_element_path_for_read_and_write() + { + var ch = new FakeCipChannel + { + Respond = req => Task.FromResult(Convert.FromHexString(req[0] == 0x4D ? "CD000000" : "D2000000C10001")), + }; + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Bits[37]", AbCipDataType.Bool)); + await rt.InitializeAsync(CancellationToken.None); + await rt.ReadAsync(CancellationToken.None); + + rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(true); + ch.Sent[^1].ShouldBe(Convert.FromHexString("5204" + "910442697473" + "2825" + "0100" + "00000000")); // Bits[37] + + rt.EncodeValue(AbCipDataType.Bool, null, false); + await rt.WriteAsync(CancellationToken.None); + ch.Sent[^1].ShouldBe(Convert.FromHexString("4D04" + "910442697473" + "2825" + "C100" + "0100" + "00")); // Bits[37] + } + /// Positive control: a BOOL answered as a plain BOOL (0xC1) still decodes its byte. [Fact] public async Task A_BOOL_element_answered_as_BOOL_decodes_the_byte() From f37cb2a4a38ac4e7be102d7680c257c58ea16923 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:05:00 -0400 Subject: [PATCH 15/15] fix(abcip): only a reply type settles the BOOL-array layout (review r2) N1: a refusal of the DWORD-addressed request permanently settled the layout as per-element, so a tag deployed before the online edit that creates its BOOL array later read DWORD n and published byte 0 as Good. The native runtime now keeps an explicit layout (Unsettled / Packed / PerElement) that ONLY a reply type changes: 0xD3 settles packed (Bits[n/32], bit n%32), 0xC1 settles per-element (Bits[n]). Refusals and comms faults leave it unsettled, so the next read asks again. A reply whose type disagrees with the addressing it was asked under is discarded and re-read once under the shown layout (it would be some other element), also when an online edit re-types a settled array. A scalar BOOL answered 0xD3 with no element bit, or not settled packed, is refused BadTypeMismatch instead of decoding byte 0. m1: the write-owned Degraded marker is set together with the CAS that installs it, under one lock, so concurrent failing writes cannot leave it naming a different instance (a good write then clears it); a refusal-only read batch still keeps it write-owned. m2/m3/m5: documented (breaker accounting of a superseded init; libplctag throw-only NotImplemented assumption; the TypeMismatch write status word is libplctag-only). m4: test for a write on an unsettled layout (settles by a read first). Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 44 ++++--- .../AbCipInitializationSupersededException.cs | 7 ++ .../LibplctagStatusTranslation.cs | 4 + .../NativeAbCipTagRuntime.cs | 109 +++++++++++++----- .../AbCipWriteHealthTests.cs | 44 +++++++ .../NativeAbCipTagRuntimeTests.cs | 102 ++++++++++++++++ 6 files changed, 266 insertions(+), 44 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 10b7a813..6be5b374 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -149,14 +149,15 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// Atomically replaces with applied to its current value. /// Computes the new health from the current one. - private void UpdateHealth(Func update) + /// The health instance this call installed. + private DriverHealth UpdateHealth(Func update) { while (true) { var current = _health; var next = update(current); #pragma warning disable CS0420 // a volatile field passed by ref to Interlocked keeps its full-fence semantics - if (ReferenceEquals(Interlocked.CompareExchange(ref _health, next, current), current)) return; + if (ReferenceEquals(Interlocked.CompareExchange(ref _health, next, current), current)) return next; #pragma warning restore CS0420 } } @@ -1505,6 +1506,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, else if (status == (int)AbStatus.TypeMismatch) { // Final review I4: deterministic — keep the handle, leave health, warn once per tag. + // Only libplctag reaches this status-word branch, where it is a real controller answer: the native + // runtime's pre-send type gate THROWS (caught below, no LastSuccessfulRead advance) and never + // returns here with the status merely set (m5). results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); RecordWriteAnswered(now); WarnTypeMismatchOnce(def.Name, @@ -1645,23 +1649,35 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// replaced it since) is cleared back to Healthy: the device just accepted a write. A Degraded the read /// path recorded is left for the read path to clear. /// - private void RecordWriteAnswered(DateTime now) => - UpdateHealth(h => h.State == DriverState.Degraded && h.LastError is { } err - && ReferenceEquals(err, Volatile.Read(ref _writeFaultError)) - ? new DriverHealth(DriverState.Healthy, now, null) - : h with { LastSuccessfulRead = now }); + private void RecordWriteAnswered(DateTime now) + { + lock (_writeHealthLock) + { + UpdateHealth(h => h.State == DriverState.Degraded && h.LastError is { } err && ReferenceEquals(err, _writeOwnedError) + ? new DriverHealth(DriverState.Healthy, now, null) + : h with { LastSuccessfulRead = now }); + } + } - // The LastError string the most recent write-side comms fault wrote (reference identity marks "write-owned"). - private string? _writeFaultError; + // The exact LastError string INSTANCE the most recent write-side comms fault installed (a fresh copy, so reference + // identity cannot alias a read-side error). A health still carrying it is write-owned: a refusal-only read batch + // keeps it (`with` copies the reference), while any read / poll fault installs a new string and takes ownership. + // Set only under _writeHealthLock TOGETHER with the CAS that installs it, so two concurrent failing writes can never + // leave the marker naming a different instance than the installed health (#737 m1). + private string? _writeOwnedError; + private readonly object _writeHealthLock = new(); /// A write-side comms fault: Degraded (never downgrading Faulted), LastSuccessfulRead preserved. private void RecordWriteCommsFault(string error) { - var owned = new string(error.AsSpan()); // a distinct instance, so reference identity cannot alias a read error - Volatile.Write(ref _writeFaultError, owned); - UpdateHealth(h => h.State == DriverState.Faulted - ? h - : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, owned)); + var owned = new string(error.AsSpan()); + lock (_writeHealthLock) + { + var installed = UpdateHealth(h => h.State == DriverState.Faulted + ? h + : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, owned)); + if (ReferenceEquals(installed.LastError, owned)) _writeOwnedError = owned; + } } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs index eeb1afef..ffa3c0b1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipInitializationSupersededException.cs @@ -7,5 +7,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// "my own token fired" must not mistake a superseded attempt for one. The superseding operation owns the driver's /// state; the caller (normally the runtime, which has already abandoned this attempt) can ignore the result. /// +/// +/// Resilience accounting (#737 m2): not being an OCE, it passes the Initialize pipeline's breaker +/// ShouldHandle (DriverResiliencePipelineBuilder) and, if its generation still matched, the actor's +/// RecordFault. That is practically unreachable — DriverInstanceActor runs initialisation single-flight +/// and drops generation-mismatched results — and a superseded attempt is at worst one counted failure. Recognising +/// it there is a runtime-side (Lane A) change if it is ever wanted. +/// /// The detail. public sealed class AbCipInitializationSupersededException(string message) : InvalidOperationException(message); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs index 002e8c21..7be295a6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagStatusTranslation.cs @@ -25,6 +25,10 @@ internal static class LibplctagStatusTranslation Status.ErrorTooSmall => AbStatus.TooSmall, // ErrorNotImplemented is the LIBRARY refusing (no I/O); the exception paths raise it as an // AbCipLocalRefusalException so it never counts as "the device answered" (see FromException). + // ASSUMPTION (#737 m3): libplctag reports a failed operation by THROWING, so a NotImplemented reaches the driver + // only through FromException. AbStatus has no "local" member (it is Driver.Ab.Transport's, Lane K), so a + // NotImplemented observed only as a STATUS WORD (GetStatus after a non-throwing op) would still classify as a + // device Unsupported refusal; that combination is not produced by libplctag.NET 1.5.2. Status.ErrorUnsupported or Status.ErrorNotImplemented => AbStatus.Unsupported, Status.ErrorPartial => AbStatus.Partial, Status.ErrorAbort => AbStatus.Abort, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index 33ce02d8..a666239c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -41,15 +41,34 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private int? _implicitBit; private byte[]? _path; - // U11: a scalar BOOL authored over a BOOL-array element "Bits[n]" — the bit (n % 32) of the DWORD the request - // addresses (n / 32). Null for every other tag, and cleared (with _path restored to _elementPath) the moment the - // controller shows its BOOL array is element-addressed (#737 I1: a 0xC1 reply, or a refusal, to the DWORD request). + // U11: a scalar BOOL authored over a BOOL-array element "Bits[n]" — the bit (n % 32) of DWORD n / 32 when the + // array is packed. Null for every other tag. private int? _boolElementBit; - // The ORIGINAL element-addressed path ("Bits[n]") while a DWORD-addressed _path is still unconfirmed; null once - // the layout is settled either way. + // Both addressings of such a tag: "Bits[n / 32]" (packed: a Logix BOOL array is stored as 0xD3 DWORDs) and the + // original "Bits[n]" (per-element: a family that answers 0xC1). _path is whichever _boolLayout selects. + private byte[]? _dwordPath; private byte[]? _elementPath; + /// How a BOOL-array element tag's array is laid out, as the controller's REPLY TYPE last showed it. + private enum BoolLayout + { + /// No reply type seen yet (or only refusals / comms faults): request DWORD-addressed, keep asking. + Unsettled, + + /// The controller answered 0xD3: address DWORD n / 32, decode bit n % 32. + Packed, + + /// The controller answered 0xC1: address element n, decode its byte. + PerElement, + } + + // #737 fix round 2 (N1): ONLY a reply type settles (or re-settles) the layout — 0xD3 ⇒ Packed, 0xC1 ⇒ PerElement. + // A refusal or comms fault says nothing about the layout and leaves it as it was, so a tag deployed before the + // online edit that creates its array starts reading correctly once the array exists. A reply whose type disagrees + // with the addressing it was asked under is DISCARDED (never decoded) and re-read under the right addressing. + private BoolLayout _boolLayout; + private ConnectionLease? _lease; private RequestScheduler? _privateScheduler; // ForTest only — production always uses the lease's private LogixTagClient? _client; @@ -130,10 +149,15 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime && TrySplitBoolArrayElement(name, out var dwordName, out var bit)) { _elementPath = CipPath.EncodeSymbolic(name); - name = dwordName; + _dwordPath = CipPath.EncodeSymbolic(dwordName); _boolElementBit = bit; + _boolLayout = BoolLayout.Unsettled; + path = _dwordPath; + } + else + { + path = CipPath.EncodeSymbolic(name); } - path = CipPath.EncodeSymbolic(name); } catch (Exception ex) when (ex is ArgumentException or FormatException or OverflowException) { @@ -161,32 +185,43 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime } /// - public async Task ReadAsync(CancellationToken cancellationToken) + public Task ReadAsync(CancellationToken cancellationToken) => ReadCoreAsync(cancellationToken, mayRelayout: true); + + private async Task ReadCoreAsync(CancellationToken cancellationToken, bool mayRelayout) { var client = Client(); var path = Path(); try { var (type, handle, data) = await client.ReadAsync(path, _elements, cancellationToken, ExpectedElementBytes(_p.AuthoredType)).ConfigureAwait(false); - if (_elementPath is not null) + if (_boolElementBit is not null) { - // First answer to the DWORD-addressed BOOL-array request settles the layout: 0xD3 confirms it; anything - // else means this family addresses BOOL-array ELEMENTS (e.g. a 0xC1 per-element array) — decoding or - // writing element n / 32 there would hit the wrong element, so go back to "Bits[n]" and read again. - if (type != LogixTypeCode.BoolArray) { FallBackToElementPath(); await ReadAsync(cancellationToken).ConfigureAwait(false); return; } - _elementPath = null; + BoolLayout? shown = type == LogixTypeCode.BoolArray ? BoolLayout.Packed + : type == LogixTypeCode.Bool ? BoolLayout.PerElement + : null; // any other type (a mistyped tag): no layout evidence; the type check below reports it + if (shown is { } layout) + { + var askedPacked = ReferenceEquals(path, _dwordPath); + SetBoolLayout(layout); + if ((layout == BoolLayout.Packed) != askedPacked) + { + // The reply is for the OTHER addressing (element n / 32 of a per-element array, or DWORD n of a + // packed one) — not this element. Discard it and ask again under the layout it just showed. + if (mayRelayout) + { + await ReadCoreAsync(cancellationToken, mayRelayout: false).ConfigureAwait(false); + return; + } + throw new AbTransportException(AbStatus.TypeMismatch, + $"'{_p.TagName}': the controller's BOOL-array layout changed between two reads; retrying next read."); + } + } } _data = data; _typeCode = type; _structHandle = handle; _status = IsTypeMismatch(type) ? AbStatus.TypeMismatch : AbStatus.Ok; } - catch (AbTransportException ex) when (_elementPath is not null && AbCipRefusals.IsDeviceRefusal(ex)) - { - // The DWORD index was refused (e.g. out of bounds on an element-addressed array): retry as "Bits[n]". - FallBackToElementPath(); - await ReadAsync(cancellationToken).ConfigureAwait(false); - } catch (AbTransportException ex) { // libplctag reports a failed operation by exception AND leaves the status readable. @@ -195,12 +230,11 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime } } - /// Abandons the DWORD addressing of a BOOL-array element: the original "Bits[n]" path, no bit extraction. - private void FallBackToElementPath() + /// Settles a BOOL-array element tag's layout and selects the matching request path. + private void SetBoolLayout(BoolLayout layout) { - _path = _elementPath; - _elementPath = null; - _boolElementBit = null; + _boolLayout = layout; + _path = layout == BoolLayout.PerElement ? _elementPath : _dwordPath; } /// @@ -236,8 +270,9 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime try { // An unsettled BOOL-array element layout is settled by a read first, so a write never goes to the - // DWORD-addressed element of an element-addressed array (#737 I1). - if (_elementPath is not null) + // DWORD-addressed element of an element-addressed array (#737 I1). A packed layout's write is refused below + // (0xD3 DWORD — it would clobber 31 neighbours); a per-element one writes "Bits[n]" as a BOOL. + if (_boolElementBit is not null && _boolLayout == BoolLayout.Unsettled) { await ReadAsync(cancellationToken).ConfigureAwait(false); path = Path(); @@ -323,9 +358,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime return type switch { - AbCipDataType.Bool => bitIndex is int bit ? GetBit(bit) - : offset == 0 && _boolElementBit is int element && _typeCode == LogixTypeCode.BoolArray ? GetBit(element) - : Span(offset, 1)[0] != 0, + AbCipDataType.Bool => bitIndex is int bit ? GetBit(bit) : DecodeScalarBool(offset), AbCipDataType.SInt => (int)(sbyte)Span(offset, 1)[0], AbCipDataType.USInt => (int)Span(offset, 1)[0], AbCipDataType.Int => (int)BinaryPrimitives.ReadInt16LittleEndian(Span(offset, 2)), @@ -471,6 +504,22 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime catch (Exception) { /* ForTest only; Dispose must not throw */ } } + /// + /// A scalar BOOL. A packed (0xD3) reply is decoded ONLY as the element's own bit of the DWORD the packed layout + /// addressed; any other 0xD3 reply (no element bit, or not settled packed) is refused rather than decoded as + /// "byte 0 != 0" — that value would be some other bit, published as Good (#737 N1). + /// + private bool DecodeScalarBool(int offset) + { + if (offset == 0 && _typeCode == LogixTypeCode.BoolArray) + { + if (_boolElementBit is int element && _boolLayout == BoolLayout.Packed) return GetBit(element); + throw new AbTransportException(AbStatus.TypeMismatch, + $"'{_p.TagName}' is a scalar BOOL but the controller answered a packed BOOL-array DWORD (0xD3) with no element bit to select."); + } + return Span(offset, 1)[0] != 0; + } + /// /// Splits a BOOL-array element reference Name[n] (one non-negative subscript, last segment) into the /// DWORD it lives in (Name[n / 32]) and its bit within that DWORD (n % 32). A Logix BOOL array diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs index 9a1458c4..db68d1f5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs @@ -128,4 +128,48 @@ public sealed class AbCipWriteHealthTests drv.GetHealth().State.ShouldBe(DriverState.Healthy); drv.GetHealth().LastError.ShouldBeNull(); } + + /// + /// m1: many concurrent failing writes leave the write-owned marker naming the installed Degraded, so one good + /// write still clears it. + /// + [Fact] + public async Task Concurrent_failing_writes_then_a_good_write_clear_the_write_side_Degraded() + { + var fail = true; + var (drv, _) = await BuildAsync(p => fail ? new ThrowingWriteTag(p) : new RealEncodingTag(p)); + for (var round = 0; round < 20; round++) + { + fail = true; + await Task.WhenAll(Enumerable.Range(0, 8).Select(_ => Task.Run(() => + drv.WriteAsync([new WriteRequest("D", 1), new WriteRequest("I", 2)], CancellationToken.None)))); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + fail = false; + (await drv.WriteAsync([new WriteRequest("D", 3)], CancellationToken.None))[0].StatusCode + .ShouldBe(AbCipStatusMapper.Good); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, $"round {round}"); + } + } + + /// + /// M3: a refusal-only read batch in between (no read comms fault happened) keeps the Degraded write-owned, so a + /// good write still clears it. + /// + [Fact] + public async Task A_refusal_only_read_between_keeps_the_Degraded_write_owned() + { + var fail = true; + var (drv, _) = await BuildAsync(p => p.TagName == "Broken" + ? new FakeAbCipTag(p) { Status = (int)ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.AbStatus.NotFound } + : fail ? new ThrowingWriteTag(p) : new RealEncodingTag(p)); + await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + (await drv.ReadAsync(["Broken"], CancellationToken.None))[0].StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + + fail = false; + await drv.WriteAsync([new WriteRequest("D", 6)], CancellationToken.None); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 1d514c91..028d6e27 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -159,6 +159,108 @@ public sealed class NativeAbCipTagRuntimeTests ch.Sent[^1].ShouldBe(Convert.FromHexString("4D04" + "910442697473" + "2825" + "C100" + "0100" + "00")); // Bits[37] } + /// A channel whose reply can be switched mid-test (read replies; writes always succeed). + private static FakeCipChannel Switchable(Func readReply) => new() + { + Respond = req => Task.FromResult(Convert.FromHexString(req[0] == 0x4D ? "CD000000" : readReply())), + }; + + private const string Dword1 = "5204" + "910442697473" + "2801" + "0100" + "00000000"; // Bits[1] (DWORD 37/32) + private const string Element37 = "5204" + "910442697473" + "2825" + "0100" + "00000000"; // Bits[37] (element) + + /// + /// #737 fix round 2 (N1): a REFUSAL of the DWORD request says nothing about the layout — it must not settle it. + /// Tag deployed before the online edit that creates Bits : BOOL[2048]: the init read is refused NotFound; + /// once the array exists the controller answers 0xD3, and the read must address DWORD n/32 and decode bit n%32 + /// (the round-1 code had permanently fallen back to Bits[37] = DWORD 37 and decoded byte 0 as Good). + /// + /// The refusal reply the DWORD request first gets. + [Theory] + [InlineData("D2000400")] // NotFound + [InlineData("D2000F00")] // NotAllowed + [InlineData("D2000A00")] // OutOfBounds + public async Task A_refusal_leaves_the_BOOL_layout_unsettled_and_a_later_DWORD_reply_decodes_the_right_bit(string refusal) + { + var reply = refusal; + var ch = Switchable(() => reply); + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Bits[37]", AbCipDataType.Bool)); + await rt.InitializeAsync(CancellationToken.None); // refused: completes with the refusal status + rt.GetStatus().ShouldNotBe((int)AbStatus.Ok); + ch.Sent[^1].ShouldBe(Convert.FromHexString(Dword1)); // never fell back to the element path + + reply = "D2000000D300" + "20000000"; // bit 5 of DWORD 1 = element 37 + await rt.ReadAsync(CancellationToken.None); + + ch.Sent[^1].ShouldBe(Convert.FromHexString(Dword1)); + rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(true); + } + + /// + /// N1 positive control: a refusal, then a 0xC1 reply, settles PER-ELEMENT — the n/32 element that came back is + /// discarded and Bits[n] is re-read and decoded. + /// + [Fact] + public async Task A_refusal_then_a_per_element_reply_settles_the_element_path() + { + var reply = "D2000A00"; + var ch = Switchable(() => reply); + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Bits[37]", AbCipDataType.Bool)); + await rt.InitializeAsync(CancellationToken.None); + + reply = "D2000000C10001"; + await rt.ReadAsync(CancellationToken.None); + + ch.Sent[^2].ShouldBe(Convert.FromHexString(Dword1)); // asked DWORD-addressed, got a plain BOOL… + ch.Sent[^1].ShouldBe(Convert.FromHexString(Element37)); // …so element 37 is re-read and decoded + rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(true); + } + + /// + /// N1: a layout that changes after it was settled (an online edit re-types the array) re-settles from the reply + /// type — it never decodes a DWORD reply to the ELEMENT request (DWORD n, byte 0) as the element's value. + /// + [Fact] + public async Task A_settled_per_element_layout_resettles_when_the_reply_becomes_a_DWORD() + { + var reply = "D2000000C10000"; + var ch = Switchable(() => reply); + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Bits[36]", AbCipDataType.Bool)); + await rt.InitializeAsync(CancellationToken.None); // settles per-element + + reply = "D2000000D300" + "01000000"; // byte 0 non-zero, bit 4 (element 36) clear + await rt.ReadAsync(CancellationToken.None); + + ch.Sent[^1].ShouldBe(Convert.FromHexString("5204" + "910442697473" + "2801" + "0100" + "00000000")); + rt.DecodeValue(AbCipDataType.Bool, null).ShouldBe(false); // bit 4 of DWORD 1, not "byte 0 != 0" + } + + /// N1 minimum guard: a scalar BOOL with no element bit answered as a packed DWORD is refused, not byte 0. + [Fact] + public async Task A_scalar_BOOL_answered_as_a_packed_DWORD_without_an_element_bit_is_refused() + { + using var rt = await ReadAsync(Replying("D300", "01000000"), Params("Flag", AbCipDataType.Bool)); + (Should.Throw(() => rt.DecodeValue(AbCipDataType.Bool, null))).Status.ShouldBe(AbStatus.TypeMismatch); + } + + /// + /// m4: a runtime whose init read failed on the wire (layout still unsettled) settles the layout by a read BEFORE + /// it writes, so the write goes to the right element of a per-element array. + /// + [Fact] + public async Task A_write_on_an_unsettled_layout_reads_first_and_writes_the_element() + { + var reply = "D2000100"; // 0x01: not a refusal — init throws, layout unsettled + var ch = Switchable(() => reply); + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("Bits[37]", AbCipDataType.Bool)); + await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None)); + + reply = "D2000000C10000"; + rt.EncodeValue(AbCipDataType.Bool, null, true); + await rt.WriteAsync(CancellationToken.None); + + ch.Sent[^1].ShouldBe(Convert.FromHexString("4D04" + "910442697473" + "2825" + "C100" + "0100" + "01")); // Bits[37] + } + /// Positive control: a BOOL answered as a plain BOOL (0xC1) still decodes its byte. [Fact] public async Task A_BOOL_element_answered_as_BOOL_decodes_the_byte()