ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
308 stars 227 forks source link

avoid lifecycle node transition exception #1319

Open fujitatomoya opened 3 months ago

fujitatomoya commented 3 months ago

addresses https://github.com/ros2/rclpy/issues/1209

replaces https://github.com/ros2/rclpy/pull/1317

fujitatomoya commented 3 months ago

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

Barry-Xu-2018 commented 3 months ago

About __change_state(), my understanding is

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 commented 3 months ago

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