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

Fix spawner behaviour on failing controller activation or deactivation #1941

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Dec 14, 2024

Before the fix proposed in this PR, the spawner reports the controller activation as successful, if the activation is failed at the last stage after calling activate_controllers. This PR addresses the issue by proposing a fix and this should now properly fail at the spawner end as well

Before the fix:

1: [INFO] [1734338012.820800005] [test_controller_manager]: Activating controllers: [ ctrl_with_joint1_and_joint2_command_interfaces ]
1: [ERROR] [1734338012.829138606] [test_controller_manager]: Resource conflict for controller 'ctrl_with_joint1_and_joint2_command_interfaces'. Command interface 'joint1/position' is already claimed.
1: [INFO] [1734338012.830469222] [spawner_ctrl_with_joint1_and_joint2_command_interfaces]: Configured and activated ctrl_with_joint1_and_joint2_command_interfaces

With the proposed fix:

1: [INFO] [1734338233.806072093] [test_controller_manager]: Activating controllers: [ ctrl_with_joint1_and_joint2_command_interfaces ]
1: [ERROR] [1734338233.814172942] [test_controller_manager]: Resource conflict for controller 'ctrl_with_joint1_and_joint2_command_interfaces'. Command interface 'joint1/position' is already claimed.
1: [ERROR] [1734338233.814291503] [test_controller_manager]: Could not activate controller : 'ctrl_with_joint1_and_joint2_command_interfaces'
1: [ERROR] [1734338233.817168509] [spawner_ctrl_with_joint1_and_joint2_command_interfaces]: Failed to activate controller : ctrl_with_joint1_and_joint2_command_interfaces

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 80.35714% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.11%. Comparing base (62b1908) to head (1ce28ff).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
controller_manager/src/controller_manager.cpp 63.15% 5 Missing and 2 partials ⚠️
...r_manager/test/test_controller/test_controller.cpp 75.00% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1941   +/-   ##
=======================================
  Coverage   88.11%   88.11%           
=======================================
  Files         121      121           
  Lines       13174    13229   +55     
  Branches     1187     1193    +6     
=======================================
+ Hits        11608    11657   +49     
- Misses       1132     1135    +3     
- Partials      434      437    +3     
Flag Coverage Δ
unittests 88.11% <80.35%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
controller_manager/controller_manager/spawner.py 74.81% <ø> (+2.22%) ⬆️
...ontroller_manager/test/test_release_interfaces.cpp 87.50% <ø> (ø)
controller_manager/test/test_spawner_unspawner.cpp 96.72% <100.00%> (+0.16%) ⬆️
...r_manager/test/test_controller/test_controller.cpp 88.05% <75.00%> (-4.10%) ⬇️
controller_manager/src/controller_manager.cpp 76.20% <63.15%> (-0.20%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Contributor

@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.

LGTM (At least the test you've written makes totally sense.)

controller_manager/test/test_spawner_unspawner.cpp Outdated Show resolved Hide resolved
@saikishor saikishor added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Dec 16, 2024
bmagyar
bmagyar previously approved these changes Dec 19, 2024
Copy link
Member

@bmagyar bmagyar 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!!

@christophfroehlich christophfroehlich dismissed stale reviews from bmagyar and themself via b436a4e December 19, 2024 10:01
Copy link
Contributor

@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.

LGTM

@christophfroehlich christophfroehlich merged commit 038a05f into ros-controls:master Dec 21, 2024
22 of 27 checks passed
@christophfroehlich christophfroehlich deleted the fix/spawner/on/resource/conflict branch December 21, 2024 20:16
mergify bot pushed a commit that referenced this pull request Dec 21, 2024
#1941)

---------

Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 038a05f)

# Conflicts:
#	controller_manager/CMakeLists.txt
#	controller_manager/src/controller_manager.cpp
#	controller_manager/test/test_controller/test_controller.cpp
saikishor added a commit that referenced this pull request Dec 21, 2024
saikishor added a commit that referenced this pull request Dec 21, 2024
saikishor added a commit that referenced this pull request Dec 21, 2024
saikishor added a commit that referenced this pull request Dec 21, 2024
christophfroehlich pushed a commit that referenced this pull request Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants