Prevent API gaps between Node and LifecycleNode

[bpwilcox]

I was working on a feature using rclcpp_lifecycle::LifecycleNode to declare a parameter when I noticed that the declare_parameter API on the lifecycle node interface does not include the ignore_overrides flag as does the version in rclcpp::Node. It seems that in addition to this, there are a handful of functions that have yet to be implemented in the lifecycle node. From a brief comparison (I didn't check all the signatures so there could be other differences), I found these functions that are missing:

• get_fully_qualified_name()
• declare_parameter (missing ignore_overrides argument)
• add_on_set_parameters_callback(OnParametersSetCallbackType callback);
• remove_on_set_parameters_callback(const OnSetParametersCallbackHandle * const handler);
• get_sub_namespace()
• get_effective_namespace()
• create_sub_node()
• assert_liveliness()

I think some PRs to implement the above functions and close the gap on the common node API is a solution, but I also would propose a discussion on a common interface by which these nodes must abide. Apparently these APIs can go out of sync with new changes, so I'd like to hear thoughts on how to better synchronize these Node classes.

[fujitatomoya]

@bpwilcox

just idea, i think rclcpp_lifecycle::LifercycleNode is built on top of rclcpp::Node. so rclcpp_lifecycle::LifecycleNode Class inherits based on rclcpp::Node and featured with lifecycle interfaces. what i really care if for code maintenance. and also i cannot think of anything that is required for rclcpp::Node but not for rclcpp_lifecycle::LifercycleNode.

if i am not understanding correctly, could you enlighten me what discussion should be done here?

[bpwilcox]

just idea, i think rclcpp_lifecycle::LifercycleNode is built on top of rclcpp::Node. so rclcpp_lifecycle::LifecycleNode Class inherits based on rclcpp::Node and featured with lifecycle interfaces.

Currently, the Node aand LifecycleNode classes are independent implementations, though both are built on top of the rclcpp::node_interfaces as members. For this reason, the APIs can go out of sync.

what i really care if for code maintenance. and also i cannot think of anything that is required for rclcpp::Node but not for rclcpp_lifecycle::LifercycleNode.

if i am not understanding correctly, could you enlighten me what discussion should be done here?

I agree, the LifecycleNode is primarily a superset of the Node interface, which is why I am opening discussion for whether we should have it inherit from the Node class or perhaps both inherit from a common base class, or other ideas.

[mjcarroll]

I'm going to add a second issue to track just adding the currently missing APIs, and we can continue discussion on how to prevent this in the future on this issue.

[Barry-Xu-2018]

@bpwilcox @fujitatomoya @mjcarroll @Karsten1987

About the relationship of node and lifecycleNode, I think the design has been discussed in #318.
@Karsten1987 said "We decided to favor composition mainly of being able to test/mock the individual components of the node class. As you might have noticed, we tried to keep the basic interface / public member function of the lifecycle node as similar as the original node class. This is a) for not breaking the API too much and b) for being able to migrate the lifecycle components into the node class."

So the decision tends to move lifecycle into node class. I think Lifecycle can be as feature(component) of node and enabled by node option.

BTW, in order to avoid growing gap of API between libcycleNode and Node, moving should be done ASAP.

[fujitatomoya]

@Barry-Xu-2018

decision's been made before, but not sure we still get along with this.

@wjwwood could you share your thoughts if possible?

BTW, in order to avoid growing gap of API between libcycleNode and Node, moving should be done ASAP.

agree.

[wjwwood]

So the decision tends to move lifecycle into node class. I think Lifecycle can be as feature(component) of node and enabled by node option.

I'm not sure that would be a good idea. Perhaps it could be the right approach, but part of the interface of a lifecycle node is that you do not create normal publishers but lifecycle publishers instead. Doing what you suggest doesn't remove the normal create_publisher() method.

So, maybe something like this could work, but not without more thought and details as to how it would work. I don't have time to pursue that right now.

[fujitatomoya]

i do not have concrete design right now, not sure which way is suitable/reasonable at this time. I would come back on this, since i do not have resource either. or somebody else takes over the consideration.

[guru-florida]

This may be far out but....what about deriving Node from LifecycleNode? IMO LifecycleNode is a beautiful implementation and should be defacto and pushed as best-practice. Node can just provide a default transition impl for on_configure|activate|deactivate|etc and also transition itself (unless a "self_manage" parameter is explicitly false or something). I think this fits the c++ best practices of "derived classes shouldnt modify behavior much" and in fact to the external observer there is no behavior change - only internal.

Benefits:

• (Possibly) can be done with no changes to existing Node derived classes. Components derived from Node dont bother caring about lifecycle and existing Node-only based conponents shouldnt need any changes.
• No API Gaps anymore
• One single Node interface reduces ROS2 API complexity
• Node writers can initially derive from Node, then later change to Lifecycle management without affecting their users (since they were always a Lifecycle node to users)

[hellantos]

I came across this problem when I tried to create a library that does provide both a LifecycleNode and rclcpp::Node. I have a class (lets call it DriverInterface) that should take LifecycleNode or rclcpp::Node in the constructor and add a bunch of services, subscriptions and publishers. In this class I would like to use the normal node functions such as create publisher, create client, create_timer, ... (not node_interface provided functions, as they are not well documented and require much reimplementation and maintenance if upstream changes). This seems to be impossible right now, even templating does not really work you, just end up reimplmenting most things seperately for both node types.

The current situation just makes handling Node and LifecycleNode instances with the same code a huge pain.
It would be super helpful if we had at least a NodeUserInterface with all common user functions - these are not in node_interfaces.

I would be willing to create and add node_user_interface.hpp and make LifecycleNode and Node inherit from it.

[clalancette]

I would be willing to create and add node_user_interface.hpp and make LifecycleNode and Node inherit from it.

To be clear; it was an explicit design goal to not require inheritance between the two of them, to avoid the myriad problems with inheritance. Whether that is the right call is not totally clear, but we would definitely want to make sure we change/update the design if we were to go this route. That said...

This seems to be impossible right now, even templating does not really work you, just end up reimplmenting most things seperately for both node types.

This should be possible today, either by using templating (like what is done in https://github.com/ros2/message_filters/blob/rolling/include/message_filters/subscriber.h), or by using the Node interfaces instead of the node directly (like what is done in https://github.com/ros2/geometry2/blob/rolling/tf2_ros/include/tf2_ros/transform_listener.h#L119-L133).

[hellantos]

This should be possible today, either by using templating (like what is done in https://github.com/ros2/message_filters/blob/rolling/include/message_filters/subscriber.h), or by using the Node interfaces instead of the node directly (like what is done in https://github.com/ros2/geometry2/blob/rolling/tf2_ros/include/tf2_ros/transform_listener.h#L119-L133).

If the approach is to use the node interfaces, it would be super nice, if they would expose the standard functions of the node i.e. create_service, create_client, create_wall_timer etc. Which they partially do, but not completely.

[methylDragon]

I think the difficulty in some of those (e.g. create_service) is that those methods require more than one node interface (which would mean implementing them as part of the node interfaces would result in overlaps between the interfaces.)

You'd call rclcpp::create_service(), passing in the node base interface and node services interfaces, I think.

[methylDragon]

Just an update,

There is now a third option aside from passing individual interfaces or templating on NodeT. (Though this only unifies any functionality that is defined within an interface, but not any of the node->XXX methods that aren't)

An aggregate class that implicitly converts node-likes into a collection of aggregated interfaces has been implemented in:

• Implement Unified Node Interface (NodeInterfaces class) #2041

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),
  // ...