You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
"Many programmers are tempted to write member functions to get the benefits of the member function syntax (e.g. "dot-autocomplete" to list member functions);[6] however, this leads to excessive coupling between classes.[7]"
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:
Another version of (b) could be to have classes that are constructed with node, e.g. Publisher(node, ...) rather than node->create_publisher(...)
NodeInterface<LifecycleNode>::<tab> -> only life cycle node methods
(karsten) use interface classes directly, e.g. node->get_X_interface()->do_thing()
(dirk) use macros to add methods to class
Question: Do we want tab-completable API (specifically list of member functions)?
Question: Is consistency in calling between core and non-core features more important than tab-completion?
Add better example of adding new feature and not needing to touch rclcpp::Node.
(dirk) methods and free functions not mutually exclusive.
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:
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
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.
Make all public methods in LifecycleNode and Node virtual so that these classes can be properly extended as needed by users.
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.
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 likerclcpp::node_interfaces::NodeTopics
, for example.Also, we have other versions of the class
rclcpp::Node
with different semantics and interfaces, likerclcpp_lifecycle::LifecycleNode
, and we have been having trouble keeping the interface provided there up to date with how things are done inrclcpp::Node
. SinceLifecycleNode
has a different API fromNode
in some important cases, it does not just inherit fromNode
.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 makeNode
multiple inherit from those classes, and thenLifecycleNode
could selectively inherit from those as well, or (b) change our calling convention fromnode->do_thing(...)
to bedo_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 comeauto 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
andLifecycleNode
without adding anything to them, it also means that user definedNode
-like classes will also work, even if they do not inherit from or provide the complete interface forrclcpp::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 aNode
-like class.Both of the above approaches address some of the main concerns, which are: keeping
Node
andLifecycleNode
in sync, reducing the size of theNode
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:
Publisher(node, ...)
rather thannode->create_publisher(...)
NodeInterface<NodeLike>::something(node_like)
NodeInterface<LifecycleNode>::<tab>
-> only life cycle node methodsrclcpp::Node
.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
The text was updated successfully, but these errors were encountered: