-
Notifications
You must be signed in to change notification settings - Fork 348
Shift to Struct based Method and Constructors, with Executor passed from CM to on_init()
#2323
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2323 +/- ##
==========================================
+ Coverage 88.96% 88.99% +0.03%
==========================================
Files 144 147 +3
Lines 16634 16707 +73
Branches 1436 1433 -3
==========================================
+ Hits 14799 14869 +70
- Misses 1277 1283 +6
+ Partials 558 555 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
A partial review here
I think as this is an API breaking change, we should open the corresponding PRs in the packages like gz_ros2_control and also webots_ros2_control probably? We can discuss this part when we finish the final review of this PR
hardware_interface/include/hardware_interface/actuator_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/sensor_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/sensor_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/actuator_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/system_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/system_interface.hpp
Outdated
Show resolved
Hide resolved
@saikishor have added PR at ros-controls/gz_ros2_control#606 |
…ce.hpp Co-authored-by: Sai Kishor Kothakota <[email protected]>
….hpp Co-authored-by: Sai Kishor Kothakota <[email protected]>
….hpp Co-authored-by: Sai Kishor Kothakota <[email protected]>
…ce.hpp Co-authored-by: Sai Kishor Kothakota <[email protected]>
….hpp Co-authored-by: Sai Kishor Kothakota <[email protected]>
….hpp Co-authored-by: Sai Kishor Kothakota <[email protected]>
Awesome. Sorry I missed that one |
Good renamings! |
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.
Partial review
hardware_interface/include/hardware_interface/actuator_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/sensor_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/system_interface.hpp
Outdated
Show resolved
Hide resolved
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.
Overall looks spot on! Just some nitpicks
Very very good job. Based on how it looks, we can even backport this to Jazzy, as there is no break of API.
hardware_interface/include/hardware_interface/resource_manager.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/resource_manager.hpp
Outdated
Show resolved
Hide resolved
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
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.
thank you!
Brief
This addition is the first step towards solving #2141 (also #1732 in some way).
The concept is simple, we are creating a Struct pathway of sorts all the way from the CM to the
on_init()
call in the hardware component. This enables future additions without too harsh API/ABI breaks.An example to show this currently is to give the user access to the MultiThreadedExecutor so that they can add nodes to it and publish if necessary by propogating the
executor_
from CM all the way toon_init()
(refer ab8d08c). A future PR will add a node by default with the hardware_components nameThree Structs have been added to help in the above
on_init()
and parsed by user to get all relevant data (hardware_info and executor at this point)Important Note
This means over time we will have to deprecate and remove the non struct based methods
Side Notes
documentation is yet to be added, in particular about notifying the user to not abuse the executor (since they can use the cancel() call internally and cripple the services etc for the CM basically )