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

MPPI: Do not set params success in dynamic params callback to allow changing params in other plugins #4908

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

pepisg
Copy link
Contributor

@pepisg pepisg commented Feb 6, 2025


Basic Info

Info Theres a bug when setting params from other plugins when using mppi
Ticket(s) this addresses
Primary OS tested on (Ubuntu, MacOS, Windows)
Robotic platform tested on (Steve's Robot, gazebo simulation of Tally, hardware turtlebot)
Does this PR contain AI generated software? (No; Yes and it is marked inline in the code)

Description of contribution in a few bullet points

Currently, the parameters handler library in the MPPI will return failure when trying to set a parameter in the controller server node that does not belong to the MPPI plugin. This is because rclcpp will stop processing dynamic params callbacks when one of them returns falure, see ros2/rclcpp#2735 for details

Description of documentation updates required from your changes

Description of how this change was tested


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@pepisg pepisg force-pushed the mppi-dynparam-fix branch from 43d379a to 8a1425a Compare February 6, 2025 22:47
@pepisg pepisg changed the title MPPI: Do not set params set success in dynamic params callback to allow changing params in other plugins MPPI: Do not set params success in dynamic params callback to allow changing params in other plugins Feb 6, 2025
Signed-off-by: pepisg <[email protected]>
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
nav2_mppi_controller/src/parameters_handler.cpp 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

@SteveMacenski SteveMacenski merged commit 5c763e2 into ros-navigation:main Feb 10, 2025
11 checks passed
mergify bot pushed a commit that referenced this pull request Feb 10, 2025
…hanging params in other plugins (#4908)

* remove result success from dyn param handler

Signed-off-by: pepisg <[email protected]>

* remove reason handling

Signed-off-by: pepisg <[email protected]>

* update tests

Signed-off-by: pepisg <[email protected]>

---------

Signed-off-by: pepisg <[email protected]>
(cherry picked from commit 5c763e2)

# Conflicts:
#	nav2_mppi_controller/src/parameters_handler.cpp
#	nav2_mppi_controller/test/parameter_handler_test.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants