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

Ensure that plugin initialization to be called before updating routines #3307

Conversation

AlexeyMerzlyakov
Copy link
Collaborator

@AlexeyMerzlyakov AlexeyMerzlyakov commented Dec 2, 2022

Fixes the situations when Costmap2D plugin is being processed before initialization


Basic Info

Info Please fill out this column
Ticket(s) this addresses #3299
Primary OS tested on Ubuntu 20.04 with ROS2 Rolling (built from sources) onboard
Robotic platform tested on colcon test of nav2_costmap_2d and nav2_system_tests: no regressions

Description of contribution in a few bullet points

  • Ensure that plugin->updateCosts()/updateBounds() won't be called unless all plugins to be initialized: the Costmap2DROS::updateMap() is being processed in the main update loop only if stopped_ is false.
  • Added the regression test for this case. There is a new OrderLayer costmap plugin that sleeps in the activate() stage checking that parallel thread won't call updateCosts()/updateBound() routines before activation will be finished. The testcase creates Costmap2DROS object with this plugin enabled and then calls it though the main Costmap2DROS callback API. If plugin API call sequence is wrong, OrderLayer will throw an exception that will cause testcase to fail. If API call sequence is OK, testcase will be finished normally.

Description of documentation updates required from your changes

  • No documentation updates are required

Future work that may be required in bullet points

  • No future work is required

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 Dec 2, 2022

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

costmap_publisher_->publishCostmap();
last_publish_ = current_time;
// Execute after start() will complete plugins activation
if (!stopped_) {
Copy link
Collaborator Author

@AlexeyMerzlyakov AlexeyMerzlyakov Dec 2, 2022

Choose a reason for hiding this comment

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

In a good way, this change should be checked through std::atomic<bool>, as well as other parallel accessed variables, such as initialized_ (https://github.com/ros-planning/navigation2/blob/main/nav2_costmap_2d/src/costmap_2d_ros.cpp#L494 and https://github.com/ros-planning/navigation2/blob/main/nav2_costmap_2d/src/costmap_2d_ros.cpp#L527). This is rather a subject of separate ticket, I think.

@SteveMacenski SteveMacenski merged commit 571ca7f into ros-navigation:main Dec 6, 2022
jwallace42 pushed a commit to jwallace42/navigation2 that referenced this pull request Dec 14, 2022
SteveMacenski added a commit that referenced this pull request Dec 21, 2022
* remove exec_depend on behaviortree_cpp_v3 (#3279)

* BT Service Node to throw if service was not available in time (#3256)

* throw if service server wasn't available in time

mimic the behavior of the bt action node constructor

* throw if action unavailable in bt cancel action

* use chrono literals namespace

* fix linting errors

* fix code style divergence

* Remove duplicate of nav2_back_up_cancel_bt_node (#3332)

* Remove unused velocity scaling config from example xml (#3330)

Signed-off-by: Borong Yuan <[email protected]>

Signed-off-by: Borong Yuan <[email protected]>

* Make mapUpdateLoop() indicator variables to be thread-safe (#3308)

* Ensure that plugin initialization to be called before updating routines (#3307)

* Solve bug when CostmapInfoServer is reactivated (#3292)

* Solve bug when CostmapInfoServer is reactivated

* Implemented smoother selector bt node (#3283)

* Implemented smoother selector bt node

Signed-off-by: Owen Hooper <[email protected]>

* updated copyright in modified file

Signed-off-by: Owen Hooper <[email protected]>

Signed-off-by: Owen Hooper <[email protected]>

* Add allow_unknown parameter to theta star planner (#3286)

* Add allow unknown parameter to theta star planner

* Add allow unknown parameter to tests

* missing comma

* Change cost of unknown tiles

* Uncrustify

* bump to 1.1.3 for humble sync 3

Signed-off-by: Borong Yuan <[email protected]>
Signed-off-by: Owen Hooper <[email protected]>
Co-authored-by: Adam Aposhian <[email protected]>
Co-authored-by: Erwin Lejeune <[email protected]>
Co-authored-by: Sven Langner <[email protected]>
Co-authored-by: Borong Yuan <[email protected]>
Co-authored-by: Alexey Merzlyakov <[email protected]>
Co-authored-by: MartiBolet <[email protected]>
Co-authored-by: Owen Hooper <[email protected]>
Co-authored-by: Pedro Alejandro González <[email protected]>
andrewlycas pushed a commit to StratomInc/navigation2 that referenced this pull request Feb 23, 2023
shrijitsingh99 pushed a commit to moss-ag/navigation2 that referenced this pull request Mar 4, 2023
* remove exec_depend on behaviortree_cpp_v3 (ros-navigation#3279)

* BT Service Node to throw if service was not available in time (ros-navigation#3256)

* throw if service server wasn't available in time

mimic the behavior of the bt action node constructor

* throw if action unavailable in bt cancel action

* use chrono literals namespace

* fix linting errors

* fix code style divergence

* Remove duplicate of nav2_back_up_cancel_bt_node (ros-navigation#3332)

* Remove unused velocity scaling config from example xml (ros-navigation#3330)

Signed-off-by: Borong Yuan <[email protected]>

Signed-off-by: Borong Yuan <[email protected]>

* Make mapUpdateLoop() indicator variables to be thread-safe (ros-navigation#3308)

* Ensure that plugin initialization to be called before updating routines (ros-navigation#3307)

* Solve bug when CostmapInfoServer is reactivated (ros-navigation#3292)

* Solve bug when CostmapInfoServer is reactivated

* Implemented smoother selector bt node (ros-navigation#3283)

* Implemented smoother selector bt node

Signed-off-by: Owen Hooper <[email protected]>

* updated copyright in modified file

Signed-off-by: Owen Hooper <[email protected]>

Signed-off-by: Owen Hooper <[email protected]>

* Add allow_unknown parameter to theta star planner (ros-navigation#3286)

* Add allow unknown parameter to theta star planner

* Add allow unknown parameter to tests

* missing comma

* Change cost of unknown tiles

* Uncrustify

* bump to 1.1.3 for humble sync 3

Signed-off-by: Borong Yuan <[email protected]>
Signed-off-by: Owen Hooper <[email protected]>
Co-authored-by: Adam Aposhian <[email protected]>
Co-authored-by: Erwin Lejeune <[email protected]>
Co-authored-by: Sven Langner <[email protected]>
Co-authored-by: Borong Yuan <[email protected]>
Co-authored-by: Alexey Merzlyakov <[email protected]>
Co-authored-by: MartiBolet <[email protected]>
Co-authored-by: Owen Hooper <[email protected]>
Co-authored-by: Pedro Alejandro González <[email protected]>
shrijitsingh99 pushed a commit to moss-ag/navigation2 that referenced this pull request Mar 4, 2023
* remove exec_depend on behaviortree_cpp_v3 (ros-navigation#3279)

* BT Service Node to throw if service was not available in time (ros-navigation#3256)

* throw if service server wasn't available in time

mimic the behavior of the bt action node constructor

* throw if action unavailable in bt cancel action

* use chrono literals namespace

* fix linting errors

* fix code style divergence

* Remove duplicate of nav2_back_up_cancel_bt_node (ros-navigation#3332)

* Remove unused velocity scaling config from example xml (ros-navigation#3330)

Signed-off-by: Borong Yuan <[email protected]>

Signed-off-by: Borong Yuan <[email protected]>

* Make mapUpdateLoop() indicator variables to be thread-safe (ros-navigation#3308)

* Ensure that plugin initialization to be called before updating routines (ros-navigation#3307)

* Solve bug when CostmapInfoServer is reactivated (ros-navigation#3292)

* Solve bug when CostmapInfoServer is reactivated

* Implemented smoother selector bt node (ros-navigation#3283)

* Implemented smoother selector bt node

Signed-off-by: Owen Hooper <[email protected]>

* updated copyright in modified file

Signed-off-by: Owen Hooper <[email protected]>

Signed-off-by: Owen Hooper <[email protected]>

* Add allow_unknown parameter to theta star planner (ros-navigation#3286)

* Add allow unknown parameter to theta star planner

* Add allow unknown parameter to tests

* missing comma

* Change cost of unknown tiles

* Uncrustify

* bump to 1.1.3 for humble sync 3

Signed-off-by: Borong Yuan <[email protected]>
Signed-off-by: Owen Hooper <[email protected]>
Co-authored-by: Adam Aposhian <[email protected]>
Co-authored-by: Erwin Lejeune <[email protected]>
Co-authored-by: Sven Langner <[email protected]>
Co-authored-by: Borong Yuan <[email protected]>
Co-authored-by: Alexey Merzlyakov <[email protected]>
Co-authored-by: MartiBolet <[email protected]>
Co-authored-by: Owen Hooper <[email protected]>
Co-authored-by: Pedro Alejandro González <[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