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

Sort controllers while configuring instead of while activating #1107

Merged

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Aug 27, 2023

This PR aims to address the requirement of sorting all controllers that on configure rather than on switching them to active.

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2023

Codecov Report

Merging #1107 (6ffa209) into master (971ff39) will increase coverage by 0.15%.
The diff coverage is 29.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1107      +/-   ##
==========================================
+ Coverage   31.62%   31.78%   +0.15%     
==========================================
  Files          94       94              
  Lines       10838    10804      -34     
  Branches     7419     7387      -32     
==========================================
+ Hits         3428     3434       +6     
+ Misses        804      792      -12     
+ Partials     6606     6578      -28     
Flag Coverage Δ
unittests 31.78% <29.62%> (+0.15%) ⬆️

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

Files Coverage Δ
...chainable_controller/test_chainable_controller.cpp 67.74% <100.00%> (+6.63%) ⬆️
...r_manager/test/test_controller/test_controller.cpp 94.11% <100.00%> (+16.84%) ⬆️
controller_manager/test/test_load_controller.cpp 26.43% <33.33%> (+0.84%) ⬆️
...ontroller_manager/test/test_controller_manager.cpp 27.90% <30.76%> (+0.70%) ⬆️
...ller_manager/test/test_controller_manager_srvs.cpp 12.39% <0.00%> (+0.59%) ⬆️
controller_manager/src/controller_manager.cpp 38.57% <27.77%> (+0.19%) ⬆️

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.

Is there consensus about doing this during configure? It makes sense to me, because read/write is also called in inactive state.

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2023

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

@destogl
Copy link
Member

destogl commented Sep 26, 2023

LGTM.

Is there consensus about doing this during configure? It makes sense to me, because read/write is also called in inactive state.

this is one thing and another thing is that in this stage we have enough information for sorting and call is not in the real-time loop.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I am not convinced about tests. I think we are losing some granularity in them. If you disagree, I am open to discuss that.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this and explaining everything.

@destogl destogl added the backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. label Oct 3, 2023
@bmagyar bmagyar merged commit 863f161 into ros-controls:master Nov 4, 2023
17 of 18 checks passed
mergify bot pushed a commit that referenced this pull request Nov 4, 2023
(cherry picked from commit 863f161)

# Conflicts:
#	controller_manager/src/controller_manager.cpp
#	controller_manager/test/test_controller_manager_srvs.cpp
saikishor added a commit to saikishor/ros2_control that referenced this pull request Nov 6, 2023
@saikishor saikishor deleted the sort_controllers_on_configure branch August 17, 2024 08:28
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants