-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add plugins Parameter to BT XML for Selective Clearing of Costmap Layers #5548
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?
Add plugins Parameter to BT XML for Selective Clearing of Costmap Layers #5548
Conversation
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Codecov Report❌ Patch coverage is
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/clear_costmap_service.hpp
Outdated
Show resolved
Hide resolved
nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/clear_costmap_service.hpp
Outdated
Show resolved
Hide resolved
nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/clear_costmap_service.hpp
Outdated
Show resolved
Hide resolved
nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/clear_costmap_service.hpp
Outdated
Show resolved
Hide resolved
nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/clear_costmap_service.hpp
Show resolved
Hide resolved
…lear_costmap_service.hpp Co-authored-by: Sushant Chavan <[email protected]> Signed-off-by: Burak Can Kaya <[email protected]>
…lear_costmap_service.hpp Co-authored-by: Sushant Chavan <[email protected]> Signed-off-by: Burak Can Kaya <[email protected]>
…lear_costmap_service.hpp Co-authored-by: Sushant Chavan <[email protected]> Signed-off-by: Burak Can Kaya <[email protected]>
…lear_costmap_service.hpp Co-authored-by: Sushant Chavan <[email protected]> Signed-off-by: Burak Can Kaya <[email protected]> Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
163e2d5
to
b03ad78
Compare
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
9aa9b0b
to
584d698
Compare
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
thanks for reviews @Sushant-Chavan |
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.
LGTM.
Awaiting Steve's comment regarding the response from the service in case a requested costmap plugin could not be cleared.
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
…ce responses Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
A number of tests related to code changes are now failing |
…ng cleared Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
If a requested plugin is not clearable, is it enough to skip it and reset the master, since I already log it? @Sushant-Chavan I’d appreciate your review on this |
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
I think if atleast one layer was reset, we should reset the master costmap. Otherwise we shouldn't. I also think that once you find a non-clearable plugin that was requested to be cleared by the client, you should stop clearing any further requested plugins. Then reset the master costmap (if any previous plugins were reset), and return a failure response. Do you agree @SteveMacenski ? |
i agree too
I think the service response should only return false if a plugin is clearable but fails to clear.
|
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
Signed-off-by: BCKSELFDRIVEWORLD <[email protected]>
I agree. I believe that @BCKSELFDRIVEWORLD implemented this in the last few commits, please re-review @Sushant-Chavan when you have a moment. Thanks for your time! |
Summary i belive that true implementation: Return false when:
Return true when:
Skipping non-clearable layers should not by itself cause the service to return false. user may provide a mixed list of plugins, and as long as at least one clearable layer was cleared, the request is considered successful. Our only additional responsibility in these cases is to log or inform that some plugins were skipped, without treating that as a failure condition. |
I think I agree with @Sushant-Chavan that if a user requests layers to clear explicitly (rather than 'clear all') all of those should be valid. If its not valid, its not a valid request and that should be made clear to a caller in more than just logging. Whether we fail immediately or continue (and log all errors) and then return |
@BCKSELFDRIVEWORLD any update? I'd love to get this in this week! |
PR #5400 was broken due to my mistake
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*
.