Proposal for NodeHandle concept that simplifies calling APIs with node-like objects

[sloretz]

Feature request

Feature description

This is an idea to simplify code that calls APIs using node interfaces. It proposes adding template classes NodeHandle<> and NodeHandle<>::SharedPtr who are given placeholder types that indicate the needed node interfaces.

When node interfaces are just needed for the lifetime of the call

create_service() needs a NodeBaseInterface and NodeServicesInterface. It doesn't need shared pointers to these interfaces; it just needs them for the duration of the call.

Before:

create_service(
  std::shared_ptr<node_interfaces::NodeBaseInterface> node_base,
  std::shared_ptr<node_interfaces::NodeServicesInterface> node_services,
  // ...

// User calls with
auto service = create_service(
  my_node_class->get_node_base_interface(),
  my_node_class->get_node_services_interface(),
  // ...

after

create_service(
     rclcpp::NodeHandle<Base, Services> node_handle,
     // ...

// User calls with
auto service = create_service(
  my_node_class,
  // ...

When node interfaces are needed long term

tf2_ros::TransformListener needs the node base interface for a long time, so it holds a shared pointer.

It currently does some logic in the header file and some in the cpp file, but the intent of this proposal is to enable moving all of it to the cpp file.

Before

  TransformListener(
    tf2::BufferCore & buffer,
    NodeT && node,
    // ...
    // in header creates subscription and calls initThread(node->get_node_base_interface()); to store shared pointer to node

After

  TransformListener(
    tf2::BufferCore & buffer,
    rclcpp::NodeHandle<Base, Subscription>::SharedPtr node_handle,
    // ...
    );  // Just declaration in header

Implementation considerations

Placeholder types should be created for each node interface. The purpose of these is to make the type of a NodeHandle more readable. In my opinion rclcpp::NodeHandle<Base> is easier to read than rclcpp::NodeHandleBase<node_interfaces::NodeBaseInterface>.

The types passed to NodeHandle<> would indicate the interfaces needed by a method. rclcpp::NodeHandle<Base, Topics> would replace get_node_base_interface and get_node_topics_interface(). Using similar template magic, it would get the base and topics interface off of whatever it's given at compile time. Code would access the interfaces with node_handle.base() or node_handle.topics() and those would return raw pointers to the interfaces.

rclcpp::NodeHandle<Base, Topics>::SharedPtr is a little more interesting because get_node_*_interface() don't address this use case, and I've seen it a couple times while porting packages. Some API's need shared pointers to the node interfaces so they can keep them alive for a while. NodeHandle<>::SharedPtr should be a shared pointer to an object that holds shared pointers to the interfaces. Interfaces would be accessed via node_handle->base() and node_handle->topics(), but the return type would be a shared pointer instead of a raw pointer. Since the return type is different, this is probably a different class internally (NodeHandle<>::SharedPtr = std::shared_ptr<NodeHandleCopyable<>>).

[wjwwood]

In your example, does my_node_class's type inherit from rclcpp::NodeHandle<Base, Services>?

How is this different from std::pair<...Base..., ...Services...>?

How does rclcpp::NodeHandle<>::SharedPtr work? Does rclcpp::NodeHandle<> not also need to keep handles to the interface's shared pointers? In that case could you not just copy the instance of rclcpp::NodeHandle<> rather than putting it in a shared poitner?

What would happen if the interfaces were coming from two places? Like if instead of a node class which has a Base and Services interface inside it, I instead had just a MockBase and MockServices as separate objects? Can a rclcpp::NodeHandle<Base, Services> be created from parts?

How does this overlap with or work along with these existing helper functions:

• rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp

  Lines 120 to 144 in 64c0f86

  	/// Get the NodeBaseInterface 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::node_interfaces::NodeBaseInterface *
  	get_node_base_interface(NodeType node_pointer)
  	{
  	// Forward pointers to detail implmentation directly.
  	return detail::get_node_base_interface_from_pointer(node_pointer);
  	}
  	/// Get the NodeBaseInterface as a pointer from a "Node like" object.
  	template<
  	typename NodeType,
  	typename std::enable_if<
  	!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
  	>::type = 0
  	>
  	rclcpp::node_interfaces::NodeBaseInterface *
  	get_node_base_interface(NodeType && node_reference)
  	{
  	// Forward references to detail implmentation as a pointer.
  	return detail::get_node_base_interface_from_pointer(&node_reference);
  	}

• rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_timers_interface.hpp

  Lines 120 to 144 in 64c0f86

  	/// Get the NodeTimersInterface 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::node_interfaces::NodeTimersInterface *
  	get_node_timers_interface(NodeType node_pointer)
  	{
  	// Forward pointers to detail implmentation directly.
  	return detail::get_node_timers_interface_from_pointer(node_pointer);
  	}
  	/// Get the NodeTimersInterface as a pointer from a "Node like" object.
  	template<
  	typename NodeType,
  	typename std::enable_if<
  	!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
  	>::type = 0
  	>
  	rclcpp::node_interfaces::NodeTimersInterface *
  	get_node_timers_interface(NodeType && node_reference)
  	{
  	// Forward references to detail implmentation as a pointer.
  	return detail::get_node_timers_interface_from_pointer(&node_reference);
  	}

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

  Lines 120 to 144 in 64c0f86

  	/// 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::node_interfaces::NodeTopicsInterface *
  	get_node_topics_interface(NodeType node_pointer)
  	{
  	// Forward pointers to detail implmentation directly.
  	return detail::get_node_topics_interface_from_pointer(node_pointer);
  	}
  	/// Get the NodeTopicsInterface as a pointer from a "Node like" object.
  	template<
  	typename NodeType,
  	typename std::enable_if<
  	!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
  	>::type = 0
  	>
  	rclcpp::node_interfaces::NodeTopicsInterface *
  	get_node_topics_interface(NodeType && node_reference)
  	{
  	// Forward references to detail implmentation as a pointer.
  	return detail::get_node_topics_interface_from_pointer(&node_reference);
  	}

Which are used like this:

• rclcpp/rclcpp/include/rclcpp/create_publisher.hpp

  Lines 67 to 82 in c769b1b

  	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>()
  	))
  	{
  	using rclcpp::node_interfaces::get_node_topics_interface;
  	auto node_topics = get_node_topics_interface(node);

[ivanpauno]

I'm ok with any of the styles proposed, maintaining the current helper functions or creating a NodeHandle template class magic (I haven't think much of how the implementation would look and if that's possible at all), but I have some concerns with the current helper functions:

• The helper functions look if the object has a get_*_interface method returning a raw pointer or a shared pointer, and in any of both cases it returns a raw pointer.
  

  rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp

  Lines 29 to 33 in 64c0f86

  	* It's able to get the NodeBaseInterface pointer so long as the class
  	* has a method called ``get_node_base_interface()`` which returns
  	* either a pointer (const or not) to a NodeBaseInterface or a
  	* std::shared_ptr to a NodeBaseInterface.
  	*/

  
  (I copied the doc, but the code is actually doing that).
  That limits user that may want to retain part ownership of the interfaces.
  We're currently doing that in some places, and not using this helper functions because they return a raw pointer.
  And actually, all our get_*_interface methods in "Node like" classes return a shared pointer (I haven't found any returning a raw pointer), so we could return it.
  If someone wants a raw pointer, they can then call get.

• The helper functions accepts any of a shared pointer, a raw pointer or a universal reference to a "Node like" object.
  I don't see any value of doing that.
  The functions aren't retaining part ownership of the shared pointer, so it shouldn't be accepted.
  Accepting a raw pointer doesn't have much sense to me.
  I would only accept a universal reference to a "Node like" object.

---

Examples of places where we're not using the helper functions (in some of them, they need a shared pointer and not a raw pointer):

• https://github.com/ros2/geometry2/blob/094d363a5b5d4d22b2ac3d4b586dc487079f7338/tf2_ros/include/tf2_ros/transform_listener.h#L75-L103

• https://github.com/ros/diagnostics/blob/678ac48626d0357dc64e858028d497422fcbe19c/diagnostic_updater/include/diagnostic_updater/diagnostic_updater.hpp#L365-L382

• rclcpp/rclcpp_components/include/rclcpp_components/node_factory_template.hpp

  Lines 31 to 48 in 64c0f86

  	template<typename NodeT>
  	class NodeFactoryTemplate : public NodeFactory
  	{
  	public:
  	NodeFactoryTemplate() = default;
  	virtual ~NodeFactoryTemplate() = default;
  	/// Create an instance of a component
  	/**
  	* \param[in] options Additional options used in the construction of the component.
  	*/
  	NodeInstanceWrapper
  	create_node_instance(const rclcpp::NodeOptions & options) override
  	{
  	auto node = std::make_shared<NodeT>(options);
  	return NodeInstanceWrapper(
  	node, std::bind(&NodeT::get_node_base_interface, node));

---

I left some similar comments in a related discussion that proposed another alternative for this (adding an is_pointer thing):
#817 (comment) and #817 (comment)

[wjwwood]

I think that's all fair feedback about the existing methods. They sort of evolved into that. I think always taking a NodeT & (not pointer or shared pointer) is ok (though Node's are basically always shared pointers right now), and always returning a shared pointer (or at least having a separate function that can do that) to the interface object is also a good idea to limit what's possible.

[methylDragon]

I think we can close this issue with the merge of #2041