ROS2 - tf2_ros::TransformBroadcaster and rclcpp_lifecycle::LifecycleNode

[Myzhar]

Feature request

Feature description

Add constructor for LifecycleNode to tf2_ros::TransformBroadcaster.

Implementation considerations

Refer to ROS answer thread:
https://answers.ros.org/question/302459/ros2-tf2_rostransformbroadcaster-and-rclcpp_lifecyclelifecyclenode/

[Myzhar]

@dirk-thomas no news about this bug? I think it is quiet important

[dirk-thomas]

Nobody is working on this atm. Please consider contributing a pull request.

[Myzhar]

@dirk-thomas I'm looking at it.

I think that I can take two ways:

1. Replace rclcpp::Node::SharedPtr node in the constructor with rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node... but the problem is that I have not access to the create_publisher method.
2. Add a new constructor that takes rclcpp_lifecycle::LifecycleNode::SharedPtr node and initializes a new LifecycleNode object. The sendTransform functions will select the correct node runtime.

Any suggestion?

[Myzhar]

I opted to try the solution 2. It seems to work.
Let me know if you prefer to take the solution 1 or to use a smarter solution

[Karsten1987]

Replace rclcpp::Node::SharedPtr node in the constructor with rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node... but the problem is that I have not access to the create_publisher method.

I believe only the node base interface won't suffice in this case. I guess you'd need also the topics interface of a node: https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node_interfaces/node_topics_interface.hpp

Creating a constructor for a lifecycle node might work for the problem above, but to me, it feels like not sustainable in case there's another node implementation (something else than lifecycle) coming in the future. All nodes however have a base-interface and most likely also a topics-interface.

[Myzhar]

It's a solution that I do not really like for the same motivation you are saying, but I do not really understand how to overcome it.
In my mind Node and LifecycleNode should have the same base class, but this is not the case and it brings to problems like this.
Somewhere I read why you chose to not derive the two Node classes from the same base class, but I do not remember the motivation.

If you can guide me to a better solution I will try to implement it

[Karsten1987]

I can understand, that this is maybe not super trivial to figure out on your own. So sorry for that :)

The design decision for a ROS 2 node was to use composition instead of inheritance. That being said, a regular node and a lifecycle node don't share a common base class but rather have the same internal components.
For a lifecycle node you can access the components here:
https://github.com/ros2/rclcpp/blob/master/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp#L358-L391
In particular for this PR, I would assume the TF2 Broadcaster constructor takes two of these components (NodeBaseInterface and NodeTopicsInterface). The latter allows you to create a regular publisher, not a lifecycle publisher.

[rasmusan]

@Myzhar do you have any plans to continue work on this? If not, I might try to implement the functionality according to @Karsten1987's suggestion.

[Myzhar]

@rasmusan I'm sorry but i have not the time to work on this now.
If you can continue it would be great

[mjeronimo]

I think that introducing the NodeBaseInterface and NodeTopicsInterface is not sufficient. The publisher created by a LifecycleNode will still need to be activated/deactivated. Since TransformBroadcaster holds the created publisher, it should expose activated() and deactivated() methods so that it could reflect these calls to the publisher->on_activate() and on_deactivate. However, an rclcpp_lifecycle::Publisher supports these methods while an rclcpp::Publisher does not.

So, my question is how shouldhelper classes like TransformBroadcaster, that create publishers support both lifecycle and non-lifecycle nodes? This seems like a general design issue for any helper class that creates publishers.

Related to this proposal

[Karsten1987]

@Myzhar Can this issue be closed? There is support for lifecycle nodes now introduced in #108