Add CM switch_controller service timeout as parameter to spawner.py

[christophfroehlich]

This is a workaround for ros-controls/gz_ros2_control#421 and ros-controls/gazebo_ros2_control#380

If the simulation is started in paused mode, one can now configure also the timeout for the switch_controller service to be completed (the simulation has to be unpaused to perform the controller switches).

Also fixes docs after changes to the spawners in #1562

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.31%. Comparing base (83fff77) to head (feb09b6).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1790      +/-   ##
==========================================
- Coverage   87.33%   87.31%   -0.02%     
==========================================
  Files         121      121              
  Lines       12140    12145       +5     
  Branches     1086     1086              
==========================================
+ Hits        10603    10605       +2     
- Misses       1148     1150       +2     
- Partials      389      390       +1     

Flag	Coverage Δ
unittests	87.31% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
controller_manager/controller_manager/spawner.py	72.22% <100.00%> (+0.44%)	⬆️
controller_manager/controller_manager/unspawner.py	69.23% <100.00%> (+1.66%)	⬆️

... and 1 file with indirect coverage changes

[fmauch]

From my point of view it isn't really clear from a user's perspective, what the difference between these two timeouts is.

Also: Wouldn't it make sense to default to 0.0 with both of them? This can probably also happen when activation of a hardware component fails, right?

[christophfroehlich]

Thanks for the feedback.

From my point of view it isn't really clear from a user's perspective, what the difference between these two timeouts is.

The old one --controller-manager-timeout is for the service call timeout

ros2_control/controller_manager/controller_manager/controller_manager_services.py

Lines 79 to 80 in 0433960

	@param service_timeout Timeout (in seconds) to wait until the service is available. 0 means
	waiting forever, retrying every 10 seconds.

while the new one --controller-manager-switch-timeout is for the switch_controller service. For example, it fails if the controller does not activate within the given timeout when the RT update method of the CM is called.

Any suggestions for a better name?

Also: Wouldn't it make sense to default to 0.0 with both of them?

5s was the hardcoded value before, 0s as switch_controller timeout will be overridden with 1s in the CM, I'm not sure which value is the best here.

This can probably also happen when activation of a hardware component fails, right?

No, this value is only used for the switch_controller service.

[fmauch]

while the new one --controller-manager-switch-timeout is for the switch_controller service. For example, it fails if the controller does not activate within the given timeout when the RT update method of the CM is called.

I see, that does make sense. I was not aware of that, sorry. So the problem is not that this particular service isn't available, but the CM itself doesn't manage to do the requested switch within the requested time. I think the name might be fine, but the docstring / the output could explain that better. My main motivation is: If "the average user" would stumble into ros-controls/gz_ros2_control#421, would she know that this timeout has to be increased? And wouldn't it make sense to allow an infinite amount of time for such as case? This way, users could specify the switch time for their spawners to infinity in launchfiles that start gz in paused mode.

[christophfroehlich]

I see, that does make sense. I was not aware of that, sorry. So the problem is not that this particular service isn't available, but the CM itself doesn't manage to do the requested switch within the requested time. I think the name might be fine, but the docstring / the output could explain that better. My main motivation is: If "the average user" would stumble into ros-controls/gz_ros2_control#421, would she know that this timeout has to be increased?

you are totally right, I try to come up with something.

And wouldn't it make sense to allow an infinite amount of time for such as case? This way, users could specify the switch time for their spawners to infinity in launchfiles that start gz in paused mode.

@saikishor what do you think, can we switch the CM behavior to wait infinitely if the time is set to 0?

[saikishor]

@saikishor what do you think, can we switch the CM behavior to wait infinitely if the time is set to 0?

Switching this wouldn't be a big issue in terms of code, but I think it is better to stick to a constant value and let the people play with it. If we leave it to infinity, if for some reason, it is blocked, we cannot access any other services at all. This would be a huge cumbersome to debug in my opinion

[saikishor]

Any suggestions for a better name?

@christophfroehlich @fmauch How about --switch-timeout I don't think we have to be that explicit in the arg naming, instead we can add the necessary information to the documentation of the arg

[christophfroehlich]

Switching this wouldn't be a big issue in terms of code, but I think it is better to stick to a constant value and let the people play with it. If we leave it to infinity, if for some reason, it is blocked, we cannot access any other services at all. This would be a huge cumbersome to debug in my opinion

ok, so do you prefer to set the default in the spawner to the same like in the cm?

[saikishor]

ok, so do you prefer to set the default in the spawner to the same like in the cm?

It can be different I don't mind, what I don't prefer to avoid is infinite waiting with no timeout at all. Usually, it needs to happen immediately in the same cycle.

[christophfroehlich]

I just tried it with unpaused gz. and with a timeout of 1s it fails: it needs more time from the plugin loading until the simulation runs.
I changed it to 5.0s again, which was the hardcoded behavior before.

[fmauch]

Looks good to me. Thanks for all the iterations!

[saikishor]

LGTM. Thank you for checking everything

[mergify]

This pull request is in conflict. Could you fix it @christophfroehlich?