-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Move dwb goal/progress checker plugins to nav2_controller #1857
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
Move dwb goal/progress checker plugins to nav2_controller #1857
Conversation
Signed-off-by: Siddarth Gore <[email protected]>
05db38d to
d7979ee
Compare
|
Check the linters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few more things I want to think about, but this looks good as a concept. There's a few topics below to discuss and after those are fixed, we can get into the more logic update items I'm thinking about.
|
Thanks for taking this on! Looks good conceptually, after those things are fixed I'll narrow more into the nav2_controller server to make sure the resets / updates look good. |
Address review comments Signed-off-by: Siddarth Gore <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1857 +/- ##
==========================================
- Coverage 69.25% 68.69% -0.57%
==========================================
Files 218 218
Lines 10604 10609 +5
==========================================
- Hits 7344 7288 -56
- Misses 3260 3321 +61
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last few issues - checked the logic and I'm good with everything else.
- Doc updates
- Plugin param reading consistency
Use new plugin declaration format and doc update Signed-off-by: Siddarth Gore <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK just some logistics left. Adding params for plugins and namespace corrections to the yaml files and the above request for documentation.
@Marwan99 can you sanity check me that these updates in the md file and website are correct?
Also remove redundent file from dwb_plugins Signed-off-by: Siddarth Gore <[email protected]>
|
I think the params below need to be removed (sorry couldn't comment on them in a review nor link them here)
|
Signed-off-by: Siddarth Gore <[email protected]>
|
@Marwan99 please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the previous comments, just a couple of minor things. Along with the two mentioned below and this also needs to be documented (it is not related to this work but we might as well add it now).
Also please don't forget to remove the param below
.goal_checker_name in dwb_local_planner
Signed-off-by: Siddarth Gore <[email protected]>
Signed-off-by: Siddarth Gore <[email protected]>
declare .plugin only if not declared before Signed-off-by: Siddarth Gore <[email protected]>
|
@Marwan99 please take another look! |
| nav2_core::ProgressChecker * pc = new SimpleProgressChecker; | ||
| pc->reset(); | ||
| delete pc; | ||
| EXPECT_TRUE(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how the reset method is being tested. Also, do we need EXPECT_TRUE(true);?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure it doesn't crash as the next test is dependent on it. EXPECT_TRUE is used as a convention for returning from TEST functions. I have based these tests on the already available goal_checker tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah alright, thanks for explaining it:)
doc/parameters/param_list.md
Outdated
| | ----------| --------| ------------| | ||
| | controller_frequency | 20.0 | Frequency to run controller | | ||
| | progress_checker_plugin | "progress_checker" | Check the progress of the robot | | ||
| | progress_checker.plugin | "nav2_controller::SimpleProgressChecker" | Default plugin | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along side mentioning that this is default plugin class for progress_checker, can we mention that "progress_checker" in progress_checker.plugin parameter name is coming from the progress_checker_plugin parameter? And that it should correspond to whatever plugin class name intended to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I was wondering what the right convention is here. <progress checker_plugin>.plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would work - I think you could also just check what was done for the planner / controller plugins that were already updated and just mirror it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. No other .plugin param is mentioned in this file yet.
doc/parameters/param_list.md
Outdated
| | progress_checker_plugin | "progress_checker" | Check the progress of the robot | | ||
| | progress_checker.plugin | "nav2_controller::SimpleProgressChecker" | Default plugin | | ||
| | goal_checker_plugin | "goal_checker" | Check if the goal has been reached | | ||
| | goal_checker.plugin | "nav2_controller::SimpleGoalChecker" | Default plugin | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
doc/parameters/param_list.md
Outdated
| | Parameter | Default | Description | | ||
| | ----------| --------| ------------| | ||
| | controller_frequency | 20.0 | Frequency to run controller | | ||
| | progress_checker_plugin | "progress_checker" | Check the progress of the robot | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explain what is meant by progress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a suggested text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin used by the controller to check whether the robot has at least covered a set distance/displacement in a set amount of time, thus checking the progress of the robot. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: Siddarth Gore <[email protected]>
|
Awesome, thanks! |
* Move dwb goal/progress checker plugins to nav2_controller Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Address review comments Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Use new plugin declaration format and doc update Signed-off-by: Siddarth Gore <[email protected]> * Update bringup.yaml for new plugins in nav2_controller Also remove redundent file from dwb_plugins Signed-off-by: Siddarth Gore <[email protected]> * Fix doc errors and update remaining yaml files Signed-off-by: Siddarth Gore <[email protected]> * Remove mention of goal_checker from dwb docs Signed-off-by: Siddarth Gore <[email protected]> * Add .plugin params to doc Signed-off-by: Siddarth Gore <[email protected]> * Tests for progress_checker plugin declare .plugin only if not declared before Signed-off-by: Siddarth Gore <[email protected]> * Tweak plugin names/description in doc Signed-off-by: Siddarth Gore <[email protected]>
* Add slam toolbox as exec dep for nav2_bringup (#1827) * Add slam toolbox as exec dep * Added slam toolbox * Changed version of slam toolbox to foxy-devel * Added modifications to allow for exec depend of slam toolbox without breaking docker / CI * reformatted skip keys * Added tabs to skip key arguments * Removed commented out code block * Add unit tests for follow path, compute path to pose, and navigate to pose BT action nodes (#1844) * Add compute path to pose unit tests Signed-off-by: Sarthak Mittal <[email protected]> * Add follow path action unit tests Signed-off-by: Sarthak Mittal <[email protected]> * Add navigate to pose action unit tests Signed-off-by: Sarthak Mittal <[email protected]> * adding foxy build icons to readme (#1853) * adding foxy build icons * adding dividers * sync master from foxy version updates (#1852) * sync master from foxy version updates * syncing foxy and master * Add unit tests for clear entire costmap and reinitialize global localization BT service nodes (#1845) * Add clear entire costmap service unit tests Signed-off-by: Sarthak Mittal <[email protected]> * Add reinitialize global localization service unit test Signed-off-by: Sarthak Mittal <[email protected]> * Temporarily disable dump_params test (#1856) Signed-off-by: Sarthak Mittal <[email protected]> * commenting out unused validPointPotential (#1854) * Adding ROS2 versions to issue template (#1861) * reload behavior tree before creating RosTopicLogger to prevent nullptr error or no /behavior_tree_log problem (#1849) Signed-off-by: Daisuke Sato <[email protected]> * Add citation for IROS paper (#1867) * Add citation We'll want to add the DOI once published * Bump citation section above build status * Fix line breaks * Changes RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED to RCUTILS_LOGGING_BUFFERED_STREAM (#1868) * Added parameters to configure amcl laser scan topic (#1870) * Added parameters to configure amcl topic * changed parameter to scan_topic and added to all the configuration files * added scan_topic amcl param to docs * Satisfied linter * Move dwb goal/progress checker plugins to nav2_controller (#1857) * Move dwb goal/progress checker plugins to nav2_controller Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Address review comments Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Use new plugin declaration format and doc update Signed-off-by: Siddarth Gore <[email protected]> * Update bringup.yaml for new plugins in nav2_controller Also remove redundent file from dwb_plugins Signed-off-by: Siddarth Gore <[email protected]> * Fix doc errors and update remaining yaml files Signed-off-by: Siddarth Gore <[email protected]> * Remove mention of goal_checker from dwb docs Signed-off-by: Siddarth Gore <[email protected]> * Add .plugin params to doc Signed-off-by: Siddarth Gore <[email protected]> * Tests for progress_checker plugin declare .plugin only if not declared before Signed-off-by: Siddarth Gore <[email protected]> * Tweak plugin names/description in doc Signed-off-by: Siddarth Gore <[email protected]> * Follow pose (#1859) * Truncate path finished Signed-off-by: Francisco Martin Rico <[email protected]> * Follow Pose finished Signed-off-by: Francisco Martin Rico <[email protected]> * Change names. Add test and doc Signed-off-by: Francisco Martin Rico <[email protected]> * Change names and check atan2 values Signed-off-by: Francisco Martin Rico <[email protected]> * Checking Inf/NaNs and trucate path changes Signed-off-by: Francisco Martin Rico <[email protected]> * Revert changes in launcher Signed-off-by: Francisco Martin Rico <[email protected]> * Documenting Tree Signed-off-by: Francisco Martin Rico <[email protected]> * Update nav2_bt_navigator/README.md Co-authored-by: Steve Macenski <[email protected]> * Update nav2_bt_navigator/README.md Co-authored-by: Steve Macenski <[email protected]> * Change TruncatePath from decorator to action node Signed-off-by: Francisco Martin Rico <[email protected]> * Update nav2_bt_navigator/README.md Co-authored-by: Steve Macenski <[email protected]> Co-authored-by: Steve Macenski <[email protected]> * Remove Deprecated Declaration (#1884) * Remove deprecated rclcpp::executor::FutureReturnCode::SUCCESS in favor of rclcpp::FutureReturnCode::SUCCESS Signed-off-by: Hunter L. Allen <[email protected]> * Update nav2_util/include/nav2_util/service_client.hpp Co-authored-by: Sarthak Mittal <[email protected]> Co-authored-by: Sarthak Mittal <[email protected]> * Added new costmap buffer to resize map safely (#1837) * Added new costmap buffer to resize map safely Signed-off-by: Aitor Miguel Blanco <[email protected]> * Update map if the layer is not being processed. Signed-off-by: Aitor Miguel Blanco <[email protected]> * Updated bool name and added buffer initialization Signed-off-by: Aitor Miguel Blanco <[email protected]> * Fix dump params tests (#1886) Signed-off-by: Sarthak Mittal <[email protected]> * Move definitions in tf_help to separate src cpp file (#1890) * Move definitions of functions in tf_help to separate src cpp file to avoid multiple definition error * Fix linting issues * Make uncrustify happier * More format fixing * resolved the simulated motion into its x & y components (#1887) Signed-off-by: Marwan Taher <[email protected]> * Fix nav2_bt_navigator cleanup (#1901) Signed-off-by: Sarthak Mittal <[email protected]> * Adding some more test coverage (#1903) * more tests * removing old cmake * remove on_errors in specific servers in favor of a general lifecycle warning (#1904) * remove on_errors in favor of a general lifecycle warning * adding removed thing * simply nodes to remove lines (#1907) * Bunches of random new tests (#1909) * trying to get A* to work again * make flake 8 happy * adding cancel and preempt test * planner tests use A* * adding A* note * test with topic * Adding failure to navigate test (#1912) * failures tests * adding copyrights * cancel test in recoveries * Costmap lock while copying data in navfn planner (#1913) * acquire the costmap lock while copying data in navfn planner Signed-off-by: Daisuke Sato <[email protected]> * add costmap lock to dwb local plannger Signed-off-by: Daisuke Sato <[email protected]> * Added map_topic parameters to static layer and amcl (#1910) * see if spin some in cancel will allow result callback (#1914) * Adding near-complete voxel grid test coverage and more to controller server (#1915) * remove erraneous handling done by prior * adding a bunch of voxel unit tests * retrigger * adding waypoint follower failure test, voxel layer integration tests, etc (#1916) * adding waypoint follower failure test * adding voxel, more logging * reset -> clear * minor changes to lower redundent warnings (#1918) * Costmap plugins declare if not declared for reset capabilities (#1919) * fixing #1917 on declare if not declared * fix API * adding CLI test (#1920) * More coverage in map server tests (#1921) * adding CLI test * adding a bunch of new coverages for map_server * Add in range sensor costmap layer (#1888) * range costmap building * range sensor layer working * nav2 costmap pass linter and uncrustify tests * Added back semicolon to range class * Added docs * Added angles dependency * Added BSD license * Added BSD license * Made functions inline * revmoed get_clock * added input_sensor_type to docs * Made defualt topic name empty to cause error * using chorno literal to denote time units * Added small initial test * Fixed linter error, line breaks, enum, logger level, and transform_tolerance issue * fixed segmentation fault in test and added transfrom tolerances to tf_->transform * Got test to pass * Added more tests * removed incorrect parameter declaration * Improved marking while moving tests and added clearing tests * removed general clearing test * changed testing helper import in test * [Test sprint] push nav2_map_server over 90% (#1922) * adding CLI test * adding tests for more lines to map_server * fix last test * adding out of bounds and higher bounds checks * adding planner tests for plugins working * add cleanup * working * ping * [testing sprint] final test coverage and debug logging in planner/controller servers (#1924) * adding CLI test * adding tests for more lines to map_server * fix last test * adding out of bounds and higher bounds checks * adding planner tests for plugins working * add cleanup * working * ping * adding more testing and debug info messages * [testing sprint] remove dead code not used in 10 years from navfn (#1926) * remove dead code not used in 10 years from navfn * ping * inverting period to rate * removing debug statement that could never be triggered by a single non-looping action server * type change * adding removed files * bump to 0.4.2 * add another missing file * add missing files * remove some tests * lint * removing nav2_bringup from binaries Co-authored-by: Michael Equi <[email protected]> Co-authored-by: Sarthak Mittal <[email protected]> Co-authored-by: Daisuke Sato <[email protected]> Co-authored-by: Ruffin <[email protected]> Co-authored-by: Siddarth Gore <[email protected]> Co-authored-by: Francisco Martín Rico <[email protected]> Co-authored-by: Hunter L. Allen <[email protected]> Co-authored-by: Aitor Miguel Blanco <[email protected]> Co-authored-by: Joe Smallman <[email protected]> Co-authored-by: Marwan Taher <[email protected]>
* Move dwb goal/progress checker plugins to nav2_controller Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Address review comments Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Use new plugin declaration format and doc update Signed-off-by: Siddarth Gore <[email protected]> * Update bringup.yaml for new plugins in nav2_controller Also remove redundent file from dwb_plugins Signed-off-by: Siddarth Gore <[email protected]> * Fix doc errors and update remaining yaml files Signed-off-by: Siddarth Gore <[email protected]> * Remove mention of goal_checker from dwb docs Signed-off-by: Siddarth Gore <[email protected]> * Add .plugin params to doc Signed-off-by: Siddarth Gore <[email protected]> * Tests for progress_checker plugin declare .plugin only if not declared before Signed-off-by: Siddarth Gore <[email protected]> * Tweak plugin names/description in doc Signed-off-by: Siddarth Gore <[email protected]>
The plan recieved is usually in global frame, but our local costmap is often in odom frame. This fixes a regression from #1857
The plan recieved is usually in global frame, but our local costmap is often in odom frame. This fixes a regression from #1857
* waypoint_follower node has _rclcpp_node as well as base node (#1940) * Add #include <vector> for vector<> (#1946) To fix cpplint * Add 'angles' dependency to nav2_costmap_2d package.xml (#1947) * transform goal to costmap frame (#1949) The plan recieved is usually in global frame, but our local costmap is often in odom frame. This fixes a regression from #1857 * Add mutex lock into inflation layer to avoid thread issue in updating footprint (#1952) Signed-off-by: Daisuke Sato <[email protected]> * Fix being unable to change StandardTrajectoryGenerator parameter vtheta_samples (#1619) * Fix tests declaring parameters real nodes don't Signed-off-by: Shane Loretz <[email protected]> * Fix loadParameterWithDeprecation not getting initial parameter values Signed-off-by: Shane Loretz <[email protected]> * Create sim_time variable before using it Signed-off-by: Shane Loretz <[email protected]> * Line length < 100 Signed-off-by: Shane Loretz <[email protected]> * Add missing { Signed-off-by: Shane Loretz <[email protected]> * Linter fixes Signed-off-by: Shane Loretz <[email protected]> * sim_granularity -> time_granularity Signed-off-by: Shane Loretz <[email protected]> * Linter fix Signed-off-by: Shane Loretz <[email protected]> * update version to 0.4.3 * removing redundant dep on angles Co-authored-by: Ruffin <[email protected]> Co-authored-by: Sarthak Mittal <[email protected]> Co-authored-by: Michael Ferguson <[email protected]> Co-authored-by: Daisuke Sato <[email protected]> Co-authored-by: Shane Loretz <[email protected]>
…tion#1857) * Move dwb goal/progress checker plugins to nav2_controller Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Address review comments Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Use new plugin declaration format and doc update Signed-off-by: Siddarth Gore <[email protected]> * Update bringup.yaml for new plugins in nav2_controller Also remove redundent file from dwb_plugins Signed-off-by: Siddarth Gore <[email protected]> * Fix doc errors and update remaining yaml files Signed-off-by: Siddarth Gore <[email protected]> * Remove mention of goal_checker from dwb docs Signed-off-by: Siddarth Gore <[email protected]> * Add .plugin params to doc Signed-off-by: Siddarth Gore <[email protected]> * Tests for progress_checker plugin declare .plugin only if not declared before Signed-off-by: Siddarth Gore <[email protected]> * Tweak plugin names/description in doc Signed-off-by: Siddarth Gore <[email protected]>
The plan recieved is usually in global frame, but our local costmap is often in odom frame. This fixes a regression from ros-navigation#1857
…tion#1857) * Move dwb goal/progress checker plugins to nav2_controller Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Address review comments Signed-off-by: Siddarth Gore <[email protected]> * Move goal/progress checker plugins to nav2_controller Use new plugin declaration format and doc update Signed-off-by: Siddarth Gore <[email protected]> * Update bringup.yaml for new plugins in nav2_controller Also remove redundent file from dwb_plugins Signed-off-by: Siddarth Gore <[email protected]> * Fix doc errors and update remaining yaml files Signed-off-by: Siddarth Gore <[email protected]> * Remove mention of goal_checker from dwb docs Signed-off-by: Siddarth Gore <[email protected]> * Add .plugin params to doc Signed-off-by: Siddarth Gore <[email protected]> * Tests for progress_checker plugin declare .plugin only if not declared before Signed-off-by: Siddarth Gore <[email protected]> * Tweak plugin names/description in doc Signed-off-by: Siddarth Gore <[email protected]>
Signed-off-by: Siddarth Gore [email protected]
Basic Info