Skip to content

Controller Manager recovery from invalid URDF errors (backport #2775)#3394

Draft
mergify[bot] wants to merge 6 commits into
jazzyfrom
mergify/bp/jazzy/pr-2775
Draft

Controller Manager recovery from invalid URDF errors (backport #2775)#3394
mergify[bot] wants to merge 6 commits into
jazzyfrom
mergify/bp/jazzy/pr-2775

Conversation

@mergify

@mergify mergify Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Goal:

Update the Controller Manager to handle errors when the ResourceManager fails to initialize due to an invalid URDF or faulty hardware plugin.

Behavior:

  • Catch exceptions caused by parsing invalid URDFs during initialization
  • Log error messages
  • Reset the resource_manager_ pointer to a minimal, safe state.
  • Continue waiting for a valid robot_description to be published

Observations:

The Controller Manager now no longer crashes on invalid URDFs. Instead, it safely recovers by initializing the resource_manager_ pointer to a minimal state, allowing the controller manager to continue operating and respond to future valid robot descriptions.

Testing:
Unit tests were created with multiple invalid URDF scenarios. All tests passed successfully. Testing can be done by executing:
./build/controller_manager/test_controller_manager_with_resource_manager


This is an automatic backport of pull request #2775 done by [Mergify](https://mergify.com).

Co-authored-by: nibanovic <nikola.banovic@b-robotized.com>
Co-authored-by: Dr. Denis <denis@stoglrobotics.de>
Co-authored-by: Dr. Denis <denis.stogl@b-robotized.com>
(cherry picked from commit 189f705)

# Conflicts:
#	controller_manager/src/controller_manager.cpp
#	hardware_interface/src/resource_manager.cpp
@mergify

mergify Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Cherry-pick of 189f705 has failed:

On branch mergify/bp/jazzy/pr-2775
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 189f705.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   controller_manager/CMakeLists.txt
	modified:   controller_manager/include/controller_manager/controller_manager.hpp
	modified:   controller_manager/test/controller_manager_test_common.hpp
	modified:   controller_manager/test/test_controller_manager_hardware_error_handling.cpp
	modified:   controller_manager/test/test_controller_manager_srvs.cpp
	modified:   controller_manager/test/test_controller_manager_urdf_passing.cpp
	modified:   controller_manager/test/test_controller_manager_with_namespace.cpp
	new file:   controller_manager/test/test_controller_manager_with_resource_manager.cpp
	new file:   controller_manager/test/test_controller_manager_with_resource_manager.hpp
	modified:   controller_manager/test/test_controllers_chaining_with_controller_manager.cpp
	modified:   controller_manager/test/test_hardware_spawner.cpp
	modified:   controller_manager/test/test_spawner_unspawner.cpp
	modified:   ros2_control_test_assets/include/ros2_control_test_assets/descriptions.hpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   controller_manager/src/controller_manager.cpp
	both modified:   hardware_interface/src/resource_manager.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify Bot added the conflicts label Jun 17, 2026
@VitezGabriela

Copy link
Copy Markdown
Contributor

I checked against current jazzy: merging this branch into jazzy is a clean merge with 0 conflicts. Label seems to be stale.

@bmagyar bmagyar removed the conflicts label Jul 3, 2026
@VitezGabriela

VitezGabriela commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

There is a failing test_controller_manager_with_resource_manager which is caused by a pre-existing jazzy bug in ResourceManager::load_and_initialize_components() (the init flag was set before the URDF parse, so an invalid URDF was reported as initialized).

I've opened a fix in #3443.. Could a maintainer please merge it?

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.40%. Comparing base (0194fdd) to head (dba1be7).

Files with missing lines Patch % Lines
controller_manager/src/controller_manager.cpp 85.24% 4 Missing and 5 partials ⚠️
controller_manager/test/test_hardware_spawner.cpp 0.00% 2 Missing and 1 partial ⚠️
controller_manager/test/test_spawner_unspawner.cpp 0.00% 2 Missing and 1 partial ⚠️
.../test_controller_manager_with_resource_manager.cpp 96.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            jazzy    #3394      +/-   ##
==========================================
- Coverage   89.44%   89.40%   -0.05%     
==========================================
  Files         161      162       +1     
  Lines       20567    20637      +70     
  Branches     1630     1639       +9     
==========================================
+ Hits        18397    18451      +54     
- Misses       1500     1508       +8     
- Partials      670      678       +8     
Flag Coverage Δ
unittests 89.40% <87.50%> (-0.05%) ⬇️

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

Files with missing lines Coverage Δ
...interface/include/controller_interface/helpers.hpp 61.53% <ø> (ø)
.../include/controller_manager/controller_manager.hpp 97.29% <ø> (ø)
...er_manager/test/controller_manager_test_common.hpp 89.04% <100.00%> (+0.88%) ⬆️
...est_controller_manager_hardware_error_handling.cpp 100.00% <100.00%> (ø)
...ller_manager/test/test_controller_manager_srvs.cpp 99.37% <100.00%> (-0.01%) ⬇️
...ager/test/test_controller_manager_urdf_passing.cpp 100.00% <100.00%> (ø)
...er/test/test_controller_manager_with_namespace.cpp 100.00% <100.00%> (ø)
...t_controllers_chaining_with_controller_manager.cpp 99.01% <100.00%> (-0.01%) ⬇️
hardware_interface/src/resource_manager.cpp 81.00% <100.00%> (-0.50%) ⬇️
.../test_controller_manager_with_resource_manager.cpp 96.00% <96.00%> (ø)
... and 3 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

destogl
destogl previously approved these changes Jul 6, 2026

@destogl destogl left a comment

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.

Thanks for the following up!

@mergify

mergify Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@christophfroehlich christophfroehlich left a comment

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.

This broke gz_ros2_control, see #3401

Let us fix this first before we merge this PR into jazzy.

@saikishor

Copy link
Copy Markdown
Member

This broke gz_ros2_control, see #3401

Let us fix this first before we merge this PR into jazzy.

We should wait for atleast a release or do we want to backport this to Jazzy?

@christophfroehlich

Copy link
Copy Markdown
Member

Needs #3459

@christophfroehlich christophfroehlich marked this pull request as draft July 13, 2026 09:36
@mergify

mergify Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

This pull request is in conflict. Could you fix it @bmagyar @destogl @christophfroehlich @saikishor?

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
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