Skip to content

Controller restart by switch_controller (minimal implementation) #1592

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

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 245 commits behind head on master.

Files with missing lines Patch % Lines
controller_manager/src/controller_manager.cpp 94.82% 0 Missing and 3 partials ⚠️
...ller_with_command/test_controller_with_command.cpp 88.23% 2 Missing ⚠️
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 with missing lines 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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

mergify bot commented Jul 5, 2024

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

Copy link
Contributor

github-actions bot commented Apr 1, 2025

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale label Apr 1, 2025
Copy link
Contributor

mergify bot commented Apr 1, 2025

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

@github-actions github-actions bot removed the stale label Apr 2, 2025
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