ros-navigation / navigation2

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

Log discrepancy when setting MPPI parameters dynamically #4704

Open PlatHum opened 4 weeks ago

PlatHum commented 4 weeks ago

Bug report

Required Info:

Steps to reproduce issue

Using ros2 param set command to set MPPI parameters triggers a NOT FOUND warning, although the command returns 'Set parameter successful'.

## General form:
ros2 param set /{$controller_server_node_name} {$mppi_controller_name}.{$parameter} {$value}

##For example:
ros2 param set /controller_server FollowPathMPPI.AckermannConstraints.min_turning_r 1.0
##or
ros2 param set /controller_server FollowPathMPPI.gamma 1.0
##or
 ros2 param set /controller_server FollowPathMPPI.ConstraintCritic.cost_weight 1.0

Expected behavior

### If parameter exists, is dynamic and changed successfully
##On set parameter terminal:
Set parameter successful
##On controller_server terminal:
[controller_server-28] [WARN] [controller_server]: {$Something that indicates success in changing parameter}
[controller_server-28] [INFO] [controller_server]: Optimizer reset
### If parameter does not exist, or is not dynamic or not changed successfully
##On set parameter terminal:
Setting parameter failed: {$reason_for_failure}
##On controller_server terminal:
[controller_server-28] [WARN] [controller_server]: {$Something that indicates failure in changing parameter}
[controller_server-28] [INFO] [controller_server]: Optimizer reset

Actual behavior

##On set parameter terminal:
Set parameter successful

##On controller_server terminal:
[controller_server-28] [WARN] [controller_server]: Parameter FollowPathMPPI.AckermannConstraints.min_turning_r not found
[controller_server-28] [INFO] [controller_server]: Optimizer reset

Additional information

Seems like parameters_handler always returns success even though a parameter might not be found.


SteveMacenski commented 4 weeks ago

You should see a log that Parameter XYZ is not found, but the callback handles a vector of parameter updates, not just a single parameter update. I agree that perhaps if we ever don't find a parameter, we set successful to false.

Can you submit a PR that creates a bool at the start of the method (true), and if a parameter isn't found, we set it to false after the log, and then set result.successful to our new bool? I think that should resolve the issue

SteveMacenski commented 3 weeks ago

I see your PR, this can be backported so once its merged into main, I'll autobackport into Humble/Iron/Jazzy

aosmw commented 3 weeks ago

This is one of the first things I played with understanding when getting my head into the mppi code.

Here is a cleaned up patch that helps explain what is happening with the "verbose" parameter that may help shed some clarity. https://github.com/ros-navigation/navigation2/pull/4711