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 (backport #4908) #4913

Closed
wants to merge 1 commit into from

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Feb 10, 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

This is an automatic backport of pull request #4908 done by [Mergify](https://mergify.com).

…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
@mergify mergify bot added the conflicts label Feb 10, 2025
Copy link
Contributor Author

mergify bot commented Feb 10, 2025

Cherry-pick of 5c763e2 has failed:

On branch mergify/bp/jazzy/pr-4908
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 5c763e22.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   nav2_mppi_controller/src/parameters_handler.cpp
	both modified:   nav2_mppi_controller/test/parameter_handler_test.cpp

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Copy link
Contributor Author

mergify bot commented Feb 10, 2025

@mergify[bot], all pull requests must be targeted towards the main development branch.
Once merged into main, it is possible to backport to @jazzy, but it must be in main
to have these changes reflected into new distributions.

@SteveMacenski SteveMacenski deleted the mergify/bp/jazzy/pr-4908 branch February 10, 2025 19:45
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.

2 participants