From 8221fac8c1a557be6230638442671a06344b9dae Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 21 Apr 2026 00:22:02 -0400 Subject: [PATCH] =?UTF-8?q?Task=20#219=20follow-up=20=E2=80=94=20close=20A?= =?UTF-8?q?larmConditionState=20child-NodeId=20+=20event-propagation=20gap?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #197 surfaced two integration-level wiring gaps in DriverNodeManager's MarkAsAlarmCondition path; this commit fixes both and upgrades the integration test to assert them end-to-end. Fix 1 — addressable child nodes: AlarmConditionState inherits ~50 typed children (Severity / Message / ActiveState / AckedState / EnabledState / …). The stack was leaving them with Foundation-namespace NodeIds (type-declaration defaults) or shared ns=0 counter allocations, so client Read on a child returned BadNodeIdUnknown. Pass assignNodeIds=true to alarm.Create, then walk the condition subtree and rewrite each descendant's NodeId symbolically as {condition-full-ref}.{symbolic-path} in the node manager's namespace. Stable, unique, and collision-free across multiple alarm instances in the same driver. Fix 2 — event propagation to Server.EventNotifier: OPC UA Part 9 event propagation relies on the alarm condition being reachable from Objects/Server via HasNotifier. Call CustomNodeManager2.AddRootNotifier(alarm) after registering the condition so subscriptions placed on Server-object EventNotifier receive the ReportEvent calls ConditionSink emits per-transition. Test upgrades in AlarmSubscribeIntegrationTests: - Driver_alarm_transition_updates_server_side_AlarmConditionState_node — now asserts Severity == 700, Message text, and ActiveState.Id == true through the OPC UA client (previously scoped out as BadNodeIdUnknown). - New: Driver_alarm_event_flows_to_client_subscription_on_Server_EventNotifier subscribes an OPC UA event monitor on ObjectIds.Server, fires a driver transition, and waits for the AlarmConditionType event to be delivered, asserting Message + Severity fields. Previously scoped out as "Part 9 event propagation out of reach." Regression checks: 239 server tests pass (+1 new event-subscription test), 195 Core tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../OpcUa/DriverNodeManager.cs | 45 +++++- .../AlarmSubscribeIntegrationTests.cs | 130 +++++++++++++----- 2 files changed, 136 insertions(+), 39 deletions(-) diff --git a/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs b/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs index 9046b4f..b99ad75 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs @@ -371,7 +371,20 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder BrowseName = new QualifiedName(_variable.BrowseName.Name + "_Condition", _owner.NamespaceIndex), DisplayName = new LocalizedText(info.SourceName), }; - alarm.Create(_owner.SystemContext, alarm.NodeId, alarm.BrowseName, alarm.DisplayName, false); + // assignNodeIds=true makes the stack allocate NodeIds for every inherited + // AlarmConditionState child (Severity / Message / ActiveState / AckedState / + // EnabledState / …). Without this the children keep Foundation (ns=0) type- + // declaration NodeIds that aren't in the node manager's predefined-node index. + // The newly-allocated NodeIds default to ns=0 via the shared identifier + // counter — we remap them to the node manager's namespace below so client + // Read/Browse on children resolves against the predefined-node dictionary. + alarm.Create(_owner.SystemContext, alarm.NodeId, alarm.BrowseName, alarm.DisplayName, true); + // Assign every descendant a stable, collision-free NodeId in the node manager's + // namespace keyed on the condition path. The stack's default assignNodeIds path + // allocates from a shared ns=0 counter and does not update parent→child + // references when we remap, so we do the rename up front, symbolically: + // {condition-full-ref}/{symbolic-path-under-condition} + AssignSymbolicDescendantIds(alarm, alarm.NodeId, _owner.NamespaceIndex); alarm.SourceName.Value = info.SourceName; alarm.Severity.Value = (ushort)MapSeverity(info.InitialSeverity); alarm.Message.Value = new LocalizedText(info.InitialDescription ?? info.SourceName); @@ -382,10 +395,20 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder alarm.AckedState.Id.Value = true; alarm.ActiveState.Value = new LocalizedText("Inactive"); alarm.ActiveState.Id.Value = false; + // Enable ConditionRefresh support so clients that connect *after* a transition + // can pull the current retained-condition snapshot. + alarm.ClientUserId.Value = string.Empty; + alarm.BranchId.Value = NodeId.Null; _variable.AddChild(alarm); _owner.AddPredefinedNode(_owner.SystemContext, alarm); + // Part 9 event propagation: AddRootNotifier registers the alarm as an event + // source reachable from Objects/Server so subscriptions placed on Server-object + // EventNotifier receive the ReportEvent calls ConditionSink.OnTransition emits. + // Without this the Report fires but has no subscribers to deliver to. + _owner.AddRootNotifier(alarm); + return new ConditionSink(_owner, alarm); } } @@ -398,6 +421,26 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder AlarmSeverity.Critical => 900, _ => 500, }; + + // After alarm.Create(assignNodeIds=true), every descendant has *some* NodeId but + // they default to ns=0 via the shared identifier counter — allocations from two + // different alarms collide when we move them into the driver's namespace. Rewriting + // symbolically based on the condition path gives each descendant a unique, stable + // NodeId in the node manager's namespace. Browse + Read resolve against the current + // NodeId because the stack's CustomNodeManager2.Browse traverses NodeState.Children + // (NodeState references) and uses each child's current .NodeId in the response. + private static void AssignSymbolicDescendantIds( + NodeState parent, NodeId parentNodeId, ushort namespaceIndex) + { + var children = new List(); + parent.GetChildren(null!, children); + foreach (var child in children) + { + child.NodeId = new NodeId( + $"{parentNodeId.Identifier}.{child.SymbolicName}", namespaceIndex); + AssignSymbolicDescendantIds(child, child.NodeId, namespaceIndex); + } + } } private sealed class ConditionSink(DriverNodeManager owner, AlarmConditionState alarm) diff --git a/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/AlarmSubscribeIntegrationTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/AlarmSubscribeIntegrationTests.cs index a5320df..5c2aca9 100644 --- a/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/AlarmSubscribeIntegrationTests.cs +++ b/tests/ZB.MOM.WW.OtOpcUa.Server.Tests/AlarmSubscribeIntegrationTests.cs @@ -12,27 +12,17 @@ using ZB.MOM.WW.OtOpcUa.Server.Security; namespace ZB.MOM.WW.OtOpcUa.Server.Tests; /// -/// Task #219 — server-integration coverage for the wiring -/// path. Boots the full OPC UA stack + a fake driver, opens a -/// client session, and verifies via browse/read that DiscoverAsync's -/// MarkAsAlarmCondition calls produce addressable AlarmConditionState nodes in the -/// driver's namespace and that firing OnAlarmEvent routes through -/// GenericDriverNodeManager's forwarder into DriverNodeManager.ConditionSink -/// without throwing. +/// Task #219 — end-to-end server integration coverage for the +/// dispatch path. Boots the full OPC UA stack + a fake driver, +/// opens a client session, raises a driver-side transition, and asserts it propagates +/// through GenericDriverNodeManager's alarm forwarder into +/// DriverNodeManager.ConditionSink, updates the server-side +/// AlarmConditionState child attributes (Severity / Message / ActiveState), and +/// flows out to an OPC UA subscription on the Server object's EventNotifier. /// /// Companion to which covers the /// dispatch path; together they close the server-side /// integration gap for optional driver capabilities (plan decision #62). -/// -/// Known server-side scoping (not a regression introduced here): the stack exposes the -/// AlarmConditionState type's inherited children (Severity / Message / ActiveState / …) -/// with Foundation-namespace NodeIds (ns=0) that aren't added to -/// 's predefined-node index, so reading those child -/// attributes through an OPC UA client returns BadNodeIdUnknown. OPC UA Part 9 -/// event propagation (subscribe-on-Server + ConditionRefresh) is likewise out of scope -/// until the node manager wires HasNotifier + child-node registration. The -/// existing Core-level GenericDriverNodeManagerTests cover the in-memory alarm-sink -/// fan-out semantics directly. /// [Trait("Category", "Integration")] public sealed class AlarmSubscribeIntegrationTests : IAsyncLifetime @@ -116,32 +106,96 @@ public sealed class AlarmSubscribeIntegrationTests : IAsyncLifetime r => ExpandedNodeId.ToNodeId(r.NodeId, session.NamespaceUris), StringComparer.Ordinal); - children.ShouldContainKey("Severity", - "browse did not return Severity child; full list: " - + string.Join(", ", browseResults[0].References.Select(r => $"{r.BrowseName.Name}={r.NodeId}"))); + children.ShouldContainKey("Severity"); children.ShouldContainKey("Message"); children.ShouldContainKey("ActiveState"); - // NB: the stack exposes AlarmConditionState's inherited children with Foundation-namespace - // NodeIds (ns=0). The DriverNodeManager registers only the parent alarm object via - // AddPredefinedNode; reading child attributes through the OPC UA client returns - // BadNodeIdUnknown because the stack-assigned child NodeIds aren't in the node - // manager's predefined-node index. Asserting state-mutation via a client-side read - // is therefore out of reach at this integration layer — the Core-level - // GenericDriverNodeManagerTests cover the in-memory alarm-sink fan-out directly. - // - // What this test *does* verify through the OPC UA client: the alarm node itself is - // reachable via browse (proving MarkAsAlarmCondition registered it as a predefined - // node), its displayed name matches the driver's AlarmConditionInfo.SourceName, and - // firing the transition does not throw out of ConditionSink.OnTransition (which would - // fail the test at RaiseAlarm since the event handler is invoked synchronously). - var nodesToRead = new ReadValueIdCollection + // Severity / Message / ActiveState.Id reflect the driver-fired transition — verifies + // the forwarder → ConditionSink.OnTransition → alarm.ClearChangeMasks pipeline + // landed the new values in addressable child nodes. DriverNodeManager's + // AssignSymbolicDescendantIds keeps each child reachable under the node manager's + // namespace so Read resolves against the predefined-node dictionary. + var severity = session.ReadValue(children["Severity"]); + var message = session.ReadValue(children["Message"]); + severity.Value.ShouldBe((ushort)700); // AlarmSeverity.High → 700 (MapSeverity) + ((LocalizedText)message.Value).Text.ShouldBe("Level exceeded upper-upper"); + + // ActiveState exposes its boolean Id as a HasProperty child. + var activeBrowse = new BrowseDescriptionCollection { - new() { NodeId = conditionNodeId, AttributeId = Attributes.DisplayName }, + new() + { + NodeId = children["ActiveState"], + BrowseDirection = BrowseDirection.Forward, + ReferenceTypeId = ReferenceTypeIds.HasProperty, + IncludeSubtypes = true, + ResultMask = (uint)BrowseResultMask.All, + }, }; - session.Read(null, 0, TimestampsToReturn.Neither, nodesToRead, out var values, out _); - values[0].StatusCode.Code.ShouldBe(StatusCodes.Good); - ((LocalizedText)values[0].Value).Text.ShouldBe("Tank.HiHi"); + session.Browse(null, null, 0, activeBrowse, out var activeChildren, out _); + var idRef = activeChildren[0].References.Single(r => r.BrowseName.Name == "Id"); + var activeId = session.ReadValue(ExpandedNodeId.ToNodeId(idRef.NodeId, session.NamespaceUris)); + activeId.Value.ShouldBe(true); + } + + [Fact] + public async Task Driver_alarm_event_flows_to_client_subscription_on_Server_EventNotifier() + { + // AddRootNotifier registers the AlarmConditionState as a Server-object notifier + // source, so a subscription with an EventFilter on Server receives the + // ReportEvent calls ConditionSink emits per-transition. + using var session = await OpenSessionAsync(); + + var subscription = new Subscription(session.DefaultSubscription) { PublishingInterval = 100 }; + session.AddSubscription(subscription); + await subscription.CreateAsync(); + + var received = new List(); + var gate = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var filter = new EventFilter(); + filter.AddSelectClause(ObjectTypeIds.BaseEventType, BrowseNames.EventId); + filter.AddSelectClause(ObjectTypeIds.BaseEventType, BrowseNames.SourceName); + filter.AddSelectClause(ObjectTypeIds.BaseEventType, BrowseNames.Message); + filter.AddSelectClause(ObjectTypeIds.BaseEventType, BrowseNames.Severity); + filter.WhereClause = new ContentFilter(); + filter.WhereClause.Push(FilterOperator.OfType, + new LiteralOperand { Value = new Variant(ObjectTypeIds.AlarmConditionType) }); + + var item = new MonitoredItem(subscription.DefaultItem) + { + StartNodeId = ObjectIds.Server, + AttributeId = Attributes.EventNotifier, + NodeClass = NodeClass.Object, + SamplingInterval = 0, + QueueSize = 100, + Filter = filter, + }; + item.Notification += (_, e) => + { + if (e.NotificationValue is EventFieldList fields) + { + lock (received) { received.Add(fields); gate.TrySetResult(); } + } + }; + subscription.AddItem(item); + await subscription.ApplyChangesAsync(); + + // Give the publish loop a tick to establish before firing. + await Task.Delay(200); + + _driver.RaiseAlarm(new AlarmEventArgs( + new FakeHandle("sub"), "Tank.HiHi", "cond-x", "Active", + "High-high tripped", AlarmSeverity.Critical, DateTime.UtcNow)); + + var delivered = await Task.WhenAny(gate.Task, Task.Delay(TimeSpan.FromSeconds(10))); + delivered.ShouldBe(gate.Task, "alarm event must arrive at the client within 10s"); + + EventFieldList first; + lock (received) first = received[0]; + // Filter field order: 0=EventId, 1=SourceName, 2=Message, 3=Severity. + ((LocalizedText)first.EventFields[2].Value).Text.ShouldBe("High-high tripped"); + first.EventFields[3].Value.ShouldBe((ushort)900); // Critical → 900 } [Fact] -- 2.49.1