Lifecycle Construction for Fault Reporter - #556
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends ros2_medkit_fault_reporter::FaultReporter construction to support rclcpp_lifecycle::LifecycleNode (and additional node-handle forms) by introducing an interface-based “base” constructor that other constructors delegate to, making lifecycle-node initialization possible without relying on shared_from_this().
Changes:
- Added an interface-based
FaultReporterconstructor and delegating overloads forrclcpp::Node/rclcpp_lifecycle::LifecycleNode. - Switched parameter loading to use
NodeParametersInterfaceand created the service client viarclcpp::create_client(...)with node interfaces. - Updated package dependencies and end-user docs (README + changelog) to reflect lifecycle support.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_fault_reporter/src/fault_reporter.cpp | Adds interface-based construction path and new delegating constructors; updates parameter loading and client creation. |
| src/ros2_medkit_fault_reporter/include/ros2_medkit_fault_reporter/fault_reporter.hpp | Exposes new public constructors and updates the parameter-loading helper signature and examples. |
| src/ros2_medkit_fault_reporter/README.md | Documents lifecycle-node usage and enumerates available constructors. |
| src/ros2_medkit_fault_reporter/package.xml | Adds rclcpp_lifecycle dependency. |
| src/ros2_medkit_fault_reporter/CMakeLists.txt | Finds/links/exports rclcpp_lifecycle for the library and tests. |
| src/ros2_medkit_fault_reporter/CHANGELOG.rst | Notes lifecycle-node construction support for the upcoming release. |
bburda
left a comment
There was a problem hiding this comment.
Thanks, and good that you wrote the issue with the design first 👍 The shape is right - base ctor with the small delegating overloads, old shared_from_this() callers still work, params and QoS unchanged.
Two things to fix before it can go in:
- Doesn't build on Lyrical. See the create_client note.
- No test actually builds a FaultReporter - test_local_filter only covers LocalFilter and the integration test calls the service directly. Add one that constructs it from a LifecycleNode and a Node&.
Smaller stuff:
- If you see a suggestion to make the ref ctors const (const Node& / const LifecycleNode&), skip it - get_node_*_interface() are non-const so it won't compile. What you have is right.
- The header pulls in rclcpp_lifecycle/lifecycle_node.hpp. The ctor takes the node by reference, so you can forward-declare it and move the include to the .cpp - then plain Node users don't get the lifecycle header. Optional.
- Node has a ref and a SharedPtr ctor, LifecycleNode only a ref one. A LifecycleNode::SharedPtr overload would round it out so people don't pass *ptr. Optional.
- Minor: this create_client skips the sub-namespace expansion node->create_client did. Only matters for a sub-node with a relative service name - default name is absolute so normal use is fine.
…nd optional comments
Pull Request
Summary
Add additional ways to construct fault reporter allowing for LifecycleNodes and standard nodes as well by leveraging a core base interfaces constructor that other node type constructors can use!
Issue
Link the related issue (required):
Type
Testing
Can be verified by constructing FaultReporter via a lifecycle node
Checklist