[Jazzy] [Controller server] Loading multiple controllers at the same time can result in parameter loading issue

[gennartan]

Bug report

Required Info:

• Operating System:
  Ubuntu22.04
• Computer:
  Thinkpad
• ROS2 Version:
  Jazzy
• Version or commit hash:
  v1.3.9
• DDS implementation:
  CycloneDDS

Steps to reproduce issue

Start the controller server with two controller plugins. All the other parameters can be kept to their default value and let the lifecycle node configure the controller server.

controller_plugins: ["FollowPath1", "FollowPath2"]
FollowPath1:
  plugin: "nav2_rotation_shim_controller::RotationShimController"
  primary_controller: "nav2_regulated_pure_pursuit_controller::RegulatedPurePursuitController"

FollowPath2:
  plugin: "nav2_mppi_controller::MPPIController"

Expected behavior

Both controllers are loaded without error.

Actual behavior

The MPPI controller (second controller to be loaded) enters reports a fault in its configuration phase when loading the parameters as vx_min has a negative value. When checking into the code, raising an error when a parameter has a negative value is a behavior of the RPP controller. [1]

[controller_server-12] [INFO] [1761137974.303031720] [controller_server]: Created controller : FollowPath2 of type nav2_mppi_controller::MPPIController
[controller_server-12] [WARN] [1761137974.320779558] [controller_server]: The value of parameter 'FollowPath2.vx_min' is incorrectly set to -0.350000, it should be >=0. Ignoring parameter update.
[controller_server-12] [ERROR] [1761137974.321174478] [controller_server]: Caught exception in callback for transition 10
[controller_server-12] [ERROR] [1761137974.321264307] [controller_server]: Original error: parameter 'FollowPath2.vx_min' could not be set: 
[controller_server-12] [WARN] [1761137974.321337982] [controller_server]: Callback returned ERROR during the transition: configure
[controller_server-12] [FATAL] [1761137974.321372749] [controller_server]: Lifecycle node controller_server does not have error state implemented
[lifecycle_manager-16] [ERROR] [1761137974.332179778] [lifecycle_manager_navigation]: Failed to change state for node: controller_server
[lifecycle_manager-16] [ERROR] [1761137974.332315367] [lifecycle_manager_navigation]: Failed to bring up all requested nodes. Aborting bringup.

Reproduction instructions

The error happens when the node is re-configured, so no specific reproduction instruction here.

Additional information

[1] https://github.com/ros-navigation/navigation2/blob/7a609aeea79a69009994651f85ab53909b2bb30c/nav2_regulated_pure_pursuit_controller/src/parameter_handler.cpp#L229C1-L229C80

[mini-1235]

I think this is similar to #5100. We either need to revert #4971 or backport #5106

edit: it seems like there is a API change in #5106

[SteveMacenski]

@mini-1235 is spot on. I think he's planning on implementing the solution this week?

That wouldn't resolve Jazzy though since we cannot backport a breaking parameter namespacing change to Jazzy.

edit: it seems like there is a API change in #5106

Just MPPI right? The others could be backported. I would say though that reverting a breaking change in Jazzy would be fine, I don't think #4971 is very critical for Jazzy and if it breaks behavior that's an acceptable backport to revert. I'm going to have to do a Jazzy resync for #5639 tomorrow anyway to fix another issue.

Do you mind opening the backport revert PR @gennartan? I'm on the go today for last minute ROSCon prep before I leave and not able to add that to my schedule today

[mini-1235]

@mini-1235 is spot on. I think he's planning on implementing the solution this week?
That wouldn't resolve Jazzy though since we cannot backport a breaking parameter namespacing change to Jazzy.

I think this issue is different from the scenario I described in #5621 (comment), backporting #5106 should solve the problem here

Just MPPI right?

Yes :)

[SteveMacenski]

OK, so we could backport everything except for MPPI, but the issue would still happen when someone wants to use MPPI. That makes me think we should backport the PR that added this in the first place to Jazzy so that its only available in Rolling and newer. I think its better to do less validation checks in Jazzy rather than knowing there's a bug with the rotation shim controller when paired with MPPI. If we just revert that, we should be good to go, no?

[mini-1235]

OK, so we could backport everything except for MPPI, but the issue would still happen when someone wants to use MPPI. That makes me think we should backport the PR that added this in the first place to Jazzy so that its only available in Rolling and newer. I think its better to do less validation checks in Jazzy rather than knowing there's a bug with the rotation shim controller when paired with MPPI. If we just revert that, we should be good to go, no?

You are right, I agree to revert the PR #4971

[mini-1235]

I have reverted it and tested in my workspace with:

controller_plugins: ["FollowPath1", "FollowPath2"]
FollowPath1:
  plugin: "nav2_rotation_shim_controller::RotationShimController"
  primary_controller: "nav2_regulated_pure_pursuit_controller::RegulatedPurePursuitController"

FollowPath2:
  plugin: "nav2_mppi_controller::MPPIController"

I will just go ahead and open the PR