From 8f5b265db8670a5959aae8876adfb90414648b73 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 17:13:54 -0400 Subject: [PATCH] fix(opcuaclient): native-alarm ConditionRefresh + transition table, branch, dedup and thread-safety fixes (#619 re-review) Re-review of 3de2cc3b/454775c6: - AlarmTransitions could not reach (inactive, unacked): (active,acked)->(inactive,unacked) emitted Clear (host stayed acked) and a first-seen cleared-unacked condition emitted nothing. The table is now total; an exhaustive test replays every (prior, target) pair through the projector's semantics. - The per-RawPath read-compute-emit was a lock-free TryGetValue/set across SDK publish threads; it now runs under one lock with the projection and emission. - Condition BRANCH events (non-null BranchId) were projected (flipping the trunk) and remembered as the ack target (acking the branch); they are dropped. BranchId is select clause 9. - EventId is copied, not aliased from the decoded notification. - An unchanged state re-emitted Unspecified; it now emits nothing unless message/severity changed (an Unspecified projects from the host projector's default after a redeploy reset it). - Exact upstream severity rides OpcUaSeverity (#626) instead of being re-bucketed. - ShutdownAsync no longer forgets the last-told state: a reinit re-subscribes the same set and the refresh would otherwise re-raise every acked alarm. A CHANGED set (host rebuilt) forgets it. ConditionRefresh: called after the alarm event item is created, after an in-place reconnect (subscriptions found on the new session by display name, since a recreate clones them) and on RefreshRequired. Refresh markers are never projected. BadNotSupported/BadMethodInvalid/... logs once. Wire test: a condition already active before subscribe is reported Raise; a release+resubscribe refresh re-raises nothing (falsified by disabling the refresh call). Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/OpcUaClient.md | 11 +- .../OpcUaClientDriver.cs | 291 +++++++++++++++--- .../OpcUaClientAlarmTests.cs | 185 ++++++++++- .../OpcUaClientDriverWireTests.cs | 62 ++++ 4 files changed, 499 insertions(+), 50 deletions(-) diff --git a/docs/drivers/OpcUaClient.md b/docs/drivers/OpcUaClient.md index 9bd36cd6..0b1c12bb 100644 --- a/docs/drivers/OpcUaClient.md +++ b/docs/drivers/OpcUaClient.md @@ -92,15 +92,20 @@ rather than surfacing as a runtime surprise. | `IHistoryProvider` | `ReadRawAsync` / `ReadProcessedAsync` / `ReadAtTimeAsync` → `Session.HistoryReadAsync` | **Unique to this driver** — passthrough history read against the upstream server | > **Native alarms (#619).** An alarm raw tag is `TagConfig` `{ "nodeId": "", "alarm": {...} }`. -> The host subscribes with RawPaths and routes on (driver, RawPath), so the driver matches each event's +> The host subscribes with RawPaths and routes on (driver, RawPath), so the driver matches each > event's `ConditionId` (then its `SourceNode`) against the tag's authored `nodeId` and reports the **RawPath** as > `ConditionId`. Author `nodeId` as the upstream **condition** node for a 1:1 mapping (required when one source carries > several conditions, e.g. Hi + HiHi); authoring the **source** node folds every condition of that source into one > tag. `Acknowledge` is called on the condition node of the last event seen for the RawPath **with that event's > EventId** (Part 9 §5.7.3). The event's `ActiveState/Id` + `AckedState/Id` are selected and turned into the > `Raise`/`Acknowledge`/`Clear`/`Retrigger` kinds the host projector consumes (without a kind no upstream alarm could ever -> show active). No `ConditionRefresh` is issued on subscribe, so an upstream alarm already active when the subscription -> opens appears at its next transition. Index keys are server-stable (`nsu=`), so a namespace reorder across a reconnect keeps +> show active); the kinds are emitted from the state **last told to the host** per RawPath, so an identical re-delivery +> emits nothing (no re-raise, which would un-ack). Condition **branch** events (non-null `BranchId`) are ignored. +> **`ConditionRefresh`** is called after the event item is created, after every in-place reconnect, and on a +> `RefreshRequired` event, so a condition already active upstream is reported at subscribe; `RefreshStart`/`RefreshEnd` +> markers are dropped. A server without it (`BadNotSupported`/`BadMethodInvalid`/…) is logged once and tolerated. +> Last-told state survives a re-initialise (the host re-subscribes the same set over nodes it did not rebuild) and is +> forgotten when the subscribed set changes (the host rebuilt, recreating the nodes inactive). Index keys are server-stable (`nsu=`), so a namespace reorder across a reconnect keeps > matching. docker-dev seeds an opc-plc driver (`MAIN-opcua`); the alarm tag is > created only when `@OpcPlcAlarmSourceNodeId` is set in `docker-dev/seed/seed-clusters.sql`. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index 85699fef..814c1f98 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -102,8 +102,19 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit /// #619 review: the (Active, Acked) state last told to the host per reported condition id, from /// which picks the Kind sequence that moves the host's projector to the /// upstream event's state. - private readonly System.Collections.Concurrent.ConcurrentDictionary _alarmStateByRawPath = - new(StringComparer.Ordinal); + private readonly Dictionary _alarmStateByRawPath = new(StringComparer.Ordinal); + + /// #619 re-review: serialises the read-compute-emit over . Event + /// notifications arrive on SDK publish threads (and, across a resubscribe, from two subscriptions at once), so + /// a lock-free prior/target exchange could emit two kind sequences computed from the same prior. + private readonly object _alarmStateLock = new(); + + /// #619 re-review: the ref set the last carried. Survives + /// deliberately — see . + private HashSet? _lastAlarmRefSet; + + /// Latched once the upstream server has refused ConditionRefresh as unsupported (logged once). + private int _conditionRefreshUnsupportedLogged; private long _nextAlarmSubscriptionId; /// Occurs when an alarm event is received from the remote OPC UA server. @@ -891,8 +902,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit catch { /* best-effort */ } } _alarmSubscriptions.Clear(); - _alarmConditionByRawPath.Clear(); - _alarmStateByRawPath.Clear(); + // #619 re-review: the last-told alarm states (and ack targets) deliberately SURVIVE a shutdown. A + // ReinitializeAsync (comms-loss watchdog / Force Reconnect) is followed by the host re-subscribing the + // same set over condition nodes it did NOT rebuild; forgetting here would make the post-subscribe + // ConditionRefresh re-raise every active+acked alarm (an un-ack flicker). BeginAlarmSubscription forgets + // them when the set changes, which is when the host did rebuild. await ReleaseSessionAsync(cancellationToken).ConfigureAwait(false); _namespaceMap = null; @@ -1637,6 +1651,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit private const int AlarmFieldConditionId = 6; private const int AlarmFieldActiveState = 7; private const int AlarmFieldAckedState = 8; + private const int AlarmFieldBranchId = 9; /// public async Task SubscribeAlarmsAsync( @@ -1664,15 +1679,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit && !NamespaceMap.TryResolve(sessionForResolve, raw, out live)) return null; return AlarmIndexKey(mapForResolve, live); }); - // Forget ack targets + last-told states for alarm tags no longer subscribed (removed/renamed RawPaths). - foreach (var known in _alarmConditionByRawPath.Keys) - if (!sourceNodeIds.Contains(known, StringComparer.Ordinal)) _alarmConditionByRawPath.TryRemove(known, out _); - foreach (var known in _alarmStateByRawPath.Keys) - if (!sourceNodeIds.Contains(known, StringComparer.Ordinal)) _alarmStateByRawPath.TryRemove(known, out _); + BeginAlarmSubscription(sourceNodeIds); var subscription = new Subscription(telemetry: null!, new SubscriptionOptions { - DisplayName = $"opcua-alarm-sub-{id}", + DisplayName = $"{AlarmSubscriptionNamePrefix}{id}", PublishingInterval = 500, // 500ms — alarms don't need fast polling; the server pushes KeepAliveCount = _options.SubscriptionKeepAliveCount, LifetimeCount = _options.SubscriptionLifetimeCount, @@ -1721,6 +1732,14 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit BrowsePath = [new QualifiedName(BrowseNames.AckedState), new QualifiedName(BrowseNames.Id)], AttributeId = Attributes.Value, }); + // #619 re-review: BranchId — a non-null one marks a condition BRANCH event (a previous state, with its own + // EventId). Only the trunk (null BranchId) is the condition's current state. + filter.SelectClauses.Add(new SimpleAttributeOperand + { + TypeDefinitionId = ObjectTypeIds.ConditionType, + BrowsePath = [new QualifiedName(BrowseNames.BranchId)], + AttributeId = Attributes.Value, + }); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); try @@ -1757,6 +1776,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit await subscription.CreateItemsAsync(cancellationToken).ConfigureAwait(false); _alarmSubscriptions[id] = new RemoteAlarmSubscription(subscription, handle, eventItem, notifHandler); + + // #619: a new event item only reports FUTURE transitions — a condition already active upstream would + // stay invisible until it next changed. ConditionRefresh re-delivers every retained condition's + // current state through the normal path (deduplicated against the last-told state). + await TryConditionRefreshAsync(subscription, cancellationToken).ConfigureAwait(false); } finally { _gate.Release(); } @@ -1898,30 +1922,194 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit MonitoredItemNotificationEventArgs args) { if (args.NotificationValue is not EventFieldList efl) return; - var e = ProjectEvent(efl.EventFields, sourceFilter, handle, - n => AlarmIndexKey(_namespaceMap, n), (raw, target) => _alarmConditionByRawPath[raw] = target, - out var state); - if (e is null) return; - if (state is not { } target) + RouteAlarmEvent(efl.EventFields, sourceFilter, handle); + } + + /// + /// #619 re-review: open (or re-open) the host's alarm subscription over . + /// A CHANGED set means the host rebuilt its address space (an alarm-bearing delta always does), recreating + /// every condition node inactive+acked — so every last-told state is forgotten and the post-subscribe + /// ConditionRefresh re-reports from that default. The SAME set (a reinit / reconnect re-subscribe) keeps + /// them, so the refresh copy of an already-reported state emits nothing. Ack targets of refs no longer + /// subscribed are pruned either way. + /// + /// The refs the host is subscribing. + internal void BeginAlarmSubscription(IReadOnlyList sourceNodeIds) + { + var set = new HashSet(sourceNodeIds, StringComparer.Ordinal); + lock (_alarmStateLock) { - // Not a condition (no ActiveState/AckedState) — a message/severity-only update, as before. - OnAlarmEvent?.Invoke(this, e); + if (_lastAlarmRefSet is null || !_lastAlarmRefSet.SetEquals(set)) _alarmStateByRawPath.Clear(); + _lastAlarmRefSet = set; + foreach (var known in _alarmConditionByRawPath.Keys) + if (!set.Contains(known)) _alarmConditionByRawPath.TryRemove(known, out _); + } + } + + /// + /// #619 — route one upstream event: drop refresh markers (a RefreshRequired asks for a new refresh), project + /// it, and emit the kinds that move the host from the state last told to the event's. An event that changes + /// nothing the host shows (a ConditionRefresh copy) emits nothing. + /// + /// The event's fields, in select-clause order. + /// The subscription's upstream-key → RawPath index. + /// The subscription handle to stamp. + internal void RouteAlarmEvent( + IReadOnlyList fields, IReadOnlyDictionary sourceFilter, IAlarmSubscriptionHandle handle) + { + if (IsRefreshRequired(fields)) + { + // Part 9 §5.11.7: the server lost events (queue overflow / restart) and asks for a refresh. + _ = Task.Run(() => RefreshAllAlarmSubscriptionsAsync(CancellationToken.None)); return; } - // Fresh prior = the host projector's own default (inactive, acked). - var prior = _alarmStateByRawPath.TryGetValue(e.ConditionId, out var p) ? p : (Active: false, Acked: true); - _alarmStateByRawPath[e.ConditionId] = target; - foreach (var kind in AlarmTransitions(prior, target)) - OnAlarmEvent?.Invoke(this, e with { Kind = kind }); + // Projection, the per-RawPath bookkeeping AND the emission run under one lock, so two notifications for + // one condition (two publish threads; two subscriptions across a resubscribe) can never compute their + // kinds from the same prior, nor reach the host out of order. The host's handler only marshals. + lock (_alarmStateLock) + { + var e = ProjectEvent(fields, sourceFilter, handle, + n => AlarmIndexKey(_namespaceMap, n), (raw, target) => _alarmConditionByRawPath[raw] = target, + out var state); + if (e is null) return; + if (state is not { } target) + { + // Not a condition (no ActiveState/AckedState) — a message/severity-only event, as before. + OnAlarmEvent?.Invoke(this, e); + return; + } + + // Fresh prior = the host projector's own default (inactive, acked). + var told = new AlarmToldState(target.Active, target.Acked, e.Message, e.OpcUaSeverity); + var prior = _alarmStateByRawPath.TryGetValue(e.ConditionId, out var p) + ? p + : new AlarmToldState(false, true, null, null); + _alarmStateByRawPath[e.ConditionId] = told; + + var kinds = AlarmTransitions((prior.Active, prior.Acked), target); + if (kinds.Count == 0) + { + // Same state: a message/severity change still reaches the host; an identical copy does not. + if (prior == told) return; + kinds = [AlarmTransitionKind.Unspecified]; + } + foreach (var kind in kinds) + OnAlarmEvent?.Invoke(this, e with { Kind = kind }); + } } + /// #619 re-review — what the host was last told about one condition: its state plus the message and + /// exact severity, so a refresh copy is recognised as a duplicate. + /// ActiveState/Id last told. + /// AckedState/Id last told. + /// Message last told (null before any event). + /// Exact severity last told (null before any event). + private readonly record struct AlarmToldState(bool Active, bool Acked, string? Message, int? Severity); + + /// True for a RefreshRequiredEventType event (the server asks the client to ConditionRefresh). + /// The event's fields. + /// Whether the event is a RefreshRequired marker. + internal static bool IsRefreshRequired(IReadOnlyList fields) => + fields.Count > AlarmFieldEventType && fields[AlarmFieldEventType].Value is NodeId t + && t == ObjectTypeIds.RefreshRequiredEventType; + + /// True for the three ConditionRefresh marker event types — never alarms. + /// The event's EventType. + /// Whether it is a refresh marker. + private static bool IsRefreshMarker(object? eventType) => + eventType is NodeId t + && (t == ObjectTypeIds.RefreshStartEventType || t == ObjectTypeIds.RefreshEndEventType + || t == ObjectTypeIds.RefreshRequiredEventType); + + /// A ConditionRefresh refusal meaning "this server does not do refresh" (tolerated, logged once). + /// The refusal status. + /// Whether the server simply lacks ConditionRefresh. + internal static bool IsConditionRefreshUnsupported(StatusCode code) => + code == StatusCodes.BadNotSupported || code == StatusCodes.BadMethodInvalid + || code == StatusCodes.BadNotImplemented || code == StatusCodes.BadServiceUnsupported + || code == StatusCodes.BadNodeIdUnknown; + + /// + /// #619 — call ConditionRefresh for so conditions already active upstream + /// are reported. Never throws: an unsupported server is logged once, any other failure warns. + /// + /// The (created) alarm subscription. + /// Cancellation. + /// A task completing when the refresh call returns. + private async Task TryConditionRefreshAsync(Subscription subscription, CancellationToken cancellationToken) + { + try + { + await subscription.ConditionRefreshAsync(cancellationToken).ConfigureAwait(false); + } + catch (ServiceResultException ex) when (IsConditionRefreshUnsupported(ex.StatusCode)) + { + if (Interlocked.Exchange(ref _conditionRefreshUnsupportedLogged, 1) == 0) + _logger.LogInformation( + "OpcUaClientDriver '{DriverInstanceId}': upstream server does not support ConditionRefresh " + + "({StatusCode}); conditions already active are reported only on their next transition.", + _driverInstanceId, ex.StatusCode); + } + catch (Exception ex) when (ex is not OperationCanceledException || !cancellationToken.IsCancellationRequested) + { + _logger.LogWarning(ex, + "OpcUaClientDriver '{DriverInstanceId}': ConditionRefresh on alarm subscription {SubscriptionId} failed; " + + "conditions already active upstream may not be reported until their next transition.", + _driverInstanceId, subscription.Id); + } + catch (OperationCanceledException) + { + // The caller cancelled; the subscription itself is in place. + } + } + + /// + /// #619 — ConditionRefresh every alarm subscription on the current session: after an in-place reconnect + /// (transferred or SDK-recreated subscriptions — matched by display name, since a recreate clones the + /// objects) and on a RefreshRequired event. Events missed during the outage + /// reach the host through the normal deduplicated path. + /// + /// Cancellation. + /// A task completing when every refresh call has returned. + private async Task RefreshAllAlarmSubscriptionsAsync(CancellationToken cancellationToken) + { + if (_alarmSubscriptions.IsEmpty) return; + try + { + await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); + } + catch (ObjectDisposedException) { return; } + try + { + var session = Session; + if (session is null) return; + foreach (var sub in session.Subscriptions.ToList()) + { + if (sub.DisplayName?.StartsWith(AlarmSubscriptionNamePrefix, StringComparison.Ordinal) == true && sub.Created) + await TryConditionRefreshAsync(sub, cancellationToken).ConfigureAwait(false); + } + } + catch (Exception ex) + { + _logger.LogWarning(ex, "OpcUaClientDriver '{DriverInstanceId}': alarm ConditionRefresh sweep failed.", + _driverInstanceId); + } + finally + { + try { _gate.Release(); } catch (ObjectDisposedException) { } + } + } + + /// Display-name prefix of every alarm subscription (how a refresh sweep finds them on a session). + private const string AlarmSubscriptionNamePrefix = "opcua-alarm-sub-"; + /// /// #619 review — the sequence that moves the host's /// NativeAlarmProjector (which knows only kinds: Raise ⇒ active+unacked, Acknowledge ⇒ acked, /// Clear ⇒ inactive keeping acked) from to . A single /// upstream event can need two (an alarm that went active and was acknowledged between two notifications). - /// No state change ⇒ one (message/severity refresh). + /// No state change ⇒ empty (the caller decides whether a message/severity change warrants an Unspecified). /// /// The state last told to the host. /// The upstream event's ActiveState/Id + AckedState/Id. @@ -1929,28 +2117,25 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit internal static IReadOnlyList AlarmTransitions( (bool Active, bool Acked) prior, (bool Active, bool Acked) target) { - var kinds = new List(2); - if (target.Active && !prior.Active) + if (prior == target) return []; + return target switch { - kinds.Add(AlarmTransitionKind.Raise); - if (target.Acked) kinds.Add(AlarmTransitionKind.Acknowledge); - } - else if (!target.Active && prior.Active) - { - kinds.Add(AlarmTransitionKind.Clear); - if (target.Acked && !prior.Acked) kinds.Add(AlarmTransitionKind.Acknowledge); - } - else if (target.Acked && !prior.Acked) - { - kinds.Add(AlarmTransitionKind.Acknowledge); - } - else if (target.Active && !target.Acked && prior.Acked) - { - kinds.Add(AlarmTransitionKind.Retrigger); - } - - if (kinds.Count == 0) kinds.Add(AlarmTransitionKind.Unspecified); - return kinds; + // Active+unacked: a fresh raise, or a re-arm of an active-acked condition. + (true, false) => [prior.Active ? AlarmTransitionKind.Retrigger : AlarmTransitionKind.Raise], + // Active+acked: raise first when it was inactive (Raise always lands unacked). + (true, true) => prior.Active + ? [AlarmTransitionKind.Acknowledge] + : [AlarmTransitionKind.Raise, AlarmTransitionKind.Acknowledge], + // Inactive+acked: Clear keeps the prior ack, so ack after it when that was unacked. + (false, true) => prior.Active + ? prior.Acked ? [AlarmTransitionKind.Clear] : [AlarmTransitionKind.Clear, AlarmTransitionKind.Acknowledge] + : [AlarmTransitionKind.Acknowledge], + // Inactive+UNacked (went active and cleared unacknowledged — e.g. first seen by a ConditionRefresh): + // the only kinds that un-ack are Raise/Retrigger, so pass through active, then Clear. + (false, false) => prior.Active + ? prior.Acked ? [AlarmTransitionKind.Retrigger, AlarmTransitionKind.Clear] : [AlarmTransitionKind.Clear] + : [AlarmTransitionKind.Raise, AlarmTransitionKind.Clear], + }; } /// @@ -1997,6 +2182,12 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit { state = null; if (fields.Count <= AlarmFieldConditionId) return null; + // ConditionRefresh brackets its re-delivery with RefreshStart/RefreshEnd — markers, never alarms. + if (IsRefreshMarker(fields[AlarmFieldEventType].Value)) return null; + // A condition BRANCH event reports a previous state with its own EventId: projecting it would flip the + // trunk's state, remembering it would make Acknowledge ack the branch. + if (fields.Count > AlarmFieldBranchId && fields[AlarmFieldBranchId].Value is NodeId branch && !NodeId.IsNull(branch)) + return null; if (fields.Count > AlarmFieldAckedState && fields[AlarmFieldActiveState].Value is bool active && fields[AlarmFieldAckedState].Value is bool acked) @@ -2029,7 +2220,9 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit if (sourceIndex.Count != 0 && upstreamCondition.Length > 0) rememberCondition(reported, new AlarmAckTarget( - upstreamCondition, fields[AlarmFieldEventId].Value as byte[] ?? Array.Empty())); + upstreamCondition, + // A copy: the decoded notification's array is not ours to alias for the ack's lifetime. + (fields[AlarmFieldEventId].Value as byte[])?.ToArray() ?? Array.Empty())); return new AlarmEventArgs( SubscriptionHandle: handle, @@ -2038,7 +2231,9 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit AlarmType: eventType, Message: message, Severity: MapSeverity(severity), - SourceTimestampUtc: time); + SourceTimestampUtc: time, + // #626: the upstream severity is already the exact Part 9 value — carry it, don't re-bucket. + OpcUaSeverity: severity == 0 ? null : severity); } /// #619 — what Acknowledge needs for one RawPath: the upstream condition node (server-stable @@ -2562,6 +2757,10 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit TransitionTo(HostState.Running); _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _diag.Record(OpcUaClientDiagnostics.ReconnectSuccess); + // #619: transitions during the outage were missed (a recreated subscription starts empty; a transferred + // one may have overflowed). ConditionRefresh re-reports current states; unchanged ones dedup to nothing. + if (!_alarmSubscriptions.IsEmpty) + _ = Task.Run(() => RefreshAllAlarmSubscriptionsAsync(CancellationToken.None)); return; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs index bd5eae51..234e1c8c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs @@ -225,13 +225,196 @@ public sealed class OpcUaClientAlarmTests [InlineData(false, false, false, true, new[] { AlarmTransitionKind.Acknowledge })] [InlineData(true, false, false, true, new[] { AlarmTransitionKind.Clear, AlarmTransitionKind.Acknowledge })] [InlineData(true, true, true, false, new[] { AlarmTransitionKind.Retrigger })] - [InlineData(true, false, true, false, new[] { AlarmTransitionKind.Unspecified })] + [InlineData(true, false, true, false, new AlarmTransitionKind[0])] public void AlarmTransitions_move_prior_to_target( bool priorActive, bool priorAcked, bool active, bool acked, AlarmTransitionKind[] expected) { OpcUaClientDriver.AlarmTransitions((priorActive, priorAcked), (active, acked)).ShouldBe(expected); } + // ---- #619 re-review: the transition table must reach EVERY target from EVERY prior. ---- + + /// The host projector's semantics, restated: the only thing the driver's kinds act upon. + private static (bool Active, bool Acked) Simulate((bool Active, bool Acked) s, AlarmTransitionKind k) => k switch + { + AlarmTransitionKind.Raise or AlarmTransitionKind.Retrigger => (true, false), + AlarmTransitionKind.Acknowledge => (s.Active, true), + AlarmTransitionKind.Clear => (false, s.Acked), + _ => s, + }; + + /// + /// Exhaustive over all 16 (prior, target) pairs: replaying the emitted kinds through the projector's + /// semantics lands on the target, and an unchanged state emits nothing. The table this replaced reached + /// (inactive, acked) from (active, acked) → (inactive, UNACKED) wrongly, and could not express a + /// cleared-but-unacked condition first seen by a ConditionRefresh at all. + /// + [Fact] + public void AlarmTransitions_reach_every_target_from_every_prior() + { + bool[] b = [false, true]; + foreach (var pa in b) foreach (var pk in b) foreach (var ta in b) foreach (var tk in b) + { + var kinds = OpcUaClientDriver.AlarmTransitions((pa, pk), (ta, tk)); + var s = (pa, pk); + foreach (var k in kinds) s = Simulate(s, k); + s.ShouldBe((ta, tk), $"prior ({pa},{pk}) -> target ({ta},{tk}) via [{string.Join(",", kinds)}]"); + if ((pa, pk) == (ta, tk)) kinds.ShouldBeEmpty(); + kinds.ShouldNotContain(AlarmTransitionKind.Unspecified); + } + } + + private static VariantCollection ConditionFields( + bool active, bool acked, string message = "tripped", ushort severity = 700, byte[]? eventId = null, + NodeId? branchId = null, NodeId? eventType = null) + { + var f = EventFields("ns=3;s=Alarms.Trip1", "ns=3;s=Alarms.Trip1.Cond"); + f[0] = new Variant(eventId ?? new byte[] { 1 }); + if (eventType is not null) f[1] = new Variant(eventType); + f[3] = new Variant(new LocalizedText(message)); + f[4] = new Variant(severity); + f.Add(new Variant(active)); + f.Add(new Variant(acked)); + f.Add(branchId is null ? Variant.Null : new Variant(branchId)); + return f; + } + + private static readonly Dictionary TripIndex = new() { ["ns=3;s=Alarms.Trip1"] = "Plant/opc/plc/Trip1" }; + + private static List Record(OpcUaClientDriver drv) + { + var seen = new List(); + drv.OnAlarmEvent += (_, e) => { lock (seen) seen.Add(e); }; + return seen; + } + + /// The remembered EventId is a COPY — the decoded notification's array is not ours to alias. + [Fact] + public void ProjectEvent_copies_the_EventId() + { + var raw = new byte[] { 7, 8, 9 }; + OpcUaClientDriver.AlarmAckTarget? target = null; + OpcUaClientDriver.ProjectEvent(ConditionFields(true, false, eventId: raw), TripIndex, new FakeAlarmHandle(), + n => n.ToString(), (_, t) => target = t); + + raw[0] = 0; + target!.Value.EventId.ShouldBe(new byte[] { 7, 8, 9 }); + } + + /// + /// A condition BRANCH event (BranchId non-null) reports a previous state of the condition and carries its + /// own EventId — projecting it would flip the trunk's state, and remembering it would ack the branch. + /// + [Fact] + public void ProjectEvent_ignores_a_condition_branch_event() + { + var remembered = false; + OpcUaClientDriver.ProjectEvent( + ConditionFields(false, false, branchId: new NodeId(Guid.NewGuid(), 3)), TripIndex, new FakeAlarmHandle(), + n => n.ToString(), (_, _) => remembered = true) + .ShouldBeNull(); + remembered.ShouldBeFalse(); + + // Positive control: the same event with a null BranchId (the trunk) projects. + OpcUaClientDriver.ProjectEvent(ConditionFields(false, false), TripIndex, new FakeAlarmHandle(), + n => n.ToString(), (_, _) => { }) + .ShouldNotBeNull(); + } + + /// RefreshStart/RefreshEnd are markers around a ConditionRefresh, never alarms — even pass-through. + [Fact] + public void Refresh_markers_are_never_projected_as_alarms() + { + foreach (var type in new[] { ObjectTypeIds.RefreshStartEventType, ObjectTypeIds.RefreshEndEventType, ObjectTypeIds.RefreshRequiredEventType }) + { + var fields = EventFields("i=2253", "i=0"); + fields[1] = new Variant(type); + OpcUaClientDriver.ProjectEvent(fields, new Dictionary(), new FakeAlarmHandle(), + n => n.ToString(), (_, _) => { }).ShouldBeNull($"{type} is a refresh marker"); + } + OpcUaClientDriver.IsRefreshRequired(EventFieldsOfType(ObjectTypeIds.RefreshRequiredEventType)).ShouldBeTrue(); + OpcUaClientDriver.IsRefreshRequired(EventFieldsOfType(ObjectTypeIds.RefreshEndEventType)).ShouldBeFalse(); + } + + private static VariantCollection EventFieldsOfType(NodeId type) + { + var f = EventFields("i=2253", "i=0"); + f[1] = new Variant(type); + return f; + } + + /// + /// A ConditionRefresh re-delivers every retained condition's CURRENT state. One the host was already told + /// must emit nothing — no re-raise (which would un-ack an acked alarm) and no Unspecified (which a host + /// projector reset by a redeploy would project from its inactive default). + /// + [Fact] + public void A_refresh_of_an_already_reported_state_emits_nothing() + { + using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-alarm-dedup"); + var seen = Record(drv); + var handle = new FakeAlarmHandle(); + + drv.RouteAlarmEvent(ConditionFields(true, false), TripIndex, handle); + drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); + seen.Select(e => e.Kind).ShouldBe([AlarmTransitionKind.Raise, AlarmTransitionKind.Acknowledge]); + + drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); // the refresh copy + seen.Count.ShouldBe(2); + + // Positive control: a genuine message change on an unchanged state still reaches the host. + drv.RouteAlarmEvent(ConditionFields(true, true, message: "worse"), TripIndex, handle); + seen.Count.ShouldBe(3); + seen[2].Kind.ShouldBe(AlarmTransitionKind.Unspecified); + seen[2].Message.ShouldBe("worse"); + } + + /// The exact upstream severity rides OpcUaSeverity (#626) so the host does not re-bucket it. + [Fact] + public void The_exact_upstream_severity_is_carried() + { + using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-alarm-sev"); + var seen = Record(drv); + drv.RouteAlarmEvent(ConditionFields(true, false, severity: 650), TripIndex, new FakeAlarmHandle()); + seen.Single().OpcUaSeverity.ShouldBe(650); + } + + /// + /// A driver re-initialise (comms-loss watchdog, Force Reconnect) re-subscribes the SAME set over nodes the + /// host did not rebuild, so the last-told state must survive it — or the refresh after it would re-raise + /// every acked active alarm (un-ack). A CHANGED set means the host rebuilt (nodes recreated inactive), so + /// there the state is forgotten and the refresh re-raises from scratch. + /// + [Fact] + public async Task Last_told_state_survives_a_reinit_of_the_same_set_but_not_a_set_change() + { + using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-alarm-reinit"); + var seen = Record(drv); + var handle = new FakeAlarmHandle(); + drv.BeginAlarmSubscription(["Plant/opc/plc/Trip1"]); + drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); + seen.Count.ShouldBe(2); + + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + drv.BeginAlarmSubscription(["Plant/opc/plc/Trip1"]); + drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); + seen.Count.ShouldBe(2, "same set after a reinit: the refresh copy is a duplicate"); + + drv.BeginAlarmSubscription(["Plant/opc/plc/Trip1", "Plant/opc/plc/Trip2"]); + drv.RouteAlarmEvent(ConditionFields(true, true), TripIndex, handle); + seen.Skip(2).Select(e => e.Kind).ShouldBe([AlarmTransitionKind.Raise, AlarmTransitionKind.Acknowledge]); + } + + /// A server without ConditionRefresh is tolerated; anything else is a real failure. + [Fact] + public void ConditionRefresh_unsupported_status_codes() + { + OpcUaClientDriver.IsConditionRefreshUnsupported(StatusCodes.BadNotSupported).ShouldBeTrue(); + OpcUaClientDriver.IsConditionRefreshUnsupported(StatusCodes.BadMethodInvalid).ShouldBeTrue(); + OpcUaClientDriver.IsConditionRefreshUnsupported(StatusCodes.BadNotImplemented).ShouldBeTrue(); + OpcUaClientDriver.IsConditionRefreshUnsupported(StatusCodes.BadTimeout).ShouldBeFalse(); + } + private sealed class FakeAlarmHandle : IAlarmSubscriptionHandle { /// Gets the diagnostic identifier for this alarm handle. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs index 63cc8b0b..0337fea9 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverWireTests.cs @@ -253,6 +253,68 @@ public sealed class OpcUaClientDriverWireTests "the writable variable and the read-only one must classify differently"); } + /// + /// #619 — ConditionRefresh over the wire: a condition already active+unacked BEFORE the driver subscribes is + /// reported (Raise, keyed by the RawPath) right after subscribe, not only on its next transition. A second + /// subscribe of the same set refreshes again and must emit nothing (no re-raise); a positive control then + /// proves the feed is still live by acknowledging upstream and seeing exactly the Acknowledge arrive. + /// + [Fact] + public async Task A_condition_already_active_before_subscribe_is_reported_by_ConditionRefresh() + { + const string AlarmRef = "Sim/dev1/TempHi"; + var ct = TestContext.Current.CancellationToken; + await using var rig = await StartRigAsync("drv-alarm-refresh", ct); + rig.MaterialiseNative(); + rig.Project(native: true, new AlarmConditionSnapshot(Active: true, Acknowledged: false, Confirmed: true, + Enabled: true, AlarmShelvingKind.Unshelved, 800, "already hot")); + + await using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions + { + EndpointUrl = rig.EndpointUrl, + RawTags = [Tag(AlarmRef, ServerEventRig.RawNsu(ServerEventRig.RawAlarmPath))], + SecurityPolicy = OpcUaSecurityPolicy.None, + SecurityMode = OpcUaSecurityMode.None, + AuthType = OpcUaAuthType.Username, + Username = "apikey", + Password = ServerEventRig.KeyPassword, + AutoAcceptCertificates = true, + TargetNamespaceKind = OpcUaTargetNamespaceKind.SystemPlatform, + Timeout = TimeSpan.FromSeconds(10), + SessionTimeout = TimeSpan.FromSeconds(30), + KeepAliveInterval = TimeSpan.FromSeconds(1), + }, "wire-alarm-refresh"); + await drv.InitializeAsync("{}", ct); + + var seen = new List(); + drv.OnAlarmEvent += (_, e) => { lock (seen) seen.Add(e); }; + AlarmEventArgs[] Seen() { lock (seen) return [.. seen]; } + + var first = await drv.SubscribeAlarmsAsync([AlarmRef], ct); + await RecordedEvents.WaitUntilAsync(() => Seen().Any(e => e.Kind == AlarmTransitionKind.Raise)); + + var raise = Seen().Single(e => e.Kind == AlarmTransitionKind.Raise); + raise.ConditionId.ShouldBe(AlarmRef); + raise.Message.ShouldBe("already hot"); + raise.OpcUaSeverity.ShouldBe(800); + Seen().ShouldAllBe(e => e.ConditionId == AlarmRef, "refresh markers must never surface as alarms"); + + // Same set again, released first exactly as DriverInstanceActor does → a fresh subscription and another + // ConditionRefresh; the identical state must not re-raise. + var before = Seen().Length; + await drv.UnsubscribeAlarmsAsync(first, ct); + await drv.SubscribeAlarmsAsync([AlarmRef], ct); + await Task.Delay(1500, ct); // let the second refresh's events land before the control transition + + // Positive control: an upstream acknowledge arrives as exactly one Acknowledge, after which nothing else. + rig.Project(native: true, new AlarmConditionSnapshot(Active: true, Acknowledged: true, Confirmed: true, + Enabled: true, AlarmShelvingKind.Unshelved, 800, "already hot")); + await RecordedEvents.WaitUntilAsync(() => Seen().Any(e => e.Kind == AlarmTransitionKind.Acknowledge)); + await Task.Delay(500, ct); + Seen().Skip(before).Select(e => e.Kind).ShouldBe([AlarmTransitionKind.Acknowledge], + "the second refresh re-delivered an unchanged state and must have emitted nothing"); + } + private sealed class RecordingBuilder : IAddressSpaceBuilder { public List<(string BrowseName, DriverAttributeInfo Info)> Variables { get; } = [];