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 Interfaces for reporting of error and warning messages from HW #1319

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

Conversation

mamueluth
Copy link
Member

@mamueluth mamueluth commented Jan 22, 2024

Part of some bigger changes to Command-/StateIntefaces and Handles in general. First this PR has to be merged.

Add report interfaces for emergency stop, error and warning:

  • The available report interfaces are: EMERGENCY_STOP_SIGNAL, ERROR_SIGNAL, ERROR_SIGNAL_MESSAGE,WARNING_SIGNAL and WARNING_SIGNAL_MESSAGE
    The idea behind this PR is that each component provides some default interfaces for reporting of errors, error messages and warnings and warning messages. StateIntefaces are used for this which default names (e.g. <component_name>/ERROR_SIGNAL).

The component interface (actuator_inteface, sensor_intefrace, system_inteface) creates and stores them locally. The interfaces are named like <hardware_name>/<report_interface_type>. E.g. if hardware is called rrbot -> interface for WARNING_SIGNAL is called: rrbot/WARNING_SIGNAL. Later with variant support the interfaces will hold following datatypes:

  • Emergency Stop (bool)
  • Error Code (vector(32))
  • Warning Code (vector(32))
  • Error Message (vector<string(128)>(32)
  • Warning Message (vector<string(128)>(32)

Where the <error/warn>message_interfaces holds the error/warn message at position X for the <error/warn>interface's error signal at position X .

Copy link
Contributor

mergify bot commented Jan 22, 2024

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

@mamueluth mamueluth changed the title Add Interfaces for reporing of error and warning messages from HW Add Interfaces for reporting of error and warning messages from HW Jan 23, 2024
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

First batch of comments.

// create error signal interface
InterfaceInfo error_interface_info;
error_interface_info.name = hardware_interface::ERROR_SIGNAL_INTERFACE_NAME;
error_interface_info.data_type = "std::array<uint8_t>";
Copy link
Member

Choose a reason for hiding this comment

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

When we define those types, we should think about how to name them in URDF too. This looks a bit complicated.

What do you think if we say that the syntax for vectors/arrays is vector<_type_>[size] for example here would be vector<uint8_t>[32]. As long as we are pre-initializing the whole memory, by using reserve for example`, we should not be breaking real-time capabilities of the system.

Suggested change
error_interface_info.data_type = "std::array<uint8_t>";
error_interface_info.data_type = "vector<uint8_t>[32]";

Copy link
Member Author

Choose a reason for hiding this comment

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

The naming is to complicated. I will change it. But since the size should not be changeable, I would rather go with array then vectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

wouldn't array also be better for realtime because of memory allocation?

Copy link
Member

Choose a reason for hiding this comment

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

Didn't we say to remove this and go directly with variants?

@mamueluth mamueluth force-pushed the extend_hw_interface branch 2 times, most recently from 730f2e7 to a7996c3 Compare January 24, 2024 08:30
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: Patch coverage is 51.89310% with 216 lines in your changes are missing coverage. Please review.

Project coverage is 48.04%. Comparing base (35bb5f7) to head (c27990a).
Report is 19 commits behind head on master.

❗ Current head c27990a differs from pull request most recent head 3d017a6. Consider uploading reports for the commit 3d017a6 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1319       +/-   ##
===========================================
- Coverage   75.51%   48.04%   -27.47%     
===========================================
  Files          41       42        +1     
  Lines        3328     3973      +645     
  Branches     1921     2160      +239     
===========================================
- Hits         2513     1909      -604     
- Misses        421      481       +60     
- Partials      394     1583     +1189     
Flag Coverage Δ
unittests 48.04% <51.89%> (-27.47%) ⬇️

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

Files Coverage Δ
..._interface/include/hardware_interface/actuator.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/sensor.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/system.hpp 100.00% <ø> (ø)
...rface/include/hardware_interface/hardware_info.hpp 60.00% <60.00%> (ø)
...re_interface/include/hardware_interface/handle.hpp 80.64% <83.33%> (-19.36%) ⬇️
hardware_interface/src/sensor.cpp 50.46% <55.55%> (-18.22%) ⬇️
hardware_interface/src/system.cpp 56.71% <61.11%> (-16.17%) ⬇️
hardware_interface/src/actuator.cpp 55.97% <55.55%> (-16.92%) ⬇️
hardware_interface/src/component_parser.cpp 44.48% <55.55%> (-48.76%) ⬇️
...ce/include/hardware_interface/sensor_interface.hpp 56.41% <52.23%> (-43.59%) ⬇️
... and 3 more

... and 23 files with indirect coverage changes

@mamueluth mamueluth force-pushed the extend_hw_interface branch 2 times, most recently from 19652e8 to 0929fe9 Compare February 1, 2024 09:48
@mamueluth mamueluth force-pushed the extend_hw_interface branch from c27990a to ccfe700 Compare April 11, 2024 08:56
@bmagyar
Copy link
Member

bmagyar commented Aug 26, 2024

Can this be closed? Or massively refactored?

@mamueluth
Copy link
Member Author

Can this be closed? Or massively refactored?

I Will refactor as soon as other stuff is merged. There are a lot of duplicate changes since i based on first PRs.

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.

3 participants