[rcl_action] Implement goal handle by jacobperron · Pull Request #320 · ros2/rcl

jacobperron · 2018-11-02T20:05:37Z

Goal handle implementation and unit tests.
Added check to goal state machine transition function for index out-of-bounds

* Refactor init signature to take an rcl_allocator_t * Add unit tests

Add check to goal state machine transition function for index out of bounds

jacobperron · 2018-11-02T20:07:53Z

CI (up to rcl_action):

Linux
Linux-aarch64
macOS
Windows

rcl_action/src/rcl_action/goal_handle.c

sloretz

LGTM, just minor comments

rcl_action/src/rcl_action/goal_handle.c

sloretz · 2018-11-02T22:18:48Z

rcl_action/src/rcl_action/goal_handle.c

+  const rcl_action_goal_handle_t * goal_handle,
+  rcl_action_goal_state_t * status)
+{
+  RCL_CHECK_ARGUMENT_FOR_NULL(goal_handle, RCL_RET_INVALID_ARGUMENT);


nit, rcl_action_goal_handle_is_valid() also checks if goal_handle is NULL. This check could be removed.

Following the service/client implementations in rcl, I thought we could make the distinction of an invalid argument vs. invalid object (ie, null argument vs uninitialized struct). But I now see that this logic is not consistent in rcl. For example,

client.c makes the distinction:

rcl/rcl/src/rcl/client.c

Lines 198 to 208 in 1120b2f

rcl_ret_t

rcl_client_fini(rcl_client_t * client, rcl_node_t * node)

{

(void)node;

RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing client");

rcl_ret_t result = RCL_RET_OK;

RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);

if (!rcl_node_is_valid(node)) {

return RCL_RET_NODE_INVALID;

}

subscription.c does not:

rcl/rcl/src/rcl/subscription.c

Lines 267 to 276 in 1120b2f

rcl_ret_t

rcl_take_serialized_message(

const rcl_subscription_t * subscription,

rcl_serialized_message_t * serialized_message,

rmw_message_info_t * message_info)

{

RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Subscription taking serialized message");

if (!rcl_subscription_is_valid(subscription)) {

return RCL_RET_SUBSCRIPTION_INVALID; // error message already set

}

Then it's a good opportunity to make it consistent :)

Removed the check (and other similar redundant checks) in 8c65da8
Rather than distinguishing a null pointer vs uninitialized object by return code, we can use the error message instead.

Follow-up for rcl #321

rcl_action/src/rcl_action/goal_state_machine.c

rcl_action/test/rcl_action/test_goal_handle.cpp

hidmic

Looks awesome! Left a few comments here and there.

rcl_action/src/rcl_action/goal_handle.c

hidmic · 2018-11-05T14:25:10Z

rcl_action/src/rcl_action/goal_handle.c

+  const rcl_action_goal_handle_t * goal_handle,
+  rcl_action_goal_state_t * status)
+{
+  RCL_CHECK_ARGUMENT_FOR_NULL(goal_handle, RCL_RET_INVALID_ARGUMENT);


Then it's a good opportunity to make it consistent :)

rcl_action/test/rcl_action/test_goal_handle.cpp

… pointer

jacobperron · 2018-11-05T23:37:43Z

CI up to rcl_action:

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM!

jacobperron · 2018-11-06T17:37:35Z

Fix Clang warning:

Linux
Linux-aarch64
macOS
Windows

jacobperron added 5 commits November 2, 2018 13:02

[rcl_action][wip] Add action goal handle implementation

a73f640

* Refactor init signature to take an rcl_allocator_t * Add unit tests

Add tests for goal handle

b461814

Refactor tests and implement update state function

0873ff1

Rebase

21cd91f

Finish implementation

49a3812

Add check to goal state machine transition function for index out of bounds

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

minor

4855f8b

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 2, 2018

jacobperron self-assigned this Nov 2, 2018

jacobperron mentioned this pull request Nov 2, 2018

Actions #306

Closed

10 tasks

jacobperron requested review from hidmic and sloretz November 2, 2018 20:59

jacobperron commented Nov 2, 2018

View reviewed changes

rcl_action/src/rcl_action/goal_handle.c Outdated Show resolved Hide resolved

sloretz reviewed Nov 2, 2018

View reviewed changes

Address review

d1fa431

hidmic reviewed Nov 5, 2018

View reviewed changes

jacobperron added 2 commits November 5, 2018 09:44

Address review

29f0183

Remove redundant error checks and add explicit error message for null…

8c65da8

… pointer

hidmic approved these changes Nov 6, 2018

View reviewed changes

Fix Clang warning

a24280f

jacobperron merged commit b2578bb into master Nov 6, 2018

jacobperron deleted the jacob/goal_handle branch November 6, 2018 18:28

jacobperron removed the in review Waiting for review (Kanban column) label Nov 6, 2018

hidmic mentioned this pull request Nov 12, 2018

Action server implementation #323

Merged

	rcl_ret_t
	rcl_client_fini(rcl_client_t * client, rcl_node_t * node)
	{
	(void)node;
	RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing client");
	rcl_ret_t result = RCL_RET_OK;
	RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT);
	RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT);
	if (!rcl_node_is_valid(node)) {
	return RCL_RET_NODE_INVALID;
	}

	rcl_ret_t
	rcl_take_serialized_message(
	const rcl_subscription_t * subscription,
	rcl_serialized_message_t * serialized_message,
	rmw_message_info_t * message_info)
	{
	RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Subscription taking serialized message");
	if (!rcl_subscription_is_valid(subscription)) {
	return RCL_RET_SUBSCRIPTION_INVALID; // error message already set
	}

Conversation

jacobperron commented Nov 2, 2018

Uh oh!

jacobperron commented Nov 2, 2018

Uh oh!

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sloretz Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

jacobperron Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

hidmic Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

jacobperron Nov 5, 2018 • 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!

jacobperron Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hidmic Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

jacobperron commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacobperron Nov 5, 2018 •

edited

Loading

jacobperron commented Nov 5, 2018 •

edited

Loading