-
Notifications
You must be signed in to change notification settings - Fork 304
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
[HW_IF] Prepare the handles for async operations #1750
[HW_IF] Prepare the handles for async operations #1750
Conversation
0630604
to
eb8a27b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1750 +/- ##
==========================================
- Coverage 87.60% 87.41% -0.19%
==========================================
Files 120 120
Lines 12223 12304 +81
Branches 1093 1105 +12
==========================================
+ Hits 10708 10756 +48
- Misses 1125 1150 +25
- Partials 390 398 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
eb8a27b
to
fe121c6
Compare
This pull request is in conflict. Could you fix it @saikishor? |
fe121c6
to
facb6a7
Compare
facb6a7
to
a55dedd
Compare
a55dedd
to
59c642a
Compare
// BEGIN (Handle export change): for backward compatibility | ||
// TODO(Manuel) return value_ if old functionality is removed | ||
THROW_ON_NULLPTR(value_ptr_); | ||
return *value_ptr_; | ||
// END | ||
} | ||
|
||
void set_value(double value) | ||
[[nodiscard]] bool get_value(double & value) const |
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.
@mamueluth please check this. This is how we can template the access to variables. This should simplify test changes.
hardware_interface/include/hardware_interface/loaned_command_interface.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/loaned_command_interface.hpp
Show resolved
Hide resolved
…t and get value method with some statistics on misses and timeouts
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.
Thanks! It looks great!
@@ -51,6 +60,27 @@ class LoanedCommandInterface | |||
|
|||
virtual ~LoanedCommandInterface() | |||
{ | |||
auto logger = rclcpp::get_logger(command_interface_.get_name()); | |||
RCLCPP_WARN_EXPRESSION( |
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.
Should we put this to "DEBUG" mode?
Or do we always want output when switching controllers?
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 is only when we deactivate the controller, i think it's good to have this statistical information.
Moreover, as it is conditioned it should only print for async cases and that too if you have some misses. This should give us some information of the functionality of the system
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.
If we have reported issues about this, we can put this to debug in future
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 like the statistics part, but now its rather verbose on the CI and some percentages seem to be wrong (or some counters not properly initialized?)
2024-10-25T08:59:54.1674417Z 7: [WARN] [1729846793.768585551] [joint2/velocity]: LoanedStateInterface joint2/velocity has 1379847120 (34.40 %) timeouts and 32586 (0.00 %) missed calls out of 4011084229 get_value calls
2024-10-25T08:59:54.1677303Z 7: [WARN] [1729846793.768598835] [joint2/position]: LoanedStateInterface joint2/position has 1379846688 (10614205292.31 %) timeouts and 32764 (252030.77 %) missed calls out of 13 get_value calls
I can't give proper feedback about the threading stuff, but it looks fine for me.
…statistical prints
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 latest commit fixes the output in the CI, so I'm happy.
As with #1688 and #1689 there are not many changes in the handles.hpp file. I think this is the right moment to do it, to have straight away working handles in async. The proposed changes would work with zero overhead in synchronous operations.
Looking forward for your feedback!