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

add a semantic command interface to "semantic_components" #1945

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

Conversation

tpoignonec
Copy link

Hi,

Is there any interest in defining a SemanticComponentCommandInterface analogous to the SemanticComponentInterface, but for hardware command interfaces?

Applications could include, among others, commonly used simple write-only devices such as LED, buzzers, beepers, etc.
It could also be used for GPIO arrays the same way the state interface can be used to read them in a standarized fashion.

This PR includes such a semantic component, as well as a basic example with a 3-channel (R/G/B) LED with two scenarios:

  • Default interface names: <name>/r, <name>/g, and <name>/b
  • Custom interface names using the following syntax:
#include "semantic_components/led_rgb_device.hpp"

std::string interface_name_r = "led/custom_r";
std::string interface_name_g = "led/custom_g";
std::string interface_name_b = "led/custom_b";

// Create LED device with custom interface names
auto led_device = std::make_unique<semantic_components::LEDRgbDevice>(
   interface_name_r, interface_name_g, interface_name_b);

Additionally, basic tests are proposed.

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 95.40230% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.40%. Comparing base (d4139b7) to head (db374fe).

Files with missing lines Patch % Lines
...test/test_semantic_component_command_interface.hpp 60.00% 2 Missing ⚠️
...ace/include/semantic_components/led_rgb_device.hpp 93.33% 1 Missing ⚠️
...omponents/semantic_component_command_interface.hpp 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1945      +/-   ##
==========================================
+ Coverage   89.38%   89.40%   +0.01%     
==========================================
  Files         132      138       +6     
  Lines       14663    14756      +93     
  Branches     1258     1262       +4     
==========================================
+ Hits        13106    13192      +86     
- Misses       1090     1096       +6     
- Partials      467      468       +1     
Flag Coverage Δ
unittests 89.40% <95.40%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
controller_interface/test/test_led_rgb_device.cpp 100.00% <100.00%> (ø)
controller_interface/test/test_led_rgb_device.hpp 100.00% <100.00%> (ø)
...test/test_semantic_component_command_interface.cpp 100.00% <100.00%> (ø)
...ace/include/semantic_components/led_rgb_device.hpp 93.33% <93.33%> (ø)
...omponents/semantic_component_command_interface.hpp 94.11% <94.11%> (ø)
...test/test_semantic_component_command_interface.hpp 60.00% <60.00%> (ø)

... and 1 file with indirect coverage changes

@Wiktor-99
Copy link
Contributor

I've thought about it a little bit, and I'm not sure if we should add such a component when the GPIO command controller, which is very generic, is already available

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.

Thanks for the PR including nice tests .
We discussed this shortly in todays WG meeting, and approve this addition.
I can understand the input @Wiktor-99, but imho

  • it is an obvious extension of an existing concept
  • it could also use joints and not only gpios

Please add it to the release_notes and some minor comments.

@tpoignonec
Copy link
Author

Great!
I do the release note and changes ASAP.

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 overall, I just added some comments to be in line with #1940

Maybe you have to format my suggestions once again to satisfy pre-commit.

@tpoignonec
Copy link
Author

My bad, hadn't rerun the unit tests after accepting the change suggestions (minor issue with LED constructor).
Should be fine now.

Copy link

@Raivias Raivias left a comment

Choose a reason for hiding this comment

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

Other than dropping the library LGTM.

…onent_command_interface.hpp

Co-authored-by: Sai Kishor Kothakota <[email protected]>
@saikishor
Copy link
Member

@tpoignonec Can you please fix the pre-commit jobs?

Copy link
Contributor

mergify bot commented Jan 22, 2025

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

@tpoignonec
Copy link
Author

tpoignonec commented Jan 22, 2025

@saikishor I removed the constructor as discussed.

Due to these changes, the name variable does not really serve a purpose anymore.
A typical component would do

MyCommandInterface(const std::string & name)
  : SemanticComponentCommandInterface(name, {name + "/my_io_1",  ... , name + "/my_io_N"})
  {
  }

However, SemanticComponentCommandInterface does not actually use the variable name_ anymore.

Regardless, LGTM at this point.

PS: feel free to revert the last commit if needed

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