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.

Sign up for GitHub

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

Improve error messages in rcl_lifecycle #742

Merged
merged 6 commits into from
Sep 28, 2020
Merged

Improve error messages in rcl_lifecycle #742

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a7a495b
Improve error messages in rcl_lifecycle
llapx Aug 12, 2020
6456f9f
Add the detail code.
llapx Aug 12, 2020
7165b8e
Update code with RCL_CHECK_FOR_NULL_WITH_MSG.
llapx Aug 17, 2020
07b4e72
Update code as suggested.
llapx Aug 18, 2020
fd58e88
Update code as suggested.
llapx Aug 25, 2020
2bc301e
remove no need returned values.
llapx Sep 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions rcl/include/rcl/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ typedef rmw_ret_t rcl_ret_t;
/// Failed to take an event from the event handle
#define RCL_RET_EVENT_TAKE_FAILED 2001

/// rcl_lifecycle state register ret codes in 30XX
/// rcl_lifecycle state registered
#define RCL_RET_LIFECYCLE_STATE_REGISTERED 3000
/// rcl_lifecycle state not registered
#define RCL_RET_LIFECYCLE_STATE_NOT_REGISTERED 3001

/// typedef for rmw_serialized_message_t;
typedef rmw_serialized_message_t rcl_serialized_message_t;

Expand Down
9 changes: 9 additions & 0 deletions rcl_lifecycle/include/rcl_lifecycle/rcl_lifecycle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ rcl_lifecycle_get_zero_initialized_state();
* \param[in] label label of the state
* \param[in] allocator a valid allocator used to initialized the lifecycle state
* \return `RCL_RET_OK` if state was initialized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -106,6 +107,7 @@ rcl_lifecycle_state_init(
* \param[inout] state struct to be finalized
* \param[in] allocator a valid allocator used to finalize the lifecycle state
* \return `RCL_RET_OK` if the state was finalized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

can this error be returned?

Copy link
Contributor Author

@llapx llapx Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

RCL_RET_ERROR is no need anymore.

*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -152,6 +154,7 @@ rcl_lifecycle_get_zero_initialized_transition();
* \param[in] goal the objetive of the transition
* \param[in] allocator a valid allocator used to finalize the lifecycle state
* \return `RCL_RET_OK` if the transition is initialized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -182,6 +185,7 @@ rcl_lifecycle_transition_init(
* \param[inout] transition struct to be finalized
* \param[in] allocator a valid allocator used to finalize the transition
* \return `RCL_RET_OK` if the state was finalized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -229,6 +233,7 @@ rcl_lifecycle_get_zero_initialized_state_machine();
* the state_machine pointer is only used to initialize the interfaces
* \param[in] allocator a valid allocator used to initialized the state machine
* \return `RCL_RET_OK` if the state machine was initialized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if input params is NULL, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -264,6 +269,7 @@ rcl_lifecycle_state_machine_init(
* \param[in] node_handle valid (not finalized) handle to the node
* \param[in] allocator a valid allocator used to finalize the state machine
* \return `RCL_RET_OK` if the state was finalized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand All @@ -290,6 +296,7 @@ rcl_lifecycle_state_machine_fini(
*
* \param[in] state_machine pointer to the state machine struct
* \return `RCL_RET_OK` if the state is initialized, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this error returned ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RCL_RET_ERROR is no need anymore.

*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -365,6 +372,7 @@ rcl_lifecycle_get_transition_by_label(
* \param[in] publish_notification if the value is `true` a message will be published
* notifying the transition, otherwise no message will be published
* \return `RCL_RET_OK` if the transition was triggered successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down Expand Up @@ -394,6 +402,7 @@ rcl_lifecycle_trigger_transition_by_id(
* \param[in] publish_notification if the value is `true` a message will be published
* notifying the transition, otherwise no message will be published
* \return `RCL_RET_OK` if the transition was triggered successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down
9 changes: 7 additions & 2 deletions rcl_lifecycle/include/rcl_lifecycle/transition_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ rcl_lifecycle_get_zero_initialized_transition_map();
*
* \param[in] transition_map pointer to the transition map struct to check
* \return `RCL_RET_OK` if the transition map is initialized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if the transition map is not initialized.
*/
RCL_LIFECYCLE_PUBLIC
Expand All @@ -78,6 +79,7 @@ rcl_lifecycle_transition_map_is_initialized(
* \param[inout] transition_map struct to be deinitialized
* \param[in] allocator a valid allocator used to deinitialized the state machine
* \return `RCL_RET_OK` if the state was deinitialized successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand All @@ -99,18 +101,19 @@ rcl_lifecycle_transition_map_fini(
* Uses Atomics | No
* Lock-Free | Yes
*
* \param[in] map to be modified
* \param[in] transition_map to be modified
* \param[in] state the state to register
* \param[in] allocator a valid allocator used to register the state machine
* \return `RCL_RET_OK` if the state was registered successfully, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_LIFECYCLE_STATE_REGISTERED` if state is already registered, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function can not return RCL_RET_ERROR anymore, right?

Copy link
Contributor Author

@llapx llapx Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, it is no need anymore, sorry for that.
I will update the doc.

*/
RCL_LIFECYCLE_PUBLIC
RCL_WARN_UNUSED
rcl_ret_t
rcl_lifecycle_register_state(
rcl_lifecycle_transition_map_t * map,
rcl_lifecycle_transition_map_t * transition_map,
rcl_lifecycle_state_t state,
const rcl_allocator_t * allocator);

Expand All @@ -131,6 +134,8 @@ rcl_lifecycle_register_state(
* \param[in] allocator a valid allocator used to register the state machine
* \return `RCL_RET_OK` if the state was deinitialized successfully, or
* \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or
* \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return `RCL_RET_LIFECYCLE_STATE_NOT_REGISTERED` if state is not registered, or
* \return `RCL_RET_ERROR` if an unspecified error occurs.
*/
RCL_LIFECYCLE_PUBLIC
Expand Down
136 changes: 53 additions & 83 deletions rcl_lifecycle/src/rcl_lifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,25 +53,19 @@ rcl_lifecycle_state_init(
const char * label,
const rcl_allocator_t * allocator)
{
if (!allocator) {
RCL_SET_ERROR_MSG("can't initialize state, no allocator given\n");
return RCL_RET_ERROR;
}
if (!state) {
RCL_SET_ERROR_MSG("state pointer is null\n");
return RCL_RET_ERROR;
}
if (!label) {
RCL_SET_ERROR_MSG("State label is null\n");
return RCL_RET_ERROR;
}
RCL_CHECK_ALLOCATOR_WITH_MSG(
allocator, "can't initialize state, no allocator given\n", return RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_FOR_NULL_WITH_MSG(
state, "state pointer is null\n", return RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_FOR_NULL_WITH_MSG(
label, "State label is null\n", return RCL_RET_INVALID_ARGUMENT);

state->id = id;
state->label = rcutils_strndup(label, strlen(label), *allocator);
if (!state->label) {
RCL_SET_ERROR_MSG("failed to duplicate label for rcl_lifecycle_state_t\n");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
state->label, "failed to duplicate label for rcl_lifecycle_state_t\n", return RCL_RET_ERROR);

return RCL_RET_OK;
}
Expand All @@ -81,12 +75,10 @@ rcl_lifecycle_state_fini(
rcl_lifecycle_state_t * state,
const rcl_allocator_t * allocator)
{
RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_ERROR);
RCUTILS_CAN_RETURN_WITH_ERROR_OF(RCL_RET_INVALID_ARGUMENT);

if (!allocator) {
RCL_SET_ERROR_MSG("can't free state, no allocator given\n");
return RCL_RET_ERROR;
}
RCL_CHECK_ALLOCATOR_WITH_MSG(
allocator, "can't free state, no allocator given\n", return RCL_RET_INVALID_ARGUMENT);
// it is already NULL
if (!state) {
return RCL_RET_OK;
Expand Down Expand Up @@ -120,30 +112,24 @@ rcl_lifecycle_transition_init(
rcl_lifecycle_state_t * goal,
const rcl_allocator_t * allocator)
{
if (!allocator) {
RCL_SET_ERROR_MSG("can't initialize transition, no allocator given\n");
return RCL_RET_ERROR;
}
RCL_CHECK_ALLOCATOR_WITH_MSG(
allocator, "can't initialize transition, no allocator given\n",
return RCL_RET_INVALID_ARGUMENT);

if (!transition) {
RCL_SET_ERROR_MSG("transition pointer is null\n");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
transition, "transition pointer is null\n", return RCL_RET_INVALID_ARGUMENT);

if (!label) {
RCL_SET_ERROR_MSG("label pointer is null\n");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
label, "label pointer is null\n", return RCL_RET_INVALID_ARGUMENT);

transition->start = start;
transition->goal = goal;

transition->id = id;
transition->label = rcutils_strndup(label, strlen(label), *allocator);
if (!transition->label) {
RCL_SET_ERROR_MSG("failed to duplicate label for rcl_lifecycle_transition_t\n");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
transition->label, "failed to duplicate label for rcl_lifecycle_transition_t\n",
return RCL_RET_ERROR);

return RCL_RET_OK;
}
Expand All @@ -153,10 +139,8 @@ rcl_lifecycle_transition_fini(
rcl_lifecycle_transition_t * transition,
const rcl_allocator_t * allocator)
{
if (!allocator) {
RCL_SET_ERROR_MSG("can't finalize transition, no allocator given\n");
return RCL_RET_ERROR;
}
RCL_CHECK_ALLOCATOR_WITH_MSG(
allocator, "can't finalize transition, no allocator given\n", return RCL_RET_INVALID_ARGUMENT);
// it is already NULL
if (!transition) {
return RCL_RET_OK;
Expand Down Expand Up @@ -206,18 +190,15 @@ rcl_lifecycle_state_machine_init(
bool default_states,
const rcl_allocator_t * allocator)
{
if (!state_machine) {
RCL_SET_ERROR_MSG("State machine is null\n");
return RCL_RET_ERROR;
}
if (!node_handle) {
RCL_SET_ERROR_MSG("Node handle is null\n");
return RCL_RET_ERROR;
}
if (!allocator) {
RCL_SET_ERROR_MSG("can't initialize state machine, no allocator given\n");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
state_machine, "State machine is null\n", return RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_FOR_NULL_WITH_MSG(
node_handle, "Node handle is null\n", return RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_ALLOCATOR_WITH_MSG(
allocator, "can't initialize state machine, no allocator given\n",
return RCL_RET_INVALID_ARGUMENT);

rcl_ret_t ret = rcl_lifecycle_com_interface_init(
&state_machine->com_interface, node_handle,
Expand Down Expand Up @@ -247,10 +228,8 @@ rcl_lifecycle_state_machine_fini(
rcl_node_t * node_handle,
const rcl_allocator_t * allocator)
{
if (!allocator) {
RCL_SET_ERROR_MSG("can't free state machine, no allocator given\n");
return RCL_RET_ERROR;
}
RCL_CHECK_ALLOCATOR_WITH_MSG(
allocator, "can't free state machine, no allocator given\n", return RCL_RET_INVALID_ARGUMENT);

rcl_ret_t fcn_ret = RCL_RET_OK;

Expand Down Expand Up @@ -278,17 +257,17 @@ rcl_lifecycle_state_machine_fini(
rcl_ret_t
rcl_lifecycle_state_machine_is_initialized(const rcl_lifecycle_state_machine_t * state_machine)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we also need to update header file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

{
if (!state_machine->com_interface.srv_get_state.impl) {
RCL_SET_ERROR_MSG("get_state service is null");
return RCL_RET_ERROR;
}
if (!state_machine->com_interface.srv_change_state.impl) {
RCL_SET_ERROR_MSG("change_state service is null");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
state_machine->com_interface.srv_get_state.impl, "get_state service is null\n",
return RCL_RET_INVALID_ARGUMENT);

RCL_CHECK_FOR_NULL_WITH_MSG(
state_machine->com_interface.srv_change_state.impl, "change_state service is null\n",
return RCL_RET_INVALID_ARGUMENT);

if (rcl_lifecycle_transition_map_is_initialized(&state_machine->transition_map) != RCL_RET_OK) {
RCL_SET_ERROR_MSG("transition map is null");
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing to set error message. I think that it would be better to be independent bug fix only.

Copy link
Contributor Author

@llapx llapx Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

it would be better to be independent bug fix only.

do you mean revert this part code and make a new commit only for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fujitatomoya
Any tips?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry my memory is little hazy, but what i meant is replace RCL_SET_ERROR_MSG into RCL_CHECK_FOR_NULL_WITH_MSG, also related to https://github.com/ros2/rcl/pull/742/files#r471208047

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it came back! previously "transition map is null" error message is deleted, i was thinking that it would be better to keep the error message to be set.

Copy link
Contributor Author

@llapx llapx Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I had updated the code to revert it back.

return RCL_RET_ERROR;
return RCL_RET_INVALID_ARGUMENT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just return the error from rcl_lifecycle_transition_map_is_initialized instead? What about the following?

  1. check state_machine->transition_map is NULL, and if so return RCL_RET_INVALID_ARGUMENT. (This is invalid argument)
  2. issue rcl_lifecycle_transition_map_is_initialized, return the result as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you, updated!

}
return RCL_RET_OK;
}
Expand Down Expand Up @@ -342,16 +321,11 @@ _trigger_transition(
bool publish_notification)
{
// If we have a faulty transition pointer
if (!transition) {
RCL_SET_ERROR_MSG("Transition is not registered.");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
transition, "Transition is not registered.", return RCL_RET_INVALID_ARGUMENT);

if (!transition->goal) {
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME, "No valid goal is set");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
transition->goal, "No valid goal is set.", return RCL_RET_INVALID_ARGUMENT);
state_machine->current_state = transition->goal;

if (publish_notification) {
Expand All @@ -374,10 +348,8 @@ rcl_lifecycle_trigger_transition_by_id(
uint8_t id,
bool publish_notification)
{
if (!state_machine) {
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "state machine pointer is null");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
state_machine, "state machine pointer is null.", return RCL_RET_INVALID_ARGUMENT);

const rcl_lifecycle_transition_t * transition =
rcl_lifecycle_get_transition_by_id(state_machine->current_state, id);
Expand All @@ -391,10 +363,8 @@ rcl_lifecycle_trigger_transition_by_label(
const char * label,
bool publish_notification)
{
if (!state_machine) {
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "state machine pointer is null");
return RCL_RET_ERROR;
}
RCL_CHECK_FOR_NULL_WITH_MSG(
state_machine, "state machine pointer is null.", return RCL_RET_INVALID_ARGUMENT);

const rcl_lifecycle_transition_t * transition =
rcl_lifecycle_get_transition_by_label(state_machine->current_state, label);
Expand Down
Loading