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

Jump to bottom

[rcl_action] Implement goal handle #320

Merged
merged 10 commits into from Nov 6, 2018
Merged

[rcl_action] Implement goal handle #320

merged 10 commits into from Nov 6, 2018

Conversation

jacobperron
Copy link
Member

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

@jacobperron
[rcl_action][wip] Add action goal handle implementation
a73f640 
* Refactor init signature to take an rcl_allocator_t
* Add unit tests
@jacobperron
Add tests for goal handle
b461814
@jacobperron
Refactor tests and implement update state function
0873ff1
@jacobperron
Rebase
21cd91f
@jacobperron
Finish implementation
49a3812 
Add check to goal state machine transition function for index out of bounds
@jacobperron jacobperron added the in progress Actively being worked on (Kanban column) label Nov 2, 2018
@jacobperron
Copy link
Member Author

CI (up to rcl_action):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 2, 2018
@jacobperron jacobperron self-assigned this Nov 2, 2018
@jacobperron jacobperron mentioned this pull request Nov 2, 2018
Closed
10 tasks
sloretz
sloretz reviewed Nov 2, 2018
View reviewed changes
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just minor comments

rcl_action/src/rcl_action/goal_handle.c Outdated Show resolved Hide resolved
rcl_action/src/rcl_action/goal_handle.c Outdated Show resolved Hide resolved
rcl_action/src/rcl_action/goal_handle.c Outdated
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up for rcl #321

rcl_action/src/rcl_action/goal_state_machine.c Outdated Show resolved Hide resolved
rcl_action/test/rcl_action/test_goal_handle.cpp Show resolved Hide resolved
hidmic
hidmic reviewed Nov 5, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! Left a few comments here and there.

rcl_action/src/rcl_action/goal_handle.c Outdated Show resolved Hide resolved
rcl_action/src/rcl_action/goal_handle.c Outdated
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rcl_action/test/rcl_action/test_goal_handle.cpp Outdated Show resolved Hide resolved
rcl_action/test/rcl_action/test_goal_handle.cpp Outdated Show resolved Hide resolved
rcl_action/test/rcl_action/test_goal_handle.cpp Outdated Show resolved Hide resolved
rcl_action/test/rcl_action/test_goal_handle.cpp Outdated Show resolved Hide resolved
@jacobperron
Copy link
Member Author

jacobperron commented Nov 5, 2018 •
edited

CI up to rcl_action:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

hidmic
hidmic approved these changes Nov 6, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jacobperron
Copy link
Member Author

Fix Clang warning:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron merged commit b2578bb into master Nov 6, 2018
@jacobperron jacobperron deleted the jacob/goal_handle branch November 6, 2018 18:28
@jacobperron jacobperron removed the in review Waiting for review (Kanban column) label Nov 6, 2018
@hidmic hidmic mentioned this pull request Nov 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz left review comments

@hidmic hidmic hidmic approved these changes

Assignees

@jacobperron jacobperron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jacobperron @sloretz @hidmic