Skip to content

fix(opcua): accept severity as an alias in event_alarms#501

Open
mfaferek93 wants to merge 2 commits into
mainfrom
feat/opcua-event-alarm-severity-alias
Open

fix(opcua): accept severity as an alias in event_alarms#501
mfaferek93 wants to merge 2 commits into
mainfrom
feat/opcua-event-alarm-severity-alias

Conversation

@mfaferek93

Copy link
Copy Markdown
Collaborator

Fixes event_alarms silently dropping a severity: key.

  • node_map.cpp: the event_alarms loader now accepts severity: as an alias for severity_override: (source-level and per-mapping), routed through validate_severity like the threshold alarm: path, and warns on unknown keys in the block.
  • The final severity resolution (explicit configured severity wins over the live event Severity; otherwise the Severity band mapping) is extracted into a small testable helper.
  • Tests: an event_alarms entry with severity: ERROR resolves to ERROR; the helper covers override-wins and the band boundaries.

Closes #500.

…ently dropped

event_alarms only read severity_override, so a severity: key was silently
dropped and the fault fell back to the live event Severity. Accept severity
as an alias (source-level and per-mapping), validated like the threshold
path; warn on unknown keys; extract severity resolution into a testable helper.
Copilot AI review requested due to automatic review settings July 4, 2026 18:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes OPC UA event_alarms configuration parsing so a severity: key is no longer silently ignored, aligning behavior with the threshold alarm: block and the acceptance criteria from issue #500. It also extracts the event-alarm severity resolution into a small helper to make the precedence/band logic unit-testable.

Changes:

  • Accept severity: as an alias for severity_override: in event_alarms (source-level and per-mapping), validated via the existing severity validation path, with severity_override taking precedence when both are present.
  • Warn on unknown keys in event_alarms entries and their mappings to prevent future typos from being silently dropped.
  • Extract live-severity band mapping + override precedence into OpcuaPlugin::map_severity(...) and add unit/regression tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp Parses severity: as an alias for severity_override: in event_alarms and warns on unknown keys.
src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp Extracts event-alarm severity resolution into map_severity() and uses it in on_event_alarm().
src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp Declares the new map_severity() helper for unit testing.
src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp Adds regression tests ensuring severity: sets the override and precedence behaves as intended.
src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp Adds unit tests covering override-wins behavior and live-severity band boundary mapping.

@mfaferek93 mfaferek93 self-assigned this Jul 4, 2026

@bburda bburda left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional notes outside the inline comments:

  • Behaviour change for existing configs: routing severity_override through validate_severity means a config with a non-canonical string (e.g. severity_override: high) no longer reaches the fault verbatim. It becomes ERROR with a warning. This matches the threshold alarm: path and is an improvement, but it changes the emitted severity for such configs. Worth a line in the PR description or CHANGELOG.
  • README.md event_alarms section documents severity_override but not the new severity alias. A one-line note that event_alarms also accepts severity: would match the fix.

Comment thread src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp Outdated
Comment thread src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp
…wins

yaml-cpp reports a present-but-null severity_override: key as defined, so
a blank override shadowed a valid severity: sibling and logged a spurious
non-string warning. Gate on a real value (IsDefined and not IsNull). Add
regression tests for the null-override fallback and invalid-bucket default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

opcua: event_alarms severity is silently dropped when set as severity:

3 participants