From 49a470a08001ea1990b7d8f07ecde5bbff5c3da7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 16:19:27 -0400 Subject: [PATCH] =?UTF-8?q?fix(client):=20alarm=20filter=20selected=20Acti?= =?UTF-8?q?veState/Id=20as=20ONE=20QualifiedName=20=E2=80=94=20every=20eve?= =?UTF-8?q?nt=20read=20inactive?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EventFilter.AddSelectClause(type, "ActiveState/Id") does not split the string; it selects a single QualifiedName named "ActiveState/Id", which no event carries. AckedState/Id, ActiveState/Id and EnabledState/Id therefore always arrived null and Client.CLI 'alarms' (and Client.UI) reported every alarm inactive and unacknowledged while a direct Read said Active. Pre-existing since a2883b82; server-side event delivery was correct (proven with two-element select clauses, incl. after #629). Select them with explicit two-element browse paths. Over-the-wire test drives OpcUaClientService.SubscribeAlarmsAsync against the in-process server (native + scripted); ServerEventRig now also selects ActiveState/Id, Severity and Retain. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/Client.CLI.md | 6 ++ .../OpcUaClientService.cs | 14 ++-- .../AlarmEventFilterTests.cs | 38 +++++++++++ .../ClientAlarmMethodTests.cs | 65 +++++++++++++++++++ .../ServerEventRig.cs | 7 ++ 5 files changed, 126 insertions(+), 4 deletions(-) create mode 100644 tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/AlarmEventFilterTests.cs diff --git a/docs/Client.CLI.md b/docs/Client.CLI.md index e72d6403..f87604f9 100644 --- a/docs/Client.CLI.md +++ b/docs/Client.CLI.md @@ -248,6 +248,12 @@ otopcua-cli alarms -u opc.tcp://localhost:4840/OtOpcUa \ | `-i` / `--interval` | Publishing interval in milliseconds (default: 1000) | | `--refresh` | Request a `ConditionRefresh` after subscribing to get current retained alarm states | +The active/acknowledged columns come from the standard `ActiveState/Id`, `AckedState/Id` and +`EnabledState/Id` event fields. Before the 2026-10 fix the client's filter selected them with a single +QualifiedName named `ActiveState/Id` (a `/`-joined string passed to `EventFilter.AddSelectClause` is **not** +split into a browse path), which matched nothing — so every event printed inactive and unacknowledged +whatever the condition's real state. A direct Read of the condition was always correct. + ### ack Acknowledges an active alarm condition via the OPC UA Part 9 `Acknowledge` method. The server gates this on the **`AlarmAck`** role (fail-closed — a session whose API-key principal holds no `AlarmAck`-conferring `NodeAcl` grant gets `BadUserAccessDenied`; the data plane does not use LDAP; see [ScriptedAlarms.md](ScriptedAlarms.md) §"AlarmAck gate"). The `--event-id` is the hex-encoded `EventId` taken from a prior `alarms` subscription notification for the same condition. 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 b64741ff..88cb407c 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs @@ -867,7 +867,7 @@ public sealed class OpcUaClientService : IOpcUaClientService catch { return false; } } - private static EventFilter CreateAlarmEventFilter() + internal static EventFilter CreateAlarmEventFilter() { var filter = new EventFilter(); // 0: EventId @@ -886,12 +886,18 @@ public sealed class OpcUaClientService : IOpcUaClientService filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.ConditionName); // 7: Retain filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.Retain); + // 8-10: the two-state /Id booleans. Each browse path is TWO QualifiedNames. Passing "AckedState/Id" as a + // string to AddSelectClause does not split it — it selects one element NAMED "AckedState/Id", which no + // event carries, so all three arrived null and every alarm read inactive + unacknowledged. // 8: AckedState/Id - filter.AddSelectClause(ObjectTypeIds.AcknowledgeableConditionType, "AckedState/Id"); + filter.SelectClauses.Add(new SimpleAttributeOperand( + ObjectTypeIds.AcknowledgeableConditionType, [BrowseNames.AckedState, BrowseNames.Id])); // 9: ActiveState/Id - filter.AddSelectClause(ObjectTypeIds.AlarmConditionType, "ActiveState/Id"); + filter.SelectClauses.Add(new SimpleAttributeOperand( + ObjectTypeIds.AlarmConditionType, [BrowseNames.ActiveState, BrowseNames.Id])); // 10: EnabledState/Id - filter.AddSelectClause(ObjectTypeIds.AlarmConditionType, "EnabledState/Id"); + filter.SelectClauses.Add(new SimpleAttributeOperand( + ObjectTypeIds.ConditionType, [BrowseNames.EnabledState, BrowseNames.Id])); // 11: SuppressedOrShelved filter.AddSelectClause(ObjectTypeIds.AlarmConditionType, "SuppressedOrShelved"); // 12: SourceNode (ConditionId for acknowledgment) diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/AlarmEventFilterTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/AlarmEventFilterTests.cs new file mode 100644 index 00000000..f0349bc5 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/AlarmEventFilterTests.cs @@ -0,0 +1,38 @@ +using Opc.Ua; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests; + +/// +/// Pins the browse paths of the alarm subscription's select clauses. EventFilter.AddSelectClause(type, +/// "ActiveState/Id") does NOT split the string — it yields ONE QualifiedName named ActiveState/Id, +/// which matches nothing — so the client reported every alarm inactive and unacknowledged. The over-the-wire +/// proof is ClientAlarmMethodTests.The_client_alarm_subscription_reports_active_acked_and_exact_severity. +/// +public sealed class AlarmEventFilterTests +{ + /// No select clause carries a QualifiedName with a path separator in it. + [Fact] + public void No_browse_path_element_contains_a_separator() + { + var filter = OpcUaClientService.CreateAlarmEventFilter(); + + filter.SelectClauses.SelectMany(c => c.BrowsePath).ShouldAllBe(q => !q.Name.Contains('/')); + } + + /// The two-state ids are selected by their two-element browse paths, at the indices the parser reads. + [Fact] + public void Two_state_ids_are_two_element_browse_paths() + { + var clauses = OpcUaClientService.CreateAlarmEventFilter().SelectClauses; + + string Path(int i) => string.Join("/", clauses[i].BrowsePath.Select(q => q.Name)); + Path(8).ShouldBe("AckedState/Id"); + clauses[8].BrowsePath.Count.ShouldBe(2); + Path(9).ShouldBe("ActiveState/Id"); + clauses[9].BrowsePath.Count.ShouldBe(2); + Path(10).ShouldBe("EnabledState/Id"); + clauses[10].BrowsePath.Count.ShouldBe(2); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs index 615ed60a..24dfeff8 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ClientAlarmMethodTests.cs @@ -3,6 +3,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Client.Shared; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; using ZB.MOM.WW.OtOpcUa.Configuration.Enums; using static ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests.ServerEventRig; @@ -135,6 +136,70 @@ public sealed class ClientAlarmMethodTests finally { await rig.DisposeAsync(); } } + /// + /// The client's alarm SUBSCRIPTION (Client.CLI alarms) reads the condition's two-state fields off a + /// real event. Its filter used to pass "ActiveState/Id" to EventFilter.AddSelectClause, which + /// does NOT split the string: the operand's browse path was ONE QualifiedName named ActiveState/Id, + /// which matches nothing, so every event arrived with Active/Acked/Enabled null and the client reported + /// them all inactive and unacknowledged — while a direct Read of the same condition said Active. The + /// fake-adapter unit tests feed field values straight in and so never exercise the filter. + /// + /// Drive the native (raw device folder) or the scripted (equipment folder) condition. + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task The_client_alarm_subscription_reports_active_acked_and_exact_severity(bool native) + { + var ct = TestContext.Current.CancellationToken; + var rig = await StartAsync($"ClientAlarmSubscribe{native}", encrypted: false, ct); + try + { + rig.Enforce(false, [Key()], ClusterGrant(KeyPrincipal, "acl-key", All)); + if (native) rig.MaterialiseNative(); else rig.MaterialiseScripted(); + + using var client = await ConnectClientAsync(rig, ct); + using var observer = await rig.ConnectAsync(anonymous: false, ct); + var source = native + ? new NodeId(RawDeviceFolder, (ushort)observer.NamespaceUris.GetIndex(V3NodeIds.RawNamespaceUri)) + : UnsNodeId(observer, EqA); + + var seen = new List(); + client.AlarmEvent += (_, e) => { lock (seen) seen.Add(e); }; + await client.SubscribeAlarmsAsync(source, 100, ct); + + AlarmEventArgs? Latest(string message) + { + lock (seen) return seen.LastOrDefault(e => e.Message == message); + } + + // 800 is not one of the SDK's five EventSeverity values (900 is): the node must carry it verbatim. + rig.Project(native, new AlarmConditionSnapshot(Active: true, Acknowledged: false, Confirmed: true, + Enabled: true, AlarmShelvingKind.Unshelved, 800, "raised")); + await RecordedEvents.WaitUntilAsync(() => Latest("raised") is not null); + var raised = Latest("raised")!; + raised.ActiveState.ShouldBeTrue("the condition is active"); + raised.AckedState.ShouldBeFalse(); + raised.Severity.ShouldBe((ushort)800); + raised.Retain.ShouldBeTrue(); + + rig.Project(native, new AlarmConditionSnapshot(Active: true, Acknowledged: true, Confirmed: true, + Enabled: true, AlarmShelvingKind.Unshelved, 800, "acked")); + await RecordedEvents.WaitUntilAsync(() => Latest("acked") is not null); + Latest("acked")!.AckedState.ShouldBeTrue("the positive control for the Acked field"); + Latest("acked")!.ActiveState.ShouldBeTrue(); + + rig.Project(native, new AlarmConditionSnapshot(Active: false, Acknowledged: true, Confirmed: true, + Enabled: true, AlarmShelvingKind.Unshelved, 800, "cleared")); + await RecordedEvents.WaitUntilAsync(() => Latest("cleared") is not null); + var cleared = Latest("cleared")!; + cleared.ActiveState.ShouldBeFalse(); + cleared.Retain.ShouldBeFalse("an inactive, acknowledged condition is not retained"); + + await client.UnsubscribeAlarmsAsync(ct); + } + finally { await rig.DisposeAsync(); } + } + private static async Task ConnectClientAsync(ServerEventRig rig, CancellationToken ct) { var client = new OpcUaClientService(); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs index 1a620b21..c6e4da42 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs @@ -315,6 +315,9 @@ internal sealed class RecordedEvents public const int ChangesField = 8; public const int SourceNodeField = 9; public const int UnshelveTimeField = 10; + public const int ActiveField = 11; + public const int SeverityField = 12; + public const int RetainField = 13; private readonly object _gate = new(); private readonly List<(uint Handle, VariantCollection Fields)> _events = []; @@ -374,6 +377,10 @@ internal sealed class RecordedEvents filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.SourceNode); filter.SelectClauses.Add(new SimpleAttributeOperand( ObjectTypeIds.AlarmConditionType, [BrowseNames.ShelvingState, BrowseNames.UnshelveTime])); + filter.SelectClauses.Add(new SimpleAttributeOperand( + ObjectTypeIds.AlarmConditionType, [BrowseNames.ActiveState, BrowseNames.Id])); + filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.Severity); + filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.Retain); var item = new MonitoredItem(subscription.DefaultItem) {