Transmission interface URDF parsing (imported from ddengster)

[guru-florida]

This PR pulls in joint_limit_interface and transmission_interface updates with #134 changes which remove state and command handles and replace with just JointHandle. It has been synced with the latest ros2_control/master branch.

WARNING: This PR also includes PR181 changes. I created PR181 with just joint_limit_interface in case the lesser scope PR is approved first. I probably should have isolated these two branches but alas I did not. oops.

Also all tests (previously commented out) have been fixed up and pass on my local system.

Suggesting review by @Karsten1987 @bmagyar as well.

This PR:
Originally coded by: @ddengster and @davetcoleman
#134, refactor + unit tests by myself: guru-florida

[guru-florida]

This may implement part of Issue #86. This code uses TinyXML lib to parse the URDF then extract the TransmissionInfo details. Perhaps it should be adapted to use the urdf class which already parses URDF? Though the interface doesnt necessarily change so we could accept this and add an issue to revisit in the future.

[bmagyar]

hi @guru-florida
I agree that the URDF beautification is not required in this PR.

Please separate the changeset from the joint limits PR though. Doing so really helps your reviewers to keep maintain their sanity and it also increases your chances of getting reviews in a timely manner.

[guru-florida]

ALL, HAVE YOU CHECKED OUT THIS BRANCH??? READ THIS!

@bmagyar Yes, I goofed there with building these changes on top of joint limits branch when i know better! I have rebased this branch to master and removed the joint limits commits. I would like to force push this now....are you ok with that? I assume the github PR will update ok even with a force push?

This is a rebase, I didnt manually move code/files. I can confirm my new branch only affects transmission_interface directory:

(after "git rebase -i master" and dropping unrelated commits)
guru@Kuba:~/src/ros/ros2_control$ git diff master | grep -E "(\-\-\-)|(\+\+\+)"
--- a/transmission_interface/CMakeLists.txt
+++ b/transmission_interface/CMakeLists.txt
--- a/transmission_interface/include/transmission_interface/transmission_info.hpp
+++ b/transmission_interface/include/transmission_interface/transmission_info.hpp
--- a/transmission_interface/include/transmission_interface/transmission_parser.hpp
+++ b/transmission_interface/include/transmission_interface/transmission_parser.hpp
--- a/transmission_interface/src/transmission_parser.cpp
+++ b/transmission_interface/src/transmission_parser.cpp
--- a/transmission_interface/test/test_transmission_parser.cpp
+++ b/transmission_interface/test/test_transmission_parser.cpp

[bmagyar]

Please make the force push.

[guru-florida]

@bmagyar push complete. 👍

[guru-florida]

@bmagyar @Karsten1987 @ddengster Any chance you guys can review and pull this in? I am using this and joint limits in the gazebo_ros2_control module and thanks to @ahcorde we have a working example of ros2_control in gazebo.

I can help port this stuff to components as well when ready.

[bmagyar]

I've got a long backlog but this is on the top of my todo list.

[destogl]

It looks good—some cosmetic suggestions.

@bmagyar: we should probably merge this fast so people can continue working with gazebo. After that we can start to rewrite this for the new URDF-structure.

[Karsten1987]

I would really like to see that we get rid of rclcpp as a dependency that low in the dependency graph.

[guru-florida]

Checked in:

• Migration to TinyXML2
• pushed XML tag literals to constexpr in anonymous namespace as well as logger name
• In TransmissionParser::parse(), all the RCLCPP_WARN/ERROR then throw error handling converted to just a throw runtime_error (this matches with error handling in hardware_interface component_parser)
• the convenience function parse_transmissions_from_urdf() does call rclcpp logging but its now the only place that uses it
• various formatting, non-code changes as requested above

TODO:
Move to rclutils logging and dump rclcpp dependency. I'll complete this tomorrow. FYI We have the rclcpp dependency in HardwareInterface component parser as well but only seems to be used for logging (not 100% sure).

[bmagyar]

I'll submit a PR against your branch @guru-florida , (once again) getting rid of the TransmissionParser class which. This will also get rid of logging, etc. The client of the library can choose what to do with those exceptions.

[bmagyar]

@guru-florida please see my suggested changes on guru-florida#1
which when merged will update the diff on this PR as well

[guru-florida]

Any particular reason to get rid of the class? or just thinking it's not really necessary to be a class when it's basically a single method?

I was looking around the other parses like URDF and SDF. Looks like URDF needs some work on error handling and logging, but SDF has established returning a "vector of parse error objects" as an out arg. I was thinking we should make it consistent and I am ok with the out arg. The error object is similar to std::runtime_error interface. We could copy/paste SDF ParseError object then later refactor URDF, SDF and this into rcutils or parse module (perhaps header only module). Thoughts?

[bmagyar]

The class never served a purpose, all the operations are stateless. We have a bunch of other parsers in ros2_control that could use some love but I think we should consider that for the future. Changing things in SDF and URDF, as well as rcutils are not something we'll get available in Foxy (due to releases cycles, etc), which is what we are aiming for now.

[guru-florida]

I reviewed your PR commits and everything looks good to me. Agreed on focusing on foxy release. Not sure what is going on with the CI checks because building, cpplint, uncrustify and tests seem good locally. CI issue? Logs showing colcon cant find the packages which I cant understand why.

So I am go for merge to master if you are...post CI check fixes.

[bmagyar]

I poked the CI

[bmagyar]

@guru-florida Please try rebasing to latest master, I'm not sure what to make of the CI's response...

[guru-florida]

@bmagyar Good catch! That did the trick.