-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add service call timeout argument in spawner #1808
Add service call timeout argument in spawner #1808
Conversation
Signed-off-by: Angsa Deployment Team <[email protected]>
Signed-off-by: Angsa Deployment Team <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1808 +/- ##
==========================================
+ Coverage 87.97% 88.01% +0.03%
==========================================
Files 121 121
Lines 12403 12404 +1
Branches 1105 1105
==========================================
+ Hits 10912 10917 +5
+ Misses 1085 1083 -2
+ Partials 406 404 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please rebase/merge on master and update the release_notes and docs as I've done in this PR.
Signed-off-by: Tony Najjar <[email protected]>
Done. I believe CI is failing not because of this PR |
Co-authored-by: Christoph Fröhlich <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the follow-up!
Signed-off-by: Tony Najjar <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
I rebased to master and resolved conflicts caused by https://github.com/ros-controls/ros2_control/pull/1790/files. Could you please review @christophfroehlich @saikishor |
@@ -124,6 +128,13 @@ def main(args=None): | |||
default=5.0, | |||
type=float, | |||
) | |||
parser.add_argument( | |||
"--controller-manager-call-timeout", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change this to --service-call--timeout
? just to have a shorter naming? I'm saying as in future if we add more services, then it would be more generic.
@christophfroehlich what's your opinion on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it already. It's an easy revert if @christophfroehlich disagrees. I don't have any strong opinion
Signed-off-by: Tony Najjar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Sai Kishor Kothakota <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
41d7393
into
ros-controls:master
--------- Signed-off-by: Angsa Deployment Team <[email protected]> Signed-off-by: Tony Najjar <[email protected]> Co-authored-by: Angsa Deployment Team <[email protected]> Co-authored-by: Christoph Fröhlich <[email protected]> Co-authored-by: Sai Kishor Kothakota <[email protected]> (cherry picked from commit 41d7393) # Conflicts: # controller_manager/controller_manager/controller_manager_services.py # controller_manager/doc/userdoc.rst # doc/release_notes.rst
--------- Signed-off-by: Angsa Deployment Team <[email protected]> Signed-off-by: Tony Najjar <[email protected]> Co-authored-by: Angsa Deployment Team <[email protected]> Co-authored-by: Christoph Fröhlich <[email protected]> Co-authored-by: Sai Kishor Kothakota <[email protected]>
--------- Signed-off-by: Angsa Deployment Team <[email protected]> Signed-off-by: Tony Najjar <[email protected]> Co-authored-by: Tony Najjar <[email protected]> Co-authored-by: Angsa Deployment Team <[email protected]>
This PR exposes the call_timeout of the spawner to be optionally set by the user