Action client implementation #319

hidmic · 2018-11-02T16:12:08Z

Connected to #306. This pull request introduces a first implementation of rcl's action client interface. It depends on ros2/rosidl#310 for basic typesupport declarations to compile and on ros2/rcl_interfaces#47 for basic typesupport implementation to test and thus it's still lacking proper test coverage.

I'm pushing this early to trigger a follow up discussion on the pros and cons of the current fully type-erased action client API.

sloretz

I think it would be worth trying to push more logic into the C layer. Currently it looks like the API is pretty thin.

rcl_action/src/rcl_action/action_client.c

sloretz · 2018-11-02T18:01:43Z

rcl_action/src/rcl_action/action_client.c

+    if (RCL_RET_SERVICE_NAME_INVALID == ret) {
+      ret = RCL_RET_ACTION_NAME_INVALID;
+    } else if (RCL_RET_BAD_ALLOC != ret) {
+      ret = RCL_RET_ERROR;


Same comment about keeping RCL_RET_BAD_ALLOC return code

Same as above.

sloretz · 2018-11-02T18:09:34Z

rcl_action/src/rcl_action/action_client.c

+            &impl->status_subscription, node,
+            &type_support->status_topic_type_support,
+            impl->action_name, &status_subscription_options);
+          if (RCL_RET_OK == ret) {


I think in cases like this goto error: is ok to avoid nested if statements. No need to change what's here though.

I thought about using goto, but I went this route to simplify the rollback. Otherwise I would've needed a label for each failure stage (which things have I initialized so far that need finalization to keep things consistent?)

I don't think a label is needed for each failure stage. It looks like rcl_action_client_fini() could be called instead of fini-ing everything individually

Instead of a label for each failure stage, check if each pointer is null and cleanup if it isn't.
I think a goto cleanup and/or goto fail (like in node.c) might be more readable. But I'm also okay with leaving this as is.

Let's be consistent with rest of the codebase then :). Changed in c5dabd0.

sloretz · 2018-11-02T18:13:36Z

rcl_action/src/rcl_action/action_client.c

+  RCUTILS_LOG_DEBUG_NAMED(
+    ROS_PACKAGE_NAME, "Initializing client for action name '%s'", action_name);
+  // Allocate space for the implementation struct.
+  rcl_action_client_impl_t *impl = allocator->allocate(


Should error if NULL != action_client->impl before allocating to avoid leaking memory if action_client is already initialized.

Good catch. Done in c5dabd0.

sloretz · 2018-11-02T18:19:56Z

rcl_action/src/rcl_action/action_client.c

+        impl->action_name, &result_client_options);
+      if (RCL_RET_OK == ret) {
+        RCUTILS_LOG_DEBUG_NAMED(
+          ROS_PACKAGE_NAME, "Action cancel result initialized");


cancel result -> get result

Fixed in c5dabd0.

sloretz · 2018-11-02T18:31:00Z

rcl_action/src/rcl_action/action_client.c

+    return RCL_RET_NODE_INVALID;
+  }
+  rcl_ret_t ret = RCL_RET_OK;
+  if (action_client->impl) {


I think the rcl_action_client_is_valid() check above already checked this

True that. Fixed in c5dabd0.

sloretz · 2018-11-02T18:32:11Z

rcl_action/src/rcl_action/action_client.c

+    if (rcl_client_is_valid(&impl->goal_client)) {
+      ret = rcl_client_fini(&impl->goal_client, node);
+      if (RCL_RET_OK != ret) {
+        return RCL_RET_ERROR;


Recommend returning the original error code here, same below

So, I tried to be consistent with rcl_action documentation instead of propagating internal errors (that are usually hard to understand because of the lack of context). If we propagate, documentation needs an update.

sloretz · 2018-11-02T18:34:45Z

rcl_action/src/rcl_action/action_client.c

+    //               finalization failed, but it seems that's currently
+    //               not possible.
+    rcl_action_client_impl_t * impl = action_client->impl;
+    if (rcl_client_is_valid(&impl->goal_client)) {


Probably don't need this, rcl_client_fini will check if the client is valid.

Yes, but there's an implicit purpose to this. If any of the five internal finalizations were to fail, some would end up finalized, some not. So this is an attempt to allow rcl_action_client_fini to be called once again and not fail unless your action client was completely finalized.

But if there is a failure finalizing, then wouldn't the same failure happen again on subsequent calls to this function?

Maybe instead of returning early, we could cache the error result and continue trying to finalize the other objects?

then wouldn't the same failure happen again

That I don't think we can assume, some finalization errors come from deep into rmw. But I do like the idea of finalizing as much as possible. Changed in c5dabd0.

rcl_action/src/rcl_action/action_client.c

jacobperron · 2018-11-02T21:02:28Z

rcl_action/src/rcl_action/action_client.c

+  rcl_allocator_t allocator = goal_client_options->allocator;
+
+  char * goal_service_name = NULL;
+  ret = rcl_action_get_goal_service_name(


nit: this fits on one line (ditto for other rcl_action_get_*_service_name() calls).

Fixed in c5dabd0.

jacobperron · 2018-11-02T21:20:30Z

rcl_action/src/rcl_action/action_client.c

+  allocator.deallocate(feedback_topic_name, allocator.state);
+
+  if (RCL_RET_OK != ret) {
+    if (RCL_RET_SERVICE_NAME_INVALID == ret) {


RCL_RET_TOPIC_NAME_INVALID

Good catch! Fixed in c5dabd0.

jacobperron · 2018-11-02T21:20:53Z

rcl_action/src/rcl_action/action_client.c

+  allocator.deallocate(status_topic_name, allocator.state);
+
+  if (RCL_RET_OK != ret) {
+    if (RCL_RET_SERVICE_NAME_INVALID == ret) {


RCL_RET_TOPIC_NAME_INVALID

Good catch! Fixed in c5dabd0.

rcl_action/src/rcl_action/action_client.c

jacobperron · 2018-11-02T21:27:49Z

rcl_action/src/rcl_action/action_client.c

+  const char * action_name,
+  const rcl_action_client_options_t * options)
+{
+  RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Initializing action client");


This debug statement is redundant with the one at L305.

Yeap, the first one is unnecessary. Fixed in c5dabd0.

jacobperron · 2018-11-02T21:37:37Z

rcl_action/src/rcl_action/action_client.c

+            &impl->status_subscription, node,
+            &type_support->status_topic_type_support,
+            impl->action_name, &status_subscription_options);
+          if (RCL_RET_OK == ret) {


Instead of a label for each failure stage, check if each pointer is null and cleanup if it isn't.
I think a goto cleanup and/or goto fail (like in node.c) might be more readable. But I'm also okay with leaving this as is.

jacobperron · 2018-11-02T21:47:51Z

rcl_action/src/rcl_action/action_client.c

+rcl_ret_t
+rcl_action_client_fini(rcl_action_client_t * action_client, rcl_node_t * node)
+{
+  RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing action client");


minor: Other finalize functions in rcl distinguish between an invalid argument vs. invalid object. Consider adding

RCL_CHECK_ARGUMENT_FOR_NULL(action_client, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);

here before the *_is_valid() checks.

This applies to the other functions as well (except init(), is_valid()`, and the getters).

I see, and it makes sense. Fixed in c5dabd0.

jacobperron · 2018-11-02T21:57:46Z

rcl_action/src/rcl_action/action_client.c

+    //               finalization failed, but it seems that's currently
+    //               not possible.
+    rcl_action_client_impl_t * impl = action_client->impl;
+    if (rcl_client_is_valid(&impl->goal_client)) {


But if there is a failure finalizing, then wouldn't the same failure happen again on subsequent calls to this function?

Maybe instead of returning early, we could cache the error result and continue trying to finalize the other objects?

hidmic · 2018-11-05T14:16:03Z

So, about making the API thicker. Per offline discussion with @sloretz and @jacobperron, I think we can agree that we need a mechanism to retrieve the goal ID info from the type-erased message before we can start pushing more logic into this layer.

@sloretz I'll push some changes to ros2/rcl_interfaces#47 as a POC for a conversion function within generated rosidl_action_type_support_t instances. cc @apojomovsky as it impacts your ongoing work.

jacobperron · 2018-11-06T02:12:52Z

rcl_action/src/rcl_action/action_client.c

+  char * result_service_name = NULL;
+  ret = rcl_action_get_result_service_name(action_name, allocator, &result_service_name);
+  if (RCL_RET_OK != ret) {
+    if (RCL_RET_BAD_ALLOC != ret) {


Might be more clearly written as:

if (RCL_RET_BAD_ALLOC == ret) { return RCL_RET_BAD_ALLOC; } return RCL_RET_ERROR;

Makes sense, changed in f29d359.

hidmic · 2018-11-06T15:11:14Z

On 011dc30, I assumed that:

the client has to be hooked to the status channel, unconditionally.
the wait set is used by the action client and the action client alone (same would go for the action server).

I'm not so sure about (1), API documentation was not consistent about the need of status. (2) seems restrictive to me but a reasonable trade off until wait set groups are implemented.

hidmic · 2018-11-12T17:31:21Z

@jacobperron I added basic tests. Let me know if/when we can move forward with having a single set of integration tests as suggested in https://github.com/ros2/rcl/pull/323/files#r232681489 and I'll add those.

hidmic · 2018-11-12T17:32:46Z

This PR still doesn't include any logic as that requires typesupport we do not yet have. I'd like to know what the rest have to say in tomorrow's retrospective and how https://github.com/ros2/rcl/pull/323/files#r232674670 unfolds. May be a neat path forward.

jacobperron

Looking good! Couple nits and one discussion point re the finalize function.

I think it is okay to have a follow-up ticket to address making the status subscriber (and feedback) optional.

I think more about the wait set, but at first glance this seems like a reasonable compromise until wait set group support.

jacobperron · 2018-11-12T18:23:08Z

rcl_action/src/rcl_action/action_client.c

+      ret = RCL_RET_ERROR;
+    }
+  }
+  if (RCL_RET_OK != ret) {


Should this be if (RCL_RET_OK == ret) ?

If there is a failure what are the chances that the caller will call finalize again? And if they do, what could change the outcome of this function? It seems better to me to deallocate as long as the pimpl is not null to avoid any memory leaks.

Similar to the other fini implementations, e.g. publisher.c:

rcl/rcl/src/rcl/publisher.c

Lines 208 to 214 in 9351fd8

rmw_ret_t ret =

rmw_destroy_publisher(rmw_node, publisher->impl->rmw_handle);

if (ret != RMW_RET_OK) {

RCL_SET_ERROR_MSG(rmw_get_error_string().str);

result = RCL_RET_ERROR;

}

allocator.deallocate(publisher->impl, allocator.state);

Even if there is an error in destroying the underlying rmw object, we still deallocate the pimpl.

Alright, let's be consistent. Fixed in 0c31382.

jacobperron · 2018-11-12T20:03:28Z

rcl_action/src/rcl_action/action_client.c

+    return RCL_RET_ACTION_CLIENT_INVALID;  // error already set
+  }
+  // Wait on action goal service response messages.
+  ret = rcl_wait_set_add_client(


nit: fit on single line. Same for other lines below.

Fixed in 0c31382.

jacobperron · 2018-11-12T20:19:26Z

rcl_action/src/rcl_action/action_client.c

+  const rcl_action_client_options_t * options)
+{
+  RCL_CHECK_ARGUMENT_FOR_NULL(action_client, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);


rcl_node_is_valid() already checks for a null pointer, so this line can be removed.

Removed in 0c31382.

* Remove redundant null check for given rcl_node_t pointer. * Always deallocate rcl_action_client_t pimpl on finalization. * Minor formatting fixes.

hidmic · 2018-11-13T20:21:56Z

@jacobperron since integration tests will likely need rcl_wait support for both client and server, let's move forward with this and get back to them when we get to refactor wait set logic for the client to use groups. What do you think?

jacobperron · 2018-11-13T23:51:39Z

since integration tests will likely need rcl_wait support for both client and server, let's move forward with this and get back to them when we get to refactor wait set logic for the client to use groups. What do you think?

@hidmic
Sounds good. I've created a task for adding integration tests: #328

sloretz

LGTM with green CI and one minor comment

sloretz · 2018-11-13T23:39:01Z

rcl_action/include/rcl_action/types.h

@@ -31,6 +31,8 @@ extern "C"
 #include "rcl/macros.h"
 #include "rcl/types.h"

+#include "rosidl_generator_c/action_type_support_struct.h"
+typedef struct rosidl_action_type_support_t rosidl_action_type_support_t;


This typedef exists already https://github.com/ros2/rosidl/blob/075fe35bec2587d5c1012cb6ffec0db61c34277a/rosidl_generator_c/include/rosidl_generator_c/action_type_support_struct.h#L28

Done in 78168bb.

jacobperron

LGTM, pending use of macros instead of the static functions as discussed (#323 (comment)) and green CI.

jacobperron · 2018-11-13T23:53:26Z

rcl_action/include/rcl_action/action_client.h

@@ -420,6 +415,7 @@ rcl_action_take_status(
  const rcl_action_client_t * action_client,
  void * ros_status_array);

+


nit: extra blank line

Argh, I forgot about the macros. Fixed in c7e6183.

…impl.

hidmic · 2018-11-14T19:59:23Z

@apojomovsky @jacobperron @sloretz may I ask one of you guys to start CI on this one for me (nope, I can't do it myself :( ) ?

sloretz · 2018-11-14T20:15:20Z

CI up to rcl_action, testing only rcl_action

Linux
Linux-aarch64
macOS
Windows

jacobperron

🎉

hidmic · 2018-11-15T16:42:01Z

Alright! Let's get this one in.

* Remove redundant null check for given rcl_node_t pointer. * Always deallocate rcl_action_client_t pimpl on finalization. * Minor formatting fixes.

This reverts commit 9e655ae.

This reverts commit 124aabb.

This reverts commit 9e655ae.

This reverts commit 124aabb.

dirk-thomas · 2018-11-27T03:49:28Z

rcl_action/CMakeLists.txt

  ament_lint_auto_find_test_dependencies()
  ament_find_gtest()
  # Gtests
+  ament_add_gtest(test_action_client
+    test/rcl_action/test_action_client.cpp
+  )


This test is only covering the default rmw impl. It would be good if the test would be invoked for every rmw impl.

dirk-thomas · 2018-11-27T04:05:24Z

@hidmic Is this branch still necessary or can it be deleted?

hidmic · 2018-11-27T04:06:39Z

@dirk-thomas branch deleted, thanks for the FYI.

hidmic requested review from jacobperron and sloretz November 2, 2018 16:12

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

sloretz reviewed Nov 2, 2018

View reviewed changes

jacobperron mentioned this pull request Nov 2, 2018

Actions #306

Closed

10 tasks

jacobperron reviewed Nov 2, 2018

View reviewed changes

hidmic added a commit that referenced this pull request Nov 5, 2018

Addresses peer review comments (#319)

c5dabd0

jacobperron reviewed Nov 6, 2018

View reviewed changes

hidmic mentioned this pull request Nov 12, 2018

Action server implementation #323

Merged

hidmic added 6 commits November 12, 2018 13:59

[rcl action] Action client first implementation.

1620587

Addresses peer review comments (#319)

d1c3990

[rcl action] Refactors action client error checking.

8956362

[rcl action] Wait set support for action clients (restrictive).

e2ee3c5

[rcl action] Fixes after changes introduced by (#310)

cca18da

[rcl action] Adds action client tests.

7efd1a1

hidmic changed the title ~~[DO NOT MERGE] Action client first implementation~~ Action client first implementation Nov 12, 2018

hidmic force-pushed the hidmic/action-client-implementation branch from 208d017 to 7efd1a1 Compare November 12, 2018 17:29

hidmic changed the title ~~Action client first implementation~~ Action client implementation Nov 12, 2018

jacobperron reviewed Nov 12, 2018

View reviewed changes

[rcl action] Addresses peer review comments (#319).

0c31382

* Remove redundant null check for given rcl_node_t pointer. * Always deallocate rcl_action_client_t pimpl on finalization. * Minor formatting fixes.

hidmic mentioned this pull request Nov 13, 2018

Add typesupport for actions in c and c++ ros2/rosidl_typesupport#36

Merged

hidmic added 2 commits November 13, 2018 12:03

[rcl action] Cleanup CMake code for test_action_client.

f2afddc

[rcl action] Fixes bad assertion in test_action_client.

629403e

hidmic added a commit that referenced this pull request Nov 13, 2018

Addresses peer review comments (#319)

4ea0855

hidmic added a commit that referenced this pull request Nov 13, 2018

[rcl action] Addresses peer review comments (#319).

0b0bc39

* Remove redundant null check for given rcl_node_t pointer. * Always deallocate rcl_action_client_t pimpl on finalization. * Minor formatting fixes.

hidmic added the in review Waiting for review (Kanban column) label Nov 13, 2018

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

jacobperron mentioned this pull request Nov 13, 2018

Add action integration tests #328

Closed

sloretz reviewed Nov 14, 2018

View reviewed changes

jacobperron reviewed Nov 14, 2018

View reviewed changes

hidmic added 2 commits November 14, 2018 15:54

[rcl action] Removes duplicate typedef in action_client header.

78168bb

[rcl action] Use macros instead of static function for action client …

c7e6183

…impl.

jacobperron approved these changes Nov 14, 2018

View reviewed changes

hidmic merged commit f256da0 into master Nov 15, 2018

tfoote removed the in review Waiting for review (Kanban column) label Nov 15, 2018

hidmic mentioned this pull request Nov 15, 2018

Completes integration tests for action client/server #331

Merged

2 tasks

nburek pushed a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Addresses peer review comments (ros2#319)

124aabb

nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Revert "[rcl action] Addresses peer review comments (ros2#319)."

d26d958

This reverts commit 9e655ae.

nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Revert "Addresses peer review comments (ros2#319)"

bf315c5

This reverts commit 124aabb.

nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Revert "[rcl action] Addresses peer review comments (ros2#319)."

96a9027

This reverts commit 9e655ae.

nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018

Revert "Addresses peer review comments (ros2#319)"

6faaf1c

This reverts commit 124aabb.

dirk-thomas reviewed Nov 27, 2018

View reviewed changes

hidmic deleted the hidmic/action-client-implementation branch November 27, 2018 04:06

	rmw_ret_t ret =
	rmw_destroy_publisher(rmw_node, publisher->impl->rmw_handle);
	if (ret != RMW_RET_OK) {
	RCL_SET_ERROR_MSG(rmw_get_error_string().str);
	result = RCL_RET_ERROR;
	}
	allocator.deallocate(publisher->impl, allocator.state);

		@@ -420,6 +415,7 @@ rcl_action_take_status(
		const rcl_action_client_t * action_client,
		void * ros_status_array);

Action client implementation #319

Action client implementation #319

Conversation

hidmic commented Nov 2, 2018

sloretz 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

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

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

hidmic commented Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

hidmic Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

hidmic commented Nov 6, 2018

hidmic commented Nov 12, 2018

hidmic commented Nov 12, 2018

jacobperron 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

hidmic commented Nov 13, 2018

jacobperron commented Nov 13, 2018

sloretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Nov 14, 2018

sloretz commented Nov 14, 2018

jacobperron left a comment

Choose a reason for hiding this comment

hidmic commented Nov 15, 2018

Choose a reason for hiding this comment

dirk-thomas commented Nov 27, 2018

hidmic commented Nov 27, 2018

hidmic commented Nov 5, 2018 •

edited

Loading

hidmic Nov 6, 2018 •

edited

Loading