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

Controller restart by switch_controller (minimal implementation) #1592

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

TakashiSato
Copy link
Contributor

This PR is related to #1568. Unlike it, this focuses on implementing the new restart controllers feature while minimizing changes to the master branch code. Additionally, this PR depends on #1591 and is expected to be merged after #1591 is merged.

Note: To focus on meaningful changes, it is recommended to compare the code using "hide whitespace" mode as follows.
https://github.com/ros-controls/ros2_control/compare/master...TakashiSato:ros2_control:feature/restart_controllers_minimal?diff=split&w=1

Explanation code changes

Since comparing the final code changes with the master branch is complicated due to the large number of changes, I'll explain the key points of each change in the order of the commits.
The changes up to commit number 005a79f are included in #1591, so I will only address the commits after that.

1. refactor (de)activate request checkrefactor (de)activate request check

In this commit, the following refactoring was performed:

  • Renamed enum class CreateRequestResult to CheckDeActivateRequestResult and moved its definition from inside switch_controller to within the class definition in the header (private).
  • Moved the implementation of clear_chained_mode_request from inside switch_controller to a member function, and changed the part of clear_requests that performed the same processing to this function call.
  • Moved the implementation of the check processing related to (de)activation from inside switch_controller to a member function.

2. add to support restart operations in switch_controller by specifying the same controller name in the start/stop requests

This commit implements the restart controllers feature and its tests.

To achieve the behavior of restart (deactivating first and then activating), I mainly made changes in the followings:

I made the following changes to the test implementation:

fix typo and unnecessary semicolon

This commit contains minor fixes.

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 98.72123% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.36%. Comparing base (fbb893b) to head (4dbe584).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
+ Coverage   87.70%   88.36%   +0.66%     
==========================================
  Files         102      105       +3     
  Lines        8704     8262     -442     
  Branches      780      675     -105     
==========================================
- Hits         7634     7301     -333     
+ Misses        790      720      -70     
+ Partials      280      241      -39     
Flag Coverage Δ
unittests 88.36% <98.72%> (+0.66%) ⬆️

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

Files Coverage Δ
.../include/controller_manager/controller_manager.hpp 100.00% <ø> (ø)
...chainable_controller/test_chainable_controller.cpp 85.24% <100.00%> (+1.03%) ⬆️
...chainable_controller/test_chainable_controller.hpp 100.00% <ø> (ø)
...r_manager/test/test_controller/test_controller.cpp 95.45% <100.00%> (+0.71%) ⬆️
...r_manager/test/test_controller/test_controller.hpp 100.00% <ø> (ø)
...ller_with_command/test_controller_with_command.hpp 100.00% <100.00%> (ø)
...t_controllers_chaining_with_controller_manager.cpp 99.32% <100.00%> (+0.47%) ⬆️
...ontroller_manager/test/test_restart_controller.cpp 100.00% <100.00%> (ø)
...ller_with_command/test_controller_with_command.cpp 88.23% <88.23%> (ø)
controller_manager/src/controller_manager.cpp 72.48% <94.82%> (-2.08%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Contributor

mergify bot commented Jul 5, 2024

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

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.

1 participant