Skip to content
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

Cleanup all the deprecations warning from the recent ros2_control variant changes #1442

Open
8 tasks
saikishor opened this issue Dec 22, 2024 · 7 comments · May be fixed by #1443
Open
8 tasks

Cleanup all the deprecations warning from the recent ros2_control variant changes #1442

saikishor opened this issue Dec 22, 2024 · 7 comments · May be fixed by #1443

Comments

@saikishor
Copy link
Member

saikishor commented Dec 22, 2024

Background

Recently after the new variants feature in Handles (ros-controls/ros2_control#1688) We have a bunch of deprecation warnings that needs to be cleanup. This could be a nice first issue.

Instructions

The best approach to this is to create the temporary variables where ever needed and then if the set_value or get_value returns False. Then, better to print a warning and simply return OK. This way, if we have some failures in getting data, the user will be able to know and can introspect properly.

Moreover, If everything is running synchronously. it should work as expected without any failures.

For example:

For the following lines of the code in the diff_drive_controller:

for (size_t index = 0; index < static_cast<size_t>(wheels_per_side_); ++index)
{
registered_left_wheel_handles_[index].velocity.get().set_value(velocity_left);
registered_right_wheel_handles_[index].velocity.get().set_value(velocity_right);
}

The expected code should look like:

  for (size_t index = 0; index < static_cast<size_t>(wheels_per_side_); ++index)
  {
    if (!registered_left_wheel_handles_[index].velocity.get().set_value(velocity_left) ||
    !registered_right_wheel_handles_[index].velocity.get().set_value(velocity_right))
    {
      RCLCPP_WARN(get_node()->get_logger(), "Error while setting the velocity_left : %f "
        "and velocity_right : %f to the command handles", velocity_left, velocity_right);
      return controller_interface::return_type::OK;
    }
  }

For the following get_value code in the diff_drive_controller:

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();

The expected outcome would be:

bool left_success = false;
const double left_feedback = [&]() {
    double temp;
    left_success = registered_left_wheel_handles_[index].feedback.get().get_value(temp);
    return temp;
}();

bool right_success = false;
const double right_feedback = [&]() {
    double temp;
    right_success = registered_right_wheel_handles_[index].feedback.get().get_value(temp);
    return temp;
}();

// Now you can use left_success and right_success to check if get_value was successful
if (!left_success || !right_success) {
    RCLCPP_WARN(get_node()->get_logger(), "Error while fetching information from the state interfaces");
    return controller_interface::return_type::OK;
}

or


double left_feedback, right_feedback; 
if(!registered_left_wheel_handles_[index].feedback.get().get_value(left_feedback) ||
    !registered_right_wheel_handles_[index].feedback.get().get_value(right_feedback))
{
  RCLCPP_WARN(get_node()->get_logger(), "Error while fetching information from the state interfaces");
  return controller_interface::return_type::OK;
}

May be the first approach is better as it retains the const ness of the variable

🤔 What you will need to know.

Nothing. This issue is meant to welcome you to Open Source :) We are happy to walk you through the process.

📋 Step by Step

  • 🙋 Claim this issue: Comment below. If someone else has claimed it, ask if they've opened a pull request already and if they're stuck -- maybe you can help them solve a problem or move it along!

  • 🗄️ Create a local workspace for making your changes and testing following these instructions, for Step3 use "Download Source Code" section with these instructions.

  • 🍴 Fork the repository using the handy button at the top of the repository page and clone it into ~/ws_ros2_control/src/ros-controls/ros2_controllers, here is a guide that you can follow (You will have to remove or empty the existing ros2_controllers folder before cloning your own fork)

  • Checkout a new branch using git checkout -b <branch_name>

  • 🤖 Apply pre-commit auto formatting, by running pip3 install pre-commit and running pre-commit install in the ros2_control repo.

  • 💾 Commit and Push your changes

  • 🔀 Start a Pull Request to request to merge your code into master. There are two ways that you can start a pull request:

  1. If you are not familiar with GitHub or how to create a pull request, here is a guide you can follow on how GitHub works.
  • 🏁 Done Ask in comments for a review :)

Is someone else already working on this?

🔗- We encourage contributors to link to the original issue in their pull request so all users can easily see if someone's already started on it.

👥- If someone seems stuck, offer them some help!

🤔❓ Questions?

Don’t hesitate to ask questions or to get help if you feel like you are getting stuck. For example leave a comment below!
Furthermore, you find helpful resources here:

Good luck with your first issue!

@saikishor
Copy link
Member Author

@kumar-sanjeeev or @MartinPeris you might be interested to work on this

@kumar-sanjeeev
Copy link
Contributor

@saikishor, thanks. I will look into this.

@kumar-sanjeeev
Copy link
Contributor

@saikishor Hi,

I took a look at this and made adjustments to the diff_drive_controller code to capture and handle the return values of the get_value and set_value where ever used. I think similar changes need to be applied to the other controllers as well? If so, do you think I should submit one PR covering all controllers, or would it be better to start with a PR for diff_drive_controller and then add commits for the others later? Let me know what you think.

@saikishor
Copy link
Member Author

@kumar-sanjeeev let's try to address all these depreciations in a single PR

@christophfroehlich
Copy link
Contributor

but you always can submit a draft PR if you want to get feedback before you work on the other controllers

@kumar-sanjeeev
Copy link
Contributor

@christophfroehlich, yeah I would like to do that, just to check if I am in right direction or not. So I will submit draft PR.

@saikishor
Copy link
Member Author

@christophfroehlich, yeah I would like to do that, just to check if I am in right direction or not. So I will submit draft PR.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants