Use lifecycle node as base for controllers #244
Conversation
|
So again most of this should be done, but auto declare doesn't seem to work as intended wit lifecycle_nodes as it is with rclcpp::Node. |
destogl
force-pushed
the
pr-lifecycle_nodes
branch
from
c9c1f18 to
1d7fdd6
Compare
destogl
changed the title
|
depends on ros-controls/ros2_control#538 but otherwise LGTM |
github-actions
bot
requested review from
Briancbn,
arne48,
bijoua29,
bmagyar,
destogl,
harderthan and
homalozoa
| */ | ||
| SegmentTolerances get_segment_tolerances( | ||
| const rclcpp::Node & node, const std::vector<std::string> & joint_names) | ||
| const rclcpp_lifecycle::LifecycleNode & node, const std::vector<std::string> & joint_names) |
There was a problem hiding this comment.
We should probably here use SharedPtr to the node and not the object itself. This is usually done in the rest of the code-base.
There was a problem hiding this comment.
I would disagree.
If the function itself doesn't care about the lifetime of the object, then passing by reference should be preferred. Passing a shared pointer has performance costs in incrementing/decrementing the reference count.
See https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters for discussion on this topic.
| std::experimental::optional< | ||
| std::reference_wrapper<hardware_interface::LoanedCommandInterface>> /* joint_handle */, | ||
| const rclcpp::Node::SharedPtr & /* node */) | ||
| std::shared_ptr<rclcpp_lifecycle::LifecycleNode> & /* node */) |
There was a problem hiding this comment.
Could have used rclcpp_lifecycle::LifecycleNode::SharedPtr(pretty sure this is defined) as with rclcpp::Node but there is no difference.
There was a problem hiding this comment.
Would it be better to pass a reference instead of shard_ptr here too?
| */ | ||
| SegmentTolerances get_segment_tolerances( | ||
| const rclcpp::Node & node, const std::vector<std::string> & joint_names) | ||
| const rclcpp_lifecycle::LifecycleNode & node, const std::vector<std::string> & joint_names) |
There was a problem hiding this comment.
I would disagree.
If the function itself doesn't care about the lifetime of the object, then passing by reference should be preferred. Passing a shared pointer has performance costs in incrementing/decrementing the reference count.
See https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters for discussion on this topic.
|
There's a bunch of warnings reported due to the use of LifecyclePublishers, here's an example: |
github-actions
bot
requested review from
DasRoteSkelett,
aprotyas,
bijoua29,
bmagyar,
destogl,
duringhof and
progtologist
|
Several CI failures are caused by the lack of release of ros2_control which'd cause similar build issues on the buildfarm. After merging this I'll push a release of both to restore the CI results |
* Add lifecycle nodes * Fix compile issues * Fix trajectory controller * pre-commit fixes * Add time and period to update function * Add lifecycle nodes * Name changes * Final fixes to get things compiled. * Fixing typo in the tests. * More changes to LifecycleNodes. Co-authored-by: Bence Magyar <[email protected]> Co-authored-by: Denis Štogl <[email protected]>
7 participants
Adds the lifecycle nodes back, that are removed in #124. Depends on ros-controls/ros2_control#538 and #241, since ros-controls/ros2_control#520 is merged but #241 hasn't.Depends on