Added tool to request transition to ErrorState #1758
Conversation
Signed-off-by: Aitor Miguel Blanco <[email protected]>
There was a problem hiding this comment.
Looks good, really small changes. Can you also include a test that covers this? I know we'll also write a test using this for all the on_error methods, but we should make sure this itself works. You can do this by making a small lifecycle node that you transition to the valid states (configure, cleanup, shut down, activate, deactivate) and then calling this and verifying that its now in the error state with a boolean.
| @@ -77,6 +77,33 @@ void reset_lifecycle_nodes( | |||
| reset_lifecycle_nodes(split(nodes, ':'), service_call_timeout, retries); | |||
| } | |||
|
|
|||
| /// Transition the given lifecycle nodes to the ErrorProcessing state in order | |||
| /** At this time, service calls frequently hang for unknown reasons. The only | |||
| * way to combat that is to timeout the service call and retry it. To use this | |||
There was a problem hiding this comment.
I'm really curious, is this timeout issue still an issue? That was circa Bouncy when that message was added to this tool, we've come a long way since then.
There was a problem hiding this comment.
I actually have no idea, I added it as well for consistency with the other tools. I can take a look at it and see if this was fixed somewhere. Or if you know how to test it?
There was a problem hiding this comment.
Eh, lets just leave it. Its not hurting anyone. But I would be curious if in your unit tests you set the number of retries to 0 so that it fails and see if we ever catch it in your development / CI later. If it doesn't cause problems, we can remove it later.
nav2_util/src/lifecycle_utils.cpp
Outdated
| // to the ErrorProcessing state. Some states do not support transitions | ||
| // to ErrorProcessing. | ||
| switch (sc.get_state(service_call_timeout)) { | ||
| case State::PRIMARY_STATE_UNKNOWN: |
There was a problem hiding this comment.
For all of these return cases, please throw an exception since they're invalid.
Yes I would like to write a test for the tool as well, but I wanted to ask for help on how to make it.
I'll try something like that then, it's similar to what I though. Thanks! |
|
For the exception, you can also just have the valid cases as cases, and then the default is throwing the exception so you don't need to list all of them. Then in the test, also make sure you trigger that exception case for one of those states, doesn't matter which . |
Signed-off-by: Aitor Miguel Blanco <[email protected]>
|
Since the lifecycle node transitions to the ErrorProcessing state are not exposed to client nodes (they are only internal transitions), it won't be possible to test the on_error state with this tool. Since the tool is very much useless, I'll close this pull request. It could be reopened though if the lifecycle transitions are updated to ErrorProcessing are updated. |
Signed-off-by: Aitor Miguel Blanco [email protected]
Included tool in lifecycle utils to call a transition to the error state in lifecycle nodes.
Basic Info
Description of contribution in a few bullet points
This is a preliminary version of the tool, and I would like to ask for some feedback.
At the moment, this tool can only transition from Transition States to ErrorProcessing. This is because the Primary States do not include (yet) the transitions to the ErrorProcessing state.
The pull requests ros2/rcl_interfaces#97, ros2/rcl#618 and ros2/rclcpp#1064 address this issue by including the extra transitions for Primary States plus a
raise_errorfunction that triggers the transition from the active and inactive states.Because of this limitation, it won't be that easy to test the
on_errorcallback from lifecycle nodes until the mentioned PRs are merged.I would also appreciate any ideas about how to test the tool itself. I can only think on creating a mock class that waits on specific states for the error to be triggered.
Description of documentation updates required from your changes
Future work that may be required in bullet points