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

Enable reloading BT xml file with same name #4209

Merged

Conversation

huemerj
Copy link
Contributor

@huemerj huemerj commented Mar 22, 2024


Basic Info

Info Please fill out this column
Ticket(s) this addresses None
Primary OS tested on Ubuntu
Robotic platform tested on Inhouse AGV & Gazebo
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Current state: In the current implementation of the BtActionServer, a new BT is not loaded if its xml name is the same as the current one. This means that if you want to change and then test a BT xml file (while keeping the same filename), you need to restart the action server each time the file changes.

  • This PR: Adds a boolean parameter always_reload_bt_xml (defaults to false) to decide if a new BT should always be loaded, even if the xml names are the same. This allows keeping the action server running while changing/developing the xml file.

Description of documentation updates required from your changes

  • Might be necessary to add the new parameter always_reload_bt_xml to the Behavior-Tree Navigator docs.

Future work that may be required in bullet points

  • Would be nice to backport this PR to Humble/Iron.

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

Copy link
Contributor

mergify bot commented Mar 22, 2024

This pull request is in conflict. Could you fix it @huemerj?

Copy link
Contributor

mergify bot commented Mar 22, 2024

@huemerj, 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).

2 similar comments
Copy link
Contributor

mergify bot commented Mar 22, 2024

@huemerj, 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
Contributor

mergify bot commented Mar 22, 2024

@huemerj, 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.

Rename to always_reload_bt_xml and add to the Nav2 configuration / migration guides and this LGTM

Please heed the DCO bot (sign commits -s)

Copy link
Contributor

mergify bot commented Mar 23, 2024

@huemerj, 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).

@huemerj
Copy link
Contributor Author

huemerj commented Mar 23, 2024

Rename to always_reload_bt_xml and add to the Nav2 configuration / migration guides and this LGTM

Please heed the DCO bot (sign commits -s)

Done. ros-navigation/docs.nav2.org#534

@SteveMacenski
Copy link
Member

SteveMacenski commented Mar 23, 2024

@huemerj please heed the DCO bot, all commits need to be signed, then I can merge this. If you click on the CI job, it'll give you instructions about how to adjust your previous commits

I'm going to merge this but I'd like to note for future onlookers of this: This is useful for the context @huemerj brings it up in and probably others. If you're using this because you have a BT that isn't properly resetting its state with your custom BT nodes, so using this to hard reset everything, I highly suggest fixing your underlying issues which could be causing other issues on the blackboard which could possibly persist in some cases. This may seem like an easy solution, but I suggest spending a little bit of time to look into the proper solution, especially in robotics where small mistakes in odd cases can cause real headaches.

christophfroehlich and others added 3 commits March 25, 2024 08:10
Co-authored-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>
@christophfroehlich
Copy link
Contributor

@SteveMacenski we fixed the DCO now.

@SteveMacenski SteveMacenski merged commit a1997db into ros-navigation:main Mar 25, 2024
7 of 9 checks passed
@christophfroehlich christophfroehlich deleted the reload_xml_master branch March 25, 2024 18:51
christophfroehlich added a commit to christophfroehlich/navigation2 that referenced this pull request Mar 26, 2024
* Let BtActionServer overwrite xml

Co-authored-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>

* Make a ROS parameter for it

Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>

* Rename flag to always reload BT xml file

Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>

---------

Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
@huemerj
Copy link
Contributor Author

huemerj commented Mar 27, 2024

@SteveMacenski Would be nice if you could please backport this to Humble/Iron.

@SteveMacenski
Copy link
Member

Next release cycle it will be

ajtudela pushed a commit to grupo-avispa/navigation2 that referenced this pull request Apr 19, 2024
* Let BtActionServer overwrite xml

Co-authored-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>

* Make a ROS parameter for it

Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>

* Rename flag to always reload BT xml file

Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>

---------

Signed-off-by: Johannes Huemer <johannes.huemer@ait.ac.at>
Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
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.

None yet

3 participants