diff --git a/docs/AlarmTracking.md b/docs/AlarmTracking.md index a220a6b1..67c37038 100644 --- a/docs/AlarmTracking.md +++ b/docs/AlarmTracking.md @@ -57,7 +57,8 @@ Every condition event — native and scripted — carries the mandatory `BaseEve fields, assigned at materialize time in `OtOpcUaNodeManager.MaterialiseAlarmCondition`. The SDK does **not** synthesize them on this path (`Create` builds the children from the type definition but leaves them unset; `ReportEvent` / `InstanceStateSnapshot` copy children verbatim), so they -are set explicitly. Leaving them unset shipped them as **null** on every event — see issue #473. +are set explicitly. Leaving them unset shipped them as **null** on every event — see issues #473 +(the `BaseEventType` trio) and #475 (the `ConditionType` classification pair). | Field | Value | Notes | |---|---|---| @@ -65,6 +66,20 @@ are set explicitly. Leaving them unset shipped them as **null** on every event | `SourceNode` | the condition's **own NodeId** — equal to `ConditionId` | The condition **is** the source: an alarm-bearing raw tag materializes only the condition, with no sibling value variable, so there is no other node to point at | | `SourceName` | the same identifying id string: **RawPath** (native) / **ScriptedAlarmId** (scripted) | Deliberately the *unique* id, **not** the leaf name | | `ConditionName` | the leaf / display name (e.g. `HR200`) | Where the short human-readable name lives | +| `ConditionClassId` | always **`BaseConditionClassType`** | Part 9's "no condition class modelled" value. Unset shipped `NodeId.Null` (#475) | +| `ConditionClassName` | always **`"BaseConditionClass"`** | Matches `ConditionClassId`. Unset shipped empty text (#475) | + +**Why `BaseConditionClassType` and not `ProcessConditionClassType`.** We hold no per-alarm +classification at the materialize seam, and `ConditionClassId` is a wire contract clients bucket on. +`BaseConditionClassType` is the honest, spec-conformant report of *"this server does not model +condition classes"* — it fixes the real defect (a null that breaks conformant clients) without +asserting a classification we cannot back. `ProcessConditionClassType` — the SDK sample's pick — +was rejected deliberately: it would be *actively wrong* for a Galaxy alarm whose upstream category is +Safety or Diagnostics, trading a detectable null for an undetectable lie. Real per-alarm +classification is a separate future feature: it needs the driver's alarm category, which today lives +only on the runtime `AlarmEventArgs` transition, carried to the deploy-time authored composition that +`MaterialiseAlarmCondition` sees. Until then the `IAlarmSource` doc comment claiming the category +"maps to `ConditionClassName` downstream" describes an intent, not the implementation. **Why `SourceName` is the id, not the leaf name.** The leaf is ambiguous across devices (`HR200` on two PLCs collides) and is already carried by `ConditionName`, so the leaf-name option would add no @@ -79,10 +94,12 @@ identifier and is unique, so it is safe to key on *by itself* — but do **not** `ConditionName` (`$"{SourceName}.{ConditionName}"`), because `SourceName` already ends in the condition's leaf name and the result stutters (`pymodbus/plc/HR200.HR200`). -Wire-level guard: `NativeAlarmEventIdentityFieldDeliveryTests` asserts the three fields arrive -populated on a real subscription using the standard `[EventType, SourceNode, SourceName, Time, -Message, Severity]` select clause; `NodeManagerAlarmSourceFieldsTests` guards the node itself -across both realms. +Wire-level guard: `NativeAlarmEventIdentityFieldDeliveryTests` asserts the three `BaseEventType` +fields arrive populated on a real subscription using the standard `[EventType, SourceNode, +SourceName, Time, Message, Severity]` select clause, and — in a second test with its own clause — +that `ConditionClassId` / `ConditionClassName` do too. The two class fields are declared on +`ConditionType`, **not** `BaseEventType`, so a client must select them against that type. +`NodeManagerAlarmSourceFieldsTests` guards the node itself across both realms. > **Do not correlate live events to HistoryRead on `SourceName` — the two paths disagree.** > The HistoryRead *events* projection (`OtOpcUaNodeManager.ProjectEventField`) returns diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 32c74758..829ffb40 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -815,6 +815,18 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 alarm.SourceNode.Value = alarm.NodeId; // Create() assigned this above; do not rebuild it alarm.SourceName.Value = alarmNodeId; + // #475 — the mandatory ConditionType classification fields, unset by Create() for the same reason as + // the fields above (mandatory, no default, nothing downstream synthesises them) ⇒ NodeId.Null + empty + // text on the wire, which buckets every alarm as unclassified in a Part 9 HMI. + // BaseConditionClassType is Part 9's "no class modelled" value and is the honest report: we hold no + // classification at this seam. Deliberately NOT ProcessConditionClassType (the SDK sample's pick) — it + // would assert a classification we cannot back, and would be actively wrong for a Galaxy alarm whose + // upstream category is Safety/Diagnostics. Real per-alarm classification needs the driver's + // AlarmCategory, which today exists only on the runtime AlarmEventArgs transition and not on the + // authored composition this deploy-time seam sees — a separate feature, not a default picked here. + if (alarm.ConditionClassId is not null) alarm.ConditionClassId.Value = ObjectTypeIds.BaseConditionClassType; + if (alarm.ConditionClassName is not null) alarm.ConditionClassName.Value = new LocalizedText("BaseConditionClass"); + // Initial state via the SDK setters (T14: basic state only, NO event firing). alarm.SetEnableState(SystemContext, true); alarm.SetActiveState(SystemContext, false); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/NativeAlarmEventIdentityFieldDeliveryTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/NativeAlarmEventIdentityFieldDeliveryTests.cs index eb53a4f1..1cc9ec8e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/NativeAlarmEventIdentityFieldDeliveryTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/NativeAlarmEventIdentityFieldDeliveryTests.cs @@ -30,6 +30,7 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests; public sealed class NativeAlarmEventIdentityFieldDeliveryTests { private const string ServerUri = "urn:OtOpcUa.AlarmEventIdentityFields"; + private const string ConditionClassServerUri = "urn:OtOpcUa.AlarmEventConditionClassFields"; private const string RawDeviceFolder = "pymodbus/plc"; private const string RawAlarmPath = "pymodbus/plc/HR200"; @@ -45,6 +46,13 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests private const int MessageIndex = 4; private const int SeverityIndex = 5; + // Field indices in BuildConditionClassEventFilter's clause (#475). A SEPARATE clause on purpose: the + // ScadaBridge one above is mirrored field-for-field from the real consumer and its indices are load-bearing, + // so appending to it would silently shift them. Message is re-selected here only as the collector's filter key. + private const int ConditionClassIdIndex = 0; + private const int ConditionClassNameIndex = 1; + private const int ConditionClassMessageIndex = 2; + /// A live native condition event delivers a populated EventType / SourceNode / SourceName to a /// Server-object subscriber using the standard BaseEventType select clause. [Fact] @@ -67,7 +75,7 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests var rawNs = (ushort)session.NamespaceUris.GetIndex(V3NodeIds.RawNamespaceUri); rawNs.ShouldBeGreaterThan((ushort)0); - var collector = new EventCollector(); + var collector = new EventCollector(MessageIndex); var subscription = new Subscription(session.DefaultSubscription) { PublishingInterval = 100 }; subscription.FastEventCallback = collector.OnEvents; session.AddSubscription(subscription); @@ -75,7 +83,7 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests // The collector filters by our unique Message, so the item's ClientHandle is not needed here // (unlike the sibling multi-notifier test, which tallies delivery per monitored item). - AddEventItem(subscription, ObjectIds.Server); + AddEventItem(subscription, ObjectIds.Server, BuildEventFilter()); await subscription.ApplyChangesAsync(ct); sink.WriteAlarmCondition(RawAlarmPath, ActiveSnapshot(), DateTime.UtcNow, AddressSpaceRealm.Raw); @@ -113,6 +121,57 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests } } + /// Issue #475 — a live condition event delivers a populated ConditionClassId / ConditionClassName. + /// Both previously arrived unset (NodeId.Null / empty text) via the same mechanism as the #473 fields, so an + /// HMI bucketing alarms by condition class dropped every OtOpcUa alarm into an unclassified bin. + [Fact] + public async Task Condition_event_carries_populated_ConditionClass_fields_on_the_wire() + { + var pkiRoot = Path.Combine(Path.GetTempPath(), $"otopcua-alarm-condclass-{Guid.NewGuid():N}"); + var port = AllocateFreePort(); + var ct = TestContext.Current.CancellationToken; + try + { + var (server, host) = await BootServerAsync(port, pkiRoot + "-srv", ConditionClassServerUri, ct); + await using var _ = host; + + var sink = new SdkAddressSpaceSink(server.NodeManager!); + WireCondition(sink); + + using var session = await OpenSessionAsync($"opc.tcp://127.0.0.1:{port}/OtOpcUa", ct); + + var collector = new EventCollector(ConditionClassMessageIndex); + var subscription = new Subscription(session.DefaultSubscription) { PublishingInterval = 100 }; + subscription.FastEventCallback = collector.OnEvents; + session.AddSubscription(subscription); + await subscription.CreateAsync(ct); + + AddEventItem(subscription, ObjectIds.Server, BuildConditionClassEventFilter()); + await subscription.ApplyChangesAsync(ct); + + sink.WriteAlarmCondition(RawAlarmPath, ActiveSnapshot(), DateTime.UtcNow, AddressSpaceRealm.Raw); + + await WaitUntilAsync(() => collector.Fields.Count >= 1, TimeSpan.FromSeconds(5)); + var fields = collector.Fields.ShouldHaveSingleItem(); + + // ConditionClassId — a resolvable class NodeId, NOT NodeId.Null. We model no condition classes, so + // Part 9's "no class modelled" value (BaseConditionClassType) is the conformant report. + fields[ConditionClassIdIndex].Value.ShouldBe(ObjectTypeIds.BaseConditionClassType, + "ConditionClassId must carry a resolvable condition class, not null"); + + // ConditionClassName — the matching human-readable name, NOT empty text. + var className = fields[ConditionClassNameIndex].Value.ShouldBeOfType(); + className.Text.ShouldBe("BaseConditionClass", + "ConditionClassName must name the reported condition class, not be empty"); + + await subscription.DeleteAsync(true, ct); + } + finally + { + SafeDelete(pkiRoot + "-srv"); + } + } + /// Materialise the raw device folder + the native condition at its RawPath, plus one referencing /// equipment folder wired as a notifier (the production topology). private static void WireCondition(SdkAddressSpaceSink sink) @@ -127,13 +186,13 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests new(Active: true, Acknowledged: false, Confirmed: true, Enabled: true, Shelving: AlarmShelvingKind.Unshelved, Severity: 700, Message: AlarmMessage); - private static MonitoredItem AddEventItem(Subscription subscription, NodeId source) + private static MonitoredItem AddEventItem(Subscription subscription, NodeId source, EventFilter filter) { var item = new MonitoredItem(subscription.DefaultItem) { StartNodeId = source, AttributeId = Attributes.EventNotifier, - Filter = BuildEventFilter(), + Filter = filter, QueueSize = 100, SamplingInterval = 0, }; @@ -154,9 +213,23 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests return filter; } + /// #475's clause: [ConditionClassId, ConditionClassName, Message]. The two class fields are declared on + /// ConditionType, not BaseEventType, so they must be selected against that type or the server returns no + /// value for them regardless of the fix. Message rides along solely as the collector's filter key. + private static EventFilter BuildConditionClassEventFilter() + { + var filter = new EventFilter(); + filter.AddSelectClause(ObjectTypes.ConditionType, BrowseNames.ConditionClassId); + filter.AddSelectClause(ObjectTypes.ConditionType, BrowseNames.ConditionClassName); + filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.Message); + return filter; + } + /// Captures the delivered event field lists, filtered to our unique alarm Message so unrelated /// server events / refresh brackets never count. - private sealed class EventCollector + /// Position of the Message field in the select clause this collector is paired + /// with — the two clauses here place it differently, so it cannot be a shared constant. + private sealed class EventCollector(int messageIndex) { private readonly object _gate = new(); private readonly List _fields = new(); @@ -172,8 +245,8 @@ public sealed class NativeAlarmEventIdentityFieldDeliveryTests { foreach (var e in notification.Events) { - if (e.EventFields.Count > MessageIndex && - e.EventFields[MessageIndex].Value is LocalizedText lt && + if (e.EventFields.Count > messageIndex && + e.EventFields[messageIndex].Value is LocalizedText lt && lt.Text == AlarmMessage) { _fields.Add(e.EventFields); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmSourceFieldsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmSourceFieldsTests.cs index 2156a337..3b8f14f7 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmSourceFieldsTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerAlarmSourceFieldsTests.cs @@ -23,6 +23,12 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; /// scripted), matching ConditionId. The leaf/display name stays on ConditionName, so /// no information is lost by SourceName carrying the unique id rather than the ambiguous leaf. /// +/// +/// Issue #475 — the same mechanism leaves the mandatory ConditionType classification fields +/// ConditionClassId / ConditionClassName unset. The contract locked in here: a server that +/// does not model condition classes must still report BaseConditionClassType (Part 9's +/// "no class modelled" value) rather than null. See docs/AlarmTracking.md. +/// /// public sealed class NodeManagerAlarmSourceFieldsTests : IDisposable { @@ -144,6 +150,52 @@ public sealed class NodeManagerAlarmSourceFieldsTests : IDisposable } + /// #475 — a NATIVE condition (Raw realm) carries the mandatory ConditionType classification fields. + /// We do not model condition classes, so Part 9's "no class modelled" value — BaseConditionClassType — is the + /// conformant answer; the point is that it is a resolvable class NodeId a client can bucket on rather than a + /// null that drops the alarm into an unclassified bin. + [Trait("Category", "Unit")] + [Fact] + public async Task Native_condition_carries_ConditionClassId_and_ConditionClassName() + { + await using var host = await BootAsync(); + var nm = host.Nm; + + nm.EnsureFolder(RawDeviceFolder, parentNodeId: null, displayName: "dev1", realm: AddressSpaceRealm.Raw); + nm.MaterialiseAlarmCondition(RawAlarmPath, RawDeviceFolder, "HR200", "OffNormalAlarm", 700, + realm: AddressSpaceRealm.Raw, isNative: true); + + var condition = nm.TryGetAlarmCondition(RawAlarmPath, AddressSpaceRealm.Raw); + condition.ShouldNotBeNull(); + + condition.ConditionClassId.ShouldNotBeNull(); + condition.ConditionClassId.Value.ShouldBe(ObjectTypeIds.BaseConditionClassType); + + condition.ConditionClassName.ShouldNotBeNull(); + condition.ConditionClassName.Value.ShouldNotBeNull(); + condition.ConditionClassName.Value.Text.ShouldBe("BaseConditionClass"); + } + + /// #475 — a SCRIPTED condition (UNS realm) carries the SAME classification fields: native and scripted + /// share the materialise path, so neither may regress independently. + [Trait("Category", "Unit")] + [Fact] + public async Task Scripted_condition_carries_ConditionClassId_and_ConditionClassName() + { + await using var host = await BootAsync(); + var nm = host.Nm; + + nm.EnsureFolder("eq-4", parentNodeId: null, displayName: "Station 4", realm: AddressSpaceRealm.Uns); + nm.MaterialiseAlarmCondition("tank-dry", "eq-4", "Tank Dry", "OffNormalAlarm", 700, + realm: AddressSpaceRealm.Uns, isNative: false); + + var condition = nm.TryGetAlarmCondition("tank-dry", AddressSpaceRealm.Uns); + condition.ShouldNotBeNull(); + + condition.ConditionClassId!.Value.ShouldBe(ObjectTypeIds.BaseConditionClassType); + condition.ConditionClassName!.Value.Text.ShouldBe("BaseConditionClass"); + } + /// A booted server + its node manager, disposed via await using so an assertion failure /// cannot leak a live server (and its bound port) into the rest of the test run. private sealed class BootedServer(OpcUaApplicationHost host, OtOpcUaNodeManager nm) : IAsyncDisposable