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.

Sign up for GitHub

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

Jump to bottom

Add flag to not send request in BTServiceNode #3431

Merged
merged 10 commits into from
Mar 20, 2023
Merged

Add flag to not send request in BTServiceNode #3431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
48217c2
Add flag to not send request in BTServiceNode
Feb 28, 2023
a31b394
rename goal to request
Feb 28, 2023
a68c971
Merge remote-tracking branch 'upstream/main'
Mar 10, 2023
67cf53e
Merge branch 'main' into add-flag-to-not-send-request-BTServiceNode
Mar 17, 2023
3a86c66
Fail if should not send goal
Mar 17, 2023
05fd124
Update nav2_behavior_tree/include/nav2_behavior_tree/bt_service_node.hpp
Mar 20, 2023
f52b108
Update nav2_behavior_tree/include/nav2_behavior_tree/bt_service_node.hpp
Mar 20, 2023
a24277d
.
Mar 20, 2023
60f47bd
fix linter
Mar 20, 2023
b47ddd9
fix CI
tonynajjar Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,10 @@ class BtActionNode : public BT::ActionNodeBase
// user defined callback, may modify "should_send_goal_".
on_tick();

if (should_send_goal_) {
send_new_goal();
if (!should_send_goal_) {
return BT::NodeStatus::FAILURE;
}
send_new_goal();
}

try {
Expand Down
13 changes: 13 additions & 0 deletions nav2_behavior_tree/include/nav2_behavior_tree/bt_service_node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,17 @@ class BtServiceNode : public BT::ActionNodeBase
BT::NodeStatus tick() override
{
if (!request_sent_) {
// reset the flag to send the request or not,
// allowing the user the option to set it in on_tick
should_send_request_ = true;

// user defined callback, may modify "should_send_request_".
on_tick();

if (!should_send_request_) {
return BT::NodeStatus::FAILURE;
}
Comment on lines +141 to +143
Copy link
Contributor Author

@tonynajjar tonynajjar Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looking back at this @SteveMacenski, we now have the use case where we want to not send a request and succeed. E.g. the BT node sees that we are already in the target state, so no need to send a request and succeed.

Would it make sense to implement something like:

      // user defined callback, may modify "should_fail_without_sending_goal_" and "should_succeed_without_sending_goal_".
      on_tick();

      if (should_fail_without_sending_goal_) {
            return BT::NodeStatus::FAILURE;
      }
      if (should_succeed_without_sending_goal_) {
            return BT::NodeStatus::SUCCESS;
      }

Copy link
Member

@SteveMacenski SteveMacenski Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that should be handled in a different BT node, if you read the BTs for AI and Robotics book, the common design pattern is to have a conditional node if a particular objective is complete to skip doing the actual task (e.g. a branch of the tree with a "if ball not in hand, --> then pick up ball" so that if the conditional node checking if the ball is in the hand will trigger or not trigger picking up the ball action)

If that's the case - which it also sounds like you may have (?) the return code for this doesn't matter because that BT node wouldn't be triggered if under the appropriate control flow node. If there's something else you're wanting to do that I'm missing, I'm not opposed to changes, but this is a well understood BT design pattern (should this fit).


future_result_ = service_client_->async_send_request(request_).share();
sent_time_ = node_->now();
request_sent_ = true;
Expand Down Expand Up @@ -243,6 +253,9 @@ class BtServiceNode : public BT::ActionNodeBase
std::shared_future<typename ServiceT::Response::SharedPtr> future_result_;
bool request_sent_{false};
rclcpp::Time sent_time_;

// Can be set in on_tick or on_wait_for_result to indicate if a request should be sent.
bool should_send_request_;
SteveMacenski marked this conversation as resolved.
Show resolved Hide resolved
};

} // namespace nav2_behavior_tree
Expand Down