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

avoid lifecycle node transition exception #1319

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

fujitatomoya
Copy link
Collaborator

addresses #1209

replaces #1317

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Copy link
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

with this fix, we can avoid the crash on action server from exception and the action client can receive the response with failure as below.

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 run lifecycle_py lifecycle_talker 
[WARN] [1721693858.103483007] [rcl_lifecycle]: No transition matching 2 found for current state unconfigured
[ERROR] [1721693858.114465029] [lc_talker]: Unable to start transition 2 from current state (1, 'unconfigured')

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 service call /lc_talker/change_state lifecycle_msgs/srv/ChangeState "{transition: {id: 2}}"
requester: making request: lifecycle_msgs.srv.ChangeState_Request(transition=lifecycle_msgs.msg.Transition(id=2, label=''))

response:
lifecycle_msgs.srv.ChangeState_Response(success=False)

@fujitatomoya
Copy link
Collaborator Author

@mjforan @Barry-Xu-2018 @chrisbitter if any of you can do the review or test, that will be really appreciated.

@Barry-Xu-2018
Copy link
Contributor

Barry-Xu-2018 commented Jul 23, 2024

About __change_state(), my understanding is

  • Before calling this function, the state machine should be in a primary state (unconfigured, inactive, active or finalized). After calling this function, it should switch to another primary state.
  • The argument transition_id for __change_state() should be one of configure, cleanup, activate, deactivate, unconfigured_shutdown, inactive_shutdown and active_shutdown

So, when returning from __change_state(), we need to ensure that the state machine is in a primary state, not a transition state (such as configuring, cleaningup, shuttingdown, etc). If trigger_transition_by_id/trigger_transition_by_label throw exception, it should call on_error and then switch to unconfigured or finalized state.

Is my understanding correct?

Besides, __change_state() may be executed concurrently (the program modifies the state while an external process modifies the state through change_state service). This function does not have lock protection

@mjforan
Copy link

mjforan commented Aug 1, 2024

Testing the basic lifecycle_py demo shows the change working properly.
Before:
Screenshot from 2024-08-01 16-16-20
After:
Screenshot from 2024-08-01 16-16-52
I had to use the Rolling branch of rclpy because lifecycle_py doesn't work under Jazzy.

I agree with @Barry-Xu-2018; the exception should not be handled the same way for all three instances.
My opinion:

x exception caused by how to handle
1 invalid transition request write to log and return error to caller
2 unable to transition out of transition state enter ErrorProcessing
3 unable to transition out of ErrorProcessing raise an exception and crash

PositiveBeat added a commit to energinet-digitalisering/energirobotter-elrik that referenced this pull request Sep 27, 2024
If bad state change is called, a Python lifecycle node will crash.
Until this PR (ros2/rclpy#1319) is merged, this
is a workaround.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants