Open TakashiSato opened 1 week ago
codecov[bot]
commented
1 week ago Attention: Patch coverage is 97.05882% with 2 lines in your changes missing coverage. Please review.
Project coverage is 87.88%. Comparing base (
fbb893b) to head (005a79f). Report is 3 commits behind head on master.
mergify[bot]
commented
2 days ago This pull request is in conflict. Could you fix it @TakashiSato?
TakashiSato
commented
2 days ago I have fixed the conflict.
Due to the changes in the recently merged #1021, BUG3 no longer manifests. Regarding BUG1 and BUG2, issues still persist. For BUG3, although the issue no longer occurs, there are still abnormal requests in the from/to_chained_mode request up until just before the error check. Therefore, I believe the fix proposed in this PR remains desirable.
This PR is separated to address only the changes related to bug fix #1568 , fixing a bug that occurs under specific conditions when handling chainable controllers in the controller_manager's switch_controller.
Note: To focus on meaningful changes, it is recommended to compare the code using "hide whitespace" mode as follows. https://github.com/ros-controls/ros2_control/compare/master...TakashiSato:ros2_control:feature/fix_switch_controller_bug?diff=split&w=1
Explanation of the Bugs
Using the typical model for controller chaining, which is also used in this test, the following explanations introduce three situations where bugs occur.
Note that these bugs occur only when the strictness value is set to BEST_EFFORT. When the strictness value is set to STRICT, switch_controller fails, and these bugs do not arise. Additionally, the test cases to detect these bugs are implemented in this commit.
BUG1
pid_left(not_chained),pid_right(not_chained)position_tracking,diff_drivediff_drivepid_rightpid_left(in_chained)position_tracking,diff_drive,pid_rightIn this situation, the start request for
diff_driveshould be rejected, and only the stop command forpid_rightshould be accepted. While this result is achieved, there is an issue wherepid_leftincorrectly enters in_chained mode.The cause of this bug lies in this section. When checking the activate request for
diff_drive, the first following controller,pid_left, is checked for any inconsistencies along with the request. Since no issues are detected,pid_leftis added to theto_chained_mode_request. However, when the check is performed with the next controller,pid_right, inconsistencies are detected becausepid_rightis targeted for deactivation, leading to the rejection of thediff_driveactivate request. Since theto_chained_mode_requestis not re-evaluated in subsequent processes, this request remains and results in the actual transition to chained mode during manage_switch.BUG2
position_tracking,diff_drive,pid_left,pid_rightdiff_drive,pid_leftpid_left(in_chained)position_tracking,diff_drive,pid_rightThis situation is similar to BUG1, where the start request for diff_drive is rejected, and only
pid_leftis activated. However,pid_leftstill incorrectly enters in_chained mode. The cause of this bug is almost the same as BUG1. During the activate check fordiff_drive, since the following controllerpid_leftis also a target for activation,pid_leftis added to theto_chained_mode_request. Then, during the subsequent check withpid_right, sincepid_rightis not a target for activation, the activate request fordiff_driveis rejected, leaving theto_chained_mode_requestforpid_left.Incidentally, in this situation, the checks are performed in the order of
pid_leftfollowed bypid_right. Therefore, ifpid_rightandpid_leftin the switch requests for BUG1 and BUG2 are swapped, the checks will function correctly, and these issues will not occur.BUG3
position_tracking(not_chained),diff_drive(in_chained),pid_left(in_chained),pid_right(in_chained)diff_driveIn this situation, since it is impossible to deactivate only
diff_drive, the request should be rejected and no changes should occur. However, the following switch requests are internally generated, and since these switches cannot be handled properly, a deadlock occurs.pid_left,pid_rightpid_left,pid_rightThe sequence in which such switch requests are generated is as follows:
diff_driveis a deactivate target, inpropagate_deactivation_of_chained_mode, its following controllers,pid_leftandpid_right, are added to thefrom_chained_mode_request.check_following_controllers_for_activatedoes nothing. (Note: The process to erase from from_chained_mode_request exists only within this function.)check_preceding_controllers_for_deactivate, the preceding controller ofdiff_drive,position_tracking, is active and not present in the deactivate_request, resulting in the rejection of thediff_drivedeactivate request.pid_leftandpid_rightare included in thefrom_chained_mode_requestand meet the conditions, they are added to the(de)activate_requestas restart targets due to this process.Bug Fix Proposal
In this part of the check process,
propagate_deactivation_of_chained_mode,check_following_controllers_for_activate, andcheck_preceding_controllers_for_deactivateperform judgments based on the content of the(de)activate_request. Therefore, if the content of the (de)activate_request changes, the process should be retried from the beginning. However, during this process, it is also possible that the content offrom/to_chained_mode_requesthas changed, so it is necessary to properly clear these as well.This commit demonstrates the simplest change to fix the bug based on this idea.
However, since the above commit uses
goto, which is generally not recommended, the final implementation in this PR has been rewritten using lambda functions and a while loop for the retry process (The corresponding commit).