Make kdl_parser_py build successfully under ros2
#48
Conversation
There was a problem hiding this comment.
First, thanks for the initial work to port this to ROS 2.
There's one nit inline, and one major problem that we have to solve before this gets in.
While we are doing this, we should also enable the tests. https://github.com/ros-teleop/teleop_tools/tree/foxy-devel/joy_teleop has some good examples on how to rewrite the tests to be ROS 2 compliant. Make sure to also remember to enable the flake8, copyright, pep257, and xmllint tests; that will ensure that the package meets our coding guidelines.
| <exec_depend version_gte="1.3.0">orocos_kdl</exec_depend> | ||
| <exec_depend>urdf</exec_depend> | ||
| <exec_depend>urdfdom_py</exec_depend> | ||
| <exec_depend>python_orocos_kdl</exec_depend> |
There was a problem hiding this comment.
Unfortunately, this is going to be a problem. We don't have python_orocos_kdl ported to ROS 2 at this time. See ros2/geometry2#360 (comment) for some of my thoughts on how we can get this ported and enabled for ROS 2.
There was a problem hiding this comment.
Sounds like you have these two options already mapped out well. Is there already a preference regarding which one to go for? Has someone already picked this up or is there a timeline? Can I help something with this? Although I probably couldn't help with the second option since I have no Windows machine available and no experience with ros on Windows...
There was a problem hiding this comment.
Nobody has picked it up as far as I know. So help there would be welcome (it would unblock both this and that geometry2 PR).
As far as which way to go, I would probably choose option 1. It's the most straightforward to do. Keep in mind that we will have to get it working for Windows (and macOS) there as well, but that should be easier than making a choco package.
There was a problem hiding this comment.
Okay, I think I can find some time to look into option 1.
|
Regarding the tests, I updated the existing one s.t. it can be run with colcon, and added flake8, copyright, pep257 and xmllint tests. Not all tests pass yet and I have two questions about this:
Thanks for reviewing this by the way! |
Good question. This should be the BSD license, for 3 reasons:
We don't typically use a custom flake8 configuration as far as I know. However, flake8 is somewhat annoying in that it will just automatically use whatever plugins you have installed. If you happen to have ones installed that we don't have, that could cause the warnings. Can you give me a better idea of what warnings you are getting? |
|
Thanks for the hints. Copyright notes are added now and flake8 tests are passing after I removed a couple of plugins. Also, I made a draft of re-enabling PyKDL, see ros2/orocos_kinematics_dynamics#19. Would be great to hear your opinion on this. |
| :param param: Parameter name, ``str`` | ||
| """ | ||
|
|
||
| return treeFromUrdfModel(urdf.URDF.from_parameter_server()) |
There was a problem hiding this comment.
As the from_parameter_server method was removed in urdf_parser_py 1.* (the version used in ROS2) probably we may need to remove this method as well.
|
Thanks for the pull request. We've done some further work here in #55, so I'm actually going to close this one out in favor of that. We'll continue the discussion over there. |
Changed the build files (mainly
setup.py) s.t. this can be built using colcon in a ROS 2 workspace. Tested building and using this with ROS 2 Foxy on Ubuntu 20.04.The only thing that is missing is also building and running the tests under ROS 2, since I don't know the standard way of running python tests in ROS 2. If you think this is necessary before merging, I can look into it.