Port moveit_cpp to ROS 2

[henningkayser]

No description provided.

[JafarAbdi]

Are you planning to port the test into a stand-alone PR .?

[mlautman]

Any reason to not use the templated approach that is popular in the ros2 code base?

template<class NodeT>
void load(NodeT && node){
    return load(node->get_node_parameters_interface())
}
void load(rclcpp::node_interfaces::NodeParametersInterface::SharedPtr& node_parameters_interface){
    ...
}

[henningkayser]

Right now this is just a migration step so I didn't consider it. We're planning to make a bigger refactoring for parameter loading that includes declaring all parameters that we use. However, I don't see a real upside to use the template version, why do you think we should use it?

[mlautman]

It would be nice to be able to use weak pointers but to be honest, I really don't see that as a strong enough use case for going back and templating everything

[henningkayser]

Ok, I see that it can make sense of you structure your library around the node components (which would actually be very nice) but introducing this in an existing library is probably not that easy. I would be interested in some discussions how we can improve node management in MoveIt for later milestones.

[henningkayser]

@mlautman I think you're right about using the node templates for end consumers, but here we are using and sharing (probably) all components of the node. We would have to start by refactoring node use in all used components and then work our way up until these base classes. A big downside of this approach would be that we would heavily restrict what we can do inside the member classes, so for now I wouldn't go this path.

[mlautman]

Something to consider: You may need to establish what time of clock is used by the tf_bufffer. Most every example I found in the ros2 code base uses auto clock = std::make_shared<rclcpp::Clock>(RCL_ROS_TIME)) to ensure that the TF buffer uses the correct clock

[henningkayser]

That's actually a good point. I wasn't aware that system time was the default in the clock constructor. However, nodes always use ROS time, so using Node::get_clock() and Node::now() is valid everywhere you want to use ROS time.

[mlautman]

Overall just a few nits. Good job!

[mlautman]

+1 feel free to ignore my comments about templates and node components in other PR's as well.

[henningkayser]

@mlautman I addressed all comments and rebased onto master

[nbbrooks]

minor

[davetcoleman]

I don't think I'm very qualified to give this ROS 2 stuff an approval, but since @nbbrooks and @mlautman approved.