-
Notifications
You must be signed in to change notification settings - Fork 355
Document order of interfaces #2394
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2394 +/- ##
=======================================
Coverage 88.91% 88.92%
=======================================
Files 148 148
Lines 16973 16973
Branches 1448 1448
=======================================
+ Hits 15092 15093 +1
Misses 1318 1318
+ Partials 563 562 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
/** Loaned command interfaces. The order of the interfaces is the same as exported in the | ||
* \ref ActuatorInterface::export_command_interfaces "export_command_interfaces" | ||
* \ref SystemInterface::export_command_interfaces "export_command_interfaces" | ||
* methods, or defined by the \ref command_interface_configuration() if auto-export is used. | ||
*/ |
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.
Isn't it defined by the order we define in the command_interface_configuration
method?
ros2_control/controller_interface/include/controller_interface/controller_interface_base.hpp
Line 117 in ad47e2a
virtual InterfaceConfiguration command_interface_configuration() const = 0; |
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 believe you are right, when we use ALL instead of INDIVIDUAL type
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.
you mean if one uses the deprecated export_command_interfaces
? Then there is no command_interface_configuration
?
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.
Isn't the export_command_interfaces
and export_state_interfaces
from HW component side?
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 thought in the wrong direction. I simplified the docstring, how is the order if interface_configuration_type::ALL
is configured? Is it determined by the order of loading the hardware components, and per component as defined in the xml tag?
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.
me gusta, barring the ongoing discussion
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.
Spot on!
(cherry picked from commit 39c588e)
(cherry picked from commit 39c588e)
See ros-controls/ros2_controllers#116
Any other location to document this?