ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.47k stars 1.25k forks source link

Do We Still Need `lifecycle_publisher->on_activate()`? #4641

Open alanxuefei opened 3 weeks ago

alanxuefei commented 3 weeks ago

The following code (from 5 years ago) may be outdated because the lifecycle of lifecycle_publisher has been managed by the LifecycleNode for the past 2 years.

We built a publisher without on_activate() and it works.

https://github.com/ros-navigation/navigation2/blob/main/nav2_amcl/src/amcl_node.cpp#L263

  pose_pub_->on_activate();
  particle_cloud_pub_->on_activate();
  ....
  pose_pub_->on_deactivate();
  particle_cloud_pub_->on_deactivate();

The following steps outline the management of the lifecycle_publisher within the LifecycleNode:

  1. The LifecycleNode::create_publisher method registers a publisher as a managed entity within the LifecycleNode.

    // lifecycle_node_impl.hpp
    
    LifecycleNode::create_publisher(
     const std::string & topic_name,
     const rclcpp::QoS & qos,
     const rclcpp::PublisherOptionsWithAllocator<AllocatorT> & options)
    {
     .......
     this->add_managed_entity(pub);
     return pub;
    }
  2. When the LifecycleNode is activated, it invokes the on_activate() method of each managed entity, including publishers.

    // lifecycle_node_interface_impl.cpp
    
    void LifecycleNode::LifecycleNodeInterfaceImpl::add_managed_entity(
      std::weak_ptr<rclcpp_lifecycle::ManagedEntityInterface> managed_entity)
    {
      weak_managed_entities_.push_back(managed_entity);
    }
    
    void LifecycleNode::LifecycleNodeInterfaceImpl::on_activate() const
    {
      for (const auto & weak_entity : weak_managed_entities_) {
        auto entity = weak_entity.lock();
        if (entity) {
          entity->on_activate();
        }
      }
    }
  3. The LifecycleNode calls the on_activate() method for each of its managed entities, such as publishers.

    // lifecycle_publisher.hpp
    
    void on_activate() override
    {
      SimpleManagedEntity::on_activate();
      should_log_ = true;
    }
SteveMacenski commented 3 weeks ago

I don't see any on_activate method in lifecycle_node_impl.hpp like you posted: https://github.com/ros2/rclcpp/blob/rolling/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp -- nor does the class have the member weak_managed_entities_ [1]

[1] https://github.com/ros2/rclcpp/blob/rolling/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

alanxuefei commented 3 weeks ago

weak_managed_entities_ is located in rclcpp_lifecycle/src/lifecycle_node_interface_impl.cpp#L563

The sequnce of adding publishers to managed_entities is as below.

  1. LifecycleNode::create_publisher in lifecycle_node_impl.hpp#L64 call this->add_managed_entity(pub).
  2. LifecycleNode::add_managed_entity in lifecycle_node.cpp#L708 call impl_->add_managed_entity(managed_entity);
  3. weak_managed_entities_.push_back(managed_entity) is triggered in lifecycle_node_interface_impl.cpp#L566

The sequence for triggering on_activate of publishers via managed_entities is as follows:

  1. LifecycleNode::on_activate(const State &) calls impl_->on_activate() at lifecycle_node.cpp#L696
  2. The on_activate functions of each managed_entieis are triggered via lifecycle_node_interface_impl.cpp#L579
SteveMacenski commented 2 weeks ago

This is also starting to jog my memory. It looks like those are implemented in on_activate and on_deactivate within the lifecycle node, which are overridable by the application for its own activation / deactivation routines https://github.com/ros2/rclcpp/blob/ee94bc63e4ce47a502891480a2796b53d54fcdfc/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp#L1055-L1057

I don't see where the base class LifecycleNode::on_activate() is called within our overridden on_activate's in Nav2 https://github.com/ros-navigation/navigation2/blob/main/nav2_controller/src/controller_server.cpp#L249-L273

Wouldn't we need to call the base class instance of on_activate in our final derived class's?

alanxuefei commented 2 weeks ago

We need to call LifecycleNode::on_activate(state); as shown here.

Therefore,

pose_pub_->on_activate();
particle_cloud_pub_->on_activate();

can be replaced with:

LifecycleNode::on_activate(state);

The pull request #1863 added the feature of "Automatically transition lifecycle entities when node transitions."

This feature was demonstrated in an example node. We can also call the default on_activate() within a customized on_activate(), or simply use the default on_activate() without customizing it.

alanxuefei commented 2 weeks ago

Thanks for explaining the code. It helped us gain a clear understanding of the ROS 2 normal publisher (rclcpp::Publisher) and lifecycle publisher (rclcpp_lifecycle::LifecyclePublisher). A normal publisher is used in my codebase, which is why it does not need to be activated or deactivated.

The key points are summarized below for closing the ticket.

  1. Normal Publisher:

    rclcpp::Publisher<std_msgs::msg::String>::SharedPtr normal_pub_;
    • A normal publisher does not require activation and can publish messages as soon as it is created.
  2. Lifecycle Publisher:

    std::shared_ptr<rclcpp_lifecycle::LifecyclePublisher<std_msgs::msg::String>> pub_;
    • A lifecycle publisher, like the one shown above, can be managed using:

      1. The default LifecycleNode::on_activate(state) method.

      2. A custom on_activate method that calls LifecycleNode::on_activate(state).

      3. Manually by calling pub_->on_activate().

SteveMacenski commented 2 weeks ago

@alanxuefei If you're open to a PR, you could add in the LifecycleNode::on_activate(state) at the start of each method (and deactivate too) and remove the publisher activation / deactivations.

I'm pretty ambivalent on it, if you like it better, then happy to merge it!

alanxuefei commented 1 week ago

@alanxuefei If you're open to a PR, you could add in the LifecycleNode::on_activate(state) at the start of each method (and deactivate too) and remove the publisher activation / deactivations.

I'm pretty ambivalent on it, if you like it better, then happy to merge it!

Sure, I will open PRs and refactor some nodes later. It’s a minor enhancement, but it’s worth trying.

SteveMacenski commented 1 week ago

Definitely!