Skip to content

Hardware component interfaces#121

Merged
bmagyar merged 29 commits intoros-controls:masterfrom
destogl:hardware_component_interfaces
Aug 22, 2020
Merged

Hardware component interfaces#121
bmagyar merged 29 commits intoros-controls:masterfrom
destogl:hardware_component_interfaces

Conversation

@destogl
Copy link
Copy Markdown
Member

@destogl destogl commented Jul 15, 2020

component interfaces files from #101

@destogl destogl requested review from Karsten1987 and bmagyar July 15, 2020 07:38
This was referenced Jul 15, 2020
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 15, 2020

Codecov Report

Merging #121 into master will decrease coverage by 0.08%.
The diff coverage is 45.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   45.93%   45.85%   -0.09%     
==========================================
  Files          23       38      +15     
  Lines         677     1230     +553     
  Branches      345      648     +303     
==========================================
+ Hits          311      564     +253     
- Misses         57       72      +15     
- Partials      309      594     +285     
Flag Coverage Δ
#unittests 45.85% <45.94%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
hardware_interface/test/test_macros.cpp 43.47% <ø> (ø)
...dware_interface/test/test_component_interfaces.cpp 31.35% <31.35%> (ø)
...face/src/components/component_lists_management.hpp 81.25% <81.25%> (ø)
hardware_interface/src/actuator_hardware.cpp 86.66% <86.66%> (ø)
hardware_interface/src/components/sensor.cpp 86.66% <86.66%> (ø)
hardware_interface/src/sensor_hardware.cpp 92.30% <92.30%> (ø)
...e/include/hardware_interface/actuator_hardware.hpp 100.00% <100.00%> (ø)
...hardware_interface/actuator_hardware_interface.hpp 100.00% <100.00%> (ø)
...ce/include/hardware_interface/components/joint.hpp 100.00% <100.00%> (ø)
...e/include/hardware_interface/components/sensor.hpp 100.00% <100.00%> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d33a7...00b73ee. Read the comment docs.

@jordan-palacios
Copy link
Copy Markdown
Member

jordan-palacios commented Jul 17, 2020

Thanks for doing this. It is way easier to approach than the original PR.

I'm still seeing some references to "actuator" in comments or variable names.

Do you think you could you add a small test case that shows how the Joint/Sensor classes are intended to be used?

Copy link
Copy Markdown
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.

just minor questions

@destogl
Copy link
Copy Markdown
Member Author

destogl commented Jul 28, 2020

@bmagyar @Karsten1987

Could you look at the extension of SensorInterface to work with example hardware? I also added components lifecycle management in there as an example. IMHO is very important to know what is happening with the hardware and the component. Would this be OK with you? If so, I would propose the change also for Joint and System.

Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

First of, I am a bit lost with the PRs floating around here now. So we closed #80 in favor of #101. Can that one be closed in favor of this one then or how are they treated?

I think the interfaces are a bit premature - functions like halt and recover are currently not yet used in a meaningful way and I would recommend holding these back until their use-case becomes prominent. In this case, we can always follow up with a PR on this.

The second reason I am requesting changes here is that I am missing the example how the URDF would look like. That in my opinion should dictate the structs such as ComponentInfo and SystemInfo. I am also happy to merge without this, if we come up with a direct follow up PR for this. But in my opinion, designing how the robots are specified in a URDF is the base of all of this.

@destogl
Copy link
Copy Markdown
Member Author

destogl commented Jul 29, 2020

First of, I am a bit lost with the PRs floating around here now. So we closed #80 in favor of #101. Can that one be closed in favor of this one then or how are they treated?

We discussed in the last meeting to split #101 into smaller PRs to make review simpler/easier.

I think the interfaces are a bit premature - functions like halt and recover are currently not yet used in a meaningful way and I would recommend holding these back until their use-case becomes prominent. In this case, we can always follow up with a PR on this.

OK, removed.

The second reason I am requesting changes here is that I am missing the example how the URDF would look like. That in my opinion should dictate the structs such as ComponentInfo and SystemInfo. I am also happy to merge without this, if we come up with a direct follow up PR for this. But in my opinion, designing how the robots are specified in a URDF is the base of all of this.

URDF example is in #122 . You are right, this is not optimal for review, but for the sake of PR's brevity...

Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

style only, but this looks good to me now.

destogl and others added 5 commits August 18, 2020 09:11
…interface.hpp

Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
…interface.hpp

Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
…interface.hpp

Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
…interface.hpp

Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

looks good to me with the outstanding comments addressed

@Karsten1987 Karsten1987 requested a review from bmagyar August 20, 2020 00:15
@destogl destogl force-pushed the hardware_component_interfaces branch from 17dd3f6 to 567b774 Compare August 20, 2020 18:31
destogl and others added 3 commits August 21, 2020 08:53
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Copy Markdown
Contributor

@destogl I hope you don't mind my two commits on your branch. They are basically only touchups. The first one addresses a couple of warnings I've encountered locally on OSX, the second one shifts the components (and their interfaces) into its own subfolder and namespace. Didn't change any logic.

@Karsten1987
Copy link
Copy Markdown
Contributor

@bmagyar please review and merge if appropriate.

Copy link
Copy Markdown
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.

haribo macht kinder froh

@bmagyar bmagyar merged commit 3a1fad5 into ros-controls:master Aug 22, 2020
@destogl
Copy link
Copy Markdown
Member Author

destogl commented Aug 22, 2020

@destogl I hope you don't mind my two commits on your branch. They are basically only touchups. The first one addresses a couple of warnings I've encountered locally on OSX, the second one shifts the components (and their interfaces) into its own subfolder and namespace. Didn't change any logic.

It looks good. Thanks!

I just don't understand why did you put .hpp file into src folder? I can understand the logic, it makes some sense, but it will confuse many people... and use of this inline functions from specialized components, e.g., PositionJoint, is now not possible

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