Rough support for sdformat robot descriptions #55

sloretz · 2020-07-15T18:28:08Z

This is a quick hack to get rough sdformat support in joint_state_publisher.

@scpeters @IanTheEngineer FYI

Needed to support ros2/urdf#13

Signed-off-by: Shane Loretz<sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

clalancette

I just took a quick look here, pointed out a couple of potential issues I saw.

clalancette · 2020-07-16T12:37:32Z

joint_state_publisher/joint_state_publisher/joint_state_publisher.py

+        if robot.getElementsByTagName('sdf'):
+            self.init_sdf(robot)
+        elif robot.getElementsByTagName('COLLADA'):


I know you are just extending this, but this check seems too broad. That is, if I understand https://docs.python.org/3.6/library/xml.dom.html#xml.dom.Document.getElementsByTagName correctly, this will look in the entire XML tree for tags of this name. I think this has some (limited) potential to produce false positives. If someone was using COLLADA, but named one of their joints 'sdf', I think we would attempt to parse it as SDF. Is there something else we can check in the document that is more specific? The root element name, for instance?

Yeah, it seems too broad to me too. I think checking the root element would be a better solution. The C++ parser is handling it a little different by scoring based on the location of the first tag text, like <COLLADA

https://github.com/ros/collada_urdf/pull/39/files#diff-f13e149a001d68baae11ac5da7f08dc5R56-R59

but that would be fooled by the text being in a comment in the xml.

 <sdf> ...

It would be nice if joint_state_publisher used the C++ parser so this would only have to be fixed in one place.

It would be nice if joint_state_publisher used the C++ parser so this would only have to be fixed in one place.

While I somewhat agree, I also don't relish the thought of putting a CPython binding on top of the URDF ecosystem. At the very least, it means we don't have a universal wheel for Python anymore.

I also don't relish the thought of putting a CPython binding on top of the URDF ecosystem. At the very least, it means we don't have a universal wheel for Python anymore.

Where were you thinking about the CPython binding living? urdf_parser_py? I was thinking of joint_state_publisher having a CPython extension that calls the urdf C++ api to parse the robot description and return an object with the joint info joint_state_publisher needs.

Where were you thinking about the CPython binding living? urdf_parser_py? I was thinking of joint_state_publisher having a CPython extension that calls the urdf C++ api to parse the robot description and return an object with the joint info joint_state_publisher needs.

I didn't have a specific place in mind.

But even in the case you are suggesting, its not clear to me that providing a CPython extension for joint_state_publisher is a win. It seems to me the pros and cons of switching to a CPython extension:

Pros:

Reduce duplication in parsing the XML.

Get support for new formats for "free" once they are in urdf_parser

Cons:

Replace Python code most Python programmers can understand with more complex CPython binding.

joint_state_publisher is no longer a Pure python wheel.

It's been a few years. Looking back, reusing the existing URDF/SDformat parsers instead of partially implementing parsers in this package seems like a big advantage, and using another language we're already extensively using in ROS seems like a small disadvantage.

I'm not about the pure python wheel con. It looks like colcon doesn't build packages as wheels, and if someone were building the package without using colcon I'm not sure what they would find more advantageous.

joint_state_publisher/joint_state_publisher/joint_state_publisher.py

ros-discourse · 2020-07-18T01:14:15Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1

clalancette · 2020-12-11T14:31:09Z

@sloretz Just out of curiousity, is this something that you are still interested in moving forward with?

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

* added test for sdf model * joint limits, root tag and dict * revolute joint and log output * fix joint type and catch value error Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

joint_state_publisher/test/multi_joint_robot.sdf

joint_state_publisher/joint_state_publisher/joint_state_publisher.py

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

ros2 sdformat support

clalancette

I've left a few more things to fix, but this is a lot closer to being ready.

joint_state_publisher/joint_state_publisher/joint_state_publisher.py

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

clalancette · 2023-07-27T14:01:47Z

@quarkytale Now that I've merged in #94, this one has a merge conflict. If you could resolve that, I'll then do another review here. Thanks.

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

clalancette

I've left two more things to fix, then I think we can get this in.

joint_state_publisher/joint_state_publisher/joint_state_publisher.py

This reverts commit 54675b5.

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

Rough support for sdformat robot descriptions

a522fc0

Signed-off-by: Shane Loretz<sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz self-assigned this Jul 15, 2020

clalancette reviewed Jul 16, 2020

View reviewed changes

sloretz mentioned this pull request Nov 3, 2020

Proposal: SDFormat URDF Parser Plugin ros/urdf#34

Closed

mjcarroll and others added 3 commits October 19, 2022 00:57

Merge branch 'ros2' into ros2-sdformat_support

daa6eb8

added test for sdf model

07224ea

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

joint limits, root tag and dict

60c8e06

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

quarkytale mentioned this pull request Jun 6, 2023

ros2 sdformat support #92

Merged

quarkytale and others added 3 commits June 12, 2023 09:55

revolute joint and log output

a87ad97

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

fix joint type and catch value error

21915ca

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

ros2 sdformat support (#92)

a0c926b

* added test for sdf model * joint limits, root tag and dict * revolute joint and log output * fix joint type and catch value error Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

clalancette requested changes Jul 17, 2023

View reviewed changes

quarkytale and others added 5 commits July 24, 2023 06:14

newline, continue, return and rename

80e9d35

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

Merge branch 'ros2-sdformat_support' into ros2-sdformat_support

d8378d6

urdf root tag check

cfd0502

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

update limit checks

3482b4d

Merge pull request #93 from quarkytale/ros2-sdformat_support

2c1d595

ros2 sdformat support

clalancette requested changes Jul 24, 2023

View reviewed changes

new parser arg and init (#95)

092e969

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

Merge branch 'ros2' into ros2-sdformat_support

7a20640

quarkytale requested a review from clalancette August 4, 2023 07:22

update tests

54675b5

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

clalancette requested changes Aug 4, 2023

View reviewed changes

joint_state_publisher/joint_state_publisher/joint_state_publisher.py Outdated Show resolved Hide resolved

joint_state_publisher/joint_state_publisher/joint_state_publisher.py Outdated Show resolved Hide resolved

quarkytale added 2 commits August 4, 2023 17:20

Revert "update tests"

b04d061

This reverts commit 54675b5.

revert argument

d9ec679

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>

clalancette approved these changes Aug 4, 2023

View reviewed changes

clalancette merged commit e70b3b3 into ros2 Aug 4, 2023
4 checks passed

clalancette deleted the ros2-sdformat_support branch August 4, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rough support for sdformat robot descriptions #55

Rough support for sdformat robot descriptions #55

sloretz commented Jul 15, 2020

clalancette left a comment

clalancette Jul 16, 2020

sloretz Jul 16, 2020

clalancette Jul 16, 2020

sloretz Jul 16, 2020

clalancette Jul 17, 2020

sloretz May 16, 2023

ros-discourse commented Jul 18, 2020

clalancette commented Dec 11, 2020

clalancette left a comment

clalancette commented Jul 27, 2023

clalancette left a comment

Rough support for sdformat robot descriptions #55

Rough support for sdformat robot descriptions #55

Conversation

sloretz commented Jul 15, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette Jul 16, 2020

Choose a reason for hiding this comment

sloretz Jul 16, 2020

Choose a reason for hiding this comment

clalancette Jul 16, 2020

Choose a reason for hiding this comment

sloretz Jul 16, 2020

Choose a reason for hiding this comment

clalancette Jul 17, 2020

Choose a reason for hiding this comment

sloretz May 16, 2023

Choose a reason for hiding this comment

ros-discourse commented Jul 18, 2020

clalancette commented Dec 11, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Jul 27, 2023

clalancette left a comment

Choose a reason for hiding this comment