Draft: Fix deprecation warnings in controllers from recent ros2_control variant changes. #1443
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1443 +/- ##
==========================================
- Coverage 83.83% 83.75% -0.09%
==========================================
Files 122 122
Lines 11120 11139 +19
Branches 944 948 +4
==========================================
+ Hits 9323 9329 +6
- Misses 1489 1498 +9
- Partials 308 312 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
saikishor
changed the title
saikishor
changed the title
|
@kumar-sanjeeev I'm discussing with @christophfroehlich on the best way to address these get_value methods. We will get back to you soon. So, you can apply the same to other controllers |
Okay, understood. I noticed that we also need to modify the test cases to suppress the warnings there as well. |
| const double left_feedback = registered_left_wheel_handles_[index].feedback.get().get_value(); | ||
| const double right_feedback = | ||
| registered_right_wheel_handles_[index].feedback.get().get_value(); | ||
| bool left_success = false; |
There was a problem hiding this comment.
shall we simplify this and use just a single bool variable if we don't use the information of left/right failure?
Please also add a comment here, that we only need the lambda functions here to be able to mark the double left/right_feedback variable as const.
There was a problem hiding this comment.
bool success = true;
// lambda function only to mark `double left_feedback` as `const`.
const double left_feedback = [&]()
{
double temp{};
success = registered_left_wheel_handles_[index].feedback.get().get_value(temp);
return temp;
}();
// lambda function only to mark `double right_feedback` as `const`.
const double right_feedback = [&]()
{
double temp{};
success = registered_right_wheel_handles_[index].feedback.get().get_value(temp);
return temp;
}();
if (!success)
{
RCLCPP_WARN(
get_node()->get_logger(),
"Error while fetching information from the state interfaces for %s wheel at index [%zu]",
(index % 2 == 0) ? "left" : "right", index);
return controller_interface::return_type::OK;
}
@christophfroehlich Is this make sense ?
There was a problem hiding this comment.
while working on another controller, I noticed that I often need to implement a similar setup for get_value()across multiple places, wouldn't it be a good idea to create a separate reusable function for this? We could then import it wherever needed, which might also be helpful in test files.
for example ( in this case I created single lambda)
bool success = true;
// lambda function only to mark return as const
auto fetch_feedback = [&](const auto & wheel_handles, size_t index) -> double
{
double temp{};
success = wheel_handles[index].feedback.get().get_value(temp);
return temp;
};
for (size_t index = 0; index < static_cast<size_t>(wheels_per_side_); ++index)
{
const double left_feedback = fetch_feedback(registered_left_wheel_handles_, index);
const double right_feedback = fetch_feedback(registered_right_wheel_handles_, index);
if (!success)
{
RCLCPP_WARN(
get_node()->get_logger(),
"Error while fetching information from the state interfaces for %s wheel at index [%zu]",
(index % 2 == 0) ? "left" : "right", index);
return controller_interface::return_type::OK;
}what do you think?
There was a problem hiding this comment.
We need to discuss API further. Thank you for making these changes on a draft PR @kumar-sanjeeev !
I really don't like what this summary tells us:
+44 -7 to achieve the exact same thing we have now.
We need to add this to the framework before it blows up the codebase.
Ideally it would not increase our overall line of code but allow for failures & error reporting to come through or be reported in the background.
get_valueandset_valueindiff_drive_controllerto fix deprecation warnings.Fixes #1442