ros2_control icon indicating copy to clipboard operation
ros2_control copied to clipboard

Split the test of lifecycle into lifecycle and strictness with unknown controllers

Open Xi-HHHM opened this issue 3 years ago • 4 comments

Hi,

this is a follow up draft PR of #582. There two tasks mentioned in the previous PR Originally posted by @destogl in https://github.com/ros-controls/ros2_control/issues/582#issuecomment-1029931561

I am not sure about some part of the test, so I think it would be better to open an draft PR for discussion.

  1. In the case of unknown controllers, two controllers with the same controllers class can be started/active at the same time. The only difference between them is their controller name in the controller manager. If I understand it right, they should have resource collision, but they actually don't. Is this the behaviour that we expected?

  2. If it is expected, then how should we produce resource collision? During the configuration of test_controller in controller_manager/test/test_controller/test_controller.cpp, no resources are claimed and the on_configure function simply returns a status code. Should we do some changes within this function?

  3. I checked the the handle_conflict in the controller_manager/src/controller_manager.cpp, where the strictness plays a role. This function handles only three cases, i.e., double stop, double start, and illegal start of an unconfigured/finalized controller. Is one of these three cases relevant for the test of "activating multiple controllers when one has a resource collision" mentioned in PR #582? Another case where strictness is involved is that the controller can not be found. For this one, our case have already covered it.

Xi-HHHM avatar Mar 10 '22 13:03 Xi-HHHM

Just a short summary after the talk with @destogl :

  • For point 1 & 2, we should explicitly define the resource configuration of the controller with set_command_interface_configuration as in controller_manager/test/test_controller_manager_srvs.cpp.
  • For point 3, we should check the resource conflict earlier in the switching function. Where exactly should we check the resource conflict is still open for discussion.

Xi-HHHM avatar Mar 14 '22 10:03 Xi-HHHM

Hi,

in the last two commits, I implemented a new test for resource conflict as described before. The new test follows this logic:

  • start a controller called test_controller at first;
  • then start another two controllers test_controller_2 and test_controller_3 at the same time, where the test_controller_3 has same resources in command_interface as the test_controller

The expected outcome is that we can start test_controller_2 by BEST_EFFORT and can NOT by STRICT. However, the current code can start the test_controller_2 in both strictness. That is the reason why the test failed.

Why does the controller_manager have same behaviour in this case? I suppose that the resource conflict is not handled related to the strictness, as mentioned in the third point above. What do you think? @destogl

Xi-HHHM avatar Mar 15 '22 14:03 Xi-HHHM

Again a short summary after talk with @destogl:

  1. We need to check the resource conflict with regard to strictness here: https://github.com/Xi-HHHM/ros2_control/blob/controller_conflicts_test/controller_manager/src/controller_manager.cpp#L594
  2. An additional test maybe wanted. In this test, there are three controllers. Two of them has the same resources. We start them at the same time/switch. The expected behaviour should be following:
  • STRICT: No controllers should be started
  • BEST_EFFORT: Only the controller without resource conflict can be started.
  • The current implementation may start the two controllers in both strictness. Why? The for loop in https://github.com/Xi-HHHM/ros2_control/blob/controller_conflicts_test/controller_manager/src/controller_manager.cpp#L842 will break until a resource conflict is found. Thus, how many controllers can be started depends on order in the start_controller_list.

Xi-HHHM avatar Mar 16 '22 14:03 Xi-HHHM

This pull request is in conflict. Could you fix it @Xi-HHHM?

mergify[bot] avatar Mar 17 '22 21:03 mergify[bot]

This pull request is in conflict. Could you fix it @Xi-HHHM?

mergify[bot] avatar Jul 09 '23 20:07 mergify[bot]

This pull request is in conflict. Could you fix it @Xi-HHHM?

mergify[bot] avatar Sep 16 '23 19:09 mergify[bot]

This pull request is in conflict. Could you fix it @Xi-HHHM?

mergify[bot] avatar Jan 29 '24 18:01 mergify[bot]