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 #1882

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

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

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
  }

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

No branches or pull requests

2 participants