expose error handling for state changes#344
Conversation
| resp->success = change_state(req->transition.id); | ||
| rcl_lifecycle_ret_t error; | ||
| auto ret = change_state(req->transition.id, error); | ||
| (void) error; |
There was a problem hiding this comment.
Maybe you should call this callback_return_code or something. error is not very descriptive.
| // error handling ?! | ||
| // TODO(karsten1987): iterate over possible ret value | ||
| if (cb_success == RCL_LIFECYCLE_RET_ERROR) { | ||
| RCUTILS_LOG_WARN("Error occured. Executing error handling.") |
There was a problem hiding this comment.
nitpick, I don't think this should be two sentences, instead maybe "Error occurred while doing error handling". (also occurred is misspelled)
| const State & | ||
| trigger_transition(const Transition & transition); | ||
|
|
||
| RCLCPP_LIFECYCLE_PUBLIC |
There was a problem hiding this comment.
Why is this function getting a rcl_ret_t argument while the other functions get a rcl_lifecycle_ret_t?
| return impl_->trigger_transition(transition_id); | ||
| } | ||
|
|
||
| const State & |
There was a problem hiding this comment.
This signature doesn't match the header (rcl_ret_t vs. rcl_lifecycle_ret_t)?
| rcl_lifecycle_ret_t error; | ||
| auto ret = change_state(req->transition.id, error); | ||
| (void) error; | ||
| resp->success = (ret == RCL_RET_OK) ? true : false; |
There was a problem hiding this comment.
The expression in the parenthesis is already the boolean you want. No need to the ternary operator here.
| if (rcl_lifecycle_trigger_transition(&state_machine_, error_resolved, true) != RCL_RET_OK) { | ||
| fprintf(stderr, "Failed to call cleanup on error state\n"); | ||
| return false; | ||
| RCUTILS_LOG_ERROR("Failed to call cleanup on error state"); |
There was a problem hiding this comment.
No semicolon after logging macros.
Same below.
|
I hopefully addressed all comments. If nobody has further comments until tomorrow afternoon, i'll go ahead and merge. ran a new batch of ci: linux: |
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
two changes in this PR:
1.) remove
fprintfs and replace them withRCUTILS_LOG..2.) every state change has now an additional parameter to see whether the action went to an erroneous state while transitioning.
CI:
linux:
osx:
windows: