Skip to content

[rqt_jtc] Use urdf_parser_py#2254

Open
lakhmanisahil wants to merge 3 commits intoros-controls:masterfrom
lakhmanisahil:feat/use-urdf-parser-py
Open

[rqt_jtc] Use urdf_parser_py#2254
lakhmanisahil wants to merge 3 commits intoros-controls:masterfrom
lakhmanisahil:feat/use-urdf-parser-py

Conversation

@lakhmanisahil
Copy link
Copy Markdown

Closes #891

Description

This PR replaces the manual XML DOM parsing in joint_limits_urdf.py with urdf_parser_py.urdf.Robot, resolving the long-standing TODO in the code.

This is a fresh implementation inspired by the stale PR #1982. It incorporates the review feedback and also fixes an additional bug that was not handled in that earlier attempt.

Why this change?

The previous implementation used xml.dom.minidom to manually parse URDF XML, extract attributes, and handle missing values using exceptions. This approach was fragile and unnecessarily complex.

The code already had a TODO suggesting migration to urdf_parser_py, but an earlier attempt failed due to: Required attribute not set in XML: upper. This issue has now been resolved in newer versions of urdf_parser_py, making the migration possible.

What changed?

joint_limits_urdf.py

  • Replaced manual XML parsing (xml.dom.minidom) with urdf_parser_py
    (using Robot.from_xml_string() and robot.joints)

  • Added preprocessing to remove <ros2_control> tags before parsing
    (needed because urdf_parser_py throws an error on unknown tags)

  • Fixed how missing joint limits are handled

    • earlier: missing values caused errors
    • now: parser sets missing limits to 0.0
    • this can create invalid ranges (min = max) and crash the slider
    • fixed by checking minval >= maxval
  • Removed the old TODO comment.

package.xml

  • Added the runtime dependency:
    <exec_depend>python3-urdf-parser-py</exec_depend>

How to reproduce and test

Setup with RRBot example 1

# Terminal 1: Launch the demo robot
ros2 launch ros2_control_demo_example_1 rrbot.launch.py

# Terminal 2: Deactivate the forward controller if it is holding the same interfaces
ros2 control set_controller_state forward_position_controller inactive

# Terminal 2: Spawn the joint trajectory controller
ros2 run controller_manager spawner joint_trajectory_position_controller   --param-file $(ros2 pkg prefix ros2_control_demo_example_1)/share/ros2_control_demo_example_1/config/rrbot_jtc.yaml

# Terminal 2: Verify the active controllers
ros2 control list_controllers

Expected output should show:

  • joint_trajectory_position_controller as active
  • joint_state_broadcaster as active
  • forward_position_controller as inactive

Launch the GUI

# Terminal 3
ros2 run rqt_joint_trajectory_controller rqt_joint_trajectory_controller

Expected behavior

After this PR

  • Joint sliders appear correctly.
  • Limits are derived from the URDF using urdf_parser_py.
  • The GUI behaves as expected for supported robots and controller setups.
  • The robot can be controlled through the sliders once the controller is active.
Screenshot from 2026-03-29 04-37-08 Screenshot from 2026-03-29 04-36-48 Screenshot from 2026-03-29 04-36-40 Screenshot from 2026-03-29 04-36-36

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 0% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.78%. Comparing base (fc570f8) to head (e3a4df4).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...t_joint_trajectory_controller/joint_limits_urdf.py 0.00% 45 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2254      +/-   ##
==========================================
+ Coverage   84.68%   84.78%   +0.10%     
==========================================
  Files         153      153              
  Lines       15319    15224      -95     
  Branches     1332     1315      -17     
==========================================
- Hits        12973    12908      -65     
+ Misses       1858     1841      -17     
+ Partials      488      475      -13     
Flag Coverage Δ
unittests 84.78% <0.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...t_joint_trajectory_controller/joint_limits_urdf.py 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I think it is time for adding unittest with #2253 first, then it's easier to check if this PR is not causing any troubles.

robot = xml.dom.minidom.parseString(description).getElementsByTagName("robot")[0]
try:
# urdf_parser_py does not recognize non-URDF tags such as
# <ros2_control> and raises an AssertionError.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are <gazebo> tags fine here?

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.

rqt_joint_trajectory_controller: Use urdf_parser

2 participants