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.

Sign up for GitHub

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

Jump to bottom

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

Open
saikishor opened this issue Nov 20, 2024 · 3 comments
Open

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

saikishor opened this issue Nov 20, 2024 · 3 comments
Labels
good first issue

Comments

@saikishor
Copy link
Member

Recently after the new variants feature in Handles (#1688) We have a bunch of deprecation warnings that needs to be cleanup. This could be a nice first issue.
@MartinPeris
Copy link

MartinPeris commented Nov 21, 2024 •
edited
Loading

I'd like to take this one. Quick couple of questions. For example in this situation:

/root/ws_ros2_control/src/ros2_control/hardware_interface/include/hardware_interface/system_interface.hpp: In member function ‘double hardware_interface::SystemInterface::get_state(const std::string&) const’:
/root/ws_ros2_control/src/ros2_control/hardware_interface/include/hardware_interface/system_interface.hpp:410:56: warning: ‘double hardware_interface::Handle::get_value() const’ is deprecated: Use bool get_value(double & value) instead to retrieve the value. [-Wdeprecated-declarations]
  410 |     return system_states_.at(interface_name)->get_value();
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

The original code is:

double get_state(const std::string & interface_name) const
  {
    return system_states_.at(interface_name)->get_value();
  }

Since the new API signature is bool get_value(double &value), how do you want me to handle the bool returned by get_value?

Ignoring it would not be an option for me, so this leaves 3 possibilities:

  1. Throw an Exception
  2. Return a Sentinel Value (e.g., NaN)
  3. Log an Error and Return Default Value

Either 1 or 2 would be acceptable for me (leaning towards 2, but that is just personal preference). In any case, new unit tests will have to be added... I charge extra karma for that :P

Also, I see a lot of -Wunused-result:

In file included from /root/ws_ros2_control/src/ros2_control/hardware_interface/include/mock_components/generic_system.hpp:25,
                 from /root/ws_ros2_control/src/ros2_control/hardware_interface/src/mock_components/generic_system.cpp:17:
/root/ws_ros2_control/src/ros2_control/hardware_interface/include/hardware_interface/system_interface.hpp: In member function ‘void hardware_interface::SystemInterface::set_state(const std::string&, const double&)’:
/root/ws_ros2_control/src/ros2_control/hardware_interface/include/hardware_interface/system_interface.hpp:405:49: warning: ignoring return value of ‘bool hardware_interface::Handle::set_value(double)’, declared with attribute ‘nodiscard’ [-Wunused-result]
  405 |     system_states_.at(interface_name)->set_value(value);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~

Should I deal with those too?

Is it OK to remove the deprecated API as I refactor? or should I leave them for backwards compatibility?

@MartinPeris
Copy link

mmmm looking at the documentation source code looks like a quiet NaN is the way to go:

[[deprecated("Use bool get_value(double & value) instead to retrieve the value.")]]
  double get_value() const
  {
    std::shared_lock<std::shared_mutex> lock(handle_mutex_, std::try_to_lock);
    if (!lock.owns_lock())
    {
      return std::numeric_limits<double>::quiet_NaN();
    }
    // 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
  }

@saikishor
Copy link
Member Author

Hello @MartinPeris!

First of all, I want to apologize for not getting back to your earlier.

For your question! I think we should also deprecate them that have void and instead they have to return bool the one from the handles. and so, we will have it similar to the Handles. Something like the following:

ros2_control/hardware_interface/include/hardware_interface/handle.hpp

Lines 175 to 188 in 038a05f

[[nodiscard]] bool set_value(double value)
{
std::unique_lock<std::shared_mutex> lock(handle_mutex_, std::try_to_lock);
if (!lock.owns_lock())
{
return false;
}
// BEGIN (Handle export change): for backward compatibility
// TODO(Manuel) set value_ directly if old functionality is removed
THROW_ON_NULLPTR(this->value_ptr_);
*this->value_ptr_ = value;
return true;
// END
}

So, the user will be able to know if what he gets is valid or not.

You can add the gcc ignore deprecated warnings to the deprecated methods, so we don't see them any more.

What do you think of this approach? @MartinPeris @ros-controls/ros2-maintainers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@MartinPeris @saikishor