From c792e596899eb4fce7bf88fb5104de72a4e49ea2 Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sat, 4 Jul 2026 20:33:48 +0200 Subject: [PATCH 1/2] fix(opcua): accept severity as alias in event_alarms so it is not silently 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. --- .../ros2_medkit_opcua/opcua_plugin.hpp | 7 ++ .../ros2_medkit_opcua/src/node_map.cpp | 53 +++++++++++++- .../ros2_medkit_opcua/src/opcua_plugin.cpp | 40 ++++++----- .../ros2_medkit_opcua/test/test_node_map.cpp | 71 +++++++++++++++++++ .../test/test_opcua_plugin.cpp | 26 +++++++ 5 files changed, 176 insertions(+), 21 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index 2656fc295..8e4562ca8 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -110,6 +111,12 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, tl::expected clear_fault(const std::string & entity_id, const std::string & fault_code) override; + // Resolve the SOVD severity bucket for an event alarm. An explicit configured + // override wins; with none configured the raw OPC-UA event Severity (1-1000) + // is mapped to a bucket by band (>=801 CRITICAL, >=501 ERROR, >=201 WARNING, + // else INFO). Pure + static so it is unit-testable without a live server. + static std::string map_severity(uint16_t live_severity, const std::string & severity_override); + private: // Route handlers void handle_plc_data(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp index 4311e6132..a40504e0e 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -569,6 +570,47 @@ bool NodeMap::load(const std::string & yaml_path) { "event_alarms has %zu entries (max 10000) - refusing to load", event_alarms_node.size()); return false; } + // Resolve the configured severity for an event_alarms entry / mapping. + // ``severity`` is accepted as an alias for ``severity_override`` because + // the threshold ``alarm:`` block uses ``severity:``; an operator who + // reuses that key here must not have it silently dropped. When both are + // present ``severity_override`` wins. An explicit value is validated like + // the threshold path; with neither key the result stays empty so the live + // event Severity band mapping applies (see OpcuaPlugin::map_severity). + auto read_severity_override = [&](const YAML::Node & node, const std::string & context) -> std::string { + const YAML::Node sev_node = node["severity_override"] ? node["severity_override"] : node["severity"]; + if (!sev_node) { + return ""; + } + const char * field = node["severity_override"] ? "severity_override" : "severity"; + auto sev = parse_string(sev_node, field, context); + if (!sev || sev->empty()) { + return ""; + } + return validate_severity(*sev, context); + }; + // Warn on keys the event_alarms loader does not recognise so a future + // typo (e.g. ``severty:``) is surfaced instead of silently dropped. + auto warn_unknown_keys = [logger](const YAML::Node & node, const std::string & context, + std::initializer_list known) { + if (!node.IsMap()) { + return; + } + for (const auto & kv : node) { + std::string key; + try { + key = kv.first.as(); + } catch (const YAML::Exception &) { + continue; + } + const bool recognised = std::any_of(known.begin(), known.end(), [&key](const char * k) { + return key == k; + }); + if (!recognised) { + RCLCPP_WARN(logger, "%s: unknown key '%s' - ignored", context.c_str(), key.c_str()); + } + } + }; for (size_t i = 0; i < event_alarms_node.size(); ++i) { const auto & a = event_alarms_node[i]; if (!a["alarm_source"] || !a["entity_id"]) { @@ -596,12 +638,15 @@ bool NodeMap::load(const std::string & yaml_path) { } alarm_fault_code = *parsed_fault_code; } + warn_unknown_keys(a, alarm_label, + {"alarm_source", "entity_id", "fault_code", "severity_override", "severity", "message", + "mappings", "associated_values"}); AlarmEventConfig cfg; cfg.source_node_id_str = *alarm_source; cfg.source_node_id = parse_node_id(cfg.source_node_id_str); cfg.entity_id = *alarm_entity_id; cfg.fault_code = alarm_fault_code; - cfg.severity_override = a["severity_override"].as(""); + cfg.severity_override = read_severity_override(a, alarm_label); cfg.message_override = a["message"].as(""); // Issue #389: per-condition-identity mappings (multi-alarm). @@ -616,12 +661,16 @@ bool NodeMap::load(const std::string & yaml_path) { if (!mapping_fault_code) { continue; } + const std::string mapping_label = alarm_label + " mapping"; + warn_unknown_keys(m, mapping_label, + {"condition_name", "source_node", "event_type", "fault_code", "severity_override", + "severity", "message"}); AlarmMapping mapping; mapping.match_condition_name = m["condition_name"].as(""); mapping.match_source_node = m["source_node"].as(""); mapping.match_event_type = m["event_type"].as(""); mapping.fault_code = *mapping_fault_code; - mapping.severity_override = m["severity_override"].as(""); + mapping.severity_override = read_severity_override(m, mapping_label); mapping.message_override = m["message"].as(""); cfg.mappings.push_back(std::move(mapping)); } diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index 341831737..60d05f4ab 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -692,30 +692,32 @@ void OpcuaPlugin::on_alarm_change(const std::string & entity_id, } } +std::string OpcuaPlugin::map_severity(uint16_t live_severity, const std::string & severity_override) { + // Map raw OPC-UA Severity (1-1000) to SOVD severity bucket. + // Selfpatch convention documented in design/index.rst; not from IEC 62682. + // The resolved severity_override (mapping- or source-level, issue #389) + // wins when set. + if (!severity_override.empty()) { + return severity_override; + } + if (live_severity >= 801) { + return std::string("CRITICAL"); + } + if (live_severity >= 501) { + return std::string("ERROR"); + } + if (live_severity >= 201) { + return std::string("WARNING"); + } + return std::string("INFO"); +} + void OpcuaPlugin::on_event_alarm(const AlarmEventDelivery & delivery) { if (shutdown_requested_.load()) { return; } - // Map raw OPC-UA Severity (1-1000) to SOVD severity bucket. - // Selfpatch convention documented in design/index.rst; not from IEC 62682. - // The resolved severity_override (mapping- or source-level, issue #389) - // wins when set. - auto severity_str = [&]() { - if (!delivery.severity_override.empty()) { - return delivery.severity_override; - } - if (delivery.severity >= 801) { - return std::string("CRITICAL"); - } - if (delivery.severity >= 501) { - return std::string("ERROR"); - } - if (delivery.severity >= 201) { - return std::string("WARNING"); - } - return std::string("INFO"); - }(); + const std::string severity_str = map_severity(delivery.severity, delivery.severity_override); switch (delivery.action) { case AlarmAction::ReportConfirmed: diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp index 7626bf556..d6789e84c 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp @@ -584,6 +584,77 @@ component_id: test EXPECT_TRUE(map.load(path)); } +TEST_F(NodeMapTest, EventAlarmSeverityAliasSetsOverride) { + // Regression: an ``event_alarms`` entry configured with ``severity:`` (the + // key the threshold ``alarm:`` block uses) must not be silently dropped. It + // is accepted as an alias for ``severity_override`` so the configured + // severity reaches the fault instead of falling back to the live event + // Severity band mapping. + std::string path = "/tmp/test_node_map_event_alarm_severity_alias.yaml"; + std::ofstream f(path); + f << R"( +area_id: test +component_id: test +event_alarms: + - alarm_source: "ns=3;s=Program_Alarm" + entity_id: plc_program + fault_code: PLC_PROGRAM_ALARM + severity: ERROR +)"; + f.close(); + + NodeMap map; + ASSERT_TRUE(map.load(path)); + ASSERT_EQ(map.event_alarms().size(), 1u); + EXPECT_EQ(map.event_alarms()[0].severity_override, "ERROR"); +} + +TEST_F(NodeMapTest, EventAlarmSeverityOverrideWinsOverAlias) { + // ``severity_override`` is the canonical key; when both are present it wins. + std::string path = "/tmp/test_node_map_event_alarm_severity_precedence.yaml"; + std::ofstream f(path); + f << R"( +area_id: test +component_id: test +event_alarms: + - alarm_source: "ns=3;s=Program_Alarm" + entity_id: plc_program + fault_code: PLC_PROGRAM_ALARM + severity_override: CRITICAL + severity: WARNING +)"; + f.close(); + + NodeMap map; + ASSERT_TRUE(map.load(path)); + ASSERT_EQ(map.event_alarms().size(), 1u); + EXPECT_EQ(map.event_alarms()[0].severity_override, "CRITICAL"); +} + +TEST_F(NodeMapTest, EventAlarmMappingSeverityAliasSetsOverride) { + // The alias also applies to per-condition ``mappings`` entries (issue #389). + std::string path = "/tmp/test_node_map_event_alarm_mapping_severity_alias.yaml"; + std::ofstream f(path); + f << R"( +area_id: test +component_id: test +event_alarms: + - alarm_source: "ns=3;s=Program_Alarm" + entity_id: plc_program + mappings: + - condition_name: Overtemp + fault_code: PLC_OVERTEMP + severity: WARNING +)"; + f.close(); + + NodeMap map; + ASSERT_TRUE(map.load(path)); + ASSERT_EQ(map.event_alarms().size(), 1u); + ASSERT_EQ(map.event_alarms()[0].mappings.size(), 1u); + EXPECT_EQ(map.event_alarms()[0].mappings[0].severity_override, "WARNING"); +} + TEST_F(NodeMapTest, RejectsAlarmSourceUnderNodes) { // Schema validation: ``alarm_source`` is only valid in the top-level // ``event_alarms:`` section. Used to be silently ignored when not paired diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index 9e2fe89e3..ac0594d30 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -502,4 +502,30 @@ TEST(ReadSnapshotClassify, ActiveButFlaggedUnreliableIsKeptNotFed) { EXPECT_EQ(d, OpcuaPoller::ReadReplayDisposition::KeepOnly); } +// -- OpcuaPlugin::map_severity (event-alarm severity resolution) ------------- + +TEST(MapSeverity, ConfiguredOverrideWinsOverLiveSeverity) { + // An explicit configured severity wins regardless of the live event Severity. + EXPECT_EQ(OpcuaPlugin::map_severity(1000, "INFO"), "INFO"); + EXPECT_EQ(OpcuaPlugin::map_severity(1, "CRITICAL"), "CRITICAL"); +} + +TEST(MapSeverity, LowLiveSeverityMapsToInfo) { + // The Siemens trap: many servers emit Program_Alarm events at Severity 1, so + // with no configured override the fault lands as INFO. This is exactly why a + // silently dropped ``severity:`` key mattered. + EXPECT_EQ(OpcuaPlugin::map_severity(1, ""), "INFO"); +} + +TEST(MapSeverity, LiveSeverityBandBoundaries) { + // Band map: >=801 CRITICAL, >=501 ERROR, >=201 WARNING, else INFO. + EXPECT_EQ(OpcuaPlugin::map_severity(200, ""), "INFO"); + EXPECT_EQ(OpcuaPlugin::map_severity(201, ""), "WARNING"); + EXPECT_EQ(OpcuaPlugin::map_severity(500, ""), "WARNING"); + EXPECT_EQ(OpcuaPlugin::map_severity(501, ""), "ERROR"); + EXPECT_EQ(OpcuaPlugin::map_severity(800, ""), "ERROR"); + EXPECT_EQ(OpcuaPlugin::map_severity(801, ""), "CRITICAL"); + EXPECT_EQ(OpcuaPlugin::map_severity(1000, ""), "CRITICAL"); +} + } // namespace ros2_medkit_gateway From 83551b8cacb5bc0015044f95a67f0f491826afa6 Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sat, 4 Jul 2026 22:27:34 +0200 Subject: [PATCH 2/2] fix(opcua): treat null severity_override as absent so severity alias 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. --- .../ros2_medkit_opcua/src/node_map.cpp | 15 ++++-- .../ros2_medkit_opcua/test/test_node_map.cpp | 47 +++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp index a40504e0e..63f162cec 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp @@ -578,11 +578,20 @@ bool NodeMap::load(const std::string & yaml_path) { // the threshold path; with neither key the result stays empty so the live // event Severity band mapping applies (see OpcuaPlugin::map_severity). auto read_severity_override = [&](const YAML::Node & node, const std::string & context) -> std::string { - const YAML::Node sev_node = node["severity_override"] ? node["severity_override"] : node["severity"]; - if (!sev_node) { + // yaml-cpp reports a present-but-null key (``severity_override:`` with + // no value / ``~``) as defined/truthy; gate on a real value so a null + // override neither shadows a valid ``severity:`` alias nor emits a + // misleading "non-string severity_override" warning. + auto has_value = [](const YAML::Node & n) { + return n.IsDefined() && !n.IsNull(); + }; + const YAML::Node override_node = node["severity_override"]; + const bool use_override = has_value(override_node); + const YAML::Node sev_node = use_override ? override_node : node["severity"]; + if (!has_value(sev_node)) { return ""; } - const char * field = node["severity_override"] ? "severity_override" : "severity"; + const char * field = use_override ? "severity_override" : "severity"; auto sev = parse_string(sev_node, field, context); if (!sev || sev->empty()) { return ""; diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp index d6789e84c..8133ff430 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp @@ -655,6 +655,53 @@ component_id: test EXPECT_EQ(map.event_alarms()[0].mappings[0].severity_override, "WARNING"); } +TEST_F(NodeMapTest, EventAlarmNullSeverityOverrideFallsBackToAlias) { + // A present-but-null ``severity_override:`` is reported as defined by yaml-cpp + // but must not shadow a real ``severity:`` sibling (nor emit a spurious + // "non-string severity_override" warning): the alias still wins. + std::string path = "/tmp/test_node_map_event_alarm_null_override.yaml"; + std::ofstream f(path); + f << R"( +area_id: test +component_id: test +event_alarms: + - alarm_source: "ns=3;s=Program_Alarm" + entity_id: plc_program + fault_code: PLC_PROGRAM_ALARM + severity_override: + severity: WARNING +)"; + f.close(); + + NodeMap map; + ASSERT_TRUE(map.load(path)); + ASSERT_EQ(map.event_alarms().size(), 1u); + EXPECT_EQ(map.event_alarms()[0].severity_override, "WARNING"); +} + +TEST_F(NodeMapTest, EventAlarmSeverityAliasInvalidBucketDefaultsToError) { + // The alias is *validated* (validate_severity), not passed through raw: an + // unknown bucket is defaulted to ERROR so a typo cannot misroute the fault to + // a bogus severity. Pins the validation behaviour the alias path adds. + std::string path = "/tmp/test_node_map_event_alarm_severity_invalid.yaml"; + std::ofstream f(path); + f << R"( +area_id: test +component_id: test +event_alarms: + - alarm_source: "ns=3;s=Program_Alarm" + entity_id: plc_program + fault_code: PLC_PROGRAM_ALARM + severity: notabucket +)"; + f.close(); + + NodeMap map; + ASSERT_TRUE(map.load(path)); + ASSERT_EQ(map.event_alarms().size(), 1u); + EXPECT_EQ(map.event_alarms()[0].severity_override, "ERROR"); +} + TEST_F(NodeMapTest, RejectsAlarmSourceUnderNodes) { // Schema validation: ``alarm_source`` is only valid in the top-level // ``event_alarms:`` section. Used to be silently ignored when not paired