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

nav2_controller: stop mutex from deadlocking on parameter change #2858

Conversation

dkuenster
Copy link
Contributor

@dkuenster dkuenster commented Mar 22, 2022


Basic Info

Info Please fill out this column
Ticket(s) this addresses #2833
Primary OS tested on Debian
Robotic platform tested on gazebo simulation

Description of contribution in a few bullet points

  • Don't cause a deadlock when we try to dynamically change controller
    parameters while the controller is running.
  • Also provide a appropriate
    error message on failure to change parameters.

Description of documentation updates required from your changes

Shouldn't be necessary

Future work that may be required in bullet points

  • Maybe add correct handling of dynamic parameter changes to the plugins

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.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

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2022

@dkuenster, please properly fill in PR template in the future. @SteveMacenski, use this instead.

  • Check that any new parameters added are updated in navigation.ros.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

@SteveMacenski
Copy link
Member

Any update? Does my concern make sense or is there something I'm overlooking?

@dkuenster dkuenster force-pushed the nav2-controller-stop-mutex-from-deadlocking-on-parameter-change branch 2 times, most recently from e872603 to f02c60a Compare March 31, 2022 12:11
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamic Callback LGTM, but I think the main server should require a lock strictly since that's the main task and is latency sensitive

nav2_controller/src/controller_server.cpp Outdated Show resolved Hide resolved
@SteveMacenski
Copy link
Member

No worries, just circling back since I hadn't heard back in a bit! :smile

Don't cause a deadlock when we try to dynamically change controller
parameters while the controller is running. Also provide a appropriate
error message on failure to change parameters.
@dkuenster dkuenster force-pushed the nav2-controller-stop-mutex-from-deadlocking-on-parameter-change branch from f02c60a to 952a73f Compare April 4, 2022 11:41
@SteveMacenski SteveMacenski merged commit c8f6799 into ros-navigation:main Apr 4, 2022
@SteveMacenski
Copy link
Member

Awesome! Thanks! That return warning will be helpful too

redvinaa pushed a commit to EnjoyRobotics/navigation2 that referenced this pull request Jun 30, 2022
…-navigation#2858)

Don't cause a deadlock when we try to dynamically change controller
parameters while the controller is running. Also provide a appropriate
error message on failure to change parameters.

Co-authored-by: dkuenster <[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
Development

Successfully merging this pull request may close these issues.

2 participants