-
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
Use singleton approach to store and reuse the service clients #1949
Use singleton approach to store and reuse the service clients #1949
Conversation
@bijoua29 Check this approach to handle the same situations |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1949 +/- ##
==========================================
- Coverage 87.73% 87.71% -0.02%
==========================================
Files 122 122
Lines 13010 13020 +10
Branches 1165 1166 +1
==========================================
+ Hits 11414 11421 +7
- Misses 1165 1167 +2
- Partials 431 432 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@saikishor Looks good to me but let me test this in the actual application |
Sure. Go ahead. Please let us know if this helps in such situations. I've only manually verified in the tests that if it is reusing it or not. This fix along with the multiple controllers spawning should be more robust |
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
@saikishor I tested the PR in our application and replicated the same results as before i.e. improvement in controller startup success. So I think we can merge this in. I approved it. BTW, I wouldn't say it completely fixes #1934 but it definitely improves it. I will close #1947 in lieu of this PR. |
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.
This seems great!
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.
What do we have to change in ros2_control_demo_testing to make this work?
Thanks for bringing it to our attention @christophfroehlich. Now it should be all good. I verified locally |
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 taking care of the downstream test failures ;)
b039baa
into
ros-controls:master
(cherry picked from commit b039baa) # Conflicts: # controller_manager/controller_manager/controller_manager_services.py
Fixes: #1934
the node.destroy_node() at the end does the same thing by destroying clients and every subscribers, publishers etc (https://github.com/ros2/rclpy/blob/8f1f16f16090184062f1993728d063ff2680f958/rclpy/rclpy/node.py#L2017-L2053)