search

ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
526 stars 411 forks source link

[api review] Calling Syntax and Keeping Node-like Class APIs in Sync #1259

Open wjwwood opened 4 years ago

wjwwood commented 4 years ago

Summary of the Issue

Currently, much of the API is exposed via the rclcpp::Node class, and due to the nature of the current architecture there is a lot of repeated code to expose these methods and then call the implementations which are in other classes like rclcpp::node_interfaces::NodeTopics, for example.

Also, we have other versions of the class rclcpp::Node with different semantics and interfaces, like rclcpp_lifecycle::LifecycleNode, and we have been having trouble keeping the interface provided there up to date with how things are done in rclcpp::Node. Since LifecycleNode has a different API from Node in some important cases, it does not just inherit from Node.

There are two main proposals (as I see it) to try and address this issue, either (a) break up the functionality in Node so that it is in separate classes and make Node multiple inherit from those classes, and then LifecycleNode could selectively inherit from those as well, or (b) change our calling convention from node->do_thing(...) to be do_thing(node, ...).

For (a) which commonly referred to as the Policy Based Design Pattern, we'd be reversing previous design decisions which we discussed at length where we decided to use composition over inheritance for various reasons. One of the reasons was testing, with the theory that having simpler separate interfaces we could more easily mock them as needed for testing. The testing goal would still be met, either by keeping the "node_interface" classes as-is or by mocking the classes that node would multiple inherit from, however it's harder to indicate that a function needs a class that multiple inherits from several classes but not others. Also having interdependency between the classes which are inherited from is a bit complicated in this design pattern.

For (b), we would be changing how we recommend all code be written (not a trivial thing to do at all), because example code like auto pub = node->create_publsiher(...); would be come auto pub = create_publisher(node, ...);. This has some distinct advantages, however, in that it allows us to write these functions, like create_publisher(node, ...), so that the node argument can be any class that meets the criteria of the function. That not only means that when we add a feature it automatically works with Node and LifecycleNode without adding anything to them, it also means that user defined Node-like classes will also work, even if they do not inherit from or provide the complete interface for rclcpp::Node. Another major downside of this approach is discoverability of the API when using auto-completion in text editors, as node-><tab> will often give you a list of methods to explore, but with the new calling convention, there's not way to get an auto complete for code who's first argument is a Node-like class.

Both of the above approaches address some of the main concerns, which are: keeping Node and LifecycleNode in sync, reducing the size of the Node class so it is more easily maintained, documented, and so that related functions are grouped more clearly.

Related Issues and Resources

The suggested action from the API review was to just discuss it and defer until after Foxy. This issues is meant to track that post-foxy work.

Below are some notes from the discussion.

Notes from 2020-03-19:

To make further progress on this, we need someone to lead a push to decide on what to do and follow through with the implementation, likely including a REP.

This is a result of the API review done in March 2020:

https://github.com/ros2/rclcpp/blob/master/rclcpp/doc/api_review_march_2020.md#calling-syntax-and-keeping-node-like-class-apis-in-sync

msmcconnell commented 2 years ago

As a user, I would like to drop some thoughts here. I have found that the API inconsistencies between Node and Lifecycle node in foxy pose a real challenge for creating any sort of Base Node class to use throughout a project. For example, I would like all my nodes to be Lifecycle nodes and perform certain standard actions (like logging, exception handling etc.) when creating subs/pubs etc. The fact that LifecycleNode and Node are not part of the same class hierarchy means that added functionality is not propagated via polymorphism to other libraries/methods when provided with one of the nodes from my package. This leads to either such functionality being duplicated or dropped as not very beneficial. To that end I would suggest two core ideas

  1. Ensure that the final resolution to this issue supports polymorphism between LifecycleNodes and Nodes such that all functionality in regular Node is supported polymorphically in LifecycleNode.
  2. Make all public methods in LifecycleNode and Node virtual so that these classes can be properly extended as needed by users.
methylDragon commented 1 year ago

Just an update,

A variant of this (an aggregate class that implicitly converts node-likes into a collection of aggregated interfaces):

(tfoote) interface class? NodeInterface::something(node_like)

Has been implemented in:

And can be used as such:

create_service(
   NodeInterfaces<NodeBaseInterface, NodeServicesInterface> node_handle,
   // ...

// User calls with (or can just pass in the (non pointer) object for implicit conversion))
auto service = create_service(
  NodeInterfaces<NodeBaseInterface, NodeServicesInterface>(*my_node_or_lifecycle_node_class_ptr),
  // ...

Though this still does not resolve the underlying issue, any methods that only rely on node interfaces can now use the NodeInterfaces class to take in any node-like that supports getter methods for the aggregated interfaces.