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

Port kdl_parser_py to ROS 2 #55

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

Port kdl_parser_py to ROS 2 #55

wants to merge 2 commits into from

Conversation

AdrianZw
Copy link

I am not sure if this is still relevant... I missed that there is already a pull request (#48) for issue #46.
However, I already did the work, and I am happy to help ;-)

On my systems I did not get flake8 to run an exception is thrown, which I was not able to figure out.

The changes done were mainly to adopt ament_python package. In addition, Python is not happy with kdl.Joint.None, I changed it to kdl.Joint.JointType(8)). I guess the KDL bindings for Python might need to be updated, since None might be reserved in Python.

@clalancette
Copy link
Collaborator

This is all currently blocked by ros2/orocos_kinematics_dynamics#19 . If you'd like to see some version of this get in, unblocking that one (particularly on macOS and Windows) would be most appreciated.

@clalancette
Copy link
Collaborator

First, thanks for this. Now that we actually have PyKDL, this can potentially move forward again. Towards that end, I've retargeted to the ros2 branch, rebased it, and cleaned it up a bit.

That said, the problem I'm currently running into while testing this is that import urdf_parser_py.urdf doesn't seem to work on my system. At the moment I don't quite remember where that comes from, but we may have to port that over as well to get this working. I'll take a closer look tomorrow.

@clalancette
Copy link
Collaborator

Oh, I see. The problem now is that we don't have urdf_parser_py as part of the base system in https://github.com/ros2/ros2/blob/master/ros2.repos . We'd have to add that to the ros2.repos list to finish this port. We'll have to think about whether we really want to do that, so this is still on hold for that.

@dreuter
Copy link

dreuter commented Jun 7, 2022

I think it would be worth restarting the CI Job once again. I think it failed because this commit: ros/rosdistro@5cf620a was only "released" on April 12th but the CI Job is from April 07th.

@clalancette
Copy link
Collaborator

It currently can't possibly work; see my comment #55 (comment)

@dreuter
Copy link

dreuter commented Jun 7, 2022

It currently can't possibly work; see my comment #55 (comment)

Sorry I am not fully getting that comment. Would there be any downside just depending on urdf_parser_py?

I am not too deep into this topic, but it seems that this is released: https://index.ros.org/p/urdfdom_py/
https://github.com/ros2-gbp/urdfdom_py-release

Or is the problem that kdl_parser is in the ros2.repos list? In that case would it make sense to split the packages into two repos (one that is in the ros2.repos and the other one that is in the distribution.yaml)?

@clalancette
Copy link
Collaborator

Or is the problem that kdl_parser is in the ros2.repos list?

The main problem is that urdf_parser_py is not in the ros2.repos file. Because of that, this can't currently pass our CI.

We could import it into ros2.repos, but we are nearing 400 packages in that already. I'm reluctant to add more, since every single one we add becomes release-critical.

So I'm not sure what to do here.

@audrow audrow changed the base branch from ros2 to rolling June 28, 2022 14:17
This will improve the readability of the code.

Co-authored-by: Daniel Reuter <[email protected]>
@MatthijsBurgh
Copy link

I think the only way to move forward is by adding urdf_parser_py to the ros2.repos.

@clalancette clalancette mentioned this pull request Oct 17, 2023
@hello-amal
Copy link

Any updates on this?

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

Successfully merging this pull request may close these issues.

5 participants