Pass diagnostic hardware_id as the source ID in the diagnostic bridge - #622
Pass diagnostic hardware_id as the source ID in the diagnostic bridge#622nnarain wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates ros2_medkit_diagnostic_bridge so faults reported from /diagnostics are attributed to the diagnostic message’s hardware_id (as the source_id), enabling faults to appear under the intended entity rather than always under /diagnostic_bridge.
Changes:
- Create/lookup a
FaultReporterper distinctsource_idderived fromDiagnosticStatus.hardware_id, with fallback to the bridge node FQN whenhardware_idis empty. - Extend the diagnostic bridge integration test harness to publish diagnostics with configurable
hardware_id. - Add integration tests validating (1)
hardware_idis propagated intoreporting_sourcesand (2) emptyhardware_idfalls back to the bridge node source.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ros2_medkit_diagnostic_bridge/test/test_integration.test.py | Adds tests and helper changes to publish diagnostics with varying hardware_id and verify reporting_sources behavior. |
| src/ros2_medkit_diagnostic_bridge/src/diagnostic_bridge_node.cpp | Introduces per-source FaultReporter selection and hardware_id→source_id resolution with fallback/warning. |
| src/ros2_medkit_diagnostic_bridge/include/ros2_medkit_diagnostic_bridge/diagnostic_bridge_node.hpp | Adds new private helpers and replaces single reporter state with a map of reporters keyed by source_id. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // parameter declaration with has_parameter(). | ||
| auto reporter = std::make_unique<ros2_medkit_fault_reporter::FaultReporter>(this->shared_from_this(), source_id); | ||
| auto * raw = reporter.get(); | ||
| reporters_[source_id] = std::move(reporter); | ||
| return raw; |
mfaferek93
left a comment
mfaferek93
left a comment
There was a problem hiding this comment.
Good direction! Everything landing under /diagnostic_bridge is a real problem, and source attribution is the right fix.
Two small things first: the reporter map needs the log bridge's cap, and the reporter should be created after the mapping check.
The bigger question is the hardware_id assumption you already noted. diagnostic_updater usually puts a serial number or none there, not a node name.
When that happens the fault gets a source that matches no entity. It then disappears from every per-entity faults list and stays only in the global /faults, which has no entity_id or href to click through. Right now those faults at least land under /diagnostic_bridge, which is a real entity. So on many systems this would swap "shown in the wrong place" for "not shown anywhere".
The manifest mapping you suggested solves that. Until it exists, it may be safer to keep the current behaviour by default and put the hardware_id path behind a parameter, so nobody loses faults after an upgrade.
This part is hard to see from the bridge as the scope rule lives in the gateway, far from what you were touching 😅
|
Ya I'll put this feature behind a flag |
| } | ||
|
|
||
| std::string DiagnosticBridgeNode::source_id_for(const diagnostic_msgs::msg::DiagnosticStatus & status) const { | ||
| if (use_hardware_id_as_source_id_ && !status.hardware_id.empty() && status.hardware_id.find('/') != std::string::npos) { |
There was a problem hiding this comment.
Only thing left red in CI. clang-format wants a wrap here.
| rclcpp::Clock clock(*get_clock()); | ||
| RCLCPP_WARN_THROTTLE( | ||
| get_logger(), clock, 10000, | ||
| "Diagnostic '%s' hardware_id '%s' is not a slash-containing node ID or attribution is disabled, using bridge " | ||
| "source_id '%s'", | ||
| status.name.c_str(), status.hardware_id.c_str(), fqn.c_str()); |
There was a problem hiding this comment.
With the flag off, which is the default, the if above is always false, so every diagnostic reaches this warning. People who upgrade and change nothing get a warning every 10 seconds about a feature they never turned on. When the flag is on it is worth one line, and then you don't need the clock copy at all :)
Pull Request
Summary
Following the same pattern as in the log bridge. I'm setting up multiple fault reporters in the diagnostic bridge based on the hardware_id in the diagnostic message.
This does assume the
hardware_idis the fully qualified node name, in order for it to be mapped in to the correct entity. But it think it's a reasonable step forward.Additional things might be:
Issue
Link the related issue (required):
Type
Testing
I added two tests. And tested it locally using the web ui.
Checklist