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