ros-controls / ros2_control

Generic and simple controls framework for ROS 2
https://control.ros.org
Apache License 2.0
479 stars 293 forks source link

Update spawner to have a timeout on service call #1566

Closed gennartan closed 1 month ago

gennartan commented 3 months ago

In some cases, the service call might never return. As a consequence, the call to spin_until_future_complete will loop infinitely.

When the error happens:


Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:

  1. Limited scope. Your PR should do one thing or one set of things. Avoid adding “random fixes” to PRs. Put those on separate PRs.
  2. Give your PR a descriptive title. Add a short summary, if required.
  3. Make sure the pipeline is green.
  4. Don’t be afraid to request reviews from maintainers.
  5. New code = new tests. If you are adding new functionality, always make sure to add some tests exercising the code and serving as live documentation of your original intention.

To send us a pull request, please:

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.64%. Comparing base (af4b48f) to head (e25efe8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1566 +/- ## ========================================== - Coverage 86.67% 86.64% -0.03% ========================================== Files 115 115 Lines 10528 10529 +1 Branches 967 967 ========================================== - Hits 9125 9123 -2 - Misses 1056 1058 +2 - Partials 347 348 +1 ``` | [Flag](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1566/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1566/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | `86.64% <100.00%> (-0.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1566?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [.../controller\_manager/controller\_manager\_services.py](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1566?src=pr&el=tree&filepath=controller_manager%2Fcontroller_manager%2Fcontroller_manager_services.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-Y29udHJvbGxlcl9tYW5hZ2VyL2NvbnRyb2xsZXJfbWFuYWdlci9jb250cm9sbGVyX21hbmFnZXJfc2VydmljZXMucHk=) | `67.74% <100.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1566/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls)
fmauch commented 3 months ago

Related: #1501

gennartan commented 3 months ago

Related: #1501

Yes, PR #1501 seems to add the usage of timeout_sec of spin_until_future_complete as I did here. With two main differences:

I don't know what would be the expected behavior from the maintainers. It is up to them to decide which behavior is prefered.

That said, I think merging one of those PR could be a priority since it is a bug in ros2_control that can prevent a robot to start. In my case, I am starting the spawner in a launchfile and in about 30% of the cases, I must manually restart the spawner because the node didn't start correctly. Other projects in my company had the same issues.

destogl commented 1 month ago

Closing in favor of #1501 (decided on WG Meeting Aug. 14th)