-
Notifications
You must be signed in to change notification settings - Fork 386
[HardwareComponentInterface] Add get state and command interface handle methods #2831
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
base: master
Are you sure you want to change the base?
[HardwareComponentInterface] Add get state and command interface handle methods #2831
Conversation
81b73e1 to
3472e37
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2831 +/- ##
==========================================
- Coverage 89.51% 89.33% -0.18%
==========================================
Files 152 152
Lines 18020 18065 +45
Branches 1468 1476 +8
==========================================
+ Hits 16130 16139 +9
- Misses 1304 1330 +26
- Partials 586 596 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
christophfroehlich
left a comment
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 suggest storing the interface handles once inside the hardware components, instead of having to lookup the maps at every set/get call?
Yes, this way the users will be able to cache the command interfaces and will be able to implement the logic they want. I think this way we have less execution time, so you might not need to do a lookup everytime. |
|
However, I'm rethinking some part of API right now. I'll update with more API |
christophfroehlich
left a comment
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.
The API seems to be very useful from the user perspective, but I'm not sure about the compiler optimization I commented below..
| T state; | ||
| get_state<T>(get_state_interface_handle(interface_name), state); | ||
| return state; |
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.
Why not leaving also T get_state(const StateInterface::SharedPtr & interface_handle) and use this here instead? I don't know how the compiler will be able to optimize that, so please ignore if this is not relevant.
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 don't think it is significant for most simple datatypes we support
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.
shall we ask the @copilot ? :D
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.
Sure
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 had a private chat with it ;)
but the difference in runtime is mostly theoretical
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.
Lol! So you are convinced?
|
Thanks @saikishor! This should address #2816 |
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.
Pull Request Overview
This PR enhances the HardwareComponentInterface by adding methods to retrieve state and command interface handles, along with overloaded getter/setter methods that accept handles directly. These additions enable caching of interface handles to improve performance by avoiding repeated map lookups during interface access operations.
Key Changes:
- Added
get_state_interface_handle()andget_command_interface_handle()methods to retrieve interface handles by name - Introduced overloaded
get_state(),set_state(),get_command(), andset_command()methods that accept interface handles directly - Added
get_value()methods in theHandleclass that use output parameters instead of return values for better performance with large data types
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| hardware_interface/include/hardware_interface/hardware_component_interface.hpp | Added handle retrieval methods and handle-based getter/setter overloads with ensure_get/ensure_set parameters for state and command interfaces |
| hardware_interface/include/hardware_interface/handle.hpp | Added get_value() methods with output parameters to support thread-safe value retrieval without copying return values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param lock The lock to access the value. | ||
| * @param value The variable to store the retrieved value. | ||
| * @return true if the value is retrieved successfully, false otherwise. | ||
| * @note The method is thread-safe and non-blocking. |
Copilot
AI
Nov 19, 2025
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.
The documentation states the method is 'non-blocking', but the method uses std::shared_lock which can block if a writer holds the mutex. Consider clarifying this as 'non-blocking only if the lock is successfully acquired' or removing the non-blocking claim.
| * @note The method is thread-safe and non-blocking. | |
| * @note The method is thread-safe and non-blocking only if the lock is already acquired. | |
| * Acquiring the lock itself may block if a writer holds the mutex. |
| * @tparam T The type of the value to be retrieved. | ||
| * @param value The variable to store the retrieved value. | ||
| * @return true if the value is retrieved successfully, false otherwise. | ||
| * @note The method is thread-safe and non-blocking. |
Copilot
AI
Nov 19, 2025
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.
The documentation incorrectly states the method is 'non-blocking'. The method uses std::try_to_lock which makes it non-blocking in terms of lock acquisition, but it returns false if the lock cannot be acquired rather than guaranteeing non-blocking behavior. The documentation should clarify that the method attempts to acquire the lock without blocking and returns false if unsuccessful.
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.
Come onnnnn copilot 😅😅
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.
it has a point in being more specific what "retrieved successfully" means?
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 don't think so. Atleast, I wrote it as per my understanding
hardware_interface/include/hardware_interface/hardware_component_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/hardware_component_interface.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
This PR adds a few new methods that could be really helpful if we want to catch the information of the exported interfaces to improve the performance of the setting and getting of handles