Rclcpp: 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)