-
Notifications
You must be signed in to change notification settings - Fork 166
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
[rcl_action] Implement goal handle #320
Conversation
jacobperron
commented
Nov 2, 2018
- 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
There was a problem hiding this 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
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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 | |
} |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
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); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!