Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log discrepancy when setting MPPI parameters dynamically #4704

Open
PlatHum opened this issue Oct 3, 2024 · 3 comments · May be fixed by #4711
Open

Log discrepancy when setting MPPI parameters dynamically #4704

PlatHum opened this issue Oct 3, 2024 · 3 comments · May be fixed by #4711

Comments

@PlatHum
Copy link

PlatHum commented Oct 3, 2024

Bug report

Required Info:

  • Operating System:

    • Ubuntu 22.04
  • ROS2 Version:

    • Humble
  • Version or commit hash:

    • 1.1.16-1jammy.20240830.231501
  • DDS implementation:

    • CycloneDDS

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
Copy link
Member

SteveMacenski commented Oct 3, 2024

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
Copy link
Member

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

@SteveMacenski SteveMacenski reopened this Oct 5, 2024
aosmw added a commit to aosmw/navigation2 that referenced this issue Oct 7, 2024
The "verbose" parameter of the parameters_handler is
a special case that needs registration before the
dynamic parameter handler callback is registered.

In verbose mode make the parameter handler info/warn/debug
messages more expressive.
@aosmw aosmw linked a pull request Oct 7, 2024 that will close this issue
7 tasks
@aosmw
Copy link
Contributor

aosmw commented Oct 7, 2024

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.
#4711

aosmw added a commit to aosmw/navigation2 that referenced this issue Oct 7, 2024
The "verbose" parameter of the parameters_handler is
a special case that needs registration before the
dynamic parameter handler callback is registered.

In verbose mode make the parameter handler info/warn/debug
messages more expressive.

Signed-off-by: Mike Wake <[email protected]>
@SteveMacenski SteveMacenski linked a pull request Oct 7, 2024 that will close this issue
7 tasks
aosmw added a commit to aosmw/navigation2 that referenced this issue Oct 8, 2024
* remove comments.
* Use RCLCPP_DEBUG instead of INFO for low level messages.
* Add test for trying to access parameters that are not declared.

Signed-off-by: Mike Wake <[email protected]>
aosmw added a commit to aosmw/navigation2 that referenced this issue Oct 8, 2024
…os-navigation#4704)

Attempts to change undefined parameters will not be successful
and will log an error.

Attempts to change static parameters will be ignored, a debug
message is logged if a change in parameters is attempted.
aosmw added a commit to aosmw/navigation2 that referenced this issue Oct 8, 2024
…os-navigation#4704)

Attempts to change undefined parameters will not be successful
and will log an error.

Attempts to change static parameters will be ignored, a debug
message is logged if a change in parameters is attempted.

Signed-off-by: Mike Wake <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants