Revived Joint Limits Interfaces with new State and Command Interfaces

[guru-florida]

Using new hardware_interface::StateInterface and CommandInterface instead of the old JointHandle class. Mostly a search and replace process at first, but CommandInterface has one surprise where the copy constructor is deleted in favor of always moving. So for command arguments the "const CommandInterface&" becomes "CommandInterface &&" and std::move() is used in implementation.

The switch to move semantics also invalidates all the tests though since the tests would create a command handle in the test fixture which cannot become a && argument. So I switched the fixture to use inline command_handle() methods to retrieve a command handle which can then be directly passed as an argument to the limit constructors.

[guru-florida]

The joint limit interfaces is just a single include file, not even a src implementation. I could move that file and the test file into the hardware_interface module, I agree it probably belongs in there.

@Karsten1987 mentioned in PR #236 :

I think that the joint limit interface can safely be ignored for now. I believe that we actually don't need a dedicated package for this. The functionality can easily be incorporated within either the hardware_interface or into a default component package. Interface seems to be wrong in this case then.

[guru-florida]

@bmagyar (As per WG meeting, regarding refactor into hardware_interface):
The joint_limits_interface module has a dependency on "urdf" package since it also parses limits out of URDF files. hardware_interface does not (looks like it uses tinyxml2). urdf_model/joint.h and urdf/urdfdom_compatibility.h are indeed included from the joint_limits_urdf header. Are you ok with hardware_interface including the urdf package? No other dependency issues.

[guru-florida]

@bmagyar
The tests are failing due to launch_testing_ament_cmake not being available in the build environment. Possibly the launch repo needs to be added to ci's vcs-repo-file-url? This package is required by the joint_limits rosparams py test. It's only a BUILD_TESTING dependency.

[guru-florida]

Other than test dep issue above, migration of joint_limits_interface into hardware_interface is complete. No significant changes in the API. The joint_limits include files, though moved into hardware_interface, are still in "joint_limits_interface/" include directory so no code changes for users are required other than dump the joint_limits_interface package ref.

As for tests, (1) they were converted to gmock instead of gtest to match other hardware_interface tests. (2) The rclcpp package dep had to be included for BUILD_TESTING for a unit tests that actually creates a rclcpp::Node for testing parameters. (3) The urdf package was added to main deps since joint_limits includes urdf parsing.

[destogl]

The joint_limits include files, though moved into hardware_interface, are still in "joint_limits_interface/" include directory
@guru-florida I would propose to add it into hardware_interface/joint_limits_interface/
The main question is how this interface should be used?

@bmagyar we have to rethink this architecture since we do not have "joints" anymore. Probably is this something for ResourceManager

[destogl]

Good job @guru-florida

I added many comments because it is unclear to me how this would work with the new "component-base" architecture.
Before merging there should be test using test_single_joint_actuator and test_two_joint_system testing the integration and also an example in the ros2_control_demos repository, where we test the whole framework.

Other suggestions/questions:

• Maybe to move JointLimits-README.md into corresponding include folder`
• @bmagyar can we remove author tag from joint_limits.hpp since we don't have it in other files...
• Why is there file joint_limits_rosparam.hpp, should we allow to set the limits using URDF and parameter server? I find this rather ambiguous...

[destogl]

Why new line?

[guru-florida]

It is using method chaining. Notice no semicolon on the end of line 32....there is another .method() call below it. Read better to me to have the multiple .method() chain calls indented to match and it passed clint/uncrustify.

[bmagyar]

@guru-florida did you have time to integrate this with hardware_interface instead? I think this could be closed at this point as we agreed not to have this package separately

[guru-florida]

I am updating my own code to use RM/CM now. Let me revisit this again before closing. I am still catching up to the current way of things.

@guru-florida did you have time to integrate this with hardware_interface instead? I think this could be closed at this point as we agreed not to have this package separately

[destogl]

@guru-florida can you please enable "Allow edits and access to secrets by maintainers" box (on the bottom of the right-options of the PR). I would like to push changes directly to your branch.

[guru-florida]

I have "Allow edits by maintainers" checked already. There is no "Allow edits and access to secrets...". Apparently that is only when there are GitHub Actions workflows. I'm not sure. It looks like you did get changed pushed here though, I see your HW interface constant changes.

@guru-florida can you please enable "Allow edits and access to secrets by maintainers" box (on the bottom of the right-options of the PR). I would like to push changes directly to your branch.

[destogl]

#441 is the follow-up