Rework PID class API

[christophfroehlich]

While fixing the clang errors I realized that lots of implicit casts to computeCommand were used, and the typical usage was to pass something like period.nanoseconds() and convert it internally to seconds. Because introducing an overload with double dt as argument is ambiguous and might silently break user code, I decided to convert the class methods from camelCase to snake_case (the ros2_control standard), remove the uint64_t version but introduce the following new overloads which converts dt to seconds in a consistent way

• double dt
• rcl_duration_value_t dt_ns
• rclcpp::Duration dt
• std::chrono::nanoseconds dt_ns

I converted the method names of PidROS accordingly as well, but left the only rclcpp::Duration version there.

The old methods remain deprecated, this should not break anything in downstream packages.
AFAIK adding non-virtual methods should not be a problem regarding ABI break?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.89617% with 13 lines in your changes missing coverage. Please review.

Project coverage is 73.86%. Comparing base (9756cb7) to head (15e132a).

Files with missing lines	Patch %	Lines
src/pid_ros.cpp	80.35%	11 Missing ⚠️
src/pid.cpp	93.75%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           ros2-master     #246      +/-   ##
===============================================
+ Coverage        72.75%   73.86%   +1.11%     
===============================================
  Files               25       25              
  Lines             1101     1148      +47     
  Branches            89       89              
===============================================
+ Hits               801      848      +47     
  Misses             252      252              
  Partials            48       48              

Flag	Coverage Δ
unittests	73.86% <92.89%> (+1.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
include/control_toolbox/pid.hpp	85.71% <100.00%> (+7.93%)	⬆️
include/control_toolbox/pid_ros.hpp	100.00% <ø> (ø)
test/pid_parameters_tests.cpp	100.00% <100.00%> (ø)
test/pid_publisher_tests.cpp	94.87% <100.00%> (ø)
test/pid_tests.cpp	100.00% <100.00%> (ø)
src/pid.cpp	93.67% <93.75%> (+1.02%)	⬆️
src/pid_ros.cpp	72.41% <80.35%> (ø)

[mergify]

This pull request is in conflict. Could you fix it @christophfroehlich?

[saikishor]

LGTM.

Thanks for the changes

[saikishor]

Rest looks good

[saikishor]

LGTM

[saikishor]

@christophfroehlich Thank you for correcting things again and again. This is a great job.

[christophfroehlich]

@saikishor one last question: do we care about adding rclcpp as dependency to the pid class? As of now, it is ROS agnostic as introduced here 205a1e2

[saikishor]

@saikishor one last question: do we care about adding rclcpp as dependency to the pid class? As of now, it is ROS agnostic as introduced here 205a1e2

If this is the case, we can remove it. It only affects the method from the rclcpp::Duration right?. If one has to use this class, they can only copy it, the rclcpp dependency is introduced from the package. If we want to leave it out, let's remove that function alone. What's your opinion on this?

[christophfroehlich]

@saikishor one last question: do we care about adding rclcpp as dependency to the pid class? As of now, it is ROS agnostic as introduced here 205a1e2

If this is the case, we can remove it. It only affects the method from the rclcpp::Duration right?. If one has to use this class, they can only copy it, the rclcpp dependency is introduced from the package. If we want to leave it out, let's remove that function alone. What's your opinion on this?

I think if someone wants to use it outside of a ROS project, then it has to be copied manually and this dependency then has to be manually removed. If it is used in a ROS project, then the rclcpp dependency is already there from the package.xml, like you said.

[saikishor]

@christophfroehlich what do you think?