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

Fix bt_navigator crashes when restarted #3546

Conversation

BriceRenaudeau
Copy link
Contributor

Basic Info

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

Description of contribution in a few bullet points

  • move declare_parameter("navigators") in the constructor
  • remove useless default_navigator_types

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

@BriceRenaudeau BriceRenaudeau changed the title Update bt_navigator.cpp Fix bt_navigator crashes when restarted Apr 28, 2023
get_parameter("navigators", navigator_ids);
if (navigator_ids == default_navigator_ids) {
Copy link
Member

Choose a reason for hiding this comment

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

This block is strictly required to declare the types of the defaults. This is non-optional. Please readd default_navigator_types definition and this block. You can use the nav2_utils::declare_if_not_declared function so that it'll only redeclare on lifecycle transition if not already declared.

You could actually do that for navigators too so that you didn't have to change anything other than 2 lines. That would actually be closer in line with best practices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using declare_if_not_declared was my first thought but it doesn't exist anymore.

Maybe default navigators are not necessary (yaml is always required) or I can also declare them in the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what you mean by that, we have that in quite a few places in Nav2 😄

https://github.com/ros-planning/navigation2/blob/main/nav2_navfn_planner/src/navfn_planner.cpp#L84

Its a nav2_utils function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I was looking for declare_if_not_declared instead of declare_parameter_if_not_declared ...
I will make the modification.

@SteveMacenski
Copy link
Member

Just minor fix to merge :-)

@SteveMacenski
Copy link
Member

SteveMacenski commented May 2, 2023

@BriceRenaudeau every single system test failed - maybe you should rebase? I also retriggered the tests in case it was a fluke. It might be a problem with CI but I'm having issues with building the nightlys to even check relative. Rebase and push and if its still failing I'll take it as an action item to figure out why

@SteveMacenski SteveMacenski merged commit c407f88 into ros-navigation:main May 2, 2023
@SteveMacenski
Copy link
Member

Actually I confirmed this was happening in other days builds. I'll need to look into that. Thanks!

jwallace42 pushed a commit to jwallace42/navigation2 that referenced this pull request May 9, 2023
* Update bt_navigator.cpp

* Clean uncrestify

* Use declare_parameter_if_not_declared
enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
* Update bt_navigator.cpp

* Clean uncrestify

* Use declare_parameter_if_not_declared

Signed-off-by: enricosutera <enricosutera@outlook.com>
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