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

Fix Hardware spawner and add tests for it #1759

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

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)

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

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

Project coverage is 87.53%. Comparing base (0433960) to head (866c6e4).

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   86.83%   87.53%   +0.69%     
==========================================
  Files         121      122       +1     
  Lines       11542    11618      +76     
  Branches     1054     1054              
==========================================
+ Hits        10023    10170     +147     
+ Misses       1144     1070      -74     
- Partials      375      378       +3     
Flag Coverage Δ
unittests 87.53% <93.68%> (+0.69%) ⬆️

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 previously approved these changes Sep 24, 2024
@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.

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
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

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?

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.

5 participants