`MapServer` class' `on_activate` should call `LifecycleNode::on_activate(state)`

[udalmia94]

Bug report

Required Info:

• Operating System:
  • Ubuntu 22.04
• ROS2 Version:
  • Humble, apt install
• Version or commit hash:
  • ros-humble-navigation2 1.1.8-2jammy.20230624.064440
• DDS implementation:
  • Fast-RTPS

Steps to reproduce issue

Expected behavior

Actual behavior

Additional information

https://github.com/ros-planning/navigation2/blob/main/nav2_map_server/src/map_server/map_server.cpp#L128C1-L144C2

The on_activate() function should call the base class LifecycleNode::on_activate(state) function to activate all publishers rather than activating the publisher explicitly. This ensures any LifecyclePublishers declared in a derived class of map server would also get activated.

[SteveMacenski]

This is the implementation of the lifecycle node’s on_activate function. If you implement your own derived class’ on_activate, you’re overriding this method. This is consistent with how all the Nav2 nodes operate.

It seems to me that your derived class should actually call this method to start, then activate the newly added interfaces. That way your program has control over explicitly what’s being called.

Sometimes order matters in these lifecycle methods so handling that behind the scenes for users wouldn’t generically work. Some will want the MapServer’s methods called first, others will want to have their own called first. Others yet will want some before and some after. Instead of registering Pre- and Post- callbacks for every single lifecycle transition (which seems like an overcomplication), its far more straight forward for derived implementations to simply use (or not use) the on_* as they see fit, since you’re overriding it, you’re taking responsibility for its specific implementation.

Thoughts?

[udalmia94]

The way we've been writing the on_activate function in my organization is that we call the LifecycleNode::on_activate function first, then proceed with anything else that needs to be done in the function. The base class function activates any "managed entities"; the implementation of this base class function can be found here. This function automatically activates all "managed entities" which, according to my understanding of lifecycle nodes, is currently limited to publishers. They also use this in the ROS2 lifecycle example.

When using MapServer as a base class, we did call MapServer::on_activate first, but there was an underlying assumption that this method made the call to the LifecycleNode::on_activate method. This, to me, seems like the recommended way of doing this by ROS2, and also makes sense.

I am sure there could niche cases where the order of activating managed entities could matter, but I think for 99.9% of ROS2 packages, the order in which managed entities is activated shouldn't matter, it should take a split second anyway. In light of this, I think a good design practice would be for MapServer class to call the base class on_activate and any derived class from this class can call MapServer::on_activate knowing that the LifecycleNode::on_activate function is being called by the MapServer::on_activate. At the same time, I understand the choices made by the nav2 team, so if you think this change isn't warranted, that's fair. It's just different to how I've been using LifecyleNodes and I'm new to it anyway.

[SteveMacenski]

Got it - I looked into it more and it looks like the add_managed_entity method was only added in the last year so that's a "new" feature relative to us that I was not previously aware of and our processes obviously pre-date that. Though, grepping the codebase, it seems to only work with publishers, though it would be great if it could be applicable with more objects (future improvement, I suppose!).

When I say that the order matters, you're right that its lessor so to do with the publishers which that would deal with and more so to the other objects we need to manage the state of before / after others are transitioned up. If you look at the orders within the activate / deactivate methods in many of the servers, breaking that order would actually break the system due to their interconnectedness.

With that in mind, I think it could make sense for us to update all of the Nav2 servers to call that base method at the start of the function to activate / deactivate the appropriate interfaces that the ROS 2 system wants to manage for us under the hood. For now, the publishers aren't particularly helpful, but I suppose we have to start somewhere before lifecycle subscriptions, services, clients, and actions are handled. There might be some issues with that, specifically where we have other objects with lifecycle transitions to be called which rely on the publishers to be "activate" within their scope, but I suppose that's a problem we can resolve during implementation by seeing if we pass a lifecycle publisher around (and deal with it as needed).

@clalancette do you support that direction?

If so, I think rather than just talking about the map server @udalmia94, we should apply this new pattern to all the nodes. That way in the future when lifecycle services / subscriptions / etc are handled, we get that for "free" everywhere and we just need to delete the manual transitions (which I'm sure will throw errors galore).

Is this something you're interested in contributing @udalmia94 ? It would certainly get you some experience in all of the Nav2 servers 😆

[udalmia94]

I can help out if y'all decide to move forward with this.

[SteveMacenski]

I spoke briefly with Chris and he expressed that the feature we're discussing with the base class is still a work in progress and he doesn't recommend that we move forward with this at this time until more of the lifecycle work is complete.

Because of that, it seems somewhat premature to make it the plan of record.

I tend to agree at this point if that's their viewpoint as well. Once the clients/services/subscriptions support it too, I think it would be extremely relevant to do.

With that recommendation / context in mind, I think the best solution for now is to hold off on re-organizing all the servers until more of the lifecycle objects are supported and perhaps you should call MapServer::on_activate within your activate function to account for this. Once the ROS interfaces have that same entity lifecycle setup, I'd be totally supportive of moving over to that pattern.

Does that sound good to you?

[udalmia94]

We started using LifecycleNode on our software stack only recently, and now my team members are working on upstream changes like lifecycle subscriber. In my opinion it would make sense to make this change as soon as lifecycle subscribers are also working, but it's possible that after that change the other interfaces are easy and get implemented soon anyway. For now we will work around this by activating the publishers explicitly in our code. Thanks for sharing your thoughts.

[SteveMacenski]

Ok! Once you have subscribers in there (and perhaps services?) I’d be happy to make the change you’ve mentioned.

Note that iRobot has some lifecycle changes its proposing in the coming weeks, so I’d just keep an eye out for that.

[carmiac]

FWIW, there is now a lifecycle subscriber PR in rclcpp, and I'll be working on adding more lifecycle objects over the next few weeks. ros2/rclcpp#2254