[rclcpp_action] Action client implementation by hidmic · Pull Request #594 · ros2/rclcpp

hidmic · 2018-11-28T00:11:31Z

This pull requests adds the first rclcpp implementation of an action client.

sloretz

LGTM with green CI.

I think it could use a little more documentation around the client goal handle, but that can be improved in a second pass.

sloretz · 2018-12-05T19:16:38Z

CI

Linux
Linux-aarch64
macOS
Windows

jacobperron

LGTM 👍 🎉

jacobperron · 2018-12-05T19:15:58Z

rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

+  return info_.stamp;
+}
+
+


excess space

jacobperron · 2018-12-05T19:16:53Z

rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

+    return;
+  }
+  std::lock_guard<std::mutex> guard(handle_mutex_);
+  if (feedback_callback_ == nullptr) {


Suggested change

if (feedback_callback_ == nullptr) {

if (nullptr == feedback_callback_) {

jacobperron · 2018-12-05T19:25:56Z

rclcpp_action/test/test_client.cpp

+
+  ASSERT_EQ(2ul, cancel_response->goals_canceling.size());
+  EXPECT_EQ(goal_handle0->get_goal_id(), cancel_response->goals_canceling[0].goal_id.uuid);
+  EXPECT_EQ(goal_handle1->get_goal_id(), cancel_response->goals_canceling[1].goal_id.uuid);


Also:

EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_CANCELED, goal_handle0->get_status()); EXPECT_EQ(rclcpp_action::GoalStatus::STATUS_CANCELED, goal_handle1->get_status());

jacobperron · 2018-12-05T19:27:57Z

rclcpp_action/test/test_client.cpp

+  auto goal_handle1 = future_goal_handle1.get();
+
+  if (goal_handle1->get_goal_id() < goal_handle0->get_goal_id()) {
+    goal_handle0.swap(goal_handle1);


To clarify, is this just so we can know the order when checking UUIDs below (L395) ?

sloretz · 2018-12-05T20:17:30Z

Windows attempt #2

sloretz · 2018-12-05T20:45:08Z

Windows attempt 3

sloretz · 2018-12-05T21:41:08Z

Windows attempt 4

sloretz · 2018-12-05T22:07:10Z

Windows CI passed, running CI again on the other three.

Linux
aarch64
macOS

* WIP * Removed async_cancel from action ClintGoalHandle API * Added status handler to action client goal handler * Added result handler to action client goal handler * Identation fix * Added get/set for action client goal handler * Changed action client goal handler attrs from rcl to cpp versions * Added check methods to action client goal handler * Removed rcl_client pointer from action client goal handler * Added basic waitable interface to action client * Updated waitable execute from action client * Added throw for rcl calls in action client * Removed duplicated ready flags from action client * Minor fix * Added header to action ClientBaseImpl execute * Mich's update to action client interface * Added trailing suffix to client pimpl attrs * Towards a consistent action client * Misc fixes for the action client * Yet more misc fixes for the action client * Few more fixes and shortcuts to deal with missing type support. * Fixed lint errors in action headers and client * Fixes to action client internal workflow. * Misc fixes to get client example to build * More misck client fixes * Remove debug print * replace logging with throw_from_rcl_error * Wrap result object given by client to user * Fix a couple bugs trying to cancel goals * Use unique_indentifier_msgs * create_client accepts group and removes waitable * Uncrustify fixes * [rclcpp_action] Adds tests for action client. * [WIP] Failing action client tests. * [rclcpp_action] Action client tests passing. * Spin both executors to make tests pass on my machine * Feedback callback uses shared pointer * comment about why make_result_aware is called * Client documentation * Execute one thing at a time * Return nullptr instead of throwing RejectedGoalError * ClientGoalHandle worries about feedback awareness * cpplint + uncrustify * Use node logging interface * ACTION -> ActionT * Make ClientBase constructor protected * Return types on different line * Avoid passing const reference to temporary * Child logger rclcpp_action * Child logger rclcpp_action * possible windows fixes * remove excess space * swap argument order * Misc test additions * Windows independent_bits_engine can't do uint8_t * Windows link issues

jdlangs · 2019-11-21T17:38:55Z

rclcpp_action/src/client.cpp

+      this->handle_cancel_response(response_header, cancel_response);
+    }
+  } else {
+    throw std::runtime_error("Executing action client but nothing is ready");


@hidmic, can I ask what was the purpose of throwing this exception? It's throwing in my code and as far as I can tell there's nothing I can do to control it. And since this function is invoked through the executor, I can't easily catch it and proceed. When I remove it, my program functions normally. Is it just to signal that there's a bug?

Is it just to signal that there's a bug?

I think so. Got an example that reproduces it? It would mean the executor thinks the action client is ready, but nothing actually seems to be ready to execute.

I'll see if I can get a simple standalone example reproducing it since my current program is pretty complex. I'll just go ahead an open an issue in that case.

It would mean the executor thinks the action client is ready, but nothing actually seems to be ready to execute.

Precisely that @jdlangs.

* Refactoring of rosbag2 performance package: - renamed since now it no longer benchmarks writer only - generalized byte_producer so that it uses a callback instead of queue, so it can be reused in publisher scheme Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Benchmark publishers based on yaml configuration - can specify multiple groups of publishers (see attached example yaml) - reuses byte producer Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Applying configured QoS settings for publishers. Also included in yaml example. Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * linters Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Towards common configuration - separating out common structures - utility class for common parameter parsing Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Barebone launchfile for benchmarks. Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * writer benchmark adapted to yaml file and publisher groups Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * refactored result writing and bag parameters Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * linters Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Launchfile for benchmarks Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Change storage config file from non optimized to resilient Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Max bag size for benchmark launchfile. Launchfile refactor. Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Copy yaml configs after benchmark is finished. Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Benchmark results csv file extended Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * added disclaimer about random data and compression Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Report gen tool for benchmarks Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Benchmarks out dir name changed Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * results writer node Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * documentation Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Transport and transportless in launchfile Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Benchmark launchfile refactor Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Wait for rosbag listening in benchmark launchfile Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Uncrustify and some comments for benchmarking tools Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Added new producers config for benchmarks Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Missing parameters in transport benchmark Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> added comment in storage_optimized.yaml * Missing rosbag record parameters in transport benchmark Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * Wait for subscriptions parameter in producers config Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> * moved utils code from header to source Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * changed compiler shortcut uint to unsigned int (should fix Windows build) Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> Co-authored-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>

hidmic added the in progress Actively being worked on (Kanban column) label Nov 28, 2018

sloretz added this to the crystal milestone Dec 1, 2018

sloretz force-pushed the sservulo_hidmic/rclcpp_action_client branch from 45830b8 to 78be46f Compare December 1, 2018 16:58

hidmic changed the title ~~Action client implementation~~ [rclcpp_action] Action client implementation Dec 3, 2018

hidmic self-assigned this Dec 3, 2018

hidmic and others added 25 commits December 4, 2018 13:09

WIP

ecf8b5f

Added basic waitable interface to action client

ad227d4

Updated waitable execute from action client

584ab25

Added throw for rcl calls in action client

e221967

Removed duplicated ready flags from action client

ce1b068

Minor fix

ffcb8bb

Added header to action ClientBaseImpl execute

d888fd5

Mich's update to action client interface

440ee03

Removed async_cancel from action ClintGoalHandle API

0c7dfd8

Added status handler to action client goal handler

a94ca35

Added result handler to action client goal handler

7fbb253

Identation fix

1a1f79a

Added get/set for action client goal handler

17a7a8d

Changed action client goal handler attrs from rcl to cpp versions

bcf1553

Added check methods to action client goal handler

5b2bc4c

Removed rcl_client pointer from action client goal handler

6b3482f

Added trailing suffix to client pimpl attrs

5967df8

Towards a consistent action client

58215f3

Misc fixes for the action client

1e9d56e

Yet more misc fixes for the action client

5ec7562

Few more fixes and shortcuts to deal with missing type support.

68a2e17

Fixed lint errors in action headers and client

4275ce7

Fixes to action client internal workflow.

9545a0b

Misc fixes to get client example to build

ab5df90

More misck client fixes

4825f95

sloretz added 8 commits December 5, 2018 10:26

cpplint + uncrustify

7489e75

Use node logging interface

f2dc2ab

ACTION -> ActionT

8e0da8f

Make ClientBase constructor protected

62c27a4

Return types on different line

cc2d038

Avoid passing const reference to temporary

5797e51

Child logger rclcpp_action

df0a3fa

Child logger rclcpp_action

fa391a1

sloretz approved these changes Dec 5, 2018

View reviewed changes

jacobperron approved these changes Dec 5, 2018

View reviewed changes

sloretz added 4 commits December 5, 2018 12:11

possible windows fixes

175213e

remove excess space

7cb39d8

swap argument order

05cfd8e

Misc test additions

9b63b47

Windows independent_bits_engine can't do uint8_t

a4b1cb4

Windows link issues

ffa4dce

jacobperron approved these changes Dec 5, 2018

View reviewed changes

sloretz merged commit 9116739 into master Dec 5, 2018

sloretz deleted the sservulo_hidmic/rclcpp_action_client branch December 5, 2018 22:51

sloretz removed the in progress Actively being worked on (Kanban column) label Dec 5, 2018

jdlangs reviewed Nov 21, 2019

View reviewed changes

brawner mentioned this pull request Sep 8, 2020

Set promise value after try-catch #1292

Closed

schornakj mentioned this pull request Oct 9, 2020

"Executing action client but nothing is ready" exception thrown when client sends goal #1399

Closed

	if (feedback_callback_ == nullptr) {
	if (nullptr == feedback_callback_) {

Conversation

hidmic commented Nov 28, 2018

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Dec 5, 2018

Uh oh!

sloretz commented Dec 5, 2018

Uh oh!

sloretz commented Dec 5, 2018

Uh oh!

sloretz commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdlangs Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sloretz commented Dec 5, 2018 •

edited

Loading

sloretz commented Dec 5, 2018 •

edited

Loading

jdlangs Nov 21, 2019 •

edited

Loading