-
Notifications
You must be signed in to change notification settings - Fork 522
Description
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
- Prevent API gaps between Node and LifecycleNode #898
- Revisit how to expose node graph API to users #509
- Parameters in LifecycleNodes #855
- expand_topic_or_service_name should use get_effective_namespace() #985
- subnode feature is in rclcpp::Node only, complicating "node using" API designs
- http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4174.pdf
- https://en.wikipedia.org/wiki/Uniform_Function_Call_Syntax#C++_proposal
- "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 thannode->create_publisher(...) - (tfoote) interface class?
NodeInterface<NodeLike>::something(node_like)- DRY?
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: