[rqt_jtc] Use urdf_parser_py

[lakhmanisahil]

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.

[codecov]

Codecov Report

❌ Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.10%. Comparing base (be5f23c) to head (294cba3).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...t_joint_trajectory_controller/joint_limits_urdf.py	80.95%	2 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2254      +/-   ##
==========================================
- Coverage   85.13%   85.10%   -0.04%     
==========================================
  Files         154      154              
  Lines       15423    15417       -6     
  Branches     1335     1338       +3     
==========================================
- Hits        13130    13120      -10     
- Misses       1800     1802       +2     
- Partials      493      495       +2     

Flag	Coverage Δ
unittests	85.10% <81.39%> (-0.04%)	⬇️

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

Files with missing lines	Coverage Δ
...ajectory_controller/test/test_joint_limits_urdf.py	100.00% <100.00%> (ø)
...t_joint_trajectory_controller/joint_limits_urdf.py	69.56% <80.95%> (-7.77%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[christophfroehlich]

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.

[lakhmanisahil]

Hi @christophfroehlich, since issue #2253 is now closed, should I continue working on this PR or close it and create a new one based on the recent changes made to the joint_limits_urdf.py file?

[christophfroehlich]

Why not continuing here?

[lakhmanisahil]

Why not continuing here?

Got it, thanks.
I just wanted to confirm if continuing with this PR is the preferred approach.

[lakhmanisahil]

Hi @christophfroehlich, I have rebased onto the current master with the whitelist preprocessing. Standard URDF tags (link, joint, transmission, material) are kept, and everything else is stripped before parsing.

21/22 unit tests pass. The one failure is test_missing_velocity_raises, which asserted on the message "Missing velocity limits" produced by our old application code. urdf_parser_py now catches the same condition during parsing and raises a ParseError with its own message "Required attribute not set in XML: velocity" before our code gets to inspect the joint.

[lakhmanisahil]

ParseError with its own message "Required attribute not set in XML: velocity" before our code gets to inspect the joint.

There are two ways I am thinking of handling this:

1. Update the test to expect the new error message from the library.
2. Catch the ParseError and re-raise it with the old message.

I’m leaning toward the first option since it reflects the new behavior. I just wanted to confirm before changing a test that recently merged.

Please let me know what you prefer.

[christophfroehlich]

Sure, just change the test if it is no behavior change but only a different message.

[lakhmanisahil]

Hi @christophfroehlich, I’ve updated the changes and successfully tested them locally. PTAL.
After the final changes, I’ll update the PR description accordingly.

[christophfroehlich]

Thanks for the follow-up. I have some comments, see below

[christophfroehlich]

if it is 0.0 when absent, when can it be None?

[lakhmanisahil]

Yes, you are right, i was doing initially to play safe,
I checked this now and urdf_parser_py returns 0 for missing lower/upper, not None. The conditional was dead code.
Will push the changes soon.

[lakhmanisahil]

Hello @christophfroehlich, updated and addressed the reviews, PTAL.

[saikishor]

Just one minor nitpick, rest LGTM

[christophfroehlich]

please see the open threads (and please don't resolve them by yourself, this should be done by reviewers only)

[lakhmanisahil]

please see the open threads (and please don't resolve them by yourself, this should be done by reviewers only)

Hi, apologies for that. I’ve reopened all the threads I had marked as resolved and reviewed the open threads.
i have made the requested updates in the thread. PTAL.

[christophfroehlich]

Thank you, LGTM! The failing tests are not related, so let's move on