Add optional callbacks to action client for goal response and result #701
Conversation
Edit: rebuild with ros2/system_tests#346 |
jacobperron
added
in review
|
Test failures appear unrelated. This is ready for review. |
rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp
Outdated
Show resolved
Hide resolved
jacobperron
added
in progress
There was a problem hiding this comment.
Do you ever need to set the exception in the promise? https://en.cppreference.com/w/cpp/thread/promise/set_exception
|
@wjwwood If there is an exception it is set here (just before invoking the callback): |
jacobperron
added
in review
Now supports callbacks for the goal response and result. This also makes it easier to incorporate action clients in composable nodes since we don't have to rely on waiting on futures. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron
force-pushed
the
action_client_callbacks
branch
from
65160ed
to
a98a964
Compare
CI also includes connected PRs: |
| EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_ACCEPTED, goal_handle->get_status()); | ||
| EXPECT_FALSE(goal_handle->is_feedback_aware()); | ||
| EXPECT_TRUE(goal_handle->is_result_aware()); | ||
| auto future_result = action_client->async_get_result(goal_handle); |
There was a problem hiding this comment.
@jacobperron nit: since goal_handle->is_result_aware()is true at this point, you can also just do goal_handle->async_result() to get the future.
There was a problem hiding this comment.
See my comment below proposing we make goal_handle->async_result() private.
| if (!ignore_result) { | ||
| std::shared_ptr<GoalHandle> goal_handle( | ||
| new GoalHandle(goal_info, options.feedback_callback, options.result_callback)); | ||
| if (options.result_callback) { |
There was a problem hiding this comment.
@jacobperron just for the record, this now means that if you just want the result future, you need to ask for it explicitly later on, always.
There was a problem hiding this comment.
Yeah, that's true. Though, this is a subtle feature. I bet that most use cases would provide a callback anyways and the case for sending a request early only to get a reference to the result future later is rare. The user can still get the future later with the client method, async_get_result().
This brings me to another point. I think it's odd to have two public facing methods for the getting the result, Client.async_get_result() and ClientGoalHandle.async_result(). Originally, the client method was essentially bypassing the "ignore result" flag. I propose we make the goal handle method private and rely on the client method to return the future. Regarding the "ignore result" flag, in the case where the goal handle is not result aware, we have 2.5 options:
- Keep the current behavior and make the goal handle result aware. I.e. Does not fail if the user previously set the "ignore result" flag.
- Or we can not even provide the "ignore result" option and make result requests on demand (this PR).
- Throw an exception, because the user chose to ignore the result explicitly.
Let me know what you think.
To not drag this PR on any longer, I suggest we make any changes related to this discussion in a follow-up PR.
| try { | ||
| this->make_result_aware(goal_handle); | ||
| } catch (...) { | ||
| promise->set_exception(std::current_exception()); | ||
| options.goal_response_callback(future); |
There was a problem hiding this comment.
this does not check goal_response_callback first
There was a problem hiding this comment.
Good catch. I've swapped the two if-blocks as suggested in #738.
| return; | ||
| } | ||
| } | ||
| std::lock_guard<std::mutex> guard(goal_handles_mutex_); | ||
| goal_handles_[goal_handle->get_goal_id()] = goal_handle; | ||
| promise->set_value(goal_handle); | ||
| if (options.goal_response_callback) { | ||
| options.goal_response_callback(future); |
There was a problem hiding this comment.
This might get called after result_callback (not 100% sure)
Since a result callback is not provided when sending the goal, the goal handle is not "result aware" and calling the action client method will make it so. The behaviour was changed in ros2/rclcpp#701. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Since a result callback is not provided when sending the goal, the goal handle is not "result aware" and calling the action client method will make it so. The behaviour was changed in ros2/rclcpp#701. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Resolves #635.
This also makes it easier to incorporate action clients in composable nodes since we don't have to rely on waiting on futures.