fix(opcua): drop non-condition events from the alarm path - #552
Merged
Conversation
The server-wide EventNotifier also emits BaseEventType/AuditEventType housekeeping (session-state, audit) with a null ConditionId. Routing those through the catch-all alarm mapping produced a PLC_ALARM that masked a real process alarm, since the fault store dedups by code. Guard on a non-null ConditionId so only genuine A&C conditions raise.
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents OPC UA server-wide EventNotifier “housekeeping” events (BaseEventType/AuditEventType) from entering the alarm/condition processing path in ros2_medkit_opcua, avoiding PLC_ALARM fault-code dedup collisions that can mask real process alarms.
Changes:
- Add an early-return guard in
OpcuaPoller::on_event()to ignore events with a nullConditionId. - Emit a debug log when such non-condition events are dropped.
The drop-non-condition guard used a raw condition_id.isNull(); switch it to the shared is_condition_event() helper, which has focused unit tests (NullConditionIdIsRejected / RealConditionIdIsAccepted) asserting the exact drop-null / keep-real decision. Remove the now-dead duplicate of the same check in the auto-alarm branch - the hoisted guard covers it.
bburda
reviewed
Jul 23, 2026
bburda
left a comment
Collaborator
There was a problem hiding this comment.
Additional findings outside the diff:
README.mdstill describes this drop as auto_alarms only. The paragraph starting "System messages:" is inside theauto_alarmschapter and saysauto_alarmsdrops those events before deriving a fault. After this change it applies to every alarm source. The same paragraph says to useexcludefor the rest, butauto_alarm_passes_filtersruns only in the auto branch, so an explicitevent_alarmsentry has no text filter at all.README.mdalso says "An event matching no mapping uses the source-levelfault_code; if that is also empty the event is ignored." This is what the README promises for the catch-allns=0;i=2253example withfault_code: PLC_GENERIC_ALARM. Non-Condition events now never reachNodeMap::resolve_alarm, so this sentence needs the exception.design/index.rstlists the decision order without the ConditionId condition. It is now a global rule that runs before that table.- The doc comment on
is_condition_eventinopcua_poller.hppand the comment aboveIsConditionEventTeststill say the filter is for auto_alarms. - Test coverage:
IsConditionEventTestonly checks the predicate. It passes the same way before and after the move, so it does not cover the routing this PR changes. The live fixture can emit Conditions only: every trigger goes throughUA_Server_triggerConditionEventon nodes built byUA_Server_createCondition, and there is no command for a plain event. To cover this, add a fixture command that fires aBaseEventTypeoni=2253withUA_Server_triggerEvent. Then add an integration case with an explicitevent_alarmsentry on that source: the plain event must produce no fault, and a real condition on the same source must still produce its fault. The second check matters too, it catches dropping more than intended. - Pre-existing, not caused by this PR:
reconcile_after_refreshdoes not filter by source, whilereconcile_after_readfilters onruntime.source_id. In open62541ConditionRefreshloops over every monitored item in the subscription and sends a separate RefreshStart/RefreshEnd pair for each one. The plugin creates one monitored item per alarm source on a single subscription. With two sources the first RefreshEnd reconciles against arefresh_seen_set that holds only the conditions of the first item. Conditions from the other source are then cleared and raised again by the next burst. Worth a separate issue.
The old comment claimed an unfiltered non-condition event maps to its fault_code and masks a genuine alarm. It does not: every alarm-state select clause resolves to null, so the state machine returns ReportHealed (a no-op) and no fault is reported. The real harm is the latched null-ConditionId entry apply_condition_state would leave in conditions_, which drives a spurious ClearFault on the next ConditionRefresh (should_clear_after_refresh treats Healed as active) and can misroute a later Acknowledge/Confirm via lookup_condition. Comment only.
…licit source The non-condition guard drops with a DEBUG line only, so on an explicit event_alarms source a real alarm arriving with a null ConditionId (edge open62541 servers, or an AlarmConditionType fired via triggerEvent) was silenced with no operator signal - a regression vs the pre-PR explicit path. Add a per-source warn-once (mirrors read_unsupported_warned_sources_) gated on the source having a fault_code or mappings, so the synthetic auto source keeps the quiet DEBUG path.
The guard now runs for every alarm source, not just auto_alarms. Update the README (mapping precedence exception + System messages section), the design decision table (new ConditionId rule 0), and the is_condition_event doc + IsConditionEventTest comment to match. Add a routing regression: a 'sysevent' fixture command fires a BaseEventType on i=2253 via UA_Server_triggerEvent (null ConditionId), and an integration case with an explicit event_alarms catch-all on ns=0;i=2253 -> PLC_ALARM asserts the sysevent never surfaces as PLC_ALARM while a real condition on the catch-all still faults.
bburda
approved these changes
Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The server-wide EventNotifier (
ns=0;i=2253, the Server object) emitsBaseEventType/AuditEventTypehousekeeping events - session-state changes, audit/system events (e.g. "Session state changed to Created") - alongside genuine A&C Condition events. The catch-all alarm mapping routed all of them to a singlePLC_ALARMfault code, and because the fault store dedups by code, a housekeeping event could overwrite and mask a real process alarm sharing that code.Only
ConditionType-derived events carry a non-nullConditionId(the Part 9 auto-prepended select clause). Guardingon_eventon a non-nullConditionIddrops the non-condition housekeeping events cleanly while leaving the real-alarm path untouched - genuine conditions always carry a non-null ConditionId (the plugin already keys per-condition tracking and Ack/Confirm on it).Verified:
colcon test ros2_medkit_opcuagreen - 265 tests including the live open62541 A&C integration tests that drive realAlarmConditionTypeevents (real conditions still flow through).