rclpy icon indicating copy to clipboard operation
rclpy copied to clipboard
verified publisher icon ros2

avoid lifecycle node transition exception

Open fujitatomoya opened this issue 1 year ago • 3 comments

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

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

fujitatomoya avatar Jul 23 '24 00:07 fujitatomoya

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

fujitatomoya avatar Jul 23 '24 00:07 fujitatomoya

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

Barry-Xu-2018 avatar Jul 23 '24 08:07 Barry-Xu-2018

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

mjforan avatar Aug 01 '24 20:08 mjforan

@mjcarroll @sloretz @ahcorde can you take a look when you have time?

fujitatomoya avatar Dec 01 '24 23:12 fujitatomoya

@ahcorde appreciate your time for the review, thanks! yeah, i will do rebase and resolve the conflict, and get back to you.

fujitatomoya avatar Dec 02 '24 17:12 fujitatomoya

@Barry-Xu-2018 about your comment on https://github.com/ros2/rclpy/pull/1319#issuecomment-2244532000 makes sense. (including locking mechanism) AFAIS, current PR is aligned with rclcpp behavior. how about addressing this comment with another issue? this is gonna be more complicated and takes time to verify.

fujitatomoya avatar Dec 02 '24 21:12 fujitatomoya

Pulls: ros2/rclpy#1319 Gist: https://gist.githubusercontent.com/fujitatomoya/1d9faa816731c8aee120548004e4bb46/raw/7ea0f998c7e152f14f1d178cf76fa6d108383ba1/ros2.repos BUILD args: --packages-above-and-dependencies rclpy TEST args: --packages-above rclpy ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14891

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

fujitatomoya avatar Dec 02 '24 22:12 fujitatomoya

@Barry-Xu-2018 friendly ping

fujitatomoya avatar Dec 12 '24 06:12 fujitatomoya

LGTM

Barry-Xu-2018 avatar Dec 12 '24 09:12 Barry-Xu-2018

  • Linux-rhel Build Status
  • Windows Build Status

fujitatomoya avatar Dec 12 '24 17:12 fujitatomoya

https://ci.ros2.org/job/ci_linux-rhel/1789/#showFailuresLink is unrelated.

fujitatomoya avatar Dec 13 '24 18:12 fujitatomoya