-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Collision monitor toggle bt plugin #5532
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
base: main
Are you sure you want to change the base?
Collision monitor toggle bt plugin #5532
Conversation
Signed-off-by: David G <[email protected]>
Signed-off-by: David G <[email protected]>
Signed-off-by: David G <[email protected]>
Signed-off-by: David G <[email protected]>
@DavidG-Develop, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: David G <[email protected]>
@DavidG-Develop, your PR has failed to build. Please check CI outputs and resolve issues. |
@DavidG-Develop please heed CI and the merge conflict |
* @note This is an Asynchronous (long-running) node which may return a RUNNING state while executing. | ||
* It will re-initialize when halted. | ||
*/ | ||
class ToggleCollisionMonitorService : public BtServiceNode<nav2_msgs::srv::Toggle> |
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.
Make sure to open a PR against docs.nav2.org to add this to the Configuration Guide, Navigation Plugins page, and Migration Guide
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, I built the docs for the Configuration guide and Navigation plugins page, but I am not sure what to do in the migration guide? Also, I will wait until this gets merged so I can add the correct link inside the navigation plugins page before opening the PR towards the docs.
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.
Basically just make a new entry for the collision monitor can be toggled on / off, and also here's a behavior tree node to do it from a BT if you choose.
P.S. you may also be interested in adding this API to the Nav2 Simple Commander so that people can also call this from the Python3 API! That would also involve updating that page for the new function as well https://docs.nav2.org/commander_api/index.html
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, so under Kilted to L-turtle just short explanation of this and #5493 ?
For the Nav2 Simple Commander I can take a look, but I have never worked with it so I am not that familiar with the structure. It might take some time for me to implement it depending on my availability in the coming weeks. I will take a look and until the end of the week I will either confirm that I will be implementing it or that we should open an issue so someone else can pick it up.
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.
Correct!
The simple commander API is ... very simple. You can essentially follow this pattern in the making of changeMap
service function: https://github.com/ros-navigation/navigation2/blob/main/nav2_simple_commander/nav2_simple_commander/robot_navigator.py#L212-L213 & https://github.com/ros-navigation/navigation2/blob/main/nav2_simple_commander/nav2_simple_commander/robot_navigator.py#L1013-L1044
This pull request is in conflict. Could you fix it @DavidG-Develop? |
Signed-off-by: DavidG-Develop <[email protected]>
Signed-off-by: David G <[email protected]>
Signed-off-by: David G <[email protected]>
@SteveMacenski the CI fails seem not to be bound to my code, the circleCI fails on |
Retriggering CI to check on those failures. Otherwise, see above with the action item(s) Edit: flake8 passed now - must have been a networking fluke |
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
CI is good now - just focus on the API & docs elements and can merge! |
@SteveMacenski The docs are waiting for this to be merged so I can grab a working link for the plugins list on the Navigation Plugins page, and I need to do the short migration guide (will be done tmrw). |
We cannot merge this without a docs PR unfortunately. They are merged as a pair. Understood on the API. I’ll file a ticket to do that once we merge. |
Ok yep understood, than for the link I will just fill it so it points to where the file should land so than |
Sounds good! It’s easy for forgetful maintainers (I.e. me) to merge this and then forget about the docs PR or the docs PR needs fixing and gets stale so the main code isn’t documented. This point of process is mostly for the benefit to keep things 1:1 synchronized. |
Basic Info
Depends on #5493
Description of contribution in a few bullet points
Added bt plugin for toggling the collision monitor. The plugin wraps the BtServiceNode and calls the toggle service using
nav2_msgs::srv::Toggle
. The toggle control is done using a boolean input port which defines if the cm should be enabled or disabled.Description of documentation updates required from your changes
Should be added to the Navigation plugins -> Behavior tree nodes -> Action plugin list
Description of how this change was tested
Wrote a test for it (used previous tests as template).
Tested also by implementing a rudimentary service that this plugin can call and adding this plugin into the default tree to be called.
Future work that may be required in bullet points
For Maintainers:
backport-*
.