diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs index b19e20fb..64d5d665 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs @@ -47,7 +47,7 @@ public class HistoryReadCommand : CommandBase /// /// Gets the optional aggregate name used when the operator wants processed history instead of raw samples. /// - [CommandOption("aggregate", Description = "Aggregate function: Average, Minimum, Maximum, Count, Start, End, StandardDeviation, TimeAverage, TimeAverage2, Total, Total2, Interpolative")] + [CommandOption("aggregate", Description = "Aggregate function: Average, Minimum, Maximum, Count, Start, End, TimeAverage, TimeAverage2, Total, Total2, Interpolative, Range, Delta, MinimumActualTime, MaximumActualTime, PercentGood, PercentBad")] public string? Aggregate { get; init; } /// @@ -199,24 +199,33 @@ public class HistoryReadCommand : CommandBase } } + private static readonly string SupportedAggregates = string.Join(", ", Enum.GetNames()); + private static AggregateType ParseAggregateType(string name) { - return name.Trim().ToLowerInvariant() switch + var trimmed = name.Trim(); + var alias = trimmed.ToLowerInvariant() switch { - "average" or "avg" => AggregateType.Average, - "minimum" or "min" => AggregateType.Minimum, - "maximum" or "max" => AggregateType.Maximum, - "count" => AggregateType.Count, - "start" or "first" => AggregateType.Start, - "end" or "last" => AggregateType.End, - "standarddeviation" or "stddev" or "stdev" => AggregateType.StandardDeviation, - "timeaverage" or "timeavg" => AggregateType.TimeAverage, - "timeaverage2" or "timeavg2" => AggregateType.TimeAverage2, - "total" => AggregateType.Total, - "total2" => AggregateType.Total2, - "interpolative" or "interp" => AggregateType.Interpolative, - _ => throw new ArgumentException( - $"Unknown aggregate: '{name}'. Supported: Average, Minimum, Maximum, Count, Start, End, StandardDeviation, TimeAverage, TimeAverage2, Total, Total2, Interpolative") + "avg" => AggregateType.Average, + "min" => AggregateType.Minimum, + "max" => AggregateType.Maximum, + "first" => AggregateType.Start, + "last" => AggregateType.End, + "timeavg" => AggregateType.TimeAverage, + "timeavg2" => AggregateType.TimeAverage2, + "interp" => AggregateType.Interpolative, + "mintime" => AggregateType.MinimumActualTime, + "maxtime" => AggregateType.MaximumActualTime, + _ => (AggregateType?)null }; + if (alias is { } a) return a; + + // Every enum name (the served set; a parity test pins it), case-insensitive. Numeric strings are refused: + // Enum.TryParse would accept "6" or "99" as an undefined value. + if (!int.TryParse(trimmed, out _) && + Enum.TryParse(trimmed, ignoreCase: true, out var parsed) && Enum.IsDefined(parsed)) + return parsed; + + throw new ArgumentException($"Unknown aggregate: '{name}'. Supported: {SupportedAggregates}"); } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs index dd871594..b07efa97 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSessionAdapter.cs @@ -115,22 +115,6 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter return (nextCp, nextRefs ?? []); } - /// - public async Task HasChildrenAsync(NodeId nodeId, CancellationToken ct) - { - var (_, _, references) = await _session.BrowseAsync( - null, - null, - nodeId, - 1u, - BrowseDirection.Forward, - ReferenceTypeIds.HierarchicalReferences, - true, - 0u); - - return references != null && references.Count > 0; - } - /// public async Task HistoryUpdateAsync(HistoryUpdateDetails details, CancellationToken ct) { @@ -164,46 +148,16 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter ReturnBounds = false }; - var nodesToRead = new HistoryReadValueIdCollection - { - new HistoryReadValueId { NodeId = nodeId } - }; - - var allValues = new List(); - byte[]? continuationPoint = null; - - do - { - if (continuationPoint != null) - nodesToRead[0].ContinuationPoint = continuationPoint; - - // Use the async overload so this method is genuinely asynchronous, - // honors the cancellation token, and does not block the caller's thread - // (which would block the UI dispatcher for client.ui consumers). - var response = await _session.HistoryReadAsync( - null, - new ExtensionObject(details), - TimestampsToReturn.Source, - continuationPoint != null, - nodesToRead, - ct).ConfigureAwait(false); - - var results = response.Results; - if (results == null || results.Count == 0) - break; - - var result = results[0]; - // A refused read (BadHistoryOperationUnsupported, BadUserAccessDenied, ...) must surface; only - // BadNoData is an empty result. Swallowing it printed "0 values returned" for a refusal. - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - - if (result.HistoryData is ExtensionObject ext && ext.Body is HistoryData historyData) - allValues.AddRange(historyData.DataValues); - - continuationPoint = result.ContinuationPoint; - } while (continuationPoint != null && continuationPoint.Length > 0 && allValues.Count < maxValues); - - return allValues; + // Continuation pages are READ, never released, until the server finishes or the cap is reached; a read stopped + // by the cap releases the server's cursor (HistoryReadPaging). This path used to pass + // `releaseContinuationPoints: continuationPoint != null`, which RELEASED the second page instead of reading it, + // silently truncating every raw read to its first server page. + return await HistoryReadPaging.ReadAllAsync( + (cp, release, token) => ReadHistoryPageAsync(details, nodeId, cp, release, TimestampsToReturn.Source, token), + result => result.HistoryData is ExtensionObject { Body: HistoryData historyData } + ? historyData.DataValues + : [], + maxValues, nodeId, ct).ConfigureAwait(false); } /// @@ -219,59 +173,41 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter ReturnBounds = false }; + return await HistoryReadPaging.ReadAllAsync( + (cp, release, token) => ReadHistoryPageAsync(details, nodeId, cp, release, TimestampsToReturn.Both, token), + ToModifiedEntries, maxValues, nodeId, ct).ConfigureAwait(false); + } + + private static IEnumerable ToModifiedEntries(HistoryReadResult result) + { + if (result.HistoryData is not ExtensionObject { Body: HistoryModifiedData modified }) yield break; + for (var i = 0; i < modified.DataValues.Count; i++) + { + var info = i < modified.ModificationInfos.Count ? modified.ModificationInfos[i] : null; + yield return new Models.HistoryModifiedEntry( + modified.DataValues[i], + info?.ModificationTime ?? DateTime.MinValue, + info?.UpdateType ?? default, + info?.UserName); + } + } + + /// Issues one single-node HistoryRead (or a release of its continuation point). + private async Task ReadHistoryPageAsync( + ReadRawModifiedDetails details, NodeId nodeId, byte[]? continuationPoint, bool release, + TimestampsToReturn timestamps, CancellationToken ct) + { var nodesToRead = new HistoryReadValueIdCollection { - new HistoryReadValueId { NodeId = nodeId } + new HistoryReadValueId { NodeId = nodeId, ContinuationPoint = continuationPoint } }; - var all = new List(); - byte[]? continuationPoint = null; + // The async overload honours the token and does not block the caller's thread (the UI dispatcher for + // client.ui consumers). + var response = await _session.HistoryReadAsync( + null, new ExtensionObject(details), timestamps, release, nodesToRead, ct).ConfigureAwait(false); - do - { - if (continuationPoint != null) - nodesToRead[0].ContinuationPoint = continuationPoint; - - var response = await _session.HistoryReadAsync( - null, - new ExtensionObject(details), - TimestampsToReturn.Both, - false, - nodesToRead, - ct).ConfigureAwait(false); - - var results = response.Results; - if (results == null || results.Count == 0) - break; - - var result = results[0]; - HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); - - if (result.HistoryData is ExtensionObject ext && ext.Body is HistoryModifiedData modified) - { - for (var i = 0; i < modified.DataValues.Count; i++) - { - var info = i < modified.ModificationInfos.Count ? modified.ModificationInfos[i] : null; - all.Add(new Models.HistoryModifiedEntry( - modified.DataValues[i], - info?.ModificationTime ?? DateTime.MinValue, - info?.UpdateType ?? default, - info?.UserName)); - } - } - - continuationPoint = result.ContinuationPoint; - } while (continuationPoint != null && continuationPoint.Length > 0 && all.Count < maxValues); - - // A read stopped by the client's own cap leaves a server cursor behind; release it. - if (continuationPoint is { Length: > 0 }) - { - nodesToRead[0].ContinuationPoint = continuationPoint; - await _session.HistoryReadAsync(null, new ExtensionObject(details), TimestampsToReturn.Both, true, - nodesToRead, ct).ConfigureAwait(false); - } - - return all; + return response.Results is { Count: > 0 } results ? results[0] : null; } /// @@ -327,9 +263,22 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter }; _session.AddSubscription(subscription); - await subscription.CreateAsync(ct); + try + { + await subscription.CreateAsync(ct); + } + catch + { + // A subscription the server never created must not linger in Session.Subscriptions either. + try { await _session.RemoveSubscriptionAsync(subscription, CancellationToken.None); } + catch (Exception ex) { Logger.Warning(ex, "Error removing a subscription that failed to create"); } + throw; + } - return new DefaultSubscriptionAdapter(subscription); + // Deleting the adapter also removes the subscription from the SDK session, so a re-targeted alarm/data + // subscription does not accumulate dead Subscription objects in Session.Subscriptions. + return new DefaultSubscriptionAdapter(subscription, + async (s, token) => await _session.RemoveSubscriptionAsync(s, token).ConfigureAwait(false)); } /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs index deb15dd5..6473abf6 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultSubscriptionAdapter.cs @@ -12,14 +12,23 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter private static readonly ILogger Logger = Log.ForContext(); private readonly Dictionary _monitoredItems = new(); private readonly Subscription _subscription; + private readonly Func? _detach; + private int _detached; /// /// Wraps a live OPC UA subscription so client code can manage monitored items through a testable abstraction. /// /// The underlying OPC UA subscription that owns monitored items for this client workflow. - public DefaultSubscriptionAdapter(Subscription subscription) + /// + /// Removes the subscription from its owning SDK Session once it is deleted. Without it a deleted + /// subscription stays in Session.Subscriptions until the session is disposed, so a UI that re-targets + /// alarms repeatedly accumulates dead subscriptions the SDK still iterates (transfer / recreate on reconnect). + /// + public DefaultSubscriptionAdapter(Subscription subscription, + Func? detach = null) { _subscription = subscription; + _detach = detach; } /// @@ -113,17 +122,39 @@ internal sealed class DefaultSubscriptionAdapter : ISubscriptionAdapter Logger.Warning(ex, "Error deleting subscription"); } + // Detach whether or not the server-side delete succeeded: the adapter is finished with it either way. + await DetachAsync(ct); _monitoredItems.Clear(); } + private async Task DetachAsync(CancellationToken ct) + { + if (_detach == null || Interlocked.Exchange(ref _detached, 1) != 0) return; + try + { + await _detach(_subscription, ct); + } + catch (Exception ex) + { + Logger.Warning(ex, "Error removing subscription from its session"); + } + } + /// /// Releases the wrapped OPC UA subscription and clears tracked monitored items held by the adapter. /// public void Dispose() { + // One bounded bridge (5 s) for delete + detach, so a dispose never blocks longer than before (review m6). + // DetachAsync swallows its own failures; a failed delete must not skip the detach. try { - DisposeBridge.Run(ct => _subscription.DeleteAsync(true, ct)); + DisposeBridge.Run(async ct => + { + try { await _subscription.DeleteAsync(true, ct); } + catch (Exception ex) { Logger.Warning(ex, "Error deleting subscription"); } + await DetachAsync(ct); + }); } catch { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs index f3d81e64..8734c50b 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/ISessionAdapter.cs @@ -90,14 +90,6 @@ internal interface ISessionAdapter : IDisposable Task<(byte[]? ContinuationPoint, ReferenceDescriptionCollection References)> BrowseNextAsync( byte[] continuationPoint, CancellationToken ct = default); - /// - /// Checks whether a node has any forward hierarchical child references. - /// - /// The node to inspect for child objects or variables. - /// The cancellation token that aborts the child lookup. - /// true when the node has at least one forward hierarchical child reference; otherwise false. - Task HasChildrenAsync(NodeId nodeId, CancellationToken ct = default); - /// /// Reads raw historical data. /// diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs index 44462199..23b70ede 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/AggregateTypeMapper.cs @@ -23,12 +23,17 @@ public static class AggregateTypeMapper AggregateType.Count => ObjectIds.AggregateFunction_Count, AggregateType.Start => ObjectIds.AggregateFunction_Start, AggregateType.End => ObjectIds.AggregateFunction_End, - AggregateType.StandardDeviation => ObjectIds.AggregateFunction_StandardDeviationPopulation, AggregateType.TimeAverage => ObjectIds.AggregateFunction_TimeAverage, AggregateType.TimeAverage2 => ObjectIds.AggregateFunction_TimeAverage2, AggregateType.Total => ObjectIds.AggregateFunction_Total, AggregateType.Total2 => ObjectIds.AggregateFunction_Total2, AggregateType.Interpolative => ObjectIds.AggregateFunction_Interpolative, + AggregateType.Range => ObjectIds.AggregateFunction_Range, + AggregateType.Delta => ObjectIds.AggregateFunction_Delta, + AggregateType.MinimumActualTime => ObjectIds.AggregateFunction_MinimumActualTime, + AggregateType.MaximumActualTime => ObjectIds.AggregateFunction_MaximumActualTime, + AggregateType.PercentGood => ObjectIds.AggregateFunction_PercentGood, + AggregateType.PercentBad => ObjectIds.AggregateFunction_PercentBad, _ => throw new ArgumentOutOfRangeException(nameof(aggregate), aggregate, "Unknown AggregateType value.") }; } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs new file mode 100644 index 00000000..16bb72b6 --- /dev/null +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Helpers/HistoryReadPaging.cs @@ -0,0 +1,96 @@ +using Opc.Ua; +using Serilog; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; + +/// +/// The client-side HistoryRead paging loop shared by the raw and modified reads. Continuation pages are +/// READ (releaseContinuationPoints = false) while the caller still wants values; a read stopped by the +/// caller's own cap while the server still holds a cursor releases that cursor so it does not occupy one of the +/// session's continuation-point slots until eviction. +/// +internal static class HistoryReadPaging +{ + private static readonly ILogger Logger = Log.ForContext(typeof(HistoryReadPaging)); + + /// Reads (or, with , releases) one page. + /// The cursor to resume, or for the first page. + /// to release the cursor instead of reading. + /// The caller's cancellation token. + /// The per-node result, or when the response carried none. + public delegate Task PageReader( + byte[]? continuationPoint, bool releaseContinuationPoints, CancellationToken ct); + + /// The bound on a best-effort release that does not ride the caller's (possibly cancelled) token. + internal static readonly TimeSpan ReleaseTimeout = TimeSpan.FromSeconds(5); + + /// Reads pages until the server finishes or the caller's cap is reached. + /// The element type extracted from each page. + /// Issues one HistoryRead for the single node. + /// Extracts the page's elements from a result. + /// The caller's cap; must be positive. The result is trimmed to it. + /// The node being read, for refusal messages. + /// The caller's cancellation token. + /// At most elements. + /// is not positive: an uncapped client + /// read would materialise a tag's whole window (the client side of review 08 1.1). + /// A page was refused (see ). + /// was cancelled mid-read; the outstanding + /// cursor is released (best effort) first. + public static async Task> ReadAllAsync( + PageReader readPage, Func> extract, int maxValues, NodeId nodeId, + CancellationToken ct) + { + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(maxValues); + + var all = new List(); + byte[]? continuationPoint = null; + + try + { + while (true) + { + var result = await readPage(continuationPoint, false, ct).ConfigureAwait(false); + if (result == null) return Trim(all, maxValues); + + HistoryReadStatus.ThrowIfRefused(result.StatusCode, nodeId); + all.AddRange(extract(result)); + + continuationPoint = result.ContinuationPoint; + if (continuationPoint is not { Length: > 0 }) return Trim(all, maxValues); + if (all.Count >= maxValues) break; + } + } + catch (OperationCanceledException) + { + // Cancelled between pages (or during page N+1) with a cursor outstanding: release it rather than leave it + // occupying one of the session's continuation-point slots, then surface the cancellation. + if (continuationPoint is { Length: > 0 }) await ReleaseAsync(readPage, continuationPoint, nodeId); + throw; + } + + // Stopped by the client's own cap: the server still holds a cursor for this session. Release it — on its own + // bounded token, so a cancellation racing the release cannot lose the values already in hand. + await ReleaseAsync(readPage, continuationPoint, nodeId); + return Trim(all, maxValues); + } + + private static List Trim(List all, int maxValues) + { + if (all.Count > maxValues) all.RemoveRange(maxValues, all.Count - maxValues); + return all; + } + + private static async Task ReleaseAsync(PageReader readPage, byte[] continuationPoint, NodeId nodeId) + { + try + { + using var cts = new CancellationTokenSource(ReleaseTimeout); + await readPage(continuationPoint, true, cts.Token).ConfigureAwait(false); + } + catch (Exception ex) + { + Logger.Warning(ex, "Releasing the HistoryRead continuation point for {NodeId} failed", nodeId); + } + } +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs index b7f48ab7..17dddc51 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AggregateType.cs @@ -1,43 +1,64 @@ namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Models; /// -/// Aggregate functions for processed history reads. +/// Aggregate functions for processed history reads — exactly the seventeen the server's SQL historian backends +/// serve (AggregateFamilies.Served; a parity test pins it). The Gateway backend serves a subset and refuses +/// the rest with BadAggregateNotSupported. /// +/// +/// Values are explicit so the pre-existing members keep their numbers. 6 (the former StandardDeviation, +/// which no backend serves) is retired; do not reuse it. +/// public enum AggregateType { /// Average of values in the interval. - Average, + Average = 0, /// Minimum value in the interval. - Minimum, + Minimum = 1, /// Maximum value in the interval. - Maximum, + Maximum = 2, /// Count of values in the interval. - Count, + Count = 3, /// First value in the interval. - Start, + Start = 4, /// Last value in the interval. - End, - - /// Population standard deviation of values in the interval. - StandardDeviation, + End = 5, /// Time-weighted average, interpolated bounds (Part 13 TimeAverage). - TimeAverage, + TimeAverage = 7, /// Time-weighted average, stepped bounds (Part 13 TimeAverage2). - TimeAverage2, + TimeAverage2 = 8, /// Time-weighted integral over the interval (Part 13 Total). - Total, + Total = 9, /// Time-weighted integral, stepped bounds (Part 13 Total2). - Total2, + Total2 = 10, /// Interpolated value at the interval start (Part 13 Interpolative). - Interpolative -} \ No newline at end of file + Interpolative = 11, + + /// Maximum minus minimum in the interval (Part 13 Range). + Range = 12, + + /// Last minus first value in the interval (Part 13 Delta). + Delta = 13, + + /// Minimum value with its own timestamp (Part 13 MinimumActualTime). + MinimumActualTime = 14, + + /// Maximum value with its own timestamp (Part 13 MaximumActualTime). + MaximumActualTime = 15, + + /// Percentage of the interval with Good data (Part 13 PercentGood). + PercentGood = 16, + + /// Percentage of the interval with Bad data (Part 13 PercentBad). + PercentBad = 17 +} diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs index 4e3658f8..d0b06838 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/BrowseResult.cs @@ -34,7 +34,9 @@ public sealed class BrowseResult public string NodeClass { get; } /// - /// Whether the node has child references. + /// Whether the node MAY have child references: for every Object, confirmed lazily when the + /// caller browses it (an expanded empty folder simply yields no children). Variables and Methods report + /// . /// public bool HasChildren { get; } } \ No newline at end of file diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs index c1d87129..272e3695 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -15,16 +15,17 @@ public sealed class OpcUaClientService : IOpcUaClientService { private static readonly ILogger Logger = Log.ForContext(); - // Guards all access to the subscription-bookkeeping state below - // (_activeDataSubscriptions and _activeAlarmSubscription). The dictionary - // and tuple are mutated from the caller thread, the keep-alive failover - // path, and DisconnectAsync, so every read/write must be inside this lock. + // Guards all access to the subscription state below (_activeDataSubscriptions, _activeAlarmSubscription, + // _dataSubscription, _alarmSubscription). They are touched from the caller thread, the keep-alive failover path + // and DisconnectAsync, so every read/write is inside this lock — including failover's null-outs. private readonly object _subscriptionLock = new(); - // Serialises SubscribeAlarmsAsync / UnsubscribeAlarmsAsync so concurrent callers - // cannot both pass the _alarmSubscription == null check and create duplicate event - // subscriptions. Capacity 1 = at most one waiter; async-friendly (no thread-block). - private readonly SemaphoreSlim _alarmSubscribeSemaphore = new(1, 1); + // Serialises every subscription operation that awaits I/O (Subscribe/Unsubscribe, SubscribeAlarms/ + // UnsubscribeAlarms) AND the failover's teardown + reconnect + replay. A subscribe is therefore either + // wholly before a failover (recorded, then replayed onto the new session) or wholly after it (on the new + // session) — never half on the dying session. Async-friendly (no thread block). Deliberately never disposed: + // async-only waits hold no kernel handle, and a failover still running at Dispose releases it afterwards. + private readonly SemaphoreSlim _subscriptionGate = new(1, 1); // Track active data subscriptions for replay after failover private readonly Dictionary _activeDataSubscriptions = new(); @@ -34,6 +35,15 @@ public sealed class OpcUaClientService : IOpcUaClientService // run at a time. 0 = idle, 1 = failover in progress (Interlocked-managed). private int _failoverInProgress; + // The connection epoch (review I1): ConnectAsync and DisconnectAsync each start a new one (under + // _subscriptionLock). A failover runs under the epoch it started in and abandons itself — disposing any session it + // opened, making no state transition — the moment the epoch moves, so a user Disconnect + Connect during a + // failover is never undone by it. A keep-alive from a previous connection's session cannot start one either. + private long _connectionEpoch; + + /// The most recent keep-alive-triggered failover task (test seam: lets a test await it). + internal Task? LastFailover { get; private set; } + private readonly IApplicationConfigurationFactory _configFactory; private readonly IEndpointDiscovery _endpointDiscovery; @@ -99,6 +109,15 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); settings.Validate(); + // Overwriting a live (or reconnecting) session would leak it and orphan its subscriptions and keep-alive + // handler. The caller must disconnect first. + if (_state != ConnectionState.Disconnected || _session != null) + throw new InvalidOperationException( + $"Already {_state.ToString().ToLowerInvariant()}; call DisconnectAsync before connecting again."); + + long epoch; + lock (_subscriptionLock) epoch = ++_connectionEpoch; + _settings = settings; _allEndpointUrls = FailoverUrlParser.Parse(settings.EndpointUrl, settings.FailoverUrls); _currentEndpointIndex = 0; @@ -108,11 +127,11 @@ public sealed class OpcUaClientService : IOpcUaClientService try { var session = await ConnectToEndpointAsync(settings, _allEndpointUrls[0], ct); - _session = session; + lock (_subscriptionLock) _session = session; session.RegisterKeepAliveHandler(isGood => { - if (!isGood) _ = HandleKeepAliveFailureAsync(); + if (!isGood) OnKeepAliveFailed(epoch); }); CurrentConnectionInfo = BuildConnectionInfo(session); @@ -133,6 +152,9 @@ public sealed class OpcUaClientService : IOpcUaClientService if (_state == ConnectionState.Disconnected) return; + // A new epoch first: a failover still connecting abandons itself instead of resurrecting the connection. + lock (_subscriptionLock) _connectionEpoch++; + var endpointUrl = _session?.EndpointUrl ?? _settings?.EndpointUrl ?? string.Empty; // Snapshot the subscription adapter references under the lock so a concurrent @@ -224,18 +246,15 @@ public sealed class OpcUaClientService : IOpcUaClientService while (references.Count > 0) { + // Child detection is lazy: an Object MAY have children and is confirmed when the caller expands or + // recurses into it. Probing each Object child with its own Browse made one level of N folders cost N + 1 + // round trips, and a recursive subscribe over a 50k-node tree ~50k extra Browse calls. foreach (var reference in references) - { - var childNodeId = ExpandedNodeId.ToNodeId(reference.NodeId, _session.NamespaceUris); - var hasChildren = reference.NodeClass == NodeClass.Object && - await _session.HasChildrenAsync(childNodeId, ct); - results.Add(new BrowseResult( reference.NodeId.ToString(), reference.DisplayName?.Text ?? string.Empty, reference.NodeClass.ToString(), - hasChildren)); - } + reference.NodeClass == NodeClass.Object)); if (continuationPoint is not { Length: > 0 } pending) break; (continuationPoint, references) = await _session.BrowseNextAsync(pending, ct); @@ -251,20 +270,39 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfNotConnected(); var nodeIdStr = nodeId.ToString(); - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - if (_activeDataSubscriptions.ContainsKey(nodeIdStr)) - return; // Already subscribed + // A failover may have run (and failed) while this call waited for the gate. + ThrowIfNotConnected(); + + ISubscriptionAdapter? subscription; + ISessionAdapter session; + lock (_subscriptionLock) + { + if (_activeDataSubscriptions.ContainsKey(nodeIdStr)) + return; // Already subscribed + subscription = _dataSubscription; + session = _session!; + } + + if (subscription == null) + { + subscription = await session.CreateSubscriptionAsync(intervalMs, ct); + lock (_subscriptionLock) _dataSubscription = subscription; + } + + var handle = await subscription.AddDataChangeMonitoredItemAsync( + nodeId, intervalMs, OnDataChangeNotification, ct); + + lock (_subscriptionLock) + { + _activeDataSubscriptions[nodeIdStr] = (nodeId, intervalMs, handle); + } } - - if (_dataSubscription == null) _dataSubscription = await _session!.CreateSubscriptionAsync(intervalMs, ct); - - var handle = await _dataSubscription.AddDataChangeMonitoredItemAsync( - nodeId, intervalMs, OnDataChangeNotification, ct); - - lock (_subscriptionLock) + finally { - _activeDataSubscriptions[nodeIdStr] = (nodeId, intervalMs, handle); + _subscriptionGate.Release(); } Logger.Debug("Subscribed to data changes on {NodeId}", nodeId); @@ -276,18 +314,35 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); var nodeIdStr = nodeId.ToString(); - (NodeId NodeId, int IntervalMs, uint Handle) sub; - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - if (!_activeDataSubscriptions.TryGetValue(nodeIdStr, out sub)) - return; // Not subscribed, safe to ignore + (NodeId NodeId, int IntervalMs, uint Handle) sub; + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) + { + if (!_activeDataSubscriptions.TryGetValue(nodeIdStr, out sub)) + return; // Not subscribed, safe to ignore + subscription = _dataSubscription; + } + + try + { + if (subscription != null) await subscription.RemoveMonitoredItemAsync(sub.Handle, ct); + } + finally + { + // The caller asked to stop: drop the bookkeeping even when the server-side remove failed (a session + // that died under us), or a later failover would replay the item onto the new session (review m5). + lock (_subscriptionLock) + { + _activeDataSubscriptions.Remove(nodeIdStr); + } + } } - - if (_dataSubscription != null) await _dataSubscription.RemoveMonitoredItemAsync(sub.Handle, ct); - - lock (_subscriptionLock) + finally { - _activeDataSubscriptions.Remove(nodeIdStr); + _subscriptionGate.Release(); } Logger.Debug("Unsubscribed from data changes on {NodeId}", nodeId); @@ -303,17 +358,25 @@ public sealed class OpcUaClientService : IOpcUaClientService // Serialise check-and-create under the semaphore: concurrent // callers cannot both pass the null-check and create duplicate alarm subscriptions. // The semaphore is async-friendly; the subscription I/O runs inside the critical section. - await _alarmSubscribeSemaphore.WaitAsync(ct); + await _subscriptionGate.WaitAsync(ct); try { - if (_alarmSubscription != null) - return; // Already subscribed to alarms + ThrowIfNotConnected(); + + ISessionAdapter session; + lock (_subscriptionLock) + { + if (_alarmSubscription != null) + return; // Already subscribed to alarms + session = _session!; + } var monitorNode = sourceNodeId ?? ObjectIds.Server; - _alarmSubscription = await _session!.CreateSubscriptionAsync(intervalMs, ct); + var subscription = await session.CreateSubscriptionAsync(intervalMs, ct); + lock (_subscriptionLock) _alarmSubscription = subscription; var filter = CreateAlarmEventFilter(); - await _alarmSubscription.AddEventMonitoredItemAsync( + await subscription.AddEventMonitoredItemAsync( monitorNode, intervalMs, filter, OnAlarmEventNotification, ct); lock (_subscriptionLock) @@ -325,7 +388,7 @@ public sealed class OpcUaClientService : IOpcUaClientService } finally { - _alarmSubscribeSemaphore.Release(); + _subscriptionGate.Release(); } } @@ -334,15 +397,27 @@ public sealed class OpcUaClientService : IOpcUaClientService { ThrowIfDisposed(); - if (_alarmSubscription == null) - return; - - await _alarmSubscription.DeleteAsync(ct); - _alarmSubscription = null; - - lock (_subscriptionLock) + await _subscriptionGate.WaitAsync(ct); + try { - _activeAlarmSubscription = null; + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) + { + subscription = _alarmSubscription; + if (subscription == null) return; + } + + await subscription.DeleteAsync(ct); + + lock (_subscriptionLock) + { + _alarmSubscription = null; + _activeAlarmSubscription = null; + } + } + finally + { + _subscriptionGate.Release(); } Logger.Debug("Unsubscribed from alarm events"); @@ -354,10 +429,12 @@ public sealed class OpcUaClientService : IOpcUaClientService ThrowIfDisposed(); ThrowIfNotConnected(); - if (_alarmSubscription == null) + ISubscriptionAdapter? subscription; + lock (_subscriptionLock) subscription = _alarmSubscription; + if (subscription == null) throw new InvalidOperationException("No alarm subscription is active."); - await _alarmSubscription.ConditionRefreshAsync(ct); + await subscription.ConditionRefreshAsync(ct); Logger.Debug("Condition refresh requested"); } @@ -640,13 +717,16 @@ public sealed class OpcUaClientService : IOpcUaClientService /// public void Dispose() { - if (_disposed) return; - _disposed = true; + lock (_subscriptionLock) + { + if (_disposed) return; + _disposed = true; + _connectionEpoch++; // a failover still running abandons itself + } _dataSubscription?.Dispose(); _alarmSubscription?.Dispose(); _session?.Dispose(); - _alarmSubscribeSemaphore.Dispose(); lock (_subscriptionLock) { @@ -688,19 +768,27 @@ public sealed class OpcUaClientService : IOpcUaClientService ct); } - private async Task HandleKeepAliveFailureAsync() + /// A bad keep-alive from the session of connection . + private void OnKeepAliveFailed(long epoch) { - // Serialize failover: the OPC UA stack raises KeepAlive repeatedly - // while a session is down, so multiple bad keep-alives can fire before - // the first failover loop finishes. CompareExchange atomically claims - // the failover slot; a re-entrant call sees 1 and returns immediately, - // guaranteeing exactly one failover loop runs at a time. + // A late keep-alive from a previous connection's session is not about the current one. + if (!IsCurrentEpoch(epoch)) return; + + // Serialize failover: the OPC UA stack raises KeepAlive repeatedly while a session is down, so several bad + // keep-alives can fire before the first failover loop finishes. CompareExchange claims the failover slot; a + // re-entrant call sees 1 and returns, so exactly one loop runs — and only the winner sets the test seam + // (review m8: a re-entrant call used to overwrite it with an already-completed no-op). if (Interlocked.CompareExchange(ref _failoverInProgress, 1, 0) != 0) return; + LastFailover = HandleKeepAliveFailureAsync(epoch); + } + + private async Task HandleKeepAliveFailureAsync(long epoch) + { try { - await RunFailoverAsync(); + await RunFailoverAsync(epoch); } finally { @@ -708,74 +796,136 @@ public sealed class OpcUaClientService : IOpcUaClientService } } - private async Task RunFailoverAsync() + private bool IsCurrentEpoch(long epoch) + { + lock (_subscriptionLock) return _connectionEpoch == epoch && !_disposed; + } + + /// + /// A state transition made only while is still current (checked and applied under + /// , the event raised outside it). + /// + /// when the epoch has moved on (the caller must abandon). + private bool TransitionStateIfCurrent(long epoch, ConnectionState newState, string endpointUrl) + { + ConnectionState oldState; + lock (_subscriptionLock) + { + if (_connectionEpoch != epoch || _disposed) return false; + oldState = _state; + if (oldState == newState) return true; + _state = newState; + } + + ConnectionStateChanged?.Invoke(this, new ConnectionStateChangedEventArgs(oldState, newState, endpointUrl)); + return true; + } + + private async Task RunFailoverAsync(long epoch) { if (_state == ConnectionState.Reconnecting || _state == ConnectionState.Disconnected) return; - var oldEndpoint = _session?.EndpointUrl ?? string.Empty; - TransitionState(ConnectionState.Reconnecting, oldEndpoint); + var oldSession = _session; + var oldEndpoint = oldSession?.EndpointUrl ?? string.Empty; + if (!TransitionStateIfCurrent(epoch, ConnectionState.Reconnecting, oldEndpoint)) return; Logger.Warning("Session lost on {EndpointUrl}. Attempting failover...", oldEndpoint); - // Close old session - if (_session != null) + // Dispose the dead session BEFORE taking the gate: an in-flight subscribe holding the gate is blocked on + // that session, and disposing it is what makes the call fail fast and release the gate. + if (oldSession != null) { try { - _session.Dispose(); + oldSession.Dispose(); } catch { } - - _session = null; } - _dataSubscription = null; - _alarmSubscription = null; - - if (_settings == null || _allEndpointUrls == null) + // The rest runs under the subscription gate (and its null-outs under _subscriptionLock): no subscribe can + // interleave with the teardown, the reconnect or the replay. + await _subscriptionGate.WaitAsync(); + try { - TransitionState(ConnectionState.Disconnected, oldEndpoint); - return; - } - - // Try each endpoint - for (var attempt = 0; attempt < _allEndpointUrls.Length; attempt++) - { - _currentEndpointIndex = (_currentEndpointIndex + 1) % _allEndpointUrls.Length; - var url = _allEndpointUrls[_currentEndpointIndex]; - - try + // The user's settings / endpoint list as of THIS epoch: a Connect during the failover replaces the fields. + ConnectionSettings? settings; + string[]? endpoints; + lock (_subscriptionLock) { - Logger.Information("Failover attempt to {EndpointUrl}", url); - var session = await ConnectToEndpointAsync(_settings, url, CancellationToken.None); - _session = session; + if (_connectionEpoch != epoch || _disposed) return; // the user disconnected / reconnected meanwhile + if (ReferenceEquals(_session, oldSession)) _session = null; + _dataSubscription = null; + _alarmSubscription = null; + settings = _settings; + endpoints = _allEndpointUrls; + } - session.RegisterKeepAliveHandler(isGood => - { - if (!isGood) _ = HandleKeepAliveFailureAsync(); - }); - - CurrentConnectionInfo = BuildConnectionInfo(session); - TransitionState(ConnectionState.Connected, url); - Logger.Information("Failover succeeded to {EndpointUrl}", url); - - // Replay subscriptions - await ReplaySubscriptionsAsync(); + if (settings == null || endpoints == null) + { + TransitionStateIfCurrent(epoch, ConnectionState.Disconnected, oldEndpoint); return; } - catch (Exception ex) - { - Logger.Warning(ex, "Failover to {EndpointUrl} failed", url); - } - } - Logger.Error("All failover endpoints unreachable"); - TransitionState(ConnectionState.Disconnected, oldEndpoint); + // Try each endpoint + for (var attempt = 0; attempt < endpoints.Length; attempt++) + { + if (!IsCurrentEpoch(epoch)) return; // stop trying once the user has moved on (m5) + _currentEndpointIndex = (_currentEndpointIndex + 1) % endpoints.Length; + var url = endpoints[_currentEndpointIndex]; + + try + { + Logger.Information("Failover attempt to {EndpointUrl}", url); + var session = await ConnectToEndpointAsync(settings, url, CancellationToken.None); + + // A Disconnect / Connect / Dispose that ran while this attempt was connecting wins: never + // resurrect the old connection or overwrite the user's new session. Checked and installed + // atomically against the epoch. + bool installed; + lock (_subscriptionLock) + { + installed = _connectionEpoch == epoch && !_disposed; + if (installed) _session = session; + } + + if (!installed) + { + session.Dispose(); + return; + } + + session.RegisterKeepAliveHandler(isGood => + { + if (!isGood) OnKeepAliveFailed(epoch); + }); + + CurrentConnectionInfo = BuildConnectionInfo(session); + if (!TransitionStateIfCurrent(epoch, ConnectionState.Connected, url)) return; + Logger.Information("Failover succeeded to {EndpointUrl}", url); + + // Replay subscriptions + await ReplaySubscriptionsAsync(session); + return; + } + catch (Exception ex) + { + Logger.Warning(ex, "Failover to {EndpointUrl} failed", url); + } + } + + if (TransitionStateIfCurrent(epoch, ConnectionState.Disconnected, oldEndpoint)) + Logger.Error("All failover endpoints unreachable"); + } + finally + { + _subscriptionGate.Release(); + } } - private async Task ReplaySubscriptionsAsync() + /// Re-creates the recorded subscriptions on . Runs under the subscription gate. + private async Task ReplaySubscriptionsAsync(ISessionAdapter session) { // Snapshot the bookkeeping state under the lock, then clear it so the // replayed handles can be recorded fresh as each monitored item is @@ -791,13 +941,17 @@ public sealed class OpcUaClientService : IOpcUaClientService } // Replay data subscriptions + ISubscriptionAdapter? dataSubscription = null; foreach (var (nodeIdStr, (nodeId, intervalMs, _)) in subscriptions) try { - if (_dataSubscription == null) - _dataSubscription = await _session!.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + if (dataSubscription == null) + { + dataSubscription = await session.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + lock (_subscriptionLock) _dataSubscription = dataSubscription; + } - var handle = await _dataSubscription.AddDataChangeMonitoredItemAsync( + var handle = await dataSubscription.AddDataChangeMonitoredItemAsync( nodeId, intervalMs, OnDataChangeNotification, CancellationToken.None); lock (_subscriptionLock) @@ -817,9 +971,10 @@ public sealed class OpcUaClientService : IOpcUaClientService try { var monitorNode = sourceNodeId ?? ObjectIds.Server; - _alarmSubscription = await _session!.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + var replayedAlarms = await session.CreateSubscriptionAsync(intervalMs, CancellationToken.None); + lock (_subscriptionLock) _alarmSubscription = replayedAlarms; var filter = CreateAlarmEventFilter(); - await _alarmSubscription.AddEventMonitoredItemAsync( + await replayedAlarms.AddEventMonitoredItemAsync( monitorNode, intervalMs, filter, OnAlarmEventNotification, CancellationToken.None); lock (_subscriptionLock) @@ -937,9 +1092,13 @@ public sealed class OpcUaClientService : IOpcUaClientService private void TransitionState(ConnectionState newState, string endpointUrl) { - var oldState = _state; - if (oldState == newState) return; - _state = newState; + ConnectionState oldState; + lock (_subscriptionLock) + { + oldState = _state; + if (oldState == newState) return; + _state = newState; + } ConnectionStateChanged?.Invoke(this, new ConnectionStateChangedEventArgs(oldState, newState, endpointUrl)); } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs index aab55811..b883be03 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/HistoryViewModel.cs @@ -43,23 +43,9 @@ public partial class HistoryViewModel : ObservableObject _dispatcher = dispatcher; } - /// Available aggregate types (null means "Raw"). + /// Available aggregate types (null means "Raw"): every client aggregate, which is the served set. public IReadOnlyList AggregateTypes { get; } = - [ - null, - AggregateType.Average, - AggregateType.Minimum, - AggregateType.Maximum, - AggregateType.Count, - AggregateType.Start, - AggregateType.End, - AggregateType.StandardDeviation, - AggregateType.TimeAverage, - AggregateType.TimeAverage2, - AggregateType.Total, - AggregateType.Total2, - AggregateType.Interpolative - ]; + [null, .. Enum.GetValues().Select(a => (AggregateType?)a)]; /// Gets a value indicating whether an aggregate read is selected. public bool IsAggregateRead => SelectedAggregateType != null; diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs index 381719a4..18d2bee4 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs @@ -1,4 +1,5 @@ using System.Collections.ObjectModel; +using System.Collections.Specialized; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using Opc.Ua; @@ -19,6 +20,11 @@ public partial class SubscriptionsViewModel : ObservableObject private readonly IUiDispatcher _dispatcher; private readonly IOpcUaClientService _service; + // NodeId -> its rows, kept in step with ActiveSubscriptions by OnActiveSubscriptionsChanged, so a notification is + // one lookup instead of a scan of every row on the UI thread (2 000 items at 1 Hz was 4M string compares/s). + // Touched only on the UI thread, like the collection itself. + private readonly Dictionary> _rowsByNodeId = new(StringComparer.Ordinal); + /// /// Last user-visible status message — set when a subscribe/unsubscribe operation fails so the /// shell can surface the diagnostic instead of silently dropping the error. Cleared on success. @@ -49,9 +55,13 @@ public partial class SubscriptionsViewModel : ObservableObject { _service = service; _dispatcher = dispatcher; + ActiveSubscriptions.CollectionChanged += OnActiveSubscriptionsChanged; _service.DataChanged += OnDataChanged; } + /// The number of distinct NodeIds in the notification index (test seam). + internal int IndexedNodeCount => _rowsByNodeId.Count; + /// Currently active subscriptions. public ObservableCollection ActiveSubscriptions { get; } = []; @@ -62,16 +72,42 @@ public partial class SubscriptionsViewModel : ObservableObject { _dispatcher.Post(() => { - foreach (var item in ActiveSubscriptions) - if (item.NodeId == e.NodeId) - { - item.Value = Helpers.ValueFormatter.Format(e.Value.Value); - item.Status = Helpers.StatusCodeFormatter.Format(e.Value.StatusCode); - item.Timestamp = e.Value.SourceTimestamp.ToString("O"); - } + if (!_rowsByNodeId.TryGetValue(e.NodeId, out var rows)) return; + foreach (var item in rows) + { + item.Value = Helpers.ValueFormatter.Format(e.Value.Value); + item.Status = Helpers.StatusCodeFormatter.Format(e.Value.StatusCode); + item.Timestamp = e.Value.SourceTimestamp.ToString("O"); + } }); } + private void OnActiveSubscriptionsChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + if (e.Action == NotifyCollectionChangedAction.Reset) + { + _rowsByNodeId.Clear(); + foreach (var item in ActiveSubscriptions) Index(item); + return; + } + + if (e.OldItems != null) + foreach (SubscriptionItemViewModel item in e.OldItems) + if (_rowsByNodeId.TryGetValue(item.NodeId, out var rows) && rows.Remove(item) && rows.Count == 0) + _rowsByNodeId.Remove(item.NodeId); + + if (e.NewItems != null) + foreach (SubscriptionItemViewModel item in e.NewItems) + Index(item); + } + + private void Index(SubscriptionItemViewModel item) + { + if (!_rowsByNodeId.TryGetValue(item.NodeId, out var rows)) + _rowsByNodeId[item.NodeId] = rows = []; + rows.Add(item); + } + private bool CanAddSubscription() { return IsConnected && !string.IsNullOrWhiteSpace(NewNodeIdText); @@ -149,7 +185,7 @@ public partial class SubscriptionsViewModel : ObservableObject if (!IsConnected || string.IsNullOrWhiteSpace(nodeIdStr)) return; // Skip if already subscribed - if (ActiveSubscriptions.Any(s => s.NodeId == nodeIdStr)) return; + if (_rowsByNodeId.ContainsKey(nodeIdStr)) return; try { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs index 804ff1bc..c914d160 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs @@ -44,17 +44,21 @@ public sealed class ProbeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync(["__probe"], ct); - var health = driver.GetHealth(); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync(["__probe"], ct); + var health = driver.GetHealth(); - await console.Output.WriteLineAsync($"Gateway: {Gateway}"); - await console.Output.WriteLineAsync($"Family: {Family}"); - await console.Output.WriteLineAsync($"Health: {health.State}"); - if (health.LastError is { } err) - await console.Output.WriteLineAsync($"Last error: {err}"); - await console.Output.WriteLineAsync(); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + await console.Output.WriteLineAsync($"Gateway: {Gateway}"); + await console.Output.WriteLineAsync($"Family: {Family}"); + await console.Output.WriteLineAsync($"Health: {health.State}"); + if (health.LastError is { } err) + await console.Output.WriteLineAsync($"Last error: {err}"); + await console.Output.WriteLineAsync(); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs index 0d0bbb80..0e4b5ad4 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs @@ -45,9 +45,13 @@ public sealed class ReadCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var snapshot = await driver.ReadAsync([tagName], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var snapshot = await driver.ReadAsync([tagName], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.Format(TagPath, snapshot[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs index b266fa3d..399b3a0e 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs @@ -50,23 +50,22 @@ public sealed class SubscribeCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); ISubscriptionHandle? handle = null; + // The poll engine raises OnDataChange on a background thread: serialise its lines against each other and + // against this thread's own, and never let a failed console write fault the poll loop (review 07 U7). + var writeLock = new object(); try { await driver.InitializeAsync("{}", ct); - await console.Output.WriteLineAsync( - $"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); - - driver.OnDataChange += (_, e) => - { - var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + - $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + - $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; - console.Output.WriteLine(line); - }; + driver.OnDataChange += CreateChangeLineWriter(console.Output, writeLock); handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); + lock (writeLock) + { + console.Output.WriteLine($"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop."); + } + try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -76,6 +75,10 @@ public sealed class SubscribeCommand : AbCipCommandBase // Expected on Ctrl+C. } } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Ctrl+C while still connecting/subscribing: a clean exit, not a stack trace (review 07 U7). + } finally { if (handle is not null) diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs index e4dadd38..b4473e02 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs @@ -57,9 +57,13 @@ public sealed class WriteCommand : AbCipCommandBase await using var driver = new AbCipDriver(options, DriverInstanceId); try { - await driver.InitializeAsync("{}", ct); - var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); - await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + // Ctrl-C prints "Cancelled." instead of an OperationCanceledException stack trace (review 07 U7). + await RunCancellableAsync(console, ct, async () => + { + await driver.InitializeAsync("{}", ct); + var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct); + await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(TagPath, results[0])); + }); } finally { diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs index 157caf2d..e96b1a75 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli/AbLegacyCommandBase.cs @@ -32,27 +32,6 @@ public abstract class AbLegacyCommandBase : DriverCommandBase "LogixPccc unverified).")] public AbLegacyCliTransport Transport { get; init; } = AbLegacyCliTransport.Default; - /// - /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints - /// Cancelled. instead of an unhandled stack trace (review 07 U7). - /// Any other exception — and an OCE the operator did not ask for — still propagates. - /// - /// The CLI console. - /// The console's Ctrl-C token. - /// The command body. - /// A task that completes when the body has finished or was cancelled. - internal static async ValueTask RunCancellableAsync(CliFx.Infrastructure.IConsole console, CancellationToken ct, Func body) - { - try - { - await body(); - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) - { - await console.Error.WriteLineAsync("Cancelled."); - } - } - /// Gets the per-operation timeout in milliseconds. [CommandOption("timeout-ms", Description = "Per-operation timeout in ms (default 5000).")] public int TimeoutMs { get; init; } = 5000; diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs index d77bff85..d5a087fd 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs @@ -79,5 +79,61 @@ public abstract class DriverCommandBase : ICommand /// protected static void FlushLogging() => Log.CloseAndFlush(); + /// + /// Runs a one-shot command's driver work so a Ctrl-C (cancellation of ) prints + /// Cancelled. instead of an unhandled stack trace (review 07 U7). + /// Any other exception — and an OCE the operator did not ask for — still propagates. + /// + /// The CLI console. + /// The console's Ctrl-C token. + /// The command body. + /// A task that completes when the body has finished or was cancelled. + public static async ValueTask RunCancellableAsync(IConsole console, CancellationToken ct, Func body) + { + try + { + await body(); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + await console.Error.WriteLineAsync("Cancelled."); + } + } + + /// + /// Builds the OnDataChange handler a subscribe command attaches: one + /// [HH:mm:ss.fff] ref = value (status) line per change. Writes are serialised on + /// (the poll engine raises from a background thread, concurrently with the + /// command thread's own lines), and a failing console write (closed pipe, redirected stream error) is logged + /// and swallowed so it cannot fault the driver's poll loop (review 07 U7). + /// + /// The console output writer. + /// The lock every console write of the command takes. + /// The event handler. + public static EventHandler CreateChangeLineWriter( + TextWriter output, object writeLock) + { + ArgumentNullException.ThrowIfNull(output); + ArgumentNullException.ThrowIfNull(writeLock); + return (_, e) => + { + try + { + var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " + + $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " + + $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})"; + lock (writeLock) + { + output.WriteLine(line); + } + } + catch (Exception ex) + { + Log.Logger.Warning(ex, "subscribe: console write failed for {Tag}; continuing the poll loop.", + e.FullReference); + } + }; + } + private bool _loggingConfigured; } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs index 906fee8e..9a49acdb 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs @@ -101,7 +101,11 @@ public static class SnapshotFormatter return sb.ToString().TrimEnd(); } - /// Formats a value for console output, handling null, bool, string, and formattable types. + /// + /// Formats a value for console output, handling null, bool, string, formattable and array/list types. An + /// array renders its elements ([1, 2, 3], each formatted as a scalar) rather than its CLR type name + /// (System.Int32[]). + /// /// The value to format. /// A formatted string representation of the value. public static string FormatValue(object? value) => value switch @@ -110,9 +114,23 @@ public static class SnapshotFormatter bool b => b ? "true" : "false", string s => $"\"{s}\"", IFormattable f => f.ToString(null, CultureInfo.InvariantCulture), + System.Collections.IEnumerable items => FormatSequence(items), _ => value.ToString() ?? "", }; + private static string FormatSequence(System.Collections.IEnumerable items) + { + var sb = new System.Text.StringBuilder("["); + var first = true; + foreach (var item in items) + { + if (!first) sb.Append(", "); + sb.Append(FormatValue(item)); + first = false; + } + return sb.Append(']').ToString(); + } + /// Formats an OPC UA status code as a hexadecimal string with named severity classification. /// The OPC UA status code to format. /// A formatted status string like "0x00000000 (Good)" or "0x80050000 (BadCommunicationError)". diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs new file mode 100644 index 00000000..e68d4daf --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbDeviceReplyException.cs @@ -0,0 +1,12 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; + +/// +/// An whose status came OFF THE WIRE: the device (or the bridge's Execute PCCC +/// object) replied with a non-zero CIP envelope status or PCCC STS. It lets a driver tell a refusal the device +/// ANSWERED from the same raised locally before anything was sent — most importantly +/// , which a native runtime also throws for operations it refuses pre-wire +/// (review lane K I2). +/// +/// The status mapped from the reply. +/// The exception message. +public sealed class AbDeviceReplyException(AbStatus status, string message) : AbTransportException(status, message); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs index b9d8d674..39c5b41d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbStatusCodes.cs @@ -1,39 +1,81 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +/// +/// Maps wire statuses (CIP general/extended, PCCC STS/EXT STS) to the transport-neutral the +/// drivers classify. The class an lands in is load-bearing for driver health (#527/#737/#738): +/// an ANSWERED refusal (NotFound / NotAllowed / OutOfBounds / TooLarge / TooSmall / TypeMismatch / +/// DeviceStateConflict, and for AbCip Unsupported) proves the device replied; +/// is the no-reply comms class; / are a reply that +/// cannot be classified. Every code not named below is deliberately . +/// public static class AbStatusCodes { /// Maps a CIP general status (and optional extended status words) to an . /// The CIP general status byte. /// The extended status words, if any. /// The mapped status. + /// + /// 0x01 (connection failure, incl. a routing hop's unconnected-send timeout) and 0x07 (connection lost) stay + /// on purpose: a bridge reports a dead END device that way, so they are neither an + /// answer nor proof of no reply (AbLegacy #738 "opaque"). A Forward Open refused with one of the request-level + /// codes (0x1C / 0x20 / 0xFF-0x2104) stays a connection failure — see + /// ConnectionManagerServices.ParseForwardOpenReply. + /// public static AbStatus FromCip(byte general, ReadOnlySpan extended) => general switch { - 0x00 or 0x0B => AbStatus.Ok, - 0x04 or 0x05 or 0x16 => AbStatus.NotFound, - 0x06 => AbStatus.Partial, - 0x08 => AbStatus.Unsupported, - 0x0A => AbStatus.OutOfBounds, - 0x0C or 0x10 => AbStatus.DeviceStateConflict, - 0x0E or 0x0F => AbStatus.NotAllowed, - 0x13 => AbStatus.TooSmall, - 0x15 => AbStatus.TooLarge, - 0xFF when extended.Length > 0 && extended[0] == 0x2105 => AbStatus.OutOfBounds, - 0xFF when extended.Length > 0 && extended[0] == 0x2107 => AbStatus.TypeMismatch, + 0x00 or 0x0B => AbStatus.Ok, // success / already in requested mode + 0x04 or 0x05 or 0x16 => AbStatus.NotFound, // path segment error / path unknown / object does not exist + 0x06 => AbStatus.Partial, // partial transfer (fragmented read continues) + 0x08 => AbStatus.Unsupported, // service not supported + 0x0A => AbStatus.OutOfBounds, // attribute list error + 0x0C or 0x10 => AbStatus.DeviceStateConflict, // object state conflict / device state conflict + 0x0E or 0x0F => AbStatus.NotAllowed, // attribute not settable / privilege violation + 0x13 => AbStatus.TooSmall, // not enough data + 0x15 => AbStatus.TooLarge, // too much data + // Lane K F1 (#737): the controller answered and refused this one request — not a garbled reply, so not a + // comms fault that degrades the driver and evicts the runtime. + 0x1C => AbStatus.Unsupported, // missing attribute list entry data + 0x20 => AbStatus.Unsupported, // invalid parameter + 0xFF when extended.Length > 0 && extended[0] == 0x2104 => AbStatus.OutOfBounds, // offset beyond end of tag + 0xFF when extended.Length > 0 && extended[0] == 0x2105 => AbStatus.OutOfBounds, // beyond end of the object + 0xFF when extended.Length > 0 && extended[0] == 0x2107 => AbStatus.TypeMismatch, // data type mismatch _ => AbStatus.BadReply, }; + /// + /// True for the CIP statuses Lane K F1 moved from to a request-level refusal. + /// A Forward Open refused with one of them is still a connection failure. + /// + /// The CIP general status byte. + /// The extended status words, if any. + /// for 0x1C, 0x20 and 0xFF/0x2104. + internal static bool IsRequestLevelRefusal(byte general, ReadOnlySpan extended) => + general is 0x1C or 0x20 || (general == 0xFF && extended.Length > 0 && extended[0] == 0x2104); + /// Maps a PCCC status byte (and optional extended status) to an . /// The PCCC STS byte. /// The extended status byte, when present. /// The mapped status. + /// + /// Per the DF1/PCCC STS table (1770-6.5.16). Lane K F3 (AbLegacy F8, review 07 r0 I4): 0x20, 0x30 and 0x90 are + /// reported by the HOST or a BRIDGE (a DHRIO, a 1761-NET-ENI) when the END device cannot be reached, so they are + /// the no-reply comms class. Mapping them to an answered refusal made a dead DH+/DH-485 node behind a bridge count + /// as reachable forever — no fail-fast backoff, no comms-loss watchdog. + /// public static AbStatus FromPccc(byte sts, byte? extSts) => sts switch { 0x00 => AbStatus.Ok, - 0x10 => AbStatus.Unsupported, - 0x20 => AbStatus.NotFound, - 0x30 => AbStatus.NotAllowed, - 0x40 or 0x50 => AbStatus.DeviceStateConflict, - 0xF0 when extSts is 0x06 or 0x07 => AbStatus.NotFound, - _ => AbStatus.BadReply, + 0x10 => AbStatus.Unsupported, // illegal command or format + 0x20 => AbStatus.BadConnection, // host has a problem and will not communicate + 0x30 => AbStatus.BadConnection, // remote node host missing, disconnected or shut down + 0x40 => AbStatus.DeviceStateConflict, // host could not complete: hardware fault + 0x50 => AbStatus.NotFound, // addressing problem or memory protect rungs + 0x60 => AbStatus.NotAllowed, // function not allowed due to command protection + 0x70 => AbStatus.DeviceStateConflict, // processor is in Program mode + 0x90 => AbStatus.BadConnection, // remote node cannot buffer command + 0xB0 => AbStatus.DeviceStateConflict, // remote node problem due to download + 0xF0 when extSts is 0x06 => AbStatus.NotFound, // EXT: address does not point to something usable + 0xF0 when extSts is 0x07 => AbStatus.OutOfBounds, // EXT: file is wrong size + _ => AbStatus.BadReply, // 0x80, 0xA0, 0xC0, other EXT STS: unclassified }; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs index 95d2dee7..91d59600 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs @@ -85,8 +85,15 @@ public static class ConnectionManagerServices public static ForwardOpenReply ParseForwardOpenReply(CipReply reply) { if (reply.GeneralStatus != 0) - throw new ForwardOpenRefusedException(reply.Status == AbStatus.BadReply ? AbStatus.BadConnection : reply.Status, - reply.GeneralStatus, reply.ExtendedStatus); + { + // An unclassified refusal, or one of the request-level codes AbStatusCodes maps to a per-request refusal + // for TAG services (0x1C / 0x20 / 0xFF-0x2104), is a connection failure here: no tag can be served. + var status = reply.Status == AbStatus.BadReply + || AbStatusCodes.IsRequestLevelRefusal(reply.GeneralStatus, reply.ExtendedStatus) + ? AbStatus.BadConnection + : reply.Status; + throw new ForwardOpenRefusedException(status, reply.GeneralStatus, reply.ExtendedStatus); + } var r = new CipReader(reply.Data.Span); var ot = r.U32(); var to = r.U32(); r.U16(); r.U16(); r.U32(); return new ForwardOpenReply(ot, to, r.U32(), r.U32()); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs index b746745e..8c911779 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs @@ -47,6 +47,10 @@ public sealed class EncapsulationClient : IAsyncDisposable private readonly SemaphoreSlim _gate = new(1, 1); private long _ctx; private ushort _seq; // connected sequence count; only touched under _gate + + // R07:L5: the 24-byte header of the frame being read. Frames are read only by RunExchangeAsync, which holds _gate + // for the whole exchange, so one buffer per session serves every read (it used to be a fresh array per reply). + private readonly byte[] _head = new byte[EncapsulationHeader.Size]; private volatile bool _faulted; private int _disposed; @@ -282,11 +286,12 @@ public sealed class EncapsulationClient : IAsyncDisposable private async Task ReadFrameAsync(CancellationToken ct) { - var head = new byte[EncapsulationHeader.Size]; - await _stream.ReadExactlyAsync(head, ct).ConfigureAwait(false); - var len = head[2] | (head[3] << 8); + // Only called under _gate (RunExchangeAsync), so the shared header buffer is never read concurrently. The frame + // itself is allocated exactly: the matched reply's CIP payload is returned as a slice of it and outlives the call. + await _stream.ReadExactlyAsync(_head, ct).ConfigureAwait(false); + var len = _head[2] | (_head[3] << 8); var frame = new byte[EncapsulationHeader.Size + len]; - head.CopyTo(frame, 0); + _head.CopyTo(frame, 0); await _stream.ReadExactlyAsync(frame.AsMemory(EncapsulationHeader.Size), ct).ConfigureAwait(false); return frame; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs index ba63cc38..85987cce 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Logix/LogixTagClient.cs @@ -1,3 +1,4 @@ +using System.Buffers; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Logix; @@ -52,34 +53,61 @@ public sealed class LogixTagClient(RequestScheduler scheduler, ICipChannel chann var reply = await _scheduler.SendAsync(LogixServices.ReadTagFragmented(path, elements, 0), ct, expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, 0)).ConfigureAwait(false); var (type, handle, first) = LogixServices.ParseReadReply(reply); // throws on any status but 0x00/0x06 - var data = new List(first.Length); - data.AddRange(first.Span); - var lastChunk = first.Length; - while (reply.GeneralStatus == StatusPartial) + // R07:L5: the common single-reply read is one copy of the reply data (it used to go through a List and a + // second ToArray); a fragmented read reassembles in a pooled buffer and copies out once at the end. + if (reply.GeneralStatus != StatusPartial) return (type, handle, first.ToArray()); + + var data = ArrayPool.Shared.Rent(Math.Max(first.Length * 4, 1024)); + var count = 0; + try { - if (lastChunk == 0) - throw new AbTransportException(AbStatus.BadReply, "Partial read reply carried no data; the fragment loop cannot progress."); - reply = await _scheduler.SendAsync( - LogixServices.ReadTagFragmented(path, elements, (uint)data.Count), ct, - expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, data.Count)).ConfigureAwait(false); - var (fragmentType, fragmentHandle, chunk) = LogixServices.ParseReadReply(reply); - if (fragmentType != type) - throw new AbTransportException(AbStatus.BadReply, - $"Read fragment type 0x{fragmentType:X4} differs from the first reply's 0x{type:X4}."); - // A mid-read change of the structure handle means the tag's type changed under us (a - // download / online edit); stitching the halves would yield a value of neither type. - if (fragmentHandle != handle) - throw new AbTransportException(AbStatus.BadReply, - $"Read fragment structure handle {FormatHandle(fragmentHandle)} differs from the first reply's {FormatHandle(handle)}; the tag changed mid-read."); - data.AddRange(chunk.Span); - lastChunk = chunk.Length; - if (data.Count > MaxReadBytes) - throw new AbTransportException(AbStatus.TooLarge, - $"Fragmented read exceeded {MaxReadBytes} bytes; the controller kept answering Partial."); - } + Append(ref data, ref count, first.Span); + var lastChunk = first.Length; - return (type, handle, data.ToArray()); + while (reply.GeneralStatus == StatusPartial) + { + if (lastChunk == 0) + throw new AbTransportException(AbStatus.BadReply, "Partial read reply carried no data; the fragment loop cannot progress."); + reply = await _scheduler.SendAsync( + LogixServices.ReadTagFragmented(path, elements, (uint)count), ct, + expectedReplyBytes: ExpectedReadReplyBytes(elements, elementBytes, count)).ConfigureAwait(false); + var (fragmentType, fragmentHandle, chunk) = LogixServices.ParseReadReply(reply); + if (fragmentType != type) + throw new AbTransportException(AbStatus.BadReply, + $"Read fragment type 0x{fragmentType:X4} differs from the first reply's 0x{type:X4}."); + // A mid-read change of the structure handle means the tag's type changed under us (a + // download / online edit); stitching the halves would yield a value of neither type. + if (fragmentHandle != handle) + throw new AbTransportException(AbStatus.BadReply, + $"Read fragment structure handle {FormatHandle(fragmentHandle)} differs from the first reply's {FormatHandle(handle)}; the tag changed mid-read."); + Append(ref data, ref count, chunk.Span); + lastChunk = chunk.Length; + if (count > MaxReadBytes) + throw new AbTransportException(AbStatus.TooLarge, + $"Fragmented read exceeded {MaxReadBytes} bytes; the controller kept answering Partial."); + } + + return (type, handle, data.AsSpan(0, count).ToArray()); + } + finally + { + ArrayPool.Shared.Return(data); + } + } + + /// Appends to a pooled buffer, growing it (rent bigger, copy, return) as needed. + private static void Append(ref byte[] buffer, ref int count, ReadOnlySpan chunk) + { + if (count + chunk.Length > buffer.Length) + { + var bigger = ArrayPool.Shared.Rent(Math.Max(buffer.Length * 2, count + chunk.Length)); + buffer.AsSpan(0, count).CopyTo(bigger); + ArrayPool.Shared.Return(buffer); + buffer = bigger; + } + chunk.CopyTo(buffer.AsSpan(count)); + count += chunk.Length; } private static string FormatHandle(ushort? handle) => handle is { } h ? $"0x{h:X4}" : "(none)"; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs index edd193ee..326dca03 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Pccc/PcccServices.cs @@ -113,7 +113,7 @@ public static class PcccServices public static ReadOnlyMemory ParseReply(CipReply reply, ushort expectedTns, byte[]? expectedRequestorId = null) { if (reply.GeneralStatus != 0) - throw new AbTransportException(reply.Status, $"Execute PCCC CIP status 0x{reply.GeneralStatus:X2}."); + throw new AbDeviceReplyException(reply.Status, $"Execute PCCC CIP status 0x{reply.GeneralStatus:X2}."); var r = new CipReader(reply.Data.Span); var idLen = r.U8(); if (idLen < 1) throw new AbTransportException(AbStatus.BadReply, "Empty requestor id in PCCC reply."); @@ -131,7 +131,7 @@ public static class PcccServices throw new AbTransportException(AbStatus.BadReply, $"PCCC TNS {tns} != {expectedTns}."); byte? ext = sts == 0xF0 ? r.U8() : null; if (sts != 0) - throw new AbTransportException(AbStatusCodes.FromPccc(sts, ext), + throw new AbDeviceReplyException(AbStatusCodes.FromPccc(sts, ext), $"PCCC STS 0x{sts:X2}{(ext is { } e ? $" EXT 0x{e:X2}" : "")}."); return reply.Data[r.Position..]; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs index 8c86f186..94b77a79 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs @@ -1,17 +1,38 @@ +using System.Buffers; using System.Buffers.Binary; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Wire; /// Growable little-endian writer for CIP / EIP frames. +/// +/// R07:L5: the working buffer is rented from (a 256-byte array per request used +/// to be allocated and thrown away) and handed back by , which therefore COMPLETES the writer: +/// the result is the caller's own array, a second returns another copy of it, and any further +/// write throws (the buffer belongs to the pool again). A writer abandoned +/// before (a builder that threw) simply leaves its buffer to the GC. +/// public sealed class CipWriter { - private byte[] _buf = new byte[256]; + private const int InitialCapacity = 256; + private byte[]? _buf = ArrayPool.Shared.Rent(InitialCapacity); + private byte[]? _result; + /// Gets the number of bytes written so far. public int Length { get; private set; } + private byte[] Buffer => _buf ?? throw new InvalidOperationException( + "This CipWriter was completed by ToArray(); its buffer has been returned to the pool."); + private Span Take(int n) { - if (Length + n > _buf.Length) Array.Resize(ref _buf, Math.Max(_buf.Length * 2, Length + n)); - var s = _buf.AsSpan(Length, n); + var buf = Buffer; + if (Length + n > buf.Length) + { + var bigger = ArrayPool.Shared.Rent(Math.Max(buf.Length * 2, Length + n)); + buf.AsSpan(0, Length).CopyTo(bigger); + ArrayPool.Shared.Return(buf); + _buf = buf = bigger; + } + var s = buf.AsSpan(Length, n); Length += n; return s; } @@ -45,14 +66,27 @@ public sealed class CipWriter /// The replacement value. /// The two bytes at are not already /// written (an offset in the spare buffer capacity would otherwise be patched silently and lost). + /// The writer was completed by . public void PatchU16(int offset, ushort v) { + var buf = Buffer; if (offset < 0 || offset > Length - 2) throw new ArgumentOutOfRangeException(nameof(offset), offset, $"Cannot patch 2 bytes at {offset}: only {Length} bytes written."); - BinaryPrimitives.WriteUInt16LittleEndian(_buf.AsSpan(offset, 2), v); + BinaryPrimitives.WriteUInt16LittleEndian(buf.AsSpan(offset, 2), v); } - /// Copies the written bytes to a new array. - /// The bytes written so far. - public byte[] ToArray() => _buf.AsSpan(0, Length).ToArray(); + /// + /// Copies the written bytes to a new array and returns the working buffer to the pool, completing the writer + /// (see the remarks on ). + /// + /// The bytes written, in an array the caller owns. + public byte[] ToArray() + { + if (_result is { } done) return (byte[])done.Clone(); + var buf = Buffer; + _result = buf.AsSpan(0, Length).ToArray(); + _buf = null; + ArrayPool.Shared.Return(buf); + return _result; + } } 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 334568a1..2bded247 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipRefusals.cs @@ -13,10 +13,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// 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. +/// The native transport maps CIP general statuses 0x1C (missing attribute list entry) and 0x20 (invalid +/// parameter) to and 0xFF/0x2104 (offset beyond end) to +/// (AbStatusCodes.FromCip, Lane K F1), so they classify here as +/// device refusals. A Forward Open refused with one of them stays a connection failure. /// /// internal static class AbCipRefusals diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 2444f3c1..9fdcddf9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -789,9 +789,9 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag results[item.Index] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now); return (OpVerdict.Neutral, null); } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { - // The native runtime raises a device's refusal (e.g. STS 0x20 "file not found") as an exception. + // The native runtime raises a device's refusal (e.g. STS 0x50 "addressing problem") as an exception. WarnRefusalOnce(device, def, "read", ex.Message, AbLegacyStatusMapper.MapStatus(ex.Status)); results[item.Index] = new DataValueSnapshot(null, AbLegacyStatusMapper.MapStatus(ex.Status), null, now); return (OpVerdict.Answered, null); @@ -838,10 +838,10 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag /// reachability (backoff reset, LastSuccessfulRead, Healthy). /// /// - /// ⚠️ Inherits the native transport's PCCC STS table (AbStatusCodes.FromPccc, Lane K): it maps STS 0x30 - /// ("remote node missing" — a dead DH+/DH-485 node behind a bridge) and 0x20 to NotAllowed/NotFound, - /// so on the NATIVE transport such a dead node still reads as answered. Follow-up F8 (review 07 r0 I4); this - /// classification needs no change once it lands. + /// Inherits the native transport's PCCC STS table (AbStatusCodes.FromPccc). Since Lane K F3 (F8, review + /// 07 r0 I4) STS 0x20 / 0x30 / 0x90 — the host or a bridge reporting an unreachable END device — map to + /// (no reply), so a dead DH+/DH-485 node behind a bridge is a comms fault, + /// not an answer (NativePcccBridgeStatusHealthTests). /// /// The transport status. /// for a device refusal. @@ -1035,7 +1035,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag results[i] = new WriteResult(AbLegacyStatusMapper.BadCommunicationError); outcome.FaultMessage ??= tex.Message; } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { device.RecordAnswered(); outcome.AnyAnswered = true; @@ -1210,7 +1210,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag try { probeRuntime?.Dispose(); } catch { } probeRuntime = null; state.ProbeInitialized = false; - if (ex is AbTransportException ate && IsDeviceRefusal(ate.Status)) state.RecordAnswered(); + if (IsAnsweredRefusal(ex)) state.RecordAnswered(); else if (ex is not AbTransportException { Status: var st } || IsNoReply(st)) state.RecordCommsFault(Clock()); } @@ -1478,7 +1478,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag { return (null, AbLegacyStatusMapper.BadNotSupported, false, false); } - catch (AbTransportException ex) when (IsDeviceRefusal(ex.Status)) + catch (AbTransportException ex) when (IsAnsweredRefusal(ex)) { // The device answered: the index file/element does not exist. Per-tag, not a comms fault. return (null, AbLegacyStatusMapper.MapStatus(ex.Status), false, false); @@ -1617,10 +1617,26 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag /// all PLC-5 writes), and libplctag's ErrorUnsupported maps to it too. It says nothing about the /// device's reachability, so it must not evict the handle or degrade driver health. /// + /// + /// An Unsupported that came OFF THE WIRE (: CIP 0x08 / 0x1C / 0x20 on the + /// Execute PCCC envelope, PCCC STS 0x10) is NOT local — the device answered — and is classified by + /// instead (review lane K I2). + /// /// The exception to classify. /// if the exception is a local refusal that never reached the wire. internal static bool IsLocalRefusal(Exception ex) => - ex is AbTransportException { Status: AbStatus.Unsupported }; + ex is AbTransportException { Status: AbStatus.Unsupported } and not AbDeviceReplyException; + + /// + /// True for an exception proving the device ANSWERED and refused this operation: a device-refusal status + /// (), or an that came off the wire + /// (). Per-tag Bad, keeps the handle, advances LastSuccessfulRead. + /// + /// The exception to classify. + /// for an answered refusal. + internal static bool IsAnsweredRefusal(Exception ex) => + ex is AbTransportException ate + && (IsDeviceRefusal(ate.Status) || ate is AbDeviceReplyException { Status: AbStatus.Unsupported }); /// /// Initializes a freshly created runtime. A NATIVE runtime whose init (one read) is refused locally is diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs index 9fd9c682..bec5055d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuFraming.cs @@ -1,3 +1,4 @@ +using System.Buffers; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; /// @@ -55,12 +56,33 @@ public static class ModbusRtuFraming /// A new array: [unitId, ...pdu, crcLo, crcHi]. public static byte[] BuildAdu(byte unitId, ReadOnlySpan pdu) { - var frame = new byte[1 + pdu.Length]; - frame[0] = unitId; - pdu.CopyTo(frame.AsSpan(1)); - return ModbusCrc.Append(frame); + var frame = new byte[AduLength(pdu.Length)]; + WriteAdu(unitId, pdu, frame); + return frame; } + /// The length of the RTU ADU for a PDU of bytes (unit id + PDU + CRC). + /// The bare PDU length. + /// The ADU length. + internal static int AduLength(int pduLength) => 1 + pduLength + 2; + + /// Writes the RTU ADU into (exactly bytes). + /// The RTU slave/unit address. + /// The bare PDU. + /// The ADU buffer, exactly (.Length) long. + internal static void WriteAdu(byte unitId, ReadOnlySpan pdu, Span destination) + { + destination[0] = unitId; + pdu.CopyTo(destination[1..]); + var body = destination[..(1 + pdu.Length)]; + var crc = ModbusCrc.Compute(body); + destination[1 + pdu.Length] = (byte)(crc & 0xFF); // CRC low byte first + destination[2 + pdu.Length] = (byte)(crc >> 8); + } + + /// The largest RTU response frame: addr + fc + byteCount + 255 data bytes + CRC. + private const int MaxResponseFrame = 3 + 255 + 2; + /// /// Reads a single RTU response frame from and returns its bare PDU /// ([fc, ...data]), with the leading unit-id and trailing CRC stripped. @@ -82,58 +104,56 @@ public static class ModbusRtuFraming public static async Task ReadResponsePduAsync( Stream stream, byte expectedUnit, CancellationToken ct) { - // Header: addr(1) + fc(1). The function code selects how many more bytes to read. - var header = new byte[2]; - await ReadExactlyAsync(stream, header, ct).ConfigureAwait(false); - var fc = header[1]; + // R07:L5: the whole frame is read into ONE pooled buffer and validated in place; only the returned PDU is + // allocated (it used to be five arrays: header, byteCount, rest, the reassembled frame and the PDU). + var frame = ArrayPool.Shared.Rent(MaxResponseFrame); + try + { + // Header: addr(1) + fc(1). The function code selects how many more bytes to read. + await ReadExactlyAsync(stream, frame.AsMemory(0, 2), ct).ConfigureAwait(false); + var fc = frame[1]; - int trailing; // bytes after addr+fc, up to and including the 2 CRC bytes. - if ((fc & 0x80) != 0) - { - // Exception frame: excCode(1) + CRC(2). - trailing = 1 + 2; - } - else if (fc is 0x01 or 0x02 or 0x03 or 0x04 or 0x17) - { - // Read response (and FC23 Read/Write Multiple, whose reply is a read): byteCount(1) then - // byteCount data bytes + CRC(2). - var bc = new byte[1]; - await ReadExactlyAsync(stream, bc, ct).ConfigureAwait(false); - var byteCount = bc[0]; - var rest = new byte[byteCount + 2]; - await ReadExactlyAsync(stream, rest, ct).ConfigureAwait(false); - return ValidateAndStrip(expectedUnit, header, bc, rest); - } - else - { - // Fixed 4-byte echo: correct for the write FCs this driver emits (05/06/0F/10). FC23 is - // variable-length and handled above; any NEW variable-length response FC must be added there - // or it is mis-sized here and surfaces as a desync. - trailing = 4 + 2; - } + int length; // the whole frame, addr through CRC + var already = 2; // bytes of the frame read so far + if ((fc & 0x80) != 0) + { + // Exception frame: excCode(1) + CRC(2). + length = 2 + 1 + 2; + } + else if (fc is 0x01 or 0x02 or 0x03 or 0x04 or 0x17) + { + // Read response (and FC23 Read/Write Multiple, whose reply is a read): byteCount(1) then + // byteCount data bytes + CRC(2). + await ReadExactlyAsync(stream, frame.AsMemory(2, 1), ct).ConfigureAwait(false); + length = 3 + frame[2] + 2; + already = 3; + } + else + { + // Fixed 4-byte echo: correct for the write FCs this driver emits (05/06/0F/10). FC23 is + // variable-length and handled above; any NEW variable-length response FC must be added there + // or it is mis-sized here and surfaces as a desync. + length = 2 + 4 + 2; + } - var tail = new byte[trailing]; - await ReadExactlyAsync(stream, tail, ct).ConfigureAwait(false); - return ValidateAndStrip(expectedUnit, header, ReadOnlySpan.Empty, tail); + await ReadExactlyAsync(stream, frame.AsMemory(already, length - already), ct).ConfigureAwait(false); + return ValidateAndStrip(expectedUnit, frame.AsSpan(0, length)); + } + finally + { + ArrayPool.Shared.Return(frame); + } } /// - /// Assembles the full frame from its pieces, validates the trailing CRC over everything - /// except the CRC bytes, then validates the response address against - /// and strips addr + CRC to return the bare PDU. + /// Validates the trailing CRC over everything except the CRC bytes, then validates the response address + /// against and strips addr + CRC to return the bare PDU. /// A CRC-valid exception PDU throws . /// - private static byte[] ValidateAndStrip( - byte expectedUnit, ReadOnlySpan header, ReadOnlySpan middle, ReadOnlySpan tail) + private static byte[] ValidateAndStrip(byte expectedUnit, ReadOnlySpan frame) { - // Reassemble the whole frame: header(addr,fc) + middle(optional byteCount) + tail. - var frame = new byte[header.Length + middle.Length + tail.Length]; - header.CopyTo(frame); - middle.CopyTo(frame.AsSpan(header.Length)); - tail.CopyTo(frame.AsSpan(header.Length + middle.Length)); - // CRC covers everything except the trailing 2 CRC bytes. - var body = frame.AsSpan(0, frame.Length - 2); + var body = frame[..^2]; var expected = ModbusCrc.Compute(body); var actual = (ushort)(frame[^2] | (frame[^1] << 8)); if (actual != expected) @@ -172,12 +192,12 @@ public static class ModbusRtuFraming /// so a length-less RTU frame that arrives /// short is classified as a desync rather than a bare . /// - private static async Task ReadExactlyAsync(Stream stream, byte[] buf, CancellationToken ct) + private static async Task ReadExactlyAsync(Stream stream, Memory buf, CancellationToken ct) { var read = 0; while (read < buf.Length) { - var n = await stream.ReadAsync(buf.AsMemory(read), ct).ConfigureAwait(false); + var n = await stream.ReadAsync(buf[read..], ct).ConfigureAwait(false); if (n == 0) throw new ModbusTransportDesyncException( ModbusTransportDesyncException.DesyncReason.TruncatedFrame, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs index 7dcc5dff..e2f4047e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusRtuOverTcpTransport.cs @@ -1,3 +1,4 @@ +using System.Buffers; using System.Net.Sockets; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -150,14 +151,17 @@ public sealed class ModbusRtuOverTcpTransport : IModbusTransport var stream = CurrentStream; if (stream is null) throw new InvalidOperationException("Transport not connected"); - // RTU ADU: [unitId] + PDU + CRC — no MBAP header, no TxId. - var adu = ModbusRtuFraming.BuildAdu(unitId, pdu); + // RTU ADU: [unitId] + PDU + CRC — no MBAP header, no TxId. Built in a pooled buffer (R07:L5); the same bytes + // ModbusRtuFraming.BuildAdu produces (pinned by a test). + var aduLength = ModbusRtuFraming.AduLength(pdu.Length); + var adu = ArrayPool.Shared.Rent(aduLength); + ModbusRtuFraming.WriteAdu(unitId, pdu, adu.AsSpan(0, aduLength)); using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); cts.CancelAfter(_timeout); try { - await stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false); + await stream.WriteAsync(adu.AsMemory(0, aduLength), cts.Token).ConfigureAwait(false); await stream.FlushAsync(cts.Token).ConfigureAwait(false); // Framing owns the length-less RTU deframe + CRC + unit-address validation. A CRC-valid @@ -181,6 +185,11 @@ public sealed class ModbusRtuOverTcpTransport : IModbusTransport ModbusTransportDesyncException.DesyncReason.Timeout, $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms"); } + finally + { + // The write has completed, or was cancelled and the stream torn down, so nothing still reads it. + ArrayPool.Shared.Return(adu); + } } /// Asynchronously disposes the transport and underlying socket resources. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs index f21ce678..6889b1e2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusTcpTransport.cs @@ -1,3 +1,4 @@ +using System.Buffers; using System.Net.Sockets; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -59,6 +60,10 @@ public sealed class ModbusTcpTransport : IModbusTransport /// The socket lifecycle this transport composes (test seam for its clock). internal ModbusSocketLifecycle Lifecycle => _life; + // R07:L5: the 7-byte MBAP header of the reply being read. SendOnceAsync runs only under _gate (single-flight), so + // one buffer per transport serves every transaction (it used to be a fresh array per reply). + private readonly byte[] _header = new byte[7]; + /// public Task ConnectAsync(CancellationToken ct) => _life.ConnectAsync(ct); @@ -139,11 +144,16 @@ public sealed class ModbusTcpTransport : IModbusTransport if (stream is null) throw new InvalidOperationException("Transport not connected"); var txId = ++_nextTx; - // MBAP: [TxId(2)][Proto=0(2)][Length(2)][UnitId(1)] + PDU - var adu = new byte[7 + pdu.Length]; + // MBAP: [TxId(2)][Proto=0(2)][Length(2)][UnitId(1)] + PDU. Built in a pooled buffer (R07:L5): the request + // never escapes, and the write has completed (or been cancelled and the socket torn down) before the finally + // hands it back. Only the reply PDU, which the caller keeps, is allocated. + // (One async method on purpose: a second awaited helper costs a state-machine box per call, which ate the saving.) + var aduLength = 7 + pdu.Length; + var adu = ArrayPool.Shared.Rent(aduLength); adu[0] = (byte)(txId >> 8); adu[1] = (byte)(txId & 0xFF); - // protocol id already zero + adu[2] = 0; // protocol id + adu[3] = 0; var len = (ushort)(1 + pdu.Length); // unit id + pdu adu[4] = (byte)(len >> 8); adu[5] = (byte)(len & 0xFF); @@ -154,10 +164,10 @@ public sealed class ModbusTcpTransport : IModbusTransport cts.CancelAfter(_timeout); try { - await stream.WriteAsync(adu.AsMemory(), cts.Token).ConfigureAwait(false); + await stream.WriteAsync(adu.AsMemory(0, aduLength), cts.Token).ConfigureAwait(false); await stream.FlushAsync(cts.Token).ConfigureAwait(false); - var header = new byte[7]; + var header = _header; await ReadExactlyAsync(stream, header, cts.Token).ConfigureAwait(false); var respTxId = (ushort)((header[0] << 8) | header[1]); if (respTxId != txId) @@ -211,6 +221,11 @@ public sealed class ModbusTcpTransport : IModbusTransport ModbusTransportDesyncException.DesyncReason.Timeout, $"Modbus per-op response timeout after {_timeout.TotalMilliseconds:0}ms"); } + finally + { + // The write has completed, or was cancelled and the socket torn down, so nothing still reads it. + ArrayPool.Shared.Return(adu); + } } private static async Task ReadExactlyAsync(Stream s, byte[] buf, CancellationToken ct) diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs new file mode 100644 index 00000000..8d150ae2 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/HistoryReadAggregateParsingTests.cs @@ -0,0 +1,87 @@ +using CliFx.Exceptions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +/// +/// historyread --aggregate accepts every aggregate the client offers (which is exactly the served set), so the +/// gate recipes in docs/Historian.md that need Range / Delta / PercentGood / ... can run from this CLI. +/// +public class HistoryReadAggregateParsingTests +{ + /// Every client aggregate, by its name. + /// Each value. + public static TheoryData AllAggregates() => new(Enum.GetValues()); + + /// Every aggregate name (in any case) reaches the service as that aggregate. + /// The aggregate. + [Theory] + [MemberData(nameof(AllAggregates))] + public async Task Every_aggregate_name_parses(AggregateType aggregate) + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = aggregate.ToString().ToUpperInvariant() + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.HistoryReadAggregateCalls.ShouldHaveSingleItem().Aggregate.ShouldBe(aggregate); + } + + /// The short aliases still parse. + /// The alias. + /// The aggregate it names. + [Theory] + [InlineData("avg", AggregateType.Average)] + [InlineData("min", AggregateType.Minimum)] + [InlineData("last", AggregateType.End)] + [InlineData("interp", AggregateType.Interpolative)] + [InlineData("mintime", AggregateType.MinimumActualTime)] + [InlineData("maxtime", AggregateType.MaximumActualTime)] + public async Task Aliases_parse(string alias, AggregateType expected) + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = alias + }; + + using var console = TestConsoleHelper.CreateConsole(); + await command.ExecuteAsync(console); + + fakeService.HistoryReadAggregateCalls.ShouldHaveSingleItem().Aggregate.ShouldBe(expected); + } + + /// + /// StandardDeviation is no longer offered (no backend serves it): it is refused before connecting, naming the + /// supported set. + /// + [Fact] + public async Task StandardDeviation_is_refused_before_connecting() + { + var fakeService = new FakeOpcUaClientService(); + var command = new HistoryReadCommand(new FakeOpcUaClientServiceFactory(fakeService)) + { + Url = "opc.tcp://localhost:4840", + NodeId = "ns=2;s=HistNode", + Aggregate = "stddev" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + + ex.Message.ShouldContain("PercentGood"); + fakeService.HistoryReadAggregateCalls.ShouldBeEmpty(); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs new file mode 100644 index 00000000..07980b36 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Adapters/DefaultSubscriptionAdapterTests.cs @@ -0,0 +1,57 @@ +using Opc.Ua.Client; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Adapters; + +/// +/// A deleted subscription must also leave the SDK session's Subscriptions list, or every alarm re-target in the UI +/// leaves a dead behind until the session is disposed. +/// +public class DefaultSubscriptionAdapterTests +{ + /// DeleteAsync detaches the wrapped subscription exactly once, even when Dispose follows. + [Fact] + public async Task DeleteAsync_detaches_the_subscription_from_its_session_once() + { + var subscription = new Subscription(); + var detached = new List(); + var adapter = new DefaultSubscriptionAdapter(subscription, (s, _) => + { + lock (detached) detached.Add(s); + return Task.CompletedTask; + }); + + await adapter.DeleteAsync(TestContext.Current.CancellationToken); + adapter.Dispose(); + + detached.ShouldHaveSingleItem().ShouldBeSameAs(subscription); + } + + /// Dispose alone (no prior DeleteAsync) detaches too. + [Fact] + public void Dispose_detaches_the_subscription_from_its_session() + { + var detached = 0; + var adapter = new DefaultSubscriptionAdapter(new Subscription(), (_, _) => + { + Interlocked.Increment(ref detached); + return Task.CompletedTask; + }); + + adapter.Dispose(); + + detached.ShouldBe(1); + } + + /// A failing detach is logged, not thrown: teardown stays best effort. + [Fact] + public async Task A_failing_detach_does_not_throw_from_DeleteAsync() + { + var adapter = new DefaultSubscriptionAdapter(new Subscription(), + (_, _) => throw new InvalidOperationException("boom")); + + await Should.NotThrowAsync(() => adapter.DeleteAsync(TestContext.Current.CancellationToken)); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs new file mode 100644 index 00000000..362bc456 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/FailoverEpochTests.cs @@ -0,0 +1,177 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests; + +/// +/// Lane K review I1: a keep-alive failover belongs to the connection it started under. A user Disconnect + Connect +/// while it is still connecting starts a new connection epoch; the stale failover must abandon itself — never +/// overwrite the user's new session, never flip its state. Every step is gate-driven (no sleeps). +/// +public sealed class FailoverEpochTests : IDisposable +{ + private const string Primary = "opc.tcp://primary:4840"; + private const string Backup = "opc.tcp://backup:4840"; + private static readonly TimeSpan Wait = TimeSpan.FromSeconds(10); + + private readonly FakeSessionFactory _factory = new(); + private readonly OpcUaClientService _service; + private readonly List _states = []; + + public FailoverEpochTests() + { + _service = new OpcUaClientService(new FakeApplicationConfigurationFactory(), new FakeEndpointDiscovery(), _factory); + _service.ConnectionStateChanged += (_, e) => { lock (_states) _states.Add(e.NewState); }; + } + + public void Dispose() => _service.Dispose(); + + private static ConnectionSettings Settings() => new() + { + EndpointUrl = Primary, FailoverUrls = [Backup], SessionTimeoutSeconds = 60 + }; + + /// Connects S1, then holds a keep-alive failover inside its connect attempt. + private async Task<(FakeSessionAdapter S1, Task Failover, TaskCompletionSource Gate)> StartHeldFailoverAsync() + { + var s1 = new FakeSessionAdapter(); + _factory.EnqueueSession(s1); + await _service.ConnectAsync(Settings()); + + var gate = new TaskCompletionSource(); + _factory.CreateGate = gate; + s1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + await _factory.CreateEntered.Task.WaitAsync(Wait); + return (s1, failover, gate); + } + + /// The stale failover's attempt SUCCEEDS after the user reconnected: its session is disposed, the user's stays. + [Fact] + public async Task A_stale_failover_that_connects_does_not_replace_the_users_new_session() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + var userSession = new FakeSessionAdapter(); + var failoverSession = new FakeSessionAdapter(); + _factory.EnqueueSession(userSession); // dequeued by the user's Connect (the failover is still held) + _factory.EnqueueSession(failoverSession); // dequeued by the failover once released + + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + lock (_states) _states.Clear(); + + gate.SetResult(); + await failover.WaitAsync(Wait); + + failoverSession.Disposed.ShouldBeTrue(); + userSession.Disposed.ShouldBeFalse(); + _service.IsConnected.ShouldBeTrue(); + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe(Primary); + lock (_states) _states.ShouldBeEmpty(); // the stale failover made no state transition at all + } + + /// The stale failover's attempts all FAIL after the user reconnected: the user's connection stays Connected. + [Fact] + public async Task A_stale_failover_that_fails_does_not_flip_the_users_connection_to_Disconnected() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + lock (_states) _states.Clear(); + var createsBefore = _factory.CreateCallCount; + + _factory.ThrowOnCreate = true; + gate.SetResult(); + await failover.WaitAsync(Wait); + + _service.IsConnected.ShouldBeTrue(); + lock (_states) _states.ShouldBeEmpty(); + // It stopped at the abandoned attempt instead of trying every remaining endpoint (review m5). + _factory.CreateCallCount.ShouldBe(createsBefore); + // And the user can still disconnect / reconnect normally afterwards. + _factory.ThrowOnCreate = false; + await _service.DisconnectAsync(); + await _service.ConnectAsync(Settings()); + _service.IsConnected.ShouldBeTrue(); + } + + /// Positive control: an undisturbed failover still reconnects to the backup. + [Fact] + public async Task A_genuine_failover_still_reconnects() + { + var (_, failover, gate) = await StartHeldFailoverAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + + gate.SetResult(); + await failover.WaitAsync(Wait); + + _service.IsConnected.ShouldBeTrue(); + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe(Backup); + lock (_states) _states.ShouldContain(ConnectionState.Reconnecting); + } + + /// + /// A late bad keep-alive from the PREVIOUS connection's session must not start a failover against the new one. + /// Positive control: the same signal from the current session does (the genuine-failover test above). + /// + [Fact] + public async Task A_keep_alive_from_a_previous_connection_does_not_fail_over_the_current_one() + { + var s1 = new FakeSessionAdapter(); + _factory.EnqueueSession(s1); + await _service.ConnectAsync(Settings()); + await _service.DisconnectAsync(); + _factory.EnqueueSession(new FakeSessionAdapter()); + await _service.ConnectAsync(Settings()); + var creates = _factory.CreateCallCount; + lock (_states) _states.Clear(); + + s1.SimulateKeepAlive(false); + if (_service.LastFailover is { } t) await t.WaitAsync(Wait); + + _factory.CreateCallCount.ShouldBe(creates); + lock (_states) _states.ShouldBeEmpty(); + _service.IsConnected.ShouldBeTrue(); + } + + /// Review m8: re-entrant bad keep-alives do not overwrite the seam with an already-completed no-op. + [Fact] + public async Task Reentrant_keep_alives_leave_the_seam_on_the_running_failover() + { + var (s1, failover, gate) = await StartHeldFailoverAsync(); + s1.SimulateKeepAlive(false); + s1.SimulateKeepAlive(false); + + _service.LastFailover.ShouldBeSameAs(failover); + failover.IsCompleted.ShouldBeFalse(); + _factory.EnqueueSession(new FakeSessionAdapter()); + gate.SetResult(); + await failover.WaitAsync(Wait); + } + + /// + /// Review m5: an Unsubscribe whose server-side remove fails (the session died under it) still drops the + /// bookkeeping, so a failover does not replay an item the caller asked to stop. Proven by a re-subscribe of the + /// same node creating a fresh monitored item instead of returning "already subscribed". + /// + [Fact] + public async Task A_failed_unsubscribe_still_forgets_the_item() + { + var sub = new FakeSubscriptionAdapter(); + _factory.EnqueueSession(new FakeSessionAdapter { NextSubscription = sub }); + await _service.ConnectAsync(Settings()); + var node = new Opc.Ua.NodeId("ns=2;s=X"); + await _service.SubscribeAsync(node); + + sub.ThrowOnRemove = true; + await Should.ThrowAsync(() => _service.UnsubscribeAsync(node)); + sub.ThrowOnRemove = false; + + await _service.SubscribeAsync(node); + sub.AddDataChangeCount.ShouldBe(2); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs index a73a9f65..27504765 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionAdapter.cs @@ -28,8 +28,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter public int BrowseCount { get; private set; } /// Gets the number of times BrowseNextAsync has been called. public int BrowseNextCount { get; private set; } - /// Gets the number of times HasChildrenAsync has been called. - public int HasChildrenCount { get; private set; } /// Gets the number of times HistoryReadRawAsync has been called. public int HistoryReadRawCount { get; private set; } /// Gets the number of times HistoryReadAggregateAsync has been called. @@ -57,8 +55,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter public ReferenceDescriptionCollection BrowseNextResponse { get; set; } = []; /// Gets or sets the continuation point for browse-next operations. public byte[]? BrowseNextContinuationPoint { get; set; } - /// Gets or sets the result returned by has-children operations. - public bool HasChildrenResponse { get; set; } = false; /// Gets or sets the historical values returned by raw history-read operations. public List HistoryReadRawResponse { get; set; } = []; @@ -150,13 +146,6 @@ internal sealed class FakeSessionAdapter : ISessionAdapter return Task.FromResult((BrowseNextContinuationPoint, BrowseNextResponse)); } - /// - public Task HasChildrenAsync(NodeId nodeId, CancellationToken ct) - { - HasChildrenCount++; - return Task.FromResult(HasChildrenResponse); - } - /// public Task> HistoryReadRawAsync( NodeId nodeId, DateTime startTime, DateTime endTime, int maxValues, CancellationToken ct) @@ -178,15 +167,31 @@ internal sealed class FakeSessionAdapter : ISessionAdapter return Task.FromResult>(HistoryReadAggregateResponse); } + /// + /// Optional gate that, when set, holds open until completed, so a test can keep + /// a subscribe in flight while something else (a failover) happens. Signalled via . + /// + public TaskCompletionSource? CreateSubscriptionGate { get; set; } + + /// Completed when is entered while a gate is set. + public TaskCompletionSource CreateSubscriptionEntered { get; } = + new(TaskCreationOptions.RunContinuationsAsynchronously); + /// - public Task CreateSubscriptionAsync(int publishingIntervalMs, CancellationToken ct) + public async Task CreateSubscriptionAsync(int publishingIntervalMs, CancellationToken ct) { + if (CreateSubscriptionGate is { } gate) + { + CreateSubscriptionEntered.TrySetResult(); + await gate.Task; + } + lock (_createdSubscriptions) { var sub = NextSubscription ?? new FakeSubscriptionAdapter(); NextSubscription = null; _createdSubscriptions.Add(sub); - return Task.FromResult(sub); + return sub; } } diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs index c1189a78..24cbd13a 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSessionFactory.cs @@ -21,6 +21,9 @@ internal sealed class FakeSessionFactory : ISessionFactory /// public TaskCompletionSource? CreateGate { get; set; } + /// Completed when a call observes . + public TaskCompletionSource CreateEntered { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + /// Gets the list of sessions created via CreateSessionAsync. public IReadOnlyList CreatedSessions => _createdSessions; @@ -39,8 +42,14 @@ internal sealed class FakeSessionFactory : ISessionFactory CreateCallCount++; LastEndpointUrl = endpoint.EndpointUrl; - if (CreateGate != null) - await CreateGate.Task; + // One-shot: the gate holds only the call that observed it, so a later call (e.g. a user's Connect while a + // failover is held) runs freely. + if (CreateGate is { } gate) + { + CreateGate = null; + CreateEntered.TrySetResult(); + await gate.Task; + } if (ThrowOnCreate) throw new InvalidOperationException("FakeSessionFactory configured to fail."); diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs index 0c7c4233..1e442575 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Fakes/FakeSubscriptionAdapter.cs @@ -37,6 +37,9 @@ internal sealed class FakeSubscriptionAdapter : ISubscriptionAdapter /// Gets the number of times AddEventMonitoredItemAsync was called. public int AddEventCount { get; private set; } + /// Gets or sets a value indicating whether RemoveMonitoredItemAsync throws (a session that died). + public bool ThrowOnRemove { get; set; } + /// Gets the number of times RemoveMonitoredItemAsync was called. public int RemoveCount { get; private set; } @@ -70,6 +73,8 @@ internal sealed class FakeSubscriptionAdapter : ISubscriptionAdapter /// public Task RemoveMonitoredItemAsync(uint clientHandle, CancellationToken ct) { + if (ThrowOnRemove) + throw new ServiceResultException(StatusCodes.BadSessionClosed, "Session closed"); lock (_itemsLock) { RemoveCount++; diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs index 663ab473..267baf02 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/AggregateTypeMapperTests.cs @@ -3,6 +3,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Driver.Historian.Sql.Common; namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Helpers; @@ -17,7 +18,6 @@ public class AggregateTypeMapperTests [InlineData(AggregateType.Count)] [InlineData(AggregateType.Start)] [InlineData(AggregateType.End)] - [InlineData(AggregateType.StandardDeviation)] [InlineData(AggregateType.TimeAverage)] [InlineData(AggregateType.TimeAverage2)] [InlineData(AggregateType.Total)] @@ -72,14 +72,6 @@ public class AggregateTypeMapperTests AggregateTypeMapper.ToNodeId(AggregateType.End).ShouldBe(ObjectIds.AggregateFunction_End); } - /// Verifies that StandardDeviation aggregate type maps to the correct OPC UA node ID. - [Fact] - public void ToNodeId_StandardDeviation_MapsCorrectly() - { - AggregateTypeMapper.ToNodeId(AggregateType.StandardDeviation) - .ShouldBe(ObjectIds.AggregateFunction_StandardDeviationPopulation); - } - /// Verifies that invalid aggregate type value throws ArgumentOutOfRangeException. [Fact] public void ToNodeId_InvalidValue_Throws() @@ -98,4 +90,31 @@ public class AggregateTypeMapperTests AggregateTypeMapper.ToNodeId(AggregateType.Total2).ShouldBe(ObjectIds.AggregateFunction_Total2); AggregateTypeMapper.ToNodeId(AggregateType.Interpolative).ShouldBe(ObjectIds.AggregateFunction_Interpolative); } -} \ No newline at end of file + + /// + /// The client offers exactly the aggregates the server's SQL backends serve (): + /// nothing the server always refuses (the old StandardDeviation), nothing served left unreachable + /// (Range, Delta, Min/MaxActualTime, PercentGood/Bad). + /// + [Fact] + public void Client_aggregates_are_exactly_the_served_set() + { + Enum.GetNames().Order() + .ShouldBe(AggregateFamilies.Served.Select(a => a.ToString()).Order()); + } + + /// Each aggregate maps to the Part 13 ObjectId of the same name. + /// The aggregate. + [Theory] + [MemberData(nameof(AllAggregates))] + public void ToNodeId_maps_to_the_same_named_AggregateFunction(AggregateType aggregate) + { + var field = typeof(ObjectIds).GetField("AggregateFunction_" + aggregate); + field.ShouldNotBeNull(); + AggregateTypeMapper.ToNodeId(aggregate).ShouldBe((NodeId)field.GetValue(null)!); + } + + /// Every client aggregate. + /// Each value. + public static TheoryData AllAggregates() => new(Enum.GetValues()); +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs new file mode 100644 index 00000000..bd8c3d1c --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Helpers/HistoryReadPagingTests.cs @@ -0,0 +1,219 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Helpers; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Helpers; + +/// +/// Pins the client-side HistoryRead paging loop shared by the raw and modified reads: continuation pages are read +/// (never released) while the caller wants more, and a read stopped by the caller's own cap releases the server cursor. +/// +public class HistoryReadPagingTests +{ + private static readonly NodeId Node = new("t", 2); + + /// One recorded call to the page reader. + private sealed record Call(byte[]? ContinuationPoint, bool Release); + + /// A scripted server: each read returns the next page; a release returns an empty Good result. + private sealed class ScriptedServer + { + private readonly Queue<(int Count, byte[]? Cp)> _pages; + public List Calls { get; } = []; + + public ScriptedServer(params (int Count, byte[]? Cp)[] pages) => _pages = new Queue<(int, byte[]?)>(pages); + + public Task ReadPageAsync(byte[]? cp, bool release, CancellationToken ct) + { + Calls.Add(new Call(cp, release)); + if (release) return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + var (count, next) = _pages.Dequeue(); + var data = new HistoryData(); + for (var i = 0; i < count; i++) data.DataValues.Add(new DataValue(new Variant(i))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, + ContinuationPoint = next, + HistoryData = new ExtensionObject(data) + }); + } + } + + private static IEnumerable Values(HistoryReadResult r) => + r.HistoryData?.Body is HistoryData d ? d.DataValues : []; + + /// + /// A server page limit below the caller's cap hands back a continuation point: the follow-up must READ the next + /// page. Releasing it instead (the old raw path passed release = cp != null) silently truncated every raw read + /// to its first server page. + /// + [Fact] + public async Task Continuation_pages_are_read_not_released_until_the_server_finishes() + { + var server = new ScriptedServer((3, [1]), (3, [2]), (2, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 100, Node, + CancellationToken.None); + + values.Count.ShouldBe(8); + server.Calls.Select(c => c.Release).ShouldAllBe(r => r == false); + server.Calls.Select(c => c.ContinuationPoint?.FirstOrDefault()).ShouldBe(new byte?[] { null, 1, 2 }); + } + + /// A read stopped by the caller's cap while the server still holds a cursor releases that cursor. + [Fact] + public async Task A_read_stopped_by_the_cap_releases_the_outstanding_continuation_point() + { + var server = new ScriptedServer((5, [7])); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 5, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.Count.ShouldBe(2); + server.Calls[1].Release.ShouldBeTrue(); + server.Calls[1].ContinuationPoint.ShouldBe(new byte[] { 7 }); + } + + /// + /// A read the server finished (no continuation point) has nothing to release — and the positive control above + /// proves a release would have been recorded had one been owed. + /// + [Fact] + public async Task A_read_the_server_finished_issues_no_release() + { + var server = new ScriptedServer((5, null)); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 5, Node, + CancellationToken.None); + + values.Count.ShouldBe(5); + server.Calls.ShouldHaveSingleItem().Release.ShouldBeFalse(); + } + + /// + /// Review m3: a non-positive cap is refused. An uncapped client read would materialise a tag's whole window + /// (the client side of R08 1.1), and (uint)-1 on the wire asked the server for everything. + /// + /// The cap. + [Theory] + [InlineData(0)] + [InlineData(-1)] + public async Task A_non_positive_cap_is_refused_before_any_read(int max) + { + var server = new ScriptedServer(); + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, max, Node, CancellationToken.None)); + server.Calls.ShouldBeEmpty(); + } + + /// Review m2: server pages that overshoot the cap are trimmed to it (60 + 60 with a cap of 100 is 100). + [Fact] + public async Task The_result_is_trimmed_to_the_cap() + { + var server = new ScriptedServer((60, [1]), (60, [2])); + + var values = await HistoryReadPaging.ReadAllAsync(server.ReadPageAsync, Values, maxValues: 100, Node, + CancellationToken.None); + + values.Count.ShouldBe(100); + server.Calls.Last().Release.ShouldBeTrue(); // the outstanding cursor is still released + } + + /// + /// Review m1: a read cancelled between pages releases the outstanding cursor (best effort, not on the caller's + /// cancelled token) and still surfaces the cancellation. Positive control: the release IS recorded. + /// + [Fact] + public async Task A_read_cancelled_mid_paging_releases_the_cursor_and_rethrows() + { + using var cts = new CancellationTokenSource(); + var calls = new List<(byte[]? Cp, bool Release, bool TokenCancelled)>(); + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + calls.Add((cp, release, ct.IsCancellationRequested)); + if (release) return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + if (cp != null) { cts.Cancel(); ct.ThrowIfCancellationRequested(); } + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [5], HistoryData = new ExtensionObject(data) + }); + } + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 10, Node, cts.Token)); + + var release = calls.Where(c => c.Release).ShouldHaveSingleItem(); + release.Cp.ShouldBe(new byte[] { 5 }); + release.TokenCancelled.ShouldBeFalse(); + } + + /// A cancellation that lands while the cap release is in flight does not lose the values read. + [Fact] + public async Task The_cap_release_does_not_ride_the_callers_token() + { + using var cts = new CancellationTokenSource(); + var releaseSawCancelled = (bool?)null; + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + if (release) + { + releaseSawCancelled = ct.IsCancellationRequested; + return Task.FromResult(new HistoryReadResult { StatusCode = StatusCodes.Good }); + } + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + cts.Cancel(); // the caller gives up right as the capped page arrives + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [5], HistoryData = new ExtensionObject(data) + }); + } + + var values = await HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 1, Node, cts.Token); + + values.Count.ShouldBe(1); + releaseSawCancelled.ShouldBe(false); + } + + /// A failed release is best effort: the values already read are still returned. + [Fact] + public async Task A_failed_release_does_not_lose_the_values_read() + { + var calls = 0; + Task Read(byte[]? cp, bool release, CancellationToken ct) + { + calls++; + if (release) throw new ServiceResultException(StatusCodes.BadSessionClosed); + var data = new HistoryData(); + data.DataValues.Add(new DataValue(new Variant(1))); + return Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.Good, ContinuationPoint = [9], HistoryData = new ExtensionObject(data) + }); + } + + var values = await HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 1, Node, CancellationToken.None); + + values.Count.ShouldBe(1); + calls.ShouldBe(2); + } + + /// A refused page still surfaces as an exception, not an empty read. + [Fact] + public async Task A_refused_page_throws() + { + Task Read(byte[]? cp, bool release, CancellationToken ct) => + Task.FromResult(new HistoryReadResult + { + StatusCode = StatusCodes.BadHistoryOperationUnsupported + }); + + await Should.ThrowAsync(() => + HistoryReadPaging.ReadAllAsync(Read, Values, maxValues: 10, Node, CancellationToken.None)); + } +} diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs index 3d3a32a4..a9f01b51 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/Models/ModelConstructionTests.cs @@ -132,10 +132,10 @@ public class ModelConstructionTests ((int)ConnectionState.Reconnecting).ShouldBe(3); } - /// Verifies that AggregateType enum has the expected number of values. + /// AggregateType holds the seventeen served aggregates (the parity test pins which). [Fact] public void AggregateType_Enum_HasExpectedValues() { - Enum.GetValues().Length.ShouldBe(12); + Enum.GetValues().Length.ShouldBe(17); } } \ No newline at end of file diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs index a604fb08..a0b89bb9 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs @@ -384,10 +384,12 @@ public class OpcUaClientServiceTests : IDisposable } /// - /// Verifies that object nodes trigger child-detection checks so the client can mark expandable branches. + /// R08 3.6: browsing a level issues ONE Browse (plus BrowseNext pages) however many Object children it holds. Child + /// detection is lazy: an Object may have children and is confirmed when it is expanded/recursed, instead of one + /// extra Browse per Object child (~50 000 calls for a recursive subscribe over a 50k-node raw tree). /// [Fact] - public async Task BrowseAsync_ObjectNode_ChecksHasChildren() + public async Task BrowseAsync_ObjectChildren_MayHaveChildren_WithoutAPerChildBrowse() { var session = new FakeSessionAdapter { @@ -398,17 +400,29 @@ public class OpcUaClientServiceTests : IDisposable NodeId = new ExpandedNodeId("ns=2;s=Folder1"), DisplayName = new LocalizedText("Folder1"), NodeClass = NodeClass.Object + }, + new ReferenceDescription + { + NodeId = new ExpandedNodeId("ns=2;s=Folder2"), + DisplayName = new LocalizedText("Folder2"), + NodeClass = NodeClass.Object + }, + new ReferenceDescription + { + NodeId = new ExpandedNodeId("ns=2;s=Tag"), + DisplayName = new LocalizedText("Tag"), + NodeClass = NodeClass.Variable } - ], - HasChildrenResponse = true + ] }; _sessionFactory.EnqueueSession(session); await _service.ConnectAsync(ValidSettings()); var results = await _service.BrowseAsync(); - results[0].HasChildren.ShouldBeTrue(); - session.HasChildrenCount.ShouldBe(1); + results.Select(r => r.HasChildren).ShouldBe([true, true, false]); + session.BrowseCount.ShouldBe(1); + session.BrowseNextCount.ShouldBe(0); } /// @@ -1432,6 +1446,97 @@ public class OpcUaClientServiceTests : IDisposable _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe("opc.tcp://backup:4840"); } + /// + /// R08 4.5: a ConnectAsync on an already-connected client is refused instead of overwriting (and leaking) the live + /// session. The positive control: after a disconnect the same client connects again. + /// + [Fact] + public async Task ConnectAsync_WhenAlreadyConnected_IsRefused_AndKeepsTheLiveSession() + { + var session1 = new FakeSessionAdapter(); + _sessionFactory.EnqueueSession(session1); + await _service.ConnectAsync(ValidSettings()); + + await Should.ThrowAsync(() => _service.ConnectAsync(ValidSettings())); + + _sessionFactory.CreateCallCount.ShouldBe(1); + session1.Disposed.ShouldBeFalse(); + _service.IsConnected.ShouldBeTrue(); + + await _service.DisconnectAsync(); + await _service.ConnectAsync(ValidSettings()); + _sessionFactory.CreateCallCount.ShouldBe(2); + _service.IsConnected.ShouldBeTrue(); + } + + /// + /// R08 4.5: a SubscribeAsync in flight when a keep-alive failover starts must not be lost. Failover runs under the + /// subscription gate, so it waits for the in-flight subscribe to record its item and then replays it onto the new + /// session. Before the fix, failover nulled the subscription state outside the lock and finished first; the subscribe + /// then completed onto the dying session's subscription and was never replayed. + /// + [Fact] + public async Task SubscribeAsync_in_flight_during_failover_is_replayed_onto_the_new_session() + { + var session1 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://primary:4840" }; + var session2 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://backup:4840" }; + _sessionFactory.EnqueueSession(session1); + _sessionFactory.EnqueueSession(session2); + var settings = ValidSettings("opc.tcp://primary:4840"); + settings.FailoverUrls = ["opc.tcp://backup:4840"]; + await _service.ConnectAsync(settings); + + var gate = new TaskCompletionSource(); + session1.CreateSubscriptionGate = gate; + var subscribe = _service.SubscribeAsync(new NodeId("ns=2;s=Racer"), 1000); + await session1.CreateSubscriptionEntered.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + session1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + + gate.SetResult(); + await subscribe.WaitAsync(TimeSpan.FromSeconds(10)); + await failover.WaitAsync(TimeSpan.FromSeconds(10)); + + _service.CurrentConnectionInfo!.EndpointUrl.ShouldBe("opc.tcp://backup:4840"); + var replayed = session2.CreatedSubscriptions.ShouldHaveSingleItem(); + replayed.AddDataChangeCount.ShouldBe(1); + + // And it is the live subscription: unsubscribing removes it from session2's subscription, not session1's. + await _service.UnsubscribeAsync(new NodeId("ns=2;s=Racer")); + replayed.RemoveCount.ShouldBe(1); + replayed.ActiveHandles.ShouldBeEmpty(); + } + + /// + /// A DisconnectAsync issued while a failover is still connecting wins: the failover disposes the session it just + /// opened instead of resurrecting the connection. The positive control is the plain failover test above, where the + /// same gated attempt ends Connected. + /// + [Fact] + public async Task DisconnectAsync_during_failover_is_not_undone_by_the_failover() + { + var session1 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://primary:4840" }; + var session2 = new FakeSessionAdapter { EndpointUrl = "opc.tcp://backup:4840" }; + _sessionFactory.EnqueueSession(session1); + _sessionFactory.EnqueueSession(session2); + var settings = ValidSettings("opc.tcp://primary:4840"); + settings.FailoverUrls = ["opc.tcp://backup:4840"]; + await _service.ConnectAsync(settings); + + var gate = new TaskCompletionSource(); + _sessionFactory.CreateGate = gate; + session1.SimulateKeepAlive(false); + var failover = _service.LastFailover.ShouldNotBeNull(); + + await _service.DisconnectAsync(); + gate.SetResult(); + await failover.WaitAsync(TimeSpan.FromSeconds(10)); + + _service.IsConnected.ShouldBeFalse(); + session2.Disposed.ShouldBeTrue(); + } + /// /// Regression for Client.Shared-005: concurrent subscribe/unsubscribe calls mutating the /// active-subscription bookkeeping must not corrupt the dictionary or throw. diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj index 903b39f9..e588e7af 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj @@ -21,6 +21,8 @@ + + diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs index e056f78b..a333e699 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/HistoryViewModelTests.cs @@ -141,7 +141,16 @@ public class HistoryViewModelTests public void AggregateTypes_ContainsNullForRaw() { _vm.AggregateTypes.ShouldContain((AggregateType?)null); - _vm.AggregateTypes.Count.ShouldBe(13); // null + 12 enum values + _vm.AggregateTypes.Count.ShouldBe(Enum.GetValues().Length + 1); // null + every aggregate + } + + /// The picker offers every client aggregate (the served set), including Range/Delta/PercentGood/... + [Fact] + public void AggregateTypes_offers_every_aggregate() + { + _vm.AggregateTypes.Where(a => a != null).Select(a => a!.Value) + .ShouldBe(Enum.GetValues(), ignoreOrder: true); + _vm.AggregateTypes.ShouldContain(AggregateType.PercentGood); } /// Verifies that the Clear method resets the view model state. diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs index 8c4fd3f5..44ca937a 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/SubscriptionsViewModelTests.cs @@ -111,6 +111,70 @@ public class SubscriptionsViewModelTests _vm.ActiveSubscriptions[0].Value.ShouldBeNull(); } + /// + /// R08 3.6: notifications are routed through a NodeId index, not a scan of every row. The index follows every + /// mutation of (add, remove, replace, clear). + /// + [Fact] + public void NodeId_index_follows_every_collection_mutation() + { + var a = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var b = new SubscriptionItemViewModel("ns=2;s=B", 1000); + var c = new SubscriptionItemViewModel("ns=2;s=C", 1000); + _vm.ActiveSubscriptions.Add(a); + _vm.ActiveSubscriptions.Add(b); + _vm.ActiveSubscriptions.Add(c); + _vm.IndexedNodeCount.ShouldBe(3); + + _vm.ActiveSubscriptions.Remove(b); + _vm.IndexedNodeCount.ShouldBe(2); + + _vm.ActiveSubscriptions[0] = new SubscriptionItemViewModel("ns=2;s=D", 1000); // replaces A + Raise("ns=2;s=A", 1); + Raise("ns=2;s=D", 2); + a.Value.ShouldBeNull(); + _vm.ActiveSubscriptions[0].Value.ShouldBe("2"); + + _vm.ActiveSubscriptions.Clear(); + _vm.IndexedNodeCount.ShouldBe(0); + } + + /// A removed row stops receiving values while its sibling (the positive control) still does. + [Fact] + public void DataChanged_after_remove_updates_only_the_remaining_rows() + { + var a = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var b = new SubscriptionItemViewModel("ns=2;s=B", 1000); + _vm.ActiveSubscriptions.Add(a); + _vm.ActiveSubscriptions.Add(b); + _vm.ActiveSubscriptions.Remove(a); + + Raise("ns=2;s=A", 7); + Raise("ns=2;s=B", 42); + + a.Value.ShouldBeNull(); + b.Value.ShouldBe("42"); + } + + /// Two rows for the same node (no dedup on the manual-add path) both update, as before. + [Fact] + public void DataChanged_updates_every_row_for_the_node() + { + var first = new SubscriptionItemViewModel("ns=2;s=A", 1000); + var second = new SubscriptionItemViewModel("ns=2;s=A", 500); + _vm.ActiveSubscriptions.Add(first); + _vm.ActiveSubscriptions.Add(second); + + Raise("ns=2;s=A", 5); + + first.Value.ShouldBe("5"); + second.Value.ShouldBe("5"); + } + + private void Raise(string nodeId, int value) => + _service.RaiseDataChanged(new DataChangedEventArgs(nodeId, + new DataValue(new Variant(value), StatusCodes.Good, DateTime.UtcNow))); + /// Verifies that Clear removes all subscriptions. [Fact] public void Clear_RemovesAllSubscriptions() diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs new file mode 100644 index 00000000..cad52821 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/CancellationHandlingTests.cs @@ -0,0 +1,50 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests; + +/// +/// Review 07 U7: the AbCip probe/read/write commands had no arm, so a +/// Ctrl-C printed a stack trace. A command cancelled by the operator now exits cleanly with Cancelled.. +/// The gateway is TEST-NET (never answers), so nothing is sent to a real device. +/// +[Trait("Category", "Unit")] +public sealed class CancellationHandlingTests +{ + private const string Blackhole = "ab://192.0.2.1/1,0"; + + private static async Task RunCancelledAsync(AbCipCommandBase command) + { + using var console = new FakeInMemoryConsole(); + console.RequestCancellation(); + + await Should.NotThrowAsync(async () => await command.ExecuteAsync(console)); + return console.ReadErrorString(); + } + + /// A cancelled read prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_read_prints_Cancelled() + { + (await RunCancelledAsync(new ReadCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled probe prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_probe_prints_Cancelled() + { + (await RunCancelledAsync(new ProbeCommand { Gateway = Blackhole, TagPath = "Motor01" })) + .ShouldContain("Cancelled."); + } + + /// A cancelled write prints Cancelled. and does not throw. + [Fact] + public async Task A_cancelled_write_prints_Cancelled() + { + (await RunCancelledAsync(new WriteCommand { Gateway = Blackhole, TagPath = "Motor01", Value = "1" })) + .ShouldContain("Cancelled."); + } +} diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs new file mode 100644 index 00000000..ecdf93f5 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/CliHelpersU7Tests.cs @@ -0,0 +1,111 @@ +using System.Text; +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests; + +/// +/// Review 07 U7, shared half: arrays render their elements (not System.Int32[]), Ctrl-C prints +/// Cancelled., and the subscribe change-line writer is serialised and never throws into the poll loop. +/// +[Trait("Category", "Unit")] +public sealed class CliHelpersU7Tests +{ + private sealed record Handle(string DiagnosticId) : ISubscriptionHandle; + + private static DataChangeEventArgs Change(string reference, object? value) => + new(new Handle("h"), reference, new DataValueSnapshot(value, 0u, DateTime.UtcNow, DateTime.UtcNow)); + + /// An array value renders as its elements, each formatted like a scalar. + [Fact] + public void FormatValue_renders_array_elements() + { + SnapshotFormatter.FormatValue(new[] { 1, 2, 3 }).ShouldBe("[1, 2, 3]"); + SnapshotFormatter.FormatValue(new[] { 1.5f, -2f }).ShouldBe("[1.5, -2]"); + SnapshotFormatter.FormatValue(new[] { true, false }).ShouldBe("[true, false]"); + SnapshotFormatter.FormatValue(new[] { "a", "b" }).ShouldBe("[\"a\", \"b\"]"); + SnapshotFormatter.FormatValue(new List { 7 }).ShouldBe("[7]"); + SnapshotFormatter.FormatValue(Array.Empty()).ShouldBe("[]"); + } + + /// A string is still a scalar, not a sequence of chars (positive control for the array arm). + [Fact] + public void FormatValue_keeps_a_string_scalar() + { + SnapshotFormatter.FormatValue("abc").ShouldBe("\"abc\""); + } + + /// Operator Ctrl-C prints Cancelled. instead of a stack trace. + [Fact] + public async Task RunCancellableAsync_operator_cancellation_prints_Cancelled() + { + using var console = new FakeInMemoryConsole(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Should.NotThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, cts.Token, () => Task.FromCanceled(cts.Token))); + + console.ReadErrorString().ShouldContain("Cancelled."); + } + + /// A cancellation the operator did not request still propagates. + [Fact] + public async Task RunCancellableAsync_unrequested_cancellation_propagates() + { + using var console = new FakeInMemoryConsole(); + + await Should.ThrowAsync(async () => + await DriverCommandBase.RunCancellableAsync(console, CancellationToken.None, + () => throw new OperationCanceledException())); + } + + /// A change line carries the reference, the formatted value and the status. + [Fact] + public void ChangeLineWriter_writes_one_formatted_line_per_change() + { + var output = new StringWriter(); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + write(this, Change("Tag:DInt", new[] { 4, 5 })); + + var line = output.ToString().TrimEnd(); + line.ShouldContain("Tag:DInt = [4, 5] (0x00000000 (Good))"); + } + + /// Concurrent poll-thread changes produce whole lines, never interleaved fragments. + [Fact] + public async Task ChangeLineWriter_serialises_concurrent_writes() + { + var output = new StringWriter(new StringBuilder()); + var write = DriverCommandBase.CreateChangeLineWriter(output, new object()); + + await Task.WhenAll(Enumerable.Range(0, 8).Select(t => Task.Run(() => + { + for (var i = 0; i < 200; i++) write(this, Change($"T{t}", i)); + }))); + + var lines = output.ToString().Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); + lines.Length.ShouldBe(1600); + lines.ShouldAllBe(l => l.StartsWith('[') && l.EndsWith("(Good))")); + } + + /// A failing console (closed pipe) is swallowed so it cannot fault the driver's poll loop. + [Fact] + public void ChangeLineWriter_swallows_a_console_write_failure() + { + var write = DriverCommandBase.CreateChangeLineWriter(new ThrowingWriter(), new object()); + + Should.NotThrow(() => write(this, Change("T", 1))); + } + + private sealed class ThrowingWriter : TextWriter + { + public override Encoding Encoding => Encoding.UTF8; + public override void Write(char value) => throw new IOException("pipe closed"); + public override void WriteLine(string? value) => throw new IOException("pipe closed"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs index 905585bc..76ee84fb 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs @@ -21,10 +21,15 @@ public sealed class AbStatusCodesTests [InlineData((byte)0x0E, AbStatus.NotAllowed)] [InlineData((byte)0x1E, AbStatus.BadReply)] [InlineData((byte)0x01, AbStatus.BadReply)] + [InlineData((byte)0x07, AbStatus.BadReply)] // connection lost: opaque on purpose (AbLegacy #738) + // Lane K F1 (#737): the controller answered and refused this request — not a garbled reply. + [InlineData((byte)0x1C, AbStatus.Unsupported)] // missing attribute list entry data + [InlineData((byte)0x20, AbStatus.Unsupported)] // invalid parameter public void Cip_general_status_maps(byte general, AbStatus expected) => AbStatusCodes.FromCip(general, ReadOnlySpan.Empty).ShouldBe(expected); [Theory] + [InlineData((ushort)0x2104, AbStatus.OutOfBounds)] // offset beyond end of the tag (Lane K F1) [InlineData((ushort)0x2105, AbStatus.OutOfBounds)] [InlineData((ushort)0x2107, AbStatus.TypeMismatch)] [InlineData((ushort)0x9999, AbStatus.BadReply)] @@ -37,16 +42,42 @@ public sealed class AbStatusCodesTests [Theory] [InlineData((byte)0x00, null, AbStatus.Ok)] - [InlineData((byte)0x10, null, AbStatus.Unsupported)] - [InlineData((byte)0x20, null, AbStatus.NotFound)] - [InlineData((byte)0x30, null, AbStatus.NotAllowed)] - [InlineData((byte)0x40, null, AbStatus.DeviceStateConflict)] - [InlineData((byte)0xF0, (byte)0x06, AbStatus.NotFound)] - [InlineData((byte)0xF0, (byte)0x07, AbStatus.NotFound)] + [InlineData((byte)0x10, null, AbStatus.Unsupported)] // illegal command or format + // Lane K F3 (AbLegacy F8, review 07 r0 I4), per the DF1/PCCC STS table (1770-6.5.16): + [InlineData((byte)0x20, null, AbStatus.BadConnection)] // host has a problem and will not communicate + [InlineData((byte)0x30, null, AbStatus.BadConnection)] // remote node host missing / disconnected / shut down + [InlineData((byte)0x40, null, AbStatus.DeviceStateConflict)] // host hardware fault (the host answered) + [InlineData((byte)0x50, null, AbStatus.NotFound)] // addressing problem or memory protect rungs + [InlineData((byte)0x60, null, AbStatus.NotAllowed)] // function not allowed due to command protection + [InlineData((byte)0x70, null, AbStatus.DeviceStateConflict)] // processor is in Program mode + [InlineData((byte)0x80, null, AbStatus.BadReply)] // compatibility-mode file missing: unclassified + [InlineData((byte)0x90, null, AbStatus.BadConnection)] // remote node cannot buffer command + [InlineData((byte)0xB0, null, AbStatus.DeviceStateConflict)] // remote node problem due to download + [InlineData((byte)0xF0, (byte)0x06, AbStatus.NotFound)] // address does not point to something usable + [InlineData((byte)0xF0, (byte)0x07, AbStatus.OutOfBounds)] // file is wrong size [InlineData((byte)0xF0, (byte)0x0A, AbStatus.BadReply)] + [InlineData((byte)0xF0, null, AbStatus.BadReply)] public void Pccc_status_maps(byte sts, byte? ext, AbStatus expected) => AbStatusCodes.FromPccc(sts, ext).ShouldBe(expected); + /// + /// A dead DH+/DH-485 node behind a bridge answers STS 0x30 (and a busy one 0x90). Neither proves the END + /// device answered, so they are the no-reply comms class, never one of the "answered" refusals — that + /// classification made such a device count as reachable forever (no backoff, no comms-loss watchdog). + /// + /// The PCCC STS byte. + [Theory] + [InlineData((byte)0x20)] + [InlineData((byte)0x30)] + [InlineData((byte)0x90)] + public void Pccc_no_reply_statuses_are_never_answered_refusals(byte sts) + { + var status = AbStatusCodes.FromPccc(sts, null); + status.ShouldBe(AbStatus.BadConnection); + status.ShouldNotBeOneOf(AbStatus.NotFound, AbStatus.NotAllowed, AbStatus.OutOfBounds, + AbStatus.DeviceStateConflict, AbStatus.TooLarge, AbStatus.TooSmall, AbStatus.TypeMismatch); + } + [Fact] public void Ok_is_zero_and_every_error_is_negative() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs index 0d4c2003..ec362206 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs @@ -115,6 +115,30 @@ public sealed class ConnectionManagerServicesTests ex.Status.ShouldBe(AbStatus.BadConnection); // general 0x01 has no specific mapping } + /// + /// Lane K F1 remapped general 0x1C / 0x20 and 0xFF/0x2104 for TAG requests (a per-request refusal). A Forward + /// Open refused with one of them is still a connection failure, exactly as before: the whole connection is + /// unusable, so it must not read as a per-tag refusal. + /// + /// The refused Forward Open reply. + [Theory] + [InlineData("DB001C00")] + [InlineData("DB002000")] + [InlineData("DB00FF010421")] + public void Forward_open_refused_with_a_request_level_status_is_still_BadConnection(string hex) + { + var ex = Should.Throw(() => + ConnectionManagerServices.ParseForwardOpenReply(CipReply.Parse(Convert.FromHexString(hex)))); + ex.Status.ShouldBe(AbStatus.BadConnection); + } + + /// Positive control: a Forward Open refusal that was already specifically mapped keeps its status. + [Fact] + public void Forward_open_refused_service_not_supported_stays_Unsupported() => + Should.Throw(() => + ConnectionManagerServices.ParseForwardOpenReply(CipReply.Parse(Convert.FromHexString("DB000800")))) + .Status.ShouldBe(AbStatus.Unsupported); + [Fact] public void Refused_forward_open_exception_is_not_aliased_to_the_reply_array() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs index b8d36f68..fbac7f56 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Pccc/PcccServicesTests.cs @@ -90,6 +90,32 @@ public sealed class PcccServicesTests [Fact] public void Plc5_write_is_not_shipped_and_flagged() => PcccServices.Plc5WriteVerified.ShouldBeFalse(); + // ---- ParseReply: golden STS replies (Lane K F3 / AbLegacy F8) ---- + + /// + /// Wire-level golden replies: the Execute PCCC reply bytes a device (or a bridge in front of it) sends for each + /// STS, through , to the status the drivers classify. + /// + /// The reply's STS (+ EXT STS) bytes as hex. + /// The status ParseReply raises. + [Theory] + [InlineData("20", AbStatus.BadConnection)] + [InlineData("30", AbStatus.BadConnection)] + [InlineData("90", AbStatus.BadConnection)] + [InlineData("50", AbStatus.NotFound)] + [InlineData("60", AbStatus.NotAllowed)] + [InlineData("70", AbStatus.DeviceStateConflict)] + [InlineData("B0", AbStatus.DeviceStateConflict)] + [InlineData("F0|06", AbStatus.NotFound)] + [InlineData("F0|07", AbStatus.OutOfBounds)] + public void ParseReply_golden_sts(string stsHex, AbStatus expected) + { + var parts = stsHex.Split('|'); + var hex = Id + "4F" + parts[0] + "9987" + (parts.Length > 1 ? parts[1] : ""); + Should.Throw(() => PcccServices.ParseReply(Reply(hex), 0x8799)) + .Status.ShouldBe(expected); + } + // ---- ParseReply ---- private static CipReply Reply(string hex, byte general = 0) => new(0x4B, general, [], Hex(hex)); @@ -122,7 +148,23 @@ public sealed class PcccServicesTests [Fact] public void ParseReply_cip_error_status_throws() => - Should.Throw(() => PcccServices.ParseReply(Reply("", 0x08), 1)).Status.ShouldBe(AbStatus.Unsupported); + Should.Throw(() => PcccServices.ParseReply(Reply("", 0x08), 1)).Status.ShouldBe(AbStatus.Unsupported); + + /// Review I2: a status the device REPLIED with is marked as such (envelope CIP status and PCCC STS). + [Fact] + public void ParseReply_marks_device_statuses_as_device_replies() + { + Should.Throw(() => PcccServices.ParseReply(Reply("", 0x20), 1)).Status.ShouldBe(AbStatus.Unsupported); + Should.Throw(() => PcccServices.ParseReply(Reply(Id + "4F10 9987"), 0x8799)).Status.ShouldBe(AbStatus.Unsupported); + } + + /// Positive control: a refusal raised by a builder before anything is sent is NOT a device reply. + [Fact] + public void A_pre_wire_builder_refusal_is_not_a_device_reply() + { + var ex = Should.Throw(() => PcccServices.SlcTypedRead(1, A("N7:0"), 999)); + ex.ShouldNotBeOfType(); + } [Fact] public void ParseReply_wrong_command_byte_is_BadReply() => diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs index d196d294..cf4a1965 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs @@ -25,3 +25,60 @@ public sealed class CipWriterTests w.ToArray().ShouldBe(new byte[] { 0, 0, 7 }); // nothing was patched } } + +/// +/// R07:L5: works in a buffer rented from and hands +/// it back on . The returned array must be the caller's own (a later writer reusing +/// the pooled buffer cannot change it), growth must keep every byte, and a writer used after ToArray must refuse +/// rather than write into a buffer it no longer owns. +/// +public sealed class CipWriterPoolingTests +{ + [Fact] + public void Growth_past_the_initial_buffer_keeps_every_byte() + { + var w = new CipWriter(); + for (var i = 0; i < 1000; i++) w.U8((byte)i); + var bytes = w.ToArray(); + bytes.Length.ShouldBe(1000); + bytes.Select((b, i) => b == (byte)i).ShouldAllBe(ok => ok); + } + + [Fact] + public void A_returned_array_is_not_affected_by_a_later_writer_reusing_the_pooled_buffer() + { + var first = new CipWriter().U32(0xAABBCCDD).ToArray(); + for (var i = 0; i < 50; i++) new CipWriter().U32(0x11111111).U32(0x22222222).ToArray(); + first.ShouldBe(new byte[] { 0xDD, 0xCC, 0xBB, 0xAA }); + } + + [Fact] + public void ToArray_twice_returns_equal_independent_copies() + { + var w = new CipWriter().U16(0x0102); + var a = w.ToArray(); + var b = w.ToArray(); + b.ShouldBe(a); + b.ShouldNotBeSameAs(a); + } + + [Fact] + public void Writing_after_ToArray_is_refused() + { + var w = new CipWriter().U8(1); + _ = w.ToArray(); + Should.Throw(() => w.U8(2)); + Should.Throw(() => w.PatchU16(0, 1)); + w.Length.ShouldBe(1); + } + + [Fact] + public void Nested_writers_produce_independent_frames() + { + // The Execute PCCC / encapsulation builders build an inner writer while an outer one is live. + var outer = new CipWriter().U8(0xAA); + var inner = new CipWriter().U8(0x01).U8(0x02).ToArray(); + var frame = outer.Bytes(inner).U8(0xBB).ToArray(); + frame.ShouldBe(new byte[] { 0xAA, 0x01, 0x02, 0xBB }); + } +} 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 028d6e27..03434b0a 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 @@ -297,6 +297,11 @@ public sealed class NativeAbCipTagRuntimeTests [InlineData("D2000A00", AbStatus.OutOfBounds)] [InlineData("D2000F00", AbStatus.NotAllowed)] [InlineData("D200FF010521", AbStatus.OutOfBounds)] + // Lane K F1 (#737): these used to arrive as BadReply, i.e. a comms fault (Degraded + evict) for a tag the + // controller had simply refused. AbCipRefusals.IsDeviceRefusal classifies the new statuses with no driver change. + [InlineData("D2001C00", AbStatus.Unsupported)] // missing attribute list entry data + [InlineData("D2002000", AbStatus.Unsupported)] // invalid parameter + [InlineData("D200FF010421", AbStatus.OutOfBounds)] // offset beyond end of the tag 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(reply)) }; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs index 8d5185fe..845a3a07 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs @@ -220,14 +220,30 @@ public sealed class NativeAbLegacyTagRuntimeTests } [Fact] - public async Task STS_0x20_on_initialize_is_NotFound() + public async Task STS_0x50_on_initialize_is_NotFound() { - await using var h = new Harness("slc500", "N99:0").Responder((_, _) => (0x20, null, [])); + await using var h = new Harness("slc500", "N99:0").Responder((_, _) => (0x50, null, [])); var ex = await Should.ThrowAsync(() => h.Runtime.InitializeAsync(CancellationToken.None)); ex.Status.ShouldBe(AbStatus.NotFound); h.Runtime.GetStatus().ShouldBe((int)AbStatus.NotFound); } + /// + /// Lane K F3 (F8): STS 0x20 / 0x30 come from the host or a bridge when the END device cannot be reached, so they + /// are the no-reply class (BadConnection), not an answered refusal. + /// + /// The PCCC STS. + [Theory] + [InlineData((byte)0x20)] + [InlineData((byte)0x30)] + public async Task STS_from_a_bridge_for_an_unreachable_node_is_BadConnection(byte sts) + { + await using var h = new Harness("slc500", "N7:0").Responder((_, _) => (sts, null, [])); + var ex = await Should.ThrowAsync(() => h.Runtime.InitializeAsync(CancellationToken.None)); + ex.Status.ShouldBe(AbStatus.BadConnection); + h.Runtime.GetStatus().ShouldBe((int)AbStatus.BadConnection); + } + [Fact] public async Task Initialize_performs_exactly_one_read() { @@ -305,7 +321,7 @@ public sealed class NativeAbLegacyTagRuntimeTests public async Task A_failed_second_write_chunk_is_reported_as_failed_not_Good() { await using var h = new Harness("slc500", "N7:0", count: 130).Responder((_, n) => - n == 1 ? (0x30, null, []) : (0x00, null, [])); + n == 1 ? (0x60, null, []) : (0x00, null, [])); var ex = await Should.ThrowAsync(() => h.Runtime.WriteAsync(CancellationToken.None)); ex.Status.ShouldBe(AbStatus.NotAllowed); h.Runtime.GetStatus().ShouldBe((int)AbStatus.NotAllowed); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs new file mode 100644 index 00000000..58eecee7 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativePcccBridgeStatusHealthTests.cs @@ -0,0 +1,131 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Lane K F3 (AbLegacy F8, review 07 r0 I4), end to end over a real socket with the PRODUCTION +/// : a bridge (DHRIO / 1761-NET-ENI) in front of a dead DH+ node answers every +/// PCCC request with STS 0x30 "remote node host missing". That is NOT the device answering, so the driver must treat +/// it as a comms fault — Degraded, LastSuccessfulRead not advanced — or the comms-loss watchdog never fires +/// for it. The positive control is a live device refusing a tag (STS 0x60, command protection): per-tag Bad, Healthy, +/// LastSuccessfulRead advanced. +/// +[Trait("Category", "Unit")] +public sealed class NativePcccBridgeStatusHealthTests +{ + private static readonly byte[] FixedId = [0x07, 0x3D, 0xF3, 0x45, 0x43, 0x50, 0x21]; + + /// A fake whose Execute PCCC envelope itself is refused with CIP general status . + private static FakeEipServer CipEnvelopeServer(byte general) + { + var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => FakeEipServer.Command(req) == Ab.Transport.Eip.EncapCommand.SendRRData + ? FakeEipServer.RRReply(req, [0xCB, 0x00, general, 0x00]) + : null; + return server; + } + + /// A PCCC fake that answers every request with one fixed STS. + private static FakeEipServer StsServer(byte sts) + { + var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => + { + if (FakeEipServer.Command(req) != Ab.Transport.Eip.EncapCommand.SendRRData) return null; + var cip = FakeEipServer.RequestCip(req); + byte[] head = [0xCB, 0x00, 0x00, 0x00, .. FixedId, 0x4F]; + return FakeEipServer.RRReply(req, [.. head, sts, cip[15], cip[16]]); + }; + return server; + } + +#pragma warning disable OTOPCUA0001 // the driver is called directly to pin its OWN classification (no resilience wrap) + private static async Task DriverAsync(FakeEipServer server) + { + var host = $"ab://127.0.0.1:{server.Port}/1,0"; + var driver = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.Slc500)], + RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("T", host, "N7:0", AbLegacyDataType.Int)]), + Timeout = TimeSpan.FromSeconds(5), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-pccc-bridge", new NativeAbLegacyTagFactory(new DeviceSessionPool())); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + return driver; + } + + /// STS 0x30 from a bridge: a comms fault, never evidence the device answered. + [Fact] + public async Task A_dead_node_behind_a_bridge_degrades_and_does_not_advance_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + await using var server = StsServer(0x30); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbLegacyStatusMapper.BadCommunicationError); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Degraded); + health.LastSuccessfulRead.ShouldBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } + + /// Positive control: STS 0x60 is the live device refusing the tag — answered, so Healthy and advanced. + [Fact] + public async Task A_live_device_refusing_the_tag_stays_Healthy_and_advances_LastSuccessfulRead() + { + var ct = TestContext.Current.CancellationToken; + await using var server = StsServer(0x60); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbOpcUaStatusMap.BadNotWritable); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead.ShouldNotBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } + + /// + /// Review I2: a refusal that came OFF THE WIRE — the device (or the bridge's Execute PCCC object) answered with + /// CIP 0x08 / 0x1C / 0x20 on the envelope, or PCCC STS 0x10 — is an answered refusal: per-tag BadNotSupported, + /// Healthy, LastSuccessfulRead advanced. It must not land in the "refused before the wire" neutral class + /// (which advances nothing). The local-refusal counterpart stays neutral (NativeAbLegacyRefusedWriteTests). + /// + /// The CIP general status of the envelope reply, or 0 for a PCCC STS 0x10 reply. + [Theory] + [InlineData((byte)0x08)] + [InlineData((byte)0x1C)] + [InlineData((byte)0x20)] + [InlineData((byte)0x00)] + public async Task A_wire_Unsupported_reply_is_an_answered_refusal(byte general) + { + var ct = TestContext.Current.CancellationToken; + await using var server = general == 0 ? StsServer(0x10) : CipEnvelopeServer(general); + var driver = await DriverAsync(server); + var before = driver.GetHealth().LastSuccessfulRead; + + var result = (await driver.ReadAsync(["T"], ct)).Single(); + + result.StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported); + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Healthy); + health.LastSuccessfulRead.ShouldNotBeNull(); + health.LastSuccessfulRead.ShouldNotBe(before); + await driver.ShutdownAsync(ct); + server.HandlerErrors.ShouldBeEmpty(); + } +#pragma warning restore OTOPCUA0001 +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs new file mode 100644 index 00000000..6cf0450b --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusPooledFramingTests.cs @@ -0,0 +1,62 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// R07:L5 (Modbus half): the RTU ADU is now written into a pooled buffer and the response is deframed in one pooled +/// buffer. The wire bytes and the decoded PDUs must be exactly what the allocating code produced, and a reused pooled +/// buffer must never leak a previous frame's bytes into the next result. +/// +[Trait("Category", "Unit")] +public sealed class ModbusPooledFramingTests +{ + /// The pooled ADU writer matches the reference construction (unit + PDU, CRC appended low byte first). + /// The PDU as hex. + [Theory] + [InlineData("0300000001")] + [InlineData("10000A0002040001000200")] + [InlineData("06")] + public void WriteAdu_matches_the_reference_ADU(string hex) + { + var pdu = Convert.FromHexString(hex); + var reference = ModbusCrc.Append([0x11, .. pdu]); + + var pooled = new byte[ModbusRtuFraming.AduLength(pdu.Length) + 7]; // oversized, like a rented array + ModbusRtuFraming.WriteAdu(0x11, pdu, pooled.AsSpan(0, ModbusRtuFraming.AduLength(pdu.Length))); + + pooled.AsSpan(0, reference.Length).ToArray().ShouldBe(reference); + ModbusRtuFraming.BuildAdu(0x11, pdu).ShouldBe(reference); + } + + /// + /// A long read reply followed by a short echo and an exception frame, each deframed through the pooled buffer: + /// every result is exactly its own PDU (no stale tail from the previous, longer frame). + /// + [Fact] + public async Task Consecutive_deframes_through_the_pooled_buffer_do_not_leak_bytes() + { + var ct = TestContext.Current.CancellationToken; + byte[] longRead = [0x03, 200, .. Enumerable.Range(0, 200).Select(i => (byte)(i + 1))]; + byte[] echo = [0x06, 0x00, 0x01, 0x00, 0x2A]; + + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, longRead)), 1, ct)) + .ShouldBe(longRead); + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, echo)), 1, ct)) + .ShouldBe(echo); + var ex = await Should.ThrowAsync(() => + ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, [0x83, 0x02])), 1, ct)); + ex.ExceptionCode.ShouldBe((byte)0x02); + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, [0x03, 2, 9, 8])), 1, ct)) + .ShouldBe(new byte[] { 0x03, 2, 9, 8 }); + } + + /// A byte-count-255 read (the largest RTU reply) fits the pooled frame. + [Fact] + public async Task The_largest_read_reply_deframes() + { + byte[] max = [0x04, 255, .. new byte[255]]; + (await ModbusRtuFraming.ReadResponsePduAsync(new MemoryStream(ModbusRtuFraming.BuildAdu(1, max)), 1, + TestContext.Current.CancellationToken)).ShouldBe(max); + } +}