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.

Sign up for GitHub

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

Jump to bottom

Fix Hardware spawner and add tests for it #1759

Merged
merged 11 commits into from
Oct 31, 2024
Merged

Fix Hardware spawner and add tests for it #1759

merged 11 commits into from
Oct 31, 2024

Conversation

fmauch
Copy link
Contributor

@fmauch fmauch commented Sep 20, 2024

This basically implements my comments from #1682 (review)

I discussed with @destogl that I would create a new PR including his commit from #1682. So, this effectively replaces #1682.

It basically changes the following things:

  • Actually makes the hardware spawner work. I think it never worked correctly the way it was structured before. (is_hardware_component_loaded checks for one single string, while activate_components and configure_components expect a list.)
  • Fully implement that the hardware spawner allows specifying a list of components that should be started. This is cmdline-interface-breaking (basically only on a parameter description level) change but since it never worked I would expect nobody has been using it so far, so we can safely backport it.
  • Add tests for hardware_spawner (mainly implemented by @destogl)

@codecov Codecov
Copy link

codecov bot commented Sep 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.75000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.26%. Comparing base (832602d) to head (93e253f).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ler_manager/controller_manager/hardware_spawner.py 68.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1759      +/-   ##
==========================================
+ Coverage   87.60%   88.26%   +0.65%     
==========================================
  Files         120      121       +1     
  Lines       12224    12301      +77     
  Branches     1093     1093              
==========================================
+ Hits        10709    10857     +148     
+ Misses       1125     1051      -74     
- Partials      390      393       +3
Flag Coverage Δ
unittests 88.26% <93.75%> (+0.65%) ⬆️

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

Files with missing lines Coverage Δ
controller_manager/test/test_hardware_spawner.cpp 100.00% <100.00%> (ø)
...ler_manager/controller_manager/hardware_spawner.py 70.23% <68.42%> (+70.23%) ⬆️

... and 2 files with indirect coverage changes

@destogl destogl mentioned this pull request Sep 24, 2024
Closed
destogl
destogl previously approved these changes Sep 24, 2024
View reviewed changes
@destogl
Copy link
Member

destogl commented Sep 24, 2024

Thanks for fixing this! And yes, we have to backport this. Not many people are using it so it is fine if we do small breakings on user-level.

saikishor
saikishor requested changes Sep 24, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

The proposed changes good to me. One minor comment about the tests and another one to update the documentation

controller_manager/test/test_hardware_spawner.cpp Outdated Show resolved Hide resolved
controller_manager/test/test_hardware_spawner.cpp Outdated Show resolved Hide resolved
controller_manager/controller_manager/hardware_spawner.py Show resolved Hide resolved
@fmauch fmauch requested a review from saikishor September 24, 2024 20:21
saikishor
saikishor approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM

christophfroehlich
christophfroehlich approved these changes Sep 25, 2024
View reviewed changes
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.

We should add the usage of this in the demos. Maybe it suits with ros-controls/ros2_control_demos#417.

And should we also add a load_hardware_component CLI verb?

@christophfroehlich christophfroehlich added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Oct 31, 2024
@christophfroehlich christophfroehlich merged commit 5dbd6d5 into ros-controls:master Oct 31, 2024
21 checks passed
mergify bot pushed a commit that referenced this pull request Oct 31, 2024
@fmauch @mergify
Fix Hardware spawner and add tests for it (#1759)
1c179d5 
* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation

---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 5dbd6d5)

# Conflicts:
#	controller_manager/CMakeLists.txt
@mergify mergify bot mentioned this pull request Oct 31, 2024
Merged
mergify bot pushed a commit that referenced this pull request Oct 31, 2024
@fmauch @mergify
Fix Hardware spawner and add tests for it (#1759)
3afba42 
* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation

---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 5dbd6d5)

# Conflicts:
#	controller_manager/CMakeLists.txt
@mergify mergify bot mentioned this pull request Oct 31, 2024
Merged
christophfroehlich pushed a commit that referenced this pull request Nov 9, 2024
@fmauch @christophfroehlich
Fix Hardware spawner and add tests for it (#1759)
4fd74e5 
* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation

---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 5dbd6d5)

# Conflicts:
#	controller_manager/CMakeLists.txt
christophfroehlich pushed a commit that referenced this pull request Nov 9, 2024
@fmauch @christophfroehlich
Fix Hardware spawner and add tests for it (#1759)
b1c45eb 
* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation

---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 5dbd6d5)

# Conflicts:
#	controller_manager/CMakeLists.txt
christophfroehlich pushed a commit that referenced this pull request Nov 10, 2024
@mergify @fmauch
@bmagyar @saikishor
Fix Hardware spawner and add tests for it (backport #1759) (#1828)
a76101d 
* Fix Hardware spawner and add tests for it (#1759)

* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation

---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 5dbd6d5)

# Conflicts:
#	controller_manager/CMakeLists.txt

* Fix conflict

* Fix the missing bcolors.ENDC in hardware_spawner log prints (#1870)

* Choose pass_urdf_as_parameter as false

---------

Co-authored-by: Felix Exner (fexner) <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
christophfroehlich added a commit that referenced this pull request Nov 10, 2024
@mergify @fmauch
@saikishor @christophfroehlich
Fix Hardware spawner and add tests for it (backport #1759) (#1827)
dcb958f 
* Fix Hardware spawner and add tests for it (#1759)

* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation

---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
(cherry picked from commit 5dbd6d5)

# Conflicts:
#	controller_manager/CMakeLists.txt

* Fix conflicts

* FIx missing lines

* Fix the missing bcolors.ENDC in hardware_spawner log prints (#1870)

* Fix pre-commit

* Fix the API of the tests and the CMakeLists.txt

* Apply target properties as other tests

---------

Co-authored-by: Felix Exner (fexner) <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>
saikishor pushed a commit to pal-robotics-forks/ros2_control that referenced this pull request Nov 12, 2024
@fmauch @destogl
@bmagyar @christophfroehlich @saikishor
Fix Hardware spawner and add tests for it (ros-controls#1759)
f901285 
* Tests for hardware spawner.

* Fully support spawning a list of hardware components

* Use python3 coverage instead of ros2 run in tests

* Actually check component's state after changing it

* Update hardware_spawner's user documentation


---------

Co-authored-by: Dr. Denis Štogl <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saikishor saikishor saikishor approved these changes

@christophfroehlich christophfroehlich christophfroehlich approved these changes

@destogl destogl destogl left review comments

@arne48 arne48 Awaiting requested review from arne48

@bmagyar bmagyar Awaiting requested review from bmagyar

@mamueluth mamueluth Awaiting requested review from mamueluth

@moriarty moriarty Awaiting requested review from moriarty

@VX792 VX792 Awaiting requested review from VX792

Assignees
No one assigned
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fmauch @destogl @christophfroehlich @saikishor @bmagyar