template node type for rclcpp action server and clients

[SteveMacenski]

Addresses #846 and has the rclcpp::Node as the default template argument so it shouldn't break users of it.

I'd add a second test to work with lifecycle nodes, but I didn't want to add lifecycle as a dependency to actions.

[Karsten1987]

Why not templating it to any pointer type? There is a type trait within rcpputils which could be used for a static assert.
But I believe that forcing a shared pointer would still not work to address the shared_from_this() problem within the constructor when used within a class inheriting from rclcpp::Node. Whereas a raw pointer to this would.
I might be wrong though.

EDIT: Also I feel the tests should be enhanced to actually create a actionclient/server from a node constructor.

[SteveMacenski]

@Karsten1987 looking across the repo, all instances I can find of node and message type templating are all base types that are later called with T::SharedPtr. I don't see why actions should differ. If this one was templated as a pointer, we'd have to template them all that way for consistency.

A few examples:
https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/create_service.hpp#L33

rclcpp/rclcpp_components/include/rclcpp_components/node_factory_template.hpp

Line 45 in af9ae4a

auto node = std::make_shared<NodeT>(options);

rclcpp/rclcpp/include/rclcpp/executor.hpp

Line 169 in dfb144d

template<typename NodeT = rclcpp::Node, typename RepT = int64_t, typename T = std::milli>

Similarly, I'm not seeing anywhere in here doing a static_assert checking if ptr for creating comms, create_*, it seems inconsistent to do that here and no where else. If you try to give it a non-shared ptr it'll fail to compile so I'm not really afraid of asserting here.

Similar statements for the shared_from_this() - that seems out of scope and the design of a node can be simply done to avoid creating in the constructor, the same way you'd have to for any other overload using the shared ptr.

I'm trying to resolve the missing feature of rclcpp_action not being able to take in various node types to make navigation2 code cleaner. Looking back on the ticket, it looks like they wanted something else - removing the addressing in the PR description

[jacobperron]

+1 for a more generic template type. E.g. similar to rclcpp publishers and subscriptions:

rclcpp/rclcpp/include/rclcpp/create_publisher.hpp

Lines 37 to 50 in 8e69b7d

	template<
	typename MessageT,
	typename AllocatorT = std::allocator<void>,
	typename PublisherT = rclcpp::Publisher<MessageT, AllocatorT>,
	typename NodeT>
	std::shared_ptr<PublisherT>
	create_publisher(
	NodeT & node,
	const std::string & topic_name,
	const rclcpp::QoS & qos,
	const rclcpp::PublisherOptionsWithAllocator<AllocatorT> & options = (
	rclcpp::PublisherOptionsWithAllocator<AllocatorT>()
	)
	)

Which passes the node to get_node_topics_interfaces() (which has templates for pointer and reference types):

rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_topics_interface.hpp

Lines 120 to 124 in 8e69b7d

	/// Get the NodeTopicsInterface as a pointer from a pointer to a "Node like" object.
	template<
	typename NodeType,
	typename std::enable_if<std::is_pointer<NodeType>::value, int>::type = 0
	>

rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_topics_interface.hpp

Lines 133 to 138 in 8e69b7d

	template<
	typename NodeType,
	typename std::enable_if<
	!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
	>::type = 0
	>

Assuming the proper template functions already exist for the various interfaces, we should be able to make the change:

-  rclcpp::Node::SharedPtr node,
+  NodeT node

And you can default NodeT to rclcpp::Node::SharedPtr.

I haven't tried it, but maybe we have to use free functions for getting the interfaces instead of the node methods (edit: I think we certainly will if we also want to support passing nodes by reference), e.g.

get_node_base_interface(node);

instead of

node->get_node_base_interface();

[SteveMacenski]

@jacobperron I've read that a few times and I'm not entirely certain if you're suggesting a change or expressing a line of thought. Is there something specific you'd like me to do? I'm hearing maybe change from node->get_xyz() to get_xyz(node) and templating in the ptr type.

I intentionally didn't template the SharedPtr because it seemed more readable from user code to do rclcpp_action::create_client<rclcpp_lifecycle::LifecycleNode> than rclcpp_action::create_client<rclcpp_lifecycle::LifecycleNode::SharedPtr> but I suppose I don't have a strong opinion beyond just convention I generally follow.

[jacobperron]

I'm not entirely certain if you're suggesting a change or expressing a line of thought. Is there something specific you'd like me to do?

Sorry, I guess it was a bit of both. I'm no wizard with templates, but I would expect the following definition to work not only for shared pointers, but a variety of pointer and reference types for node-like objects (ie. objects that implement the required node interfaces):

template<typename ActionT, typename NodeT>
typename Client<ActionT>::SharedPtr
create_client(
  NodeT node,
  const std::string & name,
  rclcpp::callback_group::CallbackGroup::SharedPtr group = nullptr)
{
  return create_client<ActionT>(
    rclcpp::node_interfaces::get_node_base_interface(node),
    rclcpp::node_interfaces::get_node_graph_interface(node),
    rclcpp::node_interfaces::get_node_logging_interface(node),
    rclcpp::node_interfaces::get_node_waitables_interface(node),
    name,
    group);
}

Then, I think template argument deduction will make it so we don't need to worry about specifying the template type. E.g.

// 'node' is a node-like thing that could be a pointer or passed in by reference
auto client = rclcpp_action::create_client<MyAction>(node, "action_name");

I'd like to see the create_client and create_server functions updated to look like what I've got above. I haven't tested it, so I'm not sure if it works out-of-the-box with lifecycle nodes.

[jacobperron]

I just realized that to do what I proposed requires the existence of the following functions:

• get_node_graph_interface
• get_node_logging_interface
• get_node_waitables_interface

I guess we could add those missing free functions.

[SteveMacenski]

I guess we could add those missing free functions.

I leave it up to you, if you feel that's necessary to merge this I can take a crack at it. I wanted to get this in before the Friday freeze so I scoped it pretty low. I can file a ticket to implement the other free methods and revisit this topic as well at a later time. I am also not a templating wizard, but I'm willing to try to become one ;-)

[SteveMacenski]

Alright cool sounds like I have the concrete action items 👍

[SteveMacenski]

@jacobperron changes made. Everything seems to be fine. CI failures unreleased to this

[jacobperron]

LGTM, just one last nitpick.

[SteveMacenski]

@jacobperron 👍

Real stickler. I like it. This should be squashed now with these many small commits

[jacobperron]

@SteveMacenski Thanks for iterating!

I think the PR CI jobs will continue to fail until the latest changes in rclcpp are released into Eloquent.
I'll trigger CI manually:

• Linux
• Linux-aarch64
• macOS
• Windows

Note, I expect some pub/sub communication tests to fail due to a known issue.

[jacobperron]

This should be squashed now with these many small commits

We typically do a squash merge 👍