From 274574763a433a2ffa221ca7f4051ee6347cb010 Mon Sep 17 00:00:00 2001 From: Ahmed Sleem <73216158+ahmedsleem109@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:38:35 +0300 Subject: [PATCH] feat(diagnostic_bridge): keep the numbers behind a fault, and stop confirming every planned STALE A DiagnosticStatus carries more than the bridge was willing to use. Key-values were read only to pick a fault code and then dropped, so a node that publishes outlier counts and gate statistics - the numbers it already computed to decide it was unhappy - left a fault record saying it complained but not what it saw. A FAILED report now carries them: ReportFault.srv gains an evidence field of diagnostic_msgs/KeyValue, the type the bridge already receives, and the fault manager keeps them in the fault's freeze frame under a reserved key that cannot collide with a topic name. Written on every FAILED report rather than at confirmation, because a code that keeps approaching confirmation without reaching it is the case the near-miss series exists for. Two existing paths had to learn about the key: a confirmation capture rebuilds the frame from the topics it sampled and would have erased it, and the read path served the frame only when no snapshots remained, which would have hidden the one thing the snapshots do not contain. Bounded per fault code in entries and value length; an over-long value is dropped whole and reported, never truncated into half a number. STALE was mapped to CRITICAL unconditionally, and CRITICAL bypasses debounce, so a GPS that goes STALE in every tunnel confirmed a CRITICAL fault on its first sample and the operator's debounce settings never ran. stale_severity and stale_severity_overrides make that a decision: longest matching prefix on the status name, the same rule the fault manager's entity thresholds use. Only STALE is configurable - the other levels are facts about the status, not deployment choices. An override that does not name a severity is reported and ignored rather than defaulted, because applying CRITICAL to a typo hands back exactly the behaviour the operator was configuring their way out of. docs/config/diagnostic-bridge.rst documented STALE as SEVERITY_ERROR. The bridge has always sent CRITICAL; the table now says what the code does. Closes #668 --- docs/config/diagnostic-bridge.rst | 68 +++++- docs/requirements/specs/faults.rst | 24 ++ src/ros2_medkit_diagnostic_bridge/README.md | 40 ++- .../config/diagnostic_bridge.yaml | 13 + .../diagnostic_bridge_node.hpp | 35 ++- .../src/diagnostic_bridge_node.cpp | 86 ++++++- .../test/test_diagnostic_bridge.cpp | 120 ++++++++- src/ros2_medkit_fault_manager/CMakeLists.txt | 9 + .../fault_manager_node.hpp | 16 ++ .../fault_storage.hpp | 39 +++ .../src/fault_manager_node.cpp | 52 +++- .../src/fault_storage.cpp | 80 ++++++ .../src/snapshot_capture.cpp | 10 +- .../test_reported_evidence.test.py | 231 ++++++++++++++++++ .../test/test_reported_evidence.cpp | 217 ++++++++++++++++ src/ros2_medkit_fault_reporter/CMakeLists.txt | 4 +- .../fault_reporter.hpp | 18 +- src/ros2_medkit_fault_reporter/package.xml | 1 + .../src/fault_reporter.cpp | 11 +- src/ros2_medkit_msgs/srv/ReportFault.srv | 14 ++ 20 files changed, 1066 insertions(+), 22 deletions(-) create mode 100644 src/ros2_medkit_fault_manager/test/integration/test_reported_evidence.test.py create mode 100644 src/ros2_medkit_fault_manager/test/test_reported_evidence.cpp diff --git a/docs/config/diagnostic-bridge.rst b/docs/config/diagnostic-bridge.rst index c40545b3e..037192f02 100644 --- a/docs/config/diagnostic-bridge.rst +++ b/docs/config/diagnostic-bridge.rst @@ -37,7 +37,19 @@ The diagnostic bridge: - SEVERITY_ERROR (2) * - STALE - FAILED - - SEVERITY_ERROR (2) + - ``stale_severity``, SEVERITY_CRITICAL (3) by default + +.. warning:: + + This table previously documented STALE as SEVERITY_ERROR. The bridge has always sent + SEVERITY_CRITICAL, and CRITICAL **bypasses debounce** in the fault manager, so a STALE + status confirms a CRITICAL fault on its first sample. + + That matters because STALE is the one level a node can reach by design: a GPS goes STALE + in every tunnel, an IMU reports covariance -1 while it settles. Left unconfigured, every + such outage is a confirmed CRITICAL fault. Use ``stale_severity`` to set the level, or + ``stale_severity_overrides`` to name just the sources that go STALE on purpose; those + statuses then debounce like any other. Parameters ---------- @@ -49,6 +61,9 @@ Parameters diagnostics_topic: "/diagnostics" # Topic to subscribe to auto_generate_codes: true # Auto-generate fault codes from names keyvalue_codes: ["fault_code"] # Take the code from these key-value keys + stale_severity: "WARN" # What a STALE status reports at + # Per-source, longest matching prefix wins: + "stale_severity_overrides.gps": "WARN" .. list-table:: :header-rows: 1 @@ -71,6 +86,57 @@ Parameters as its value, so a publisher can name its own code instead of relying on a mapping. Checked after ``name_to_code`` and before auto-generation. Empty strings in the list are ignored. + * - ``stale_severity`` + - ``CRITICAL`` + - Severity a STALE status reports at, one of ``INFO``, ``WARN``, ``ERROR``, + ``CRITICAL`` (case-insensitive). Applies to STALE only: the other levels are + facts about the status, not deployment decisions. A name that does not parse + is reported and ``CRITICAL`` is used. + * - ``stale_severity_overrides.`` + - ``-`` + - Severity for STALE statuses whose name starts with ````. Diagnostic names + are conventionally ``: ``, so a component prefix covers every + check it publishes. The **longest matching prefix** wins. An override that does + not parse is reported and **ignored**, leaving ``stale_severity`` in force - + applying ``CRITICAL`` to a typo would restore the immediate-confirm behaviour the + operator was configuring their way out of. + +Evidence +-------- + +A FAILED report carries the ``DiagnosticStatus`` key-values to the fault manager, which keeps +them in the fault's freeze frame and serves them from +``GET /api/v1/apps/{app}/faults/{code}``: + +.. code-block:: console + + $ curl -s localhost:8080/api/v1/apps/sensor_fusion/faults/FUSION_DIVERGED | jq '.environment_data.snapshots[0].data | fromjson' + { + "x-reported": { + "rejected_fixes": "37", + "nis": "0.03" + } + } + +A node that publishes outlier counts and gate statistics has already computed why it is +unhappy; before this the bridge read those values only to pick a fault code and dropped the +rest, so the fault record said a node complained but not what it saw. + +Evidence is written on **every** FAILED report, not only the one that confirms the fault, so a +code that keeps approaching confirmation without reaching it still carries the numbers behind +its near misses. + +The frame's other keys are topic names sampled by the fault manager, always fully qualified +and so always starting with ``/``. ``x-reported`` cannot collide with one, which is what lets +a reader tell a value the reporter asserted from one the fault manager sampled. + +.. note:: + + Evidence is bounded per fault code: at most 32 entries, and values longer than 512 + characters are dropped whole rather than truncated, because half a number read back later + is worse than a logged absence. Drops are reported with a throttled warning and the fault + is recorded either way. A key already stored can always be updated, so a steady reporter + at the bound can still refresh its own numbers. Custom Fault Code Mappings -------------------------- diff --git a/docs/requirements/specs/faults.rst b/docs/requirements/specs/faults.rst index f3434a3f6..c9c928b48 100644 --- a/docs/requirements/specs/faults.rst +++ b/docs/requirements/specs/faults.rst @@ -48,6 +48,30 @@ Faults shall take precedence over global defaults. Unspecified fields shall inherit from global configuration. When no entity prefix matches, global defaults shall apply. +.. req:: Configurable STALE Severity + :id: REQ_INTEROP_109 + :status: verified + :tags: Faults + + The diagnostic bridge shall map a ``DiagnosticStatus`` of level STALE to a configurable + fault severity, defaulting to CRITICAL, with per-diagnostic overrides selected by longest + matching prefix on the status name. A STALE status resolving to a non-CRITICAL severity + shall be subject to debounce filtering like any other report. An override that does not + name a valid severity shall be reported and ignored, leaving the configured default in + force. + +.. req:: Reporter-Supplied Fault Evidence + :id: REQ_INTEROP_110 + :status: verified + :tags: Faults + + A FAILED fault report shall be able to carry key-value measurements, which the fault + manager shall retain in the fault's freeze frame under a reserved key distinct from + captured topic values, and serve with the fault's environment data. Evidence shall be + retained across reports that carry none, preserved when a confirmation capture rebuilds + the frame, and bounded per fault code in entry count and value length, with entries + exceeding a bound dropped whole and reported rather than truncated. + .. req:: Fault Snapshot and Rosbag Capture :id: REQ_INTEROP_088 :status: verified diff --git a/src/ros2_medkit_diagnostic_bridge/README.md b/src/ros2_medkit_diagnostic_bridge/README.md index da42c3528..cbf564c43 100644 --- a/src/ros2_medkit_diagnostic_bridge/README.md +++ b/src/ros2_medkit_diagnostic_bridge/README.md @@ -15,7 +15,43 @@ to the FaultManager as faults. | OK (0) | - | Sends PASSED event (healing) | | WARN (1) | WARN (1) | Reports fault | | ERROR (2) | ERROR (2) | Reports fault | -| STALE (3) | CRITICAL (3) | Reports fault | +| STALE (3) | `stale_severity` (CRITICAL by default) | Reports fault | + +STALE is the one level whose severity is a deployment decision rather than a fact. A GPS in +every tunnel and a dead sensor both publish STALE, and CRITICAL +[bypasses debounce](../ros2_medkit_fault_manager/README.md), so an expected outage confirms a +CRITICAL fault on its first sample. Set `stale_severity`, or name the noisy sources with +`stale_severity_overrides`, and those statuses go through debounce like any other. + +## Evidence + +A FAILED report carries the status's key-values to the fault manager, which keeps them in the +fault's freeze frame under the reserved key `x-reported` and serves them back from +`GET /api/v1/apps/{app}/faults/{code}`: + +```jsonc +{ + "environment_data": { + "snapshots": [ + { + "type": "freeze_frame", + "name": "freeze_frame", + "data": "{\"x-reported\":{\"rejected_fixes\":\"37\",\"nis\":\"0.03\"}}" + } + ] + } +} +``` + +The numbers a publisher already computed to decide something was wrong are usually the whole +explanation of the fault, and before this they were dropped. The frame's other keys are topic +names sampled by the fault manager, which are always fully qualified and start with `/`, so a +reader can always tell a value the reporter asserted from one the fault manager sampled. + +`keyvalue_codes` still reads the same values to pick a fault code; the two uses are +independent and either can be used without the other. Evidence is bounded per fault code (32 +entries, 512 characters per value); entries past the bound are dropped with a throttled +warning rather than truncated, and the fault is recorded either way. ## Quick Start @@ -39,6 +75,8 @@ ros2 run ros2_medkit_diagnostic_bridge diagnostic_bridge_node | `max_tracked_sources` | integer | `512` | Maximum number of per-source FaultReporter instances retained by the bridge | | `name_to_code.` | string | - | Custom mapping from diagnostic name to fault code | | `keyvalue_codes` | string[] | - | List of keys used to search the diagnostic values for the fault code | +| `stale_severity` | string | `CRITICAL` | Severity a STALE status reports at. One of `INFO`, `WARN`, `ERROR`, `CRITICAL` | +| `stale_severity_overrides.` | string | - | Severity for STALE statuses whose name starts with ``. Longest matching prefix wins | ### Example Configuration diff --git a/src/ros2_medkit_diagnostic_bridge/config/diagnostic_bridge.yaml b/src/ros2_medkit_diagnostic_bridge/config/diagnostic_bridge.yaml index af88d6aaf..3247e5bcc 100644 --- a/src/ros2_medkit_diagnostic_bridge/config/diagnostic_bridge.yaml +++ b/src/ros2_medkit_diagnostic_bridge/config/diagnostic_bridge.yaml @@ -14,6 +14,19 @@ diagnostic_bridge: # Maximum number of per-source FaultReporter instances retained by the bridge. max_tracked_sources: 512 + # Severity a STALE status reports at. STALE is the one level a node can reach by + # design - a GPS in every tunnel, an IMU reporting covariance -1 - and CRITICAL + # bypasses debounce, so leaving it CRITICAL confirms a fault on the first sample. + # One of INFO, WARN, ERROR, CRITICAL. + stale_severity: "CRITICAL" + + # Per-diagnostic STALE severity. Matches on the start of the diagnostic name, so a + # component prefix covers every check it publishes. Longest matching prefix wins. + # Format: "stale_severity_overrides.": "" + # Example: + # "stale_severity_overrides.gps": "WARN" + # "stale_severity_overrides.gps: antenna": "ERROR" + # Custom diagnostic name to fault code mappings # Format: "name_to_code.": "" # Example: diff --git a/src/ros2_medkit_diagnostic_bridge/include/ros2_medkit_diagnostic_bridge/diagnostic_bridge_node.hpp b/src/ros2_medkit_diagnostic_bridge/include/ros2_medkit_diagnostic_bridge/diagnostic_bridge_node.hpp index 302be9c90..03fd4d4d3 100644 --- a/src/ros2_medkit_diagnostic_bridge/include/ros2_medkit_diagnostic_bridge/diagnostic_bridge_node.hpp +++ b/src/ros2_medkit_diagnostic_bridge/include/ros2_medkit_diagnostic_bridge/diagnostic_bridge_node.hpp @@ -26,6 +26,7 @@ #include "rclcpp/rclcpp.hpp" #include "diagnostic_msgs/msg/diagnostic_array.hpp" #include "ros2_medkit_fault_reporter/fault_reporter.hpp" +#include "ros2_medkit_msgs/msg/fault.hpp" namespace ros2_medkit_diagnostic_bridge { @@ -38,7 +39,11 @@ namespace ros2_medkit_diagnostic_bridge { /// - OK (0) -> PASSED event (healing) /// - WARN (1) -> WARN severity (1) /// - ERROR (2) -> ERROR severity (2) -/// - STALE (3) -> CRITICAL severity (3) +/// - STALE (3) -> stale_severity (CRITICAL by default, overridable per diagnostic name) +/// +/// STALE is configurable because a node can be STALE by design - a GPS in every tunnel, an +/// IMU reporting covariance -1 - and CRITICAL bypasses debounce, so an expected outage would +/// confirm a CRITICAL fault on its first sample. /// /// Example launch configuration: /// @code{.yaml} @@ -61,9 +66,20 @@ class DiagnosticBridgeNode : public rclcpp::Node { /// 4. Return empty string if no mapping found std::string map_to_fault_code(const diagnostic_msgs::msg::DiagnosticStatus & status) const; - /// Map DiagnosticStatus level to Fault severity - /// Returns std::nullopt if level is OK (should send PASSED instead) - static std::optional map_to_severity(uint8_t diagnostic_level); + /// Map DiagnosticStatus level to Fault severity for a named diagnostic. + /// Returns std::nullopt if level is OK (should send PASSED instead). + /// STALE resolves through stale_severity and stale_severity_overrides; every other + /// level maps fixed, so only the level that can be a design decision is configurable. + std::optional map_to_severity(uint8_t diagnostic_level, const std::string & diagnostic_name) const; + + /// Severity a STALE status from @p diagnostic_name reports at. + /// Longest matching prefix among stale_severity_overrides wins; stale_severity otherwise. + uint8_t stale_severity_for(const std::string & diagnostic_name) const; + + /// Parse a severity name ("INFO", "WARN", "ERROR", "CRITICAL", case-insensitive). + /// Returns std::nullopt for anything else, so a caller can report the typo rather than + /// silently substituting a severity nobody configured. + static std::optional parse_severity_name(const std::string & name); /// Check if diagnostic level indicates OK status static bool is_ok_level(uint8_t diagnostic_level); @@ -110,6 +126,17 @@ class DiagnosticBridgeNode : public rclcpp::Node { int max_tracked_sources_{512}; std::map name_to_code_; std::vector keyvalue_codes_; + + /// Severity a STALE status reports at when no override matches. + uint8_t stale_severity_{ros2_medkit_msgs::msg::Fault::SEVERITY_CRITICAL}; + + /// Per-diagnostic STALE severity, sorted by prefix length descending so the first + /// match found is the longest one. + struct StaleSeverityOverride { + std::string prefix; + uint8_t severity; + }; + std::vector stale_severity_overrides_; }; } // namespace ros2_medkit_diagnostic_bridge diff --git a/src/ros2_medkit_diagnostic_bridge/src/diagnostic_bridge_node.cpp b/src/ros2_medkit_diagnostic_bridge/src/diagnostic_bridge_node.cpp index 9bc3d87d2..a0b58bd61 100644 --- a/src/ros2_medkit_diagnostic_bridge/src/diagnostic_bridge_node.cpp +++ b/src/ros2_medkit_diagnostic_bridge/src/diagnostic_bridge_node.cpp @@ -58,18 +58,56 @@ void DiagnosticBridgeNode::load_parameters() { return !s.empty(); }); + // STALE is the one level whose severity is a deployment decision rather than a fact: + // a GPS in a tunnel and a dead sensor both publish STALE, and only the operator knows + // which is which. Default CRITICAL keeps today's behaviour for anyone not configuring it. + const std::string stale_severity_name = declare_parameter("stale_severity", "CRITICAL"); + if (auto parsed = parse_severity_name(stale_severity_name)) { + stale_severity_ = *parsed; + } else { + RCLCPP_WARN(get_logger(), + "stale_severity '%s' is not a severity name; using CRITICAL. " + "Expected one of INFO, WARN, ERROR, CRITICAL.", + stale_severity_name.c_str()); + } + // Load custom name_to_code mappings from parameter overrides // Format: name_to_code. = // Example: --ros-args -p "name_to_code.motor_temp:=MOTOR_OVERHEAT" auto params = get_node_parameters_interface()->get_parameter_overrides(); const std::string prefix = "name_to_code."; + const std::string stale_prefix = "stale_severity_overrides."; for (const auto & [name, value] : params) { if (name.rfind(prefix, 0) == 0 && value.get_type() == rclcpp::ParameterType::PARAMETER_STRING) { std::string diag_name = name.substr(prefix.length()); name_to_code_[diag_name] = value.get(); RCLCPP_DEBUG(get_logger(), "Loaded mapping: '%s' -> '%s'", diag_name.c_str(), name_to_code_[diag_name].c_str()); + continue; + } + + // Format: stale_severity_overrides. = + if (name.rfind(stale_prefix, 0) == 0 && value.get_type() == rclcpp::ParameterType::PARAMETER_STRING) { + const std::string diag_prefix = name.substr(stale_prefix.length()); + const std::string severity_name = value.get(); + auto parsed = parse_severity_name(severity_name); + if (!parsed) { + // Skipped, not defaulted: an operator who wrote a typo asked for something specific, + // and quietly applying CRITICAL would confirm the fault they were trying to debounce. + RCLCPP_WARN(get_logger(), + "stale_severity_overrides.%s = '%s' is not a severity name; ignoring this override. " + "Expected one of INFO, WARN, ERROR, CRITICAL.", + diag_prefix.c_str(), severity_name.c_str()); + continue; + } + stale_severity_overrides_.push_back(StaleSeverityOverride{diag_prefix, *parsed}); } } + + // Longest prefix first, so the first match found is the most specific one. + std::sort(stale_severity_overrides_.begin(), stale_severity_overrides_.end(), + [](const StaleSeverityOverride & a, const StaleSeverityOverride & b) { + return a.prefix.size() > b.prefix.size(); + }); } void DiagnosticBridgeNode::diagnostics_callback(const diagnostic_msgs::msg::DiagnosticArray::ConstSharedPtr & msg) { @@ -147,9 +185,14 @@ void DiagnosticBridgeNode::process_diagnostic(const diagnostic_msgs::msg::Diagno RCLCPP_DEBUG(get_logger(), "Diagnostic OK: %s -> PASSED for %s", status.name.c_str(), fault_code.c_str()); } else { // WARN, ERROR, STALE -> send FAILED event - auto severity = map_to_severity(status.level); + auto severity = map_to_severity(status.level, status.name); // severity is guaranteed to have value here (not OK level) - reporter->report(fault_code, *severity, status.message); + // + // The key-values travel with the report as evidence. They are the numbers the publisher + // already computed to decide something was wrong - outlier counts, gate statistics - and + // without them the fault record says a node complained but not what it saw. keyvalue_codes + // still reads the same values to pick the code; the two uses are independent. + reporter->report(fault_code, *severity, status.message, status.values); RCLCPP_DEBUG(get_logger(), "Diagnostic %s: %s -> fault %s (severity=%d)", status.name.c_str(), status.message.c_str(), fault_code.c_str(), *severity); } @@ -187,7 +230,8 @@ std::string DiagnosticBridgeNode::map_to_fault_code(const diagnostic_msgs::msg:: return ""; } -std::optional DiagnosticBridgeNode::map_to_severity(uint8_t diagnostic_level) { +std::optional DiagnosticBridgeNode::map_to_severity(uint8_t diagnostic_level, + const std::string & diagnostic_name) const { using DiagStatus = diagnostic_msgs::msg::DiagnosticStatus; using Fault = ros2_medkit_msgs::msg::Fault; @@ -199,12 +243,46 @@ std::optional DiagnosticBridgeNode::map_to_severity(uint8_t diagnostic_ case DiagStatus::ERROR: return Fault::SEVERITY_ERROR; case DiagStatus::STALE: - return Fault::SEVERITY_CRITICAL; + return stale_severity_for(diagnostic_name); default: return Fault::SEVERITY_ERROR; // Unknown level -> ERROR } } +uint8_t DiagnosticBridgeNode::stale_severity_for(const std::string & diagnostic_name) const { + // Entries are sorted longest-prefix-first, so the first hit is the most specific. + for (const auto & entry : stale_severity_overrides_) { + if (diagnostic_name.rfind(entry.prefix, 0) == 0) { + return entry.severity; + } + } + return stale_severity_; +} + +std::optional DiagnosticBridgeNode::parse_severity_name(const std::string & name) { + using Fault = ros2_medkit_msgs::msg::Fault; + + std::string upper; + upper.reserve(name.size()); + for (char c : name) { + upper += static_cast(std::toupper(static_cast(c))); + } + + if (upper == "INFO") { + return Fault::SEVERITY_INFO; + } + if (upper == "WARN") { + return Fault::SEVERITY_WARN; + } + if (upper == "ERROR") { + return Fault::SEVERITY_ERROR; + } + if (upper == "CRITICAL") { + return Fault::SEVERITY_CRITICAL; + } + return std::nullopt; +} + bool DiagnosticBridgeNode::is_ok_level(uint8_t diagnostic_level) { return diagnostic_level == diagnostic_msgs::msg::DiagnosticStatus::OK; } diff --git a/src/ros2_medkit_diagnostic_bridge/test/test_diagnostic_bridge.cpp b/src/ros2_medkit_diagnostic_bridge/test/test_diagnostic_bridge.cpp index 8dc16554f..a68aaf962 100644 --- a/src/ros2_medkit_diagnostic_bridge/test/test_diagnostic_bridge.cpp +++ b/src/ros2_medkit_diagnostic_bridge/test/test_diagnostic_bridge.cpp @@ -55,36 +55,53 @@ std::shared_ptr make_node_with_keyvalue_codes(std::vector< options.append_parameter_override("keyvalue_codes", keyvalue_codes); return std::make_shared(options); } + +/// A bridge with no configuration beyond the defaults. Severity mapping is a method +/// rather than a free function now that STALE is configurable, so the cases that used +/// to call it statically need an instance to call it on. +std::shared_ptr make_default_node() { + return std::make_shared(rclcpp::NodeOptions()); +} + +std::shared_ptr +make_node_with_params(const std::vector> & overrides) { + rclcpp::NodeOptions options; + for (const auto & [name, value] : overrides) { + options.append_parameter_override(name, value); + } + return std::make_shared(options); +} } // namespace // Test severity mapping TEST_F(DiagnosticBridgeTest, MapToSeverity_Warn) { - auto result = DiagnosticBridgeNode::map_to_severity(DiagStatus::WARN); + auto result = make_default_node()->map_to_severity(DiagStatus::WARN, "any"); ASSERT_TRUE(result.has_value()); EXPECT_EQ(*result, Fault::SEVERITY_WARN); } TEST_F(DiagnosticBridgeTest, MapToSeverity_Error) { - auto result = DiagnosticBridgeNode::map_to_severity(DiagStatus::ERROR); + auto result = make_default_node()->map_to_severity(DiagStatus::ERROR, "any"); ASSERT_TRUE(result.has_value()); EXPECT_EQ(*result, Fault::SEVERITY_ERROR); } TEST_F(DiagnosticBridgeTest, MapToSeverity_Stale) { - auto result = DiagnosticBridgeNode::map_to_severity(DiagStatus::STALE); + // Unconfigured, STALE still maps to CRITICAL: the change is opt-in. + auto result = make_default_node()->map_to_severity(DiagStatus::STALE, "any"); ASSERT_TRUE(result.has_value()); EXPECT_EQ(*result, Fault::SEVERITY_CRITICAL); } TEST_F(DiagnosticBridgeTest, MapToSeverity_Ok) { // OK should return nullopt (use is_ok_level and send PASSED instead) - auto result = DiagnosticBridgeNode::map_to_severity(DiagStatus::OK); + auto result = make_default_node()->map_to_severity(DiagStatus::OK, "any"); EXPECT_FALSE(result.has_value()); } TEST_F(DiagnosticBridgeTest, MapToSeverity_Unknown) { // Unknown level defaults to ERROR - auto result = DiagnosticBridgeNode::map_to_severity(99); + auto result = make_default_node()->map_to_severity(99, "any"); ASSERT_TRUE(result.has_value()); EXPECT_EQ(*result, Fault::SEVERITY_ERROR); } @@ -238,6 +255,99 @@ TEST_F(DiagnosticBridgeTest, MapToFaultCode_NameToCodeOverride_PrecedesKeyValueC EXPECT_EQ(node->map_to_fault_code(diagnostic_status("/motor", 1, "", {{"code", "FROM_ATTRIBUTE"}})), "OVERRIDE_CODE"); } +// --------------------------------------------------------------------------- +// STALE severity: the level a node can be by design +// --------------------------------------------------------------------------- + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_DefaultIsCriticalForEveryName) { + auto node = make_default_node(); + EXPECT_EQ(node->stale_severity_for("gps"), Fault::SEVERITY_CRITICAL); + EXPECT_EQ(node->stale_severity_for("anything else"), Fault::SEVERITY_CRITICAL); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_GlobalParameterAppliesToAll) { + auto node = make_node_with_params({{"stale_severity", "WARN"}}); + EXPECT_EQ(node->map_to_severity(DiagStatus::STALE, "gps").value(), Fault::SEVERITY_WARN); + EXPECT_EQ(node->map_to_severity(DiagStatus::STALE, "imu").value(), Fault::SEVERITY_WARN); + // Only STALE moves; the levels that are facts rather than decisions stay put. + EXPECT_EQ(node->map_to_severity(DiagStatus::ERROR, "gps").value(), Fault::SEVERITY_ERROR); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_OverrideBeatsGlobalDefault) { + auto node = make_node_with_params({{"stale_severity_overrides.gps", "WARN"}}); + EXPECT_EQ(node->map_to_severity(DiagStatus::STALE, "gps").value(), Fault::SEVERITY_WARN); + // The issue's own acceptance criterion: without an override it stays CRITICAL. + EXPECT_EQ(node->map_to_severity(DiagStatus::STALE, "lidar").value(), Fault::SEVERITY_CRITICAL); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_OverrideMatchesOnPrefix) { + auto node = make_node_with_params({{"stale_severity_overrides.gps", "WARN"}}); + // Diagnostic names are conventionally ": ", so a prefix is how an + // operator names every check a component publishes without listing them. + EXPECT_EQ(node->stale_severity_for("gps: fix quality"), Fault::SEVERITY_WARN); + EXPECT_EQ(node->stale_severity_for("gpsd"), Fault::SEVERITY_WARN); + EXPECT_EQ(node->stale_severity_for("imu: covariance"), Fault::SEVERITY_CRITICAL); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_LongestPrefixWins) { + auto node = make_node_with_params({ + {"stale_severity_overrides.gps", "WARN"}, + {"stale_severity_overrides.gps: antenna", "ERROR"}, + }); + EXPECT_EQ(node->stale_severity_for("gps: antenna shorted"), Fault::SEVERITY_ERROR); + EXPECT_EQ(node->stale_severity_for("gps: fix quality"), Fault::SEVERITY_WARN); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_OverrideCombinesWithNonCriticalDefault) { + auto node = make_node_with_params({ + {"stale_severity", "WARN"}, + {"stale_severity_overrides.safety_chain", "CRITICAL"}, + }); + // A deployment that treats STALE as routine still needs the one sensor where it is not. + EXPECT_EQ(node->stale_severity_for("safety_chain: estop"), Fault::SEVERITY_CRITICAL); + EXPECT_EQ(node->stale_severity_for("gps"), Fault::SEVERITY_WARN); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_UnparseableGlobalFallsBackToCritical) { + auto node = make_node_with_params({{"stale_severity", "not-a-severity"}}); + EXPECT_EQ(node->stale_severity_for("gps"), Fault::SEVERITY_CRITICAL); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_UnparseableOverrideIsIgnoredNotDefaulted) { + auto node = make_node_with_params({ + {"stale_severity", "WARN"}, + {"stale_severity_overrides.gps", "WHOOPS"}, + }); + // The typo must not be read as CRITICAL: an operator debouncing a noisy GPS would get + // the immediate-confirm behaviour they were configuring their way out of. + EXPECT_EQ(node->stale_severity_for("gps"), Fault::SEVERITY_WARN); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, StaleSeverity_NamesAreCaseInsensitive) { + auto node = make_node_with_params({{"stale_severity", "warn"}}); + EXPECT_EQ(node->stale_severity_for("gps"), Fault::SEVERITY_WARN); +} + +// @verifies REQ_INTEROP_109 +TEST_F(DiagnosticBridgeTest, ParseSeverityName_AcceptsTheFourNamesAndNothingElse) { + EXPECT_EQ(DiagnosticBridgeNode::parse_severity_name("INFO").value(), Fault::SEVERITY_INFO); + EXPECT_EQ(DiagnosticBridgeNode::parse_severity_name("WARN").value(), Fault::SEVERITY_WARN); + EXPECT_EQ(DiagnosticBridgeNode::parse_severity_name("ERROR").value(), Fault::SEVERITY_ERROR); + EXPECT_EQ(DiagnosticBridgeNode::parse_severity_name("CRITICAL").value(), Fault::SEVERITY_CRITICAL); + EXPECT_FALSE(DiagnosticBridgeNode::parse_severity_name("").has_value()); + EXPECT_FALSE(DiagnosticBridgeNode::parse_severity_name("2").has_value()); + EXPECT_FALSE(DiagnosticBridgeNode::parse_severity_name("FATAL").has_value()); +} + int main(int argc, char ** argv) { testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/ros2_medkit_fault_manager/CMakeLists.txt b/src/ros2_medkit_fault_manager/CMakeLists.txt index 064cf44f1..744623168 100644 --- a/src/ros2_medkit_fault_manager/CMakeLists.txt +++ b/src/ros2_medkit_fault_manager/CMakeLists.txt @@ -168,6 +168,12 @@ if(BUILD_TESTING) medkit_add_gtest(test_correlation_engine test/test_correlation_engine.cpp) target_link_libraries(test_correlation_engine fault_manager_lib) + # Reporter-supplied evidence: the merge, its bounds, and the capture path that + # must not erase it. + medkit_add_gtest(test_reported_evidence test/test_reported_evidence.cpp) + target_link_libraries(test_reported_evidence fault_manager_lib) + medkit_target_dependencies(test_reported_evidence rclcpp ros2_medkit_msgs) + # Entity threshold resolver tests medkit_add_gtest(test_entity_thresholds test/test_entity_thresholds.cpp) target_link_libraries(test_entity_thresholds fault_manager_lib) @@ -189,6 +195,9 @@ if(BUILD_TESTING) medkit_add_launch_test(test_rosbag_default_format test/integration/test_rosbag_default_format.test.py TIMEOUT 90 LABELS "integration") + medkit_add_launch_test(test_reported_evidence_integration + test/integration/test_reported_evidence.test.py TIMEOUT 60 LABELS "integration") + medkit_add_launch_test(test_entity_thresholds_integration test/integration/test_entity_thresholds_integration.test.py TIMEOUT 60 LABELS "integration") diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp index 9157102d9..37f65a7b1 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp @@ -20,8 +20,11 @@ #include #include #include +#include +#include #include "rclcpp/rclcpp.hpp" +#include "diagnostic_msgs/msg/key_value.hpp" #include "ros2_medkit_fault_manager/capture_thread_pool.hpp" #include "ros2_medkit_fault_manager/correlation/correlation_engine.hpp" #include "ros2_medkit_fault_manager/entity_threshold_resolver.hpp" @@ -191,6 +194,19 @@ class FaultManagerNode : public rclcpp::Node { /// Falls back to global config if no entity-specific overrides match. DebounceConfig resolve_config(const std::string & source_id) const; + /// Merge reporter-supplied evidence into a fault's freeze frame. + /// + /// Called for every FAILED report that carries any. Bounded per fault code; entries the + /// bounds reject are dropped and logged rather than truncated. The frame is read, merged + /// and written back, which is safe because the node's single-threaded executor serialises + /// this with every other callback that touches storage. + /// @param fault_code The code just reported. + /// @param evidence Key-value measurements from the report. + /// @param event_time Wall-clock time of the report. + void store_reported_evidence(const std::string & fault_code, + const std::vector & evidence, + const rclcpp::Time & event_time); + /// Create the tamper-evident audit log from parameters (nullptr if disabled). std::unique_ptr create_audit_log(); diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp index 3bc60dc35..07cce041b 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "rclcpp/rclcpp.hpp" @@ -137,6 +138,44 @@ struct FreezeFrameData { int64_t captured_at_ns{0}; }; +/// Key inside the freeze-frame JSON object under which reporter-supplied evidence is kept. +/// +/// The frame's other keys are ROS topic names, which are always fully qualified and so +/// always start with '/'. This name cannot collide with one, which is what lets a reader +/// tell a value the reporter asserted from one the fault manager sampled off a topic. +inline constexpr const char * kReportedEvidenceKey = "x-reported"; + +/// Most evidence entries kept for one fault code. A reporter publishing at 10 Hz must not +/// be able to grow the fault store without limit, and a fault explained by more than this +/// many numbers is not explained by them. +inline constexpr size_t kMaxEvidenceEntries = 32; + +/// Longest evidence value kept, in characters. Longer values are dropped whole rather than +/// truncated: half a number read back later is worse than a logged absence. +inline constexpr size_t kMaxEvidenceValueChars = 512; + +/// Merge reporter-supplied evidence into a freeze-frame JSON document. +/// +/// @param frame_json Existing frame ("" or invalid JSON is treated as an empty object). +/// @param evidence Key-value pairs from the report, in arrival order. +/// @param dropped Set to the number of entries rejected by the bounds above. +/// @return The frame with the evidence merged under kReportedEvidenceKey. Later reports +/// update the keys they name and leave the rest, so a fault accumulates what its +/// sources said rather than keeping only the last report's view. +std::string merge_reported_evidence(const std::string & frame_json, + const std::vector> & evidence, + size_t & dropped); + +/// Carry reporter evidence from an existing frame into a newly built one. +/// +/// A confirmation capture rebuilds the frame from the topics it sampled and would otherwise +/// drop evidence reported before it. Ordering decides which of the two writes lands last, so +/// the capture path must preserve rather than replace. +std::string preserve_reported_evidence(const std::string & frame_json, const std::string & previous_json); + +/// Whether a freeze-frame document carries reporter evidence. +bool has_reported_evidence(const std::string & frame_json); + /// Derive a recording's public identity from its bag path: the directory basename, /// `fault__`. Faults of one burst share a recording and therefore share /// this id. diff --git a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp index 146f0e3b4..20827e4a4 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -818,6 +818,14 @@ void FaultManagerNode::handle_report_fault( response->accepted = true; + // Keep the numbers the reporter had when it decided the condition held. Written on every + // FAILED report, not only on the one that confirms: a fault that never confirms still owes + // its near-miss series an explanation, and a reporter that stops publishing after the first + // report would otherwise leave nothing behind. + if (request->event_type == ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED && !request->evidence.empty()) { + store_reported_evidence(request->fault_code, request->evidence, event_time); + } + // Get updated fault state to publish event auto fault_after = storage_->get_fault(request->fault_code); if (fault_after) { @@ -1135,8 +1143,14 @@ void FaultManagerNode::handle_get_fault(const std::shared_ptr