fix(alarms): populate EventType/SourceNode/SourceName on native + scripted conditions (#473) #474
Assignees
dohertj2 (dohertj2)
Clear assignees
dohertj2 (dohertj2)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dohertj2/lmxopcua#474
Reference in New Issue
Block a user
Delete Branch "fix/alarm-condition-source-fields"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #473.
What
MaterialiseAlarmConditionnever assigned the three mandatoryBaseEventTypeidentity fields, so all three arrived null on every condition event — native and scripted. Three lines of production code:I confirmed the issue's root-cause analysis end to end: neither
MaterialiseAlarmConditionnorReportConditionEventsets these, and the snapshot copies node children verbatim, so nothing downstream fills them in. Notably the same file already setsSourceNode/SourceNamecarefully on model-change and audit events — consistent with this being an oversight on the alarm path only.The open question, decided
The issue flagged
SourceName(leaf vs RawPath) as needing a deliberate call. Decision: the identifying id — RawPath for native, ScriptedAlarmId for scripted. Uniform rule, and it matchesConditionId/SourceNode.Rationale: the leaf collides across devices (
HR200on two PLCs) and is already carried byConditionName, so the leaf option adds no identity while costing uniqueness. The full spread a client now gets:EventTypeOffNormalAlarmType(concrete type; base fallback for unknown types)SourceNodens=2;s=pymodbus/plc/HR200(==ConditionId)SourceNamepymodbus/plc/HR200(unique)ConditionNameHR200(leaf still available)SourceNodeself-references the condition because an alarm-bearing raw tag materialises only the condition — there is no sibling value variable to point at.Two things reviewers should know
1. ScadaBridge will show a stuttered key until it fixes its own keying. The issue says ScadaBridge "derives its alarm identity from SourceName" — it actually composes
$"{sourceName}.{conditionName}"(RealOpcUaClient.cs:755), which is why its broken key is".HR200"with a leading dot. So its key becomespymodbus/plc/HR200.HR200— stuttered, but unique and correct, versus today's collision. ScadaBridge should key onConditionIdas the issue itself notes. Documented explicitly: clients must not composeSourceNamewithConditionName.2.
SourceNameis NOT a live↔history join key. HistoryRead nullsEventType/SourceNodeby design, but it does projectSourceName— stamped by the alarm-history writer as the EquipmentPath (AlarmEventMapper), not the RawPath. So it's the one field both paths populate with different values. Pre-existing and unchanged by this PR, but the docs now warn against it rather than papering over it.Tests
NativeAlarmEventIdentityFieldDeliveryTests(wire-level, the guard the issue asked for) — real server + real client subscription using ScadaBridge's exact[EventType, SourceNode, SourceName, Time, Message, Severity]select clause. Verified to fail against the pre-fix server (EventTypearrived empty), not just to pass after.NodeManagerAlarmSourceFieldsTests— node-level across both realms, the base-type fallback, and the kind-swap re-materialise.All 4 node tests were watched failing first. Full suites green: OpcUaServer.Tests 379, IntegrationTests 11, Runtime.Tests 378, Commons.Tests 306.
NodeManagerHistoryReadEventsTests' deliberateEventType => Variant.Nullassertions still hold (separate path).Follow-up (not in this PR)
A high-effort review found the same bug class in the same function:
ConditionClassId/ConditionClassNameare assigned nowhere in the tree, so they're also null on every event. Out of scope for #473 (its own wire-contract decision + tests) — filed separately.Verification note
Proven by the wire-level integration test (real
OtOpcUaSdkServer+ real OPC UA client over TCP, driven through the productionSdkAddressSpaceSink), not by a docker-dev deploy. The fix sits in the single terminal node-manager method every materialise path funnels through, and adds no new interface surface — so theDeferredAddressSpaceSinkprod-inertness trap doesn't apply here.MaterialiseAlarmCondition never assigned the three mandatory BaseEventType identity fields, so all three arrived null on every condition event — native and scripted. The SDK does not synthesise them on this path: Create() builds the children from the type definition but leaves them unset, the auto-filling BaseEventState.Initialize overload is only used for transient events, and ReportEvent / InstanceStateSnapshot copy children verbatim. A conforming client could not attribute an alarm to its source. EventType = the concrete materialised type (TypeDefinitionId) SourceNode = the condition's own NodeId (== ConditionId) — the condition IS the source; an alarm-bearing raw tag materialises only the condition, with no sibling value variable SourceName = the same identifying id string: RawPath (native) / ScriptedAlarmId (scripted) SourceName carries the unique id rather than the leaf name: the leaf collides across devices (HR200 on two PLCs) and is already carried by ConditionName, so nothing is lost. Documented in docs/AlarmTracking.md, including that clients must key on ConditionId and must not compose SourceName with ConditionName, and that SourceName is NOT a live<->history join key (the alarm-history writer stamps it with the EquipmentPath — a pre-existing divergence, now called out). Tests: NativeAlarmEventIdentityFieldDeliveryTests is the wire-level guard — a real client subscription using the standard [EventType, SourceNode, SourceName, Time, Message, Severity] select clause, verified to fail against the pre-fix server. NodeManagerAlarmSourceFieldsTests guards the node across both realms, the base-type fallback, and the kind-swap re-materialise. The HistoryRead events projection is a separate path (it projects historian rows, not node fields) and is unaffected — its EventType => Variant.Null assertions still hold.