Skeleton for Action Server and Client #579

sloretz · 2018-11-02T17:24:20Z

This is the start of C++ code matching the API in ros2/examples#216

sloretz · 2018-11-16T03:01:41Z

CI

Linux
Linux-aarch64
macOS
Windows

hidmic

@sloretz thank you for pushing this! I left some comments here and there. Feel free to focus on the server, taking care of the client is on me.

hidmic · 2018-11-16T13:16:21Z

rclcpp_action/CMakeLists.txt

+    )
+    target_link_libraries(test_server
+      ${PROJECT_NAME}
+      ${test_msgs_LIBRARIES}


@sloretz I've also seen CMake code using the ament_target_dependencies() macro to do just this. Which one's the recommended idiom?

ament_target_dependencies() is the recommended one. I keep forgetting the ament_cmake macros and fallback to the usual CMake functions.

hidmic · 2018-11-16T14:19:07Z

rclcpp_action/include/rclcpp_action/server_goal_handle.hpp

+
+  // TODO(sloretz) shared pointer to keep rcl_server_ alive while goal handles are alive
+  rcl_action_server_t * rcl_server_;
+  rcl_action_goal_handle_t * rcl_handle_;


@sloretz I agree we should use smart pointers, for both (I'd initially say shared and unique respectively).

hidmic · 2018-11-16T14:25:57Z

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

+
+  /// TODO(sloretz) examples have this on client as `async_cancel_goal(goal_handle)`
+  std::future<bool>
+  async_cancel();


@sloretz I do like the idea of calling cancel on the goal handle, but we have to support cancellation requests that expand multiple goals (e.g. cancel all goals). So I'd propose to keep this async_cancel() method here, plus async_cancel_all_goals() and async_cancel_goals_before() methods in the client, and maybe expose the low level method that just takes goal ID and timestamp for completeness.

hidmic · 2018-11-16T14:27:58Z

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

+
+  // TODO(sloretz) `async_result` to make method names consistent?
+  std::future<typename ACTION::Result>
+  result_future();


@sloretz hmm, I do prefer async_result, but it's not a strong preference.

Sounds good. I just need to remember to update this line in the examples https://github.com/ros2/examples/pull/216/files#diff-9df9aa44ab873a3a211a764112190449R37

hidmic · 2018-11-16T14:31:36Z

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

+
+private:
+  // The templated Server creates goal handles
+  friend Client<ACTION>;


@sloretz I don't know if this is a typical pattern in ROS2, but why not using an abstract base class and have the client implement their own?

I don't think using friend and a private constructor is a typical pattern. The base class approach would work too.

@hidmic is 193eb4a what you had in mind?

Hmm, I was actually thinking of something like (in client_goal_handle.hpp):

class ClientGoalHandle { public: virtual std::future<bool> async_cancel() = 0; };

and then (in client.cpp):

class ClientGoalHandleImpl : public ClientGoalHandle { public: std::future<bool> async_cancel() override { ... } };

External code never instantiates a GoalHandle because it's just an interface. We have to watch out for slicing though.

I don't think ClientGoalHandleImpl can be in client.cpp because std::future<typename ACTION::Result> async_result() requires knowing the user's type.

Combined base class with virtual methods and friendship of implementation class with Server in acdfcd9. How does that look?

Argh, right, there's no common base type for action user defined types, silly me.

I think it's fine. Another alternative (for the sake of completeness :) ) would be to define the goal handle subclass as a private nested class of the client/server class. Same code, no friendship.

hidmic · 2018-11-16T14:32:15Z

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

+
+  // TODO(sloretz) shared pointer to keep rcl_client_ alive while goal handles are alive
+  rcl_action_client_t * rcl_client_;
+  rcl_action_goal_info_t rcl_info_;


@sloretz I agree we should use smart pointers.

hidmic · 2018-11-16T14:33:10Z

rclcpp_action/include/rclcpp_action/client_goal_handle.hpp

+
+
+// TODO(sloretz) goal_handle->result_future()
+// TODO(sloretz) goal_handle->cancel()


@sloretz I think we can remove these TODOs.

hidmic · 2018-11-16T14:36:50Z

rclcpp_action/include/rclcpp_action/server_goal_handle.hpp

+
+private:
+  // The templated Server creates goal handles
+  friend Server<ACTION>;


@sloretz same as above about using friend.

sloretz · 2018-11-16T20:26:54Z

CI

Linux
Linux-aarch64
macOS
Windows

This reverts commit 193eb4a.

jacobperron

Pending green CI, I think this can be merged as a place to iterate from.

sloretz · 2018-11-19T18:40:31Z

CI

Linux
Linux-aarch64
macOS
Windows

hidmic · 2018-11-20T15:57:39Z

I agree with Jacob.

dirk-thomas · 2019-03-11T20:41:34Z

rclcpp_action/CMakeLists.txt

+    target_link_libraries(test_server
+      ${PROJECT_NAME}
+    )
+  endif()


Both of these tests only run with the default RMW impl. It would be good to update this to run the tests for each RMW impl.

* Skeleton for ActionServer and ActionClient

…shutdown` (ros2#579) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

…2#579) Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Skeleton for ActionServer

5884224

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

sloretz added 10 commits November 15, 2018 14:42

Remove creating rclcpp services and publishers

7b816de

Remove unused node interfaces

1f7c4d3

Remove unused node interfaces

e464d29

Add files and TODO's

97afb60

more todo

2dd56bb

Skeleton for server goal handle

d4bd643

TODO about setting state for goals

95e8da5

Skeleton for client goal handle

569b129

First attempt at client skeleton

1afe22b

Server goal callback given info and message

5b733e3

sloretz changed the title ~~Skeleton for ActionServer~~ Skeleton for Action Server and Client Nov 16, 2018

sloretz added 5 commits November 15, 2018 17:59

Note about method name

12be247

ament_lint_auto already finds this

2551091

Add ament_package() so linters are called

459717e

Linter dependencies

0a9be04

Fix cpplint and uncrustify issues

d0e4a5b

sloretz requested a review from hidmic November 16, 2018 03:00

hidmic reviewed Nov 16, 2018

View reviewed changes

sloretz added 8 commits November 16, 2018 11:19

no dllimport/dllexport on templated types

656ffc6

Suppress unused variable warning

5aac377

Use ament_target_dependencies

37a3753

Remove old todo's

9e514fb

result_future -> async_future

4fc2246

result_future() -> async_result()

a2b0cbe

async_cancel...() on client

e79f5da

Make ServerGoalHandle an abstract template type using CRTP

193eb4a

sloretz and others added 5 commits November 16, 2018 13:54

Revert "Make ServerGoalHandle an abstract template type using CRTP"

2df634f

This reverts commit 193eb4a.

Combine friendship with base goal handle class

acdfcd9

Add cmake install command for include directory

0039948

Add rclcpp_action.hpp

f2c7cdf

Export rcl_action dependency

dcadeaa

jacobperron approved these changes Nov 19, 2018

View reviewed changes

sloretz merged commit be010cb into master Nov 21, 2018

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

sloretz deleted the rclcpp_action_prototype branch November 21, 2018 17:17

dirk-thomas reviewed Mar 11, 2019

View reviewed changes

cho3 pushed a commit to cho3/rclcpp that referenced this pull request Jun 3, 2019

Skeleton for Action Server and Client (ros2#579)

e0ce3bc

* Skeleton for ActionServer and ActionClient

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Don't call rcl_logging_configure/rcl_logging_fini in `rcl_init/rcl_…

8a0ec5f

…shutdown` (ros2#579) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

Patch zstd 1.4.4 to include cmake_minimum_version bump to 2.8.12 (ros…

e519cd9

…2#579) Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skeleton for Action Server and Client #579

Skeleton for Action Server and Client #579

sloretz commented Nov 2, 2018 •

edited

Loading

sloretz commented Nov 16, 2018

hidmic left a comment

hidmic Nov 16, 2018

sloretz Nov 16, 2018

sloretz Nov 16, 2018

hidmic Nov 16, 2018

hidmic Nov 16, 2018

sloretz Nov 16, 2018

hidmic Nov 16, 2018

sloretz Nov 16, 2018

sloretz Nov 16, 2018

hidmic Nov 16, 2018

sloretz Nov 16, 2018 •

edited

Loading

sloretz Nov 16, 2018

hidmic Nov 16, 2018 •

edited

Loading

sloretz Nov 16, 2018

sloretz Nov 16, 2018

hidmic Nov 20, 2018

hidmic Nov 16, 2018

hidmic Nov 16, 2018

sloretz Nov 16, 2018

hidmic Nov 16, 2018

sloretz commented Nov 16, 2018

jacobperron left a comment

sloretz commented Nov 19, 2018

hidmic commented Nov 20, 2018

dirk-thomas Mar 11, 2019



		// TODO(sloretz) goal_handle->result_future()
		// TODO(sloretz) goal_handle->cancel()

Skeleton for Action Server and Client #579

Skeleton for Action Server and Client #579

Conversation

sloretz commented Nov 2, 2018 • edited Loading

sloretz commented Nov 16, 2018

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Nov 16, 2018

jacobperron left a comment

Choose a reason for hiding this comment

sloretz commented Nov 19, 2018

hidmic commented Nov 20, 2018

Choose a reason for hiding this comment

sloretz commented Nov 2, 2018 •

edited

Loading

sloretz Nov 16, 2018 •

edited

Loading

hidmic Nov 16, 2018 •

edited

Loading