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 multiple chainable controller activation bug #1401

Conversation

saikishor
Copy link
Member

Fixes the issue of the failing multiple chainable controller activation by the resource manager. This is happening due to the newly added code in the PR: #1218 under the method prepare_command_mode_switch method

Fixes #1400
Closes #1369 (I think this should fix the same issue addressed in this PR)

@saikishor saikishor force-pushed the fix/multiple/chainable_controller_activation branch from c7c6ca2 to 8cb1154 Compare February 16, 2024 22:09
@saikishor
Copy link
Member Author

@christophfroehlich @bmagyar It seems the review lottery is failing after merging #1385, should we do something about it?

Run uesteibar/reviewer-lottery@v3
Error: ENOENT: no such file or directory, open 'ros-controls/ros2_control_ci/.github/reviewer-lottery.yml'

FYI : https://github.com/ros-controls/ros2_control/actions/runs/7936764298/job/21672717332?pr=1401

@saikishor saikishor added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Feb 16, 2024
@christophfroehlich
Copy link
Contributor

@christophfroehlich @bmagyar It seems the review lottery is failing after merging #1385, should we do something about it?

Run uesteibar/reviewer-lottery@v3
Error: ENOENT: no such file or directory, open 'ros-controls/ros2_control_ci/.github/reviewer-lottery.yml'

FYI : https://github.com/ros-controls/ros2_control/actions/runs/7936764298/job/21672717332?pr=1401

damn, thanks for reporting. anyone proficient with typescript for reverse engineering of the reviewer-lottery?

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (786d5b5) 47.49% compared to head (c2c2f44) 75.90%.
Report is 16 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1401       +/-   ##
===========================================
+ Coverage   47.49%   75.90%   +28.41%     
===========================================
  Files          41       41               
  Lines        3556     3362      -194     
  Branches     1938     1937        -1     
===========================================
+ Hits         1689     2552      +863     
+ Misses        459      417       -42     
+ Partials     1408      393     -1015     
Flag Coverage Δ
unittests 75.90% <100.00%> (+28.41%) ⬆️

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

Files Coverage Δ
controller_manager/src/controller_manager.cpp 69.51% <100.00%> (+30.74%) ⬆️

... and 31 files with indirect coverage changes

@saikishor
Copy link
Member Author

damn, thanks for reporting. anyone proficient with typescript for reverse engineering of the reviewer-lottery?

Not me 😞

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

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@AugusteBourgois
Copy link
Contributor

Hi everyone,

I can confirm that this fix solves #1400. The unit tests pass and our code runs normally now on humble.

Kindly
Auguste

@bmagyar bmagyar merged commit 1cc73c2 into ros-controls:master Feb 22, 2024
16 checks passed
mergify bot pushed a commit that referenced this pull request Feb 22, 2024
(cherry picked from commit 1cc73c2)

# Conflicts:
#	controller_manager/src/controller_manager.cpp
#	controller_manager/test/test_controller_manager_srvs.cpp
mergify bot pushed a commit that referenced this pull request Feb 22, 2024
(cherry picked from commit 1cc73c2)

# Conflicts:
#	controller_manager/src/controller_manager.cpp
#	controller_manager/test/test_controller_manager_srvs.cpp
@saikishor saikishor deleted the fix/multiple/chainable_controller_activation branch August 17, 2024 08:24
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 maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot activate multiple chainable controllers at once anymore
4 participants