Refactor spawner to be able to reuse code for ros2controlcli #1661
Conversation
github-actions
bot
requested review from
aprotyas,
bijoua29,
bmagyar,
christophfroehlich,
destogl,
fmauch and
moriarty
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1661 +/- ##
==========================================
+ Coverage 86.64% 86.66% +0.01%
==========================================
Files 115 115
Lines 10542 10519 -23
Branches 970 970
==========================================
- Hits 9134 9116 -18
+ Misses 1059 1054 -5
Partials 349 349
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| class bcolors: | ||
| HEADER = "\033[95m" | ||
| OKBLUE = "\033[94m" | ||
| OKCYAN = "\033[96m" | ||
| OKGREEN = "\033[92m" | ||
| WARNING = "\033[93m" | ||
| FAIL = "\033[91m" | ||
| ENDC = "\033[0m" | ||
| BOLD = "\033[1m" | ||
| UNDERLINE = "\033[4m" |
There was a problem hiding this comment.
Please see this only as a comment, not as a request:
This might be considered to be moved to a different file? Importing this from the controller_manager_services module seems to work fine for the given use cases, but maybe it would be more intuitive to declare it in the init file directly or move it to a separate helpers module.
There was a problem hiding this comment.
At the very beginning i wanted to do it in the init file itself, but then if that's the case I couldn't use it inside the controller_manager_services file and then later add it as a module inside the init file. So, in theory you should be able to import in other python files as from controller_manager import bcolors, this is how it's done for the ros2controlcli
There was a problem hiding this comment.
And besides that, we should rethink on renaming colors, the names are quite fuzzy.
There was a problem hiding this comment.
Sure! Good idea. Maybe we can do it in a separate PR? What do you say?
saikishor
force-pushed
the
cli/add/params_file_arg
branch
from
f6e0725
to
3e62e81
Compare
saikishor
changed the title
|
Sure! I agree it's much easier for maintenance |
bmagyar
added
backport-humble
(cherry picked from commit 0631e3e) # Conflicts: # controller_manager/controller_manager/spawner.py # controller_manager/test/test_spawner_unspawner.cpp
(cherry picked from commit 0631e3e) # Conflicts: # controller_manager/controller_manager/spawner.py # controller_manager/test/test_spawner_unspawner.cpp
…#1661) (#1696) * Refactor spawner to be able to reuse code for ros2controlcli (#1661) (cherry picked from commit 0631e3e) # Conflicts: # controller_manager/controller_manager/spawner.py # controller_manager/test/test_spawner_unspawner.cpp * [Iron] Fix conflicts of 1661 backport (#1736) * Fix conflicts * added controller_type parameter setting to the spawner --------- Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
…#1661) (#1695) * Refactor spawner to be able to reuse code for ros2controlcli (#1661) (cherry picked from commit 0631e3e) # Conflicts: # controller_manager/controller_manager/spawner.py # controller_manager/test/test_spawner_unspawner.cpp * [Humble] Fix conflicts of 1661 backport (#1735) * Fix conflicts * added controller_type parameter setting to the spawner --------- Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
@christophfroehlich has mentioned that ros2 control CLI is missing the functionality to load controllers with the parameter files as we have now with the spawner. upon reviewing the code, the code would need some refactoring to reduce some duplication of code in both places and to be able also to load controllers with namespaced controller manager
This PR would be the first part of refactoring that can go along with #1562, once both the PRs are done, the ros2 control CLI can be modified to add the parameter parsing from the file and proper usage with namespacing.