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

Dynamic reconfigure for AMCL #2932

Merged
merged 14 commits into from
May 9, 2022

Conversation

padhupradheep
Copy link
Member

@padhupradheep padhupradheep commented May 3, 2022


Basic Info

Info Please fill out this column
Ticket(s) this addresses #956
Primary OS tested on Ubuntu
Robotic platform tested on TB3-Simulation

I'm really not sure, if I'm doing justice to the Mutex.. @SteveMacenski
I gave it a test and seems to work fine though... I will take a look once again, before I make this PR to be a "ready for review"

@padhupradheep padhupradheep marked this pull request as draft May 3, 2022 11:53
@mergify
Copy link
Contributor

mergify bot commented May 3, 2022

@padhupradheep, 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

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.

In terms of the actual function, it looks good except where noted!

However, AMCL has alot of objects that use these parameters that will need to be reinitialized / reset / new values given to it. Simply setting the variable in the callback doesn't make it used in the actual node, most, if not all, of these parameters are passed to other objects for use in the node which aren't going to be affected by the current PR. You can see some examples of that in the Smac Planner dynamic reconfiguration callback & should probably look at the AMCL code closer about where each parameter is used so that you can update the appropriate objects / re-instantiate them.

nav2_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
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.

Really good, much better actually than what I had in mind if I did it, kudos!

nav2_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
nav2_amcl/src/amcl_node.cpp Show resolved Hide resolved
Copy link
Contributor

@doisyg doisyg left a comment

Choose a reason for hiding this comment

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

Overall looks good, left 2 potential improvements and a question for debate

nav2_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
nav2_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
nav2_amcl/src/amcl_node.cpp Outdated Show resolved Hide resolved
@SteveMacenski
Copy link
Member

Toggling to get CI to turn over

@padhupradheep
Copy link
Member Author

Thanks for the feedback @doisyg

@padhupradheep padhupradheep marked this pull request as ready for review May 9, 2022 09:13
@mergify
Copy link
Contributor

mergify bot commented May 9, 2022

@padhupradheep, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

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.

Looks good! Thanks for taking this on, and that's a wrap folks, we have dynamic parameter support across the stack now!

@SteveMacenski SteveMacenski merged commit 14f02a4 into ros-navigation:main May 9, 2022
@doisyg
Copy link
Contributor

doisyg commented May 9, 2022

Amazing!
(and to continue and the subject, we may open soon an issue with @anaelle-sw about a potential race condition in the way dyn parameters are implemented in plugins when using "on new parameter" event, but that's for another day)

redvinaa pushed a commit to EnjoyRobotics/navigation2 that referenced this pull request Jun 30, 2022
* not finished

* update

* satisfying linters

* add mutex

* fix mistake

* dealing each parameters

* segregating variables and instantiating the objects correspondingly

* remove unwanted mutex

* reconfigure for map topic

* review update

* careful updation of min and max particles

* resetting and reinitilaizing the laser scanner and message filters

* fixing the mutex
@padhupradheep padhupradheep deleted the dynamic-amcl branch July 22, 2022 07:56
dimaxano pushed a commit to cmrobotics/navigation2 that referenced this pull request Jan 4, 2023
* not finished

* update

* satisfying linters

* add mutex

* fix mistake

* dealing each parameters

* segregating variables and instantiating the objects correspondingly

* remove unwanted mutex

* reconfigure for map topic

* review update

* careful updation of min and max particles

* resetting and reinitilaizing the laser scanner and message filters

* fixing the mutex
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.

3 participants