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

Preparation of Handles for Variant Support #1678

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

mamueluth
Copy link
Member

@mamueluth mamueluth commented Aug 14, 2024

This PR is the first part of multiple breaking down #1240 in smaller changes. For an overview explanation and a lot of comments/discussion, please refer to #1240.
This prepares the handle to store the value directly instead of using pointers. The variant is going to be changed to hold different types. The Handles are adapted in this way because the pointer is removed, but the hardware (hw) still needs to be able to set values to the handle and we want to avoid creating and assigning hw loans the handle is now read and write able. The StateInterface is still read only in the sense, that its set_value function is not exposed to controllers via the loan but only to the hw which indeed needs to set values to the StateInterface. A more detailed explanation can be found at #1240 .

NOTE: Everything is fully backward compatible at this point. There is no change visible to Controllers or Hardware. Tests do not need to adapted since we Test through Interface (State-/CommandInteface & Loans which are not changed) Demos can not be adapted to this since in their perspective nothing changed.

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.54%. Comparing base (07fb4f3) to head (ae9bef7).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1678      +/-   ##
==========================================
- Coverage   84.59%   84.54%   -0.06%     
==========================================
  Files         115      115              
  Lines       10537    10467      -70     
  Branches      972      970       -2     
==========================================
- Hits         8914     8849      -65     
+ Misses       1295     1293       -2     
+ Partials      328      325       -3     
Flag Coverage Δ
unittests 84.54% <100.00%> (-0.06%) ⬇️

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

Files Coverage Δ
...re_interface/include/hardware_interface/handle.hpp 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

Copy link
Member

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

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

A couple of comments. The changes look good

hardware_interface/include/hardware_interface/handle.hpp Outdated Show resolved Hide resolved
hardware_interface/include/hardware_interface/handle.hpp Outdated Show resolved Hide resolved
@mamueluth mamueluth changed the title rework handles to prepare for variant support Preparation of Handles for Variant Support Aug 15, 2024
destogl
destogl previously approved these changes Aug 15, 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.

Great thanks!

bmagyar
bmagyar previously approved these changes Aug 15, 2024
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Great PR and great PR summary! No questions!

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.

Just some minor nitpicking things.
Overall looks good

hardware_interface/include/hardware_interface/handle.hpp Outdated Show resolved Hide resolved
hardware_interface/include/hardware_interface/handle.hpp Outdated Show resolved Hide resolved
hardware_interface/include/hardware_interface/handle.hpp Outdated Show resolved Hide resolved
hardware_interface/include/hardware_interface/handle.hpp Outdated Show resolved Hide resolved
}

protected:
std::string prefix_name_;
std::string interface_name_;
HANDLE_DATATYPE value_;
Copy link
Member

@saikishor saikishor Aug 15, 2024

Choose a reason for hiding this comment

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

Just to confirm, in this version we define it here but not use it. I believe this is intended

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
@bmagyar bmagyar dismissed stale reviews from destogl and themself via ae9bef7 August 15, 2024 10:58
@bmagyar bmagyar merged commit eb4316b into ros-controls:master Aug 15, 2024
17 of 19 checks passed
@destogl destogl deleted the handle_refactor branch August 15, 2024 13:05
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