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

Add more callbacks to rclcpp_action::Client #635

Closed
stertingen opened this issue Feb 21, 2019 · 1 comment · Fixed by #701
Closed

Add more callbacks to rclcpp_action::Client #635

stertingen opened this issue Feb 21, 2019 · 1 comment · Fixed by #701
Assignees
Labels
enhancement New feature or request

Comments

@stertingen
Copy link

Feature request

Feature description

Using the action client in rclcpp_action requires polling using timers on the future objects returned by Client::async_send_goal and Client::async_get_result. It would be far more convenient to add callbacks to these functions.

(spin_until_future_complete won't work because my node is already spinning.)

Implementation considerations

Possible function signatures could look like this:

using GoalResultCallback =
  std::function<void(std::shared_future<typename GoalHandle::SharedPtr>)>;
using ResultCallback =
  std::function<void(std::shared_future<Result>)>;

std::shared_future<typename GoalHandle::SharedPtr>
async_send_goal(
  const Goal & goal, GoalResultCallback = nullptr,
  FeedbackCallback callback = nullptr, ResultCallback = nullptr,
  bool ignore_result = false);

std::shared_future<Result>
async_get_result(typename GoalHandle::SharedPtr goal_handle, ResultCallback = nullptr)

While these are the callbacks I currently need, it would be consistent to add appropriate callbacks to all functions returning future objects.

@mjcarroll mjcarroll added enhancement New feature or request backlog labels Feb 28, 2019
@jacobperron
Copy link
Member

This seems like a reasonable addition.

Until a mechanism like std::future::then is available, future objects are of limited use. I suspect this is why the service client also takes a callback parameter.

A PR is welcome for the proposed change 🙂


An alternative to the issue is to refactor the spin functions so that it is possible to recursively spin on futures. I noticed a TODO in the code:

// TODO(wjwwood): does not work recursively; can't call spin_node_until_future_complete
// inside a callback executed by an executor.

I haven't thought too much about how this might be done though.

@jacobperron jacobperron added ready Work is about to start (Kanban column) and removed backlog labels Apr 8, 2019
@jacobperron jacobperron self-assigned this Apr 17, 2019
@jacobperron jacobperron added in progress Actively being worked on (Kanban column) and removed ready Work is about to start (Kanban column) labels Apr 17, 2019
jacobperron added a commit that referenced this issue Apr 18, 2019
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.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 18, 2019
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Apr 26, 2019
nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
…#635)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this issue Aug 5, 2022
The cache consumer requires that you call close()
before calling change_consumer_callback().  The test
forgot the close() call, so was running into a race
where changing out the callback was not atomic.
Remember to call close() in the test, which should fix
some occasional failures in CI.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants