Skip to content

Add ActuatorHandle and Implement string-based interface handle-handling using DynamicJointState message#112

Merged
bmagyar merged 5 commits intoros-controls:masterfrom
bmagyar:string-based-registerinterface
Jul 8, 2020
Merged

Add ActuatorHandle and Implement string-based interface handle-handling using DynamicJointState message#112
bmagyar merged 5 commits intoros-controls:masterfrom
bmagyar:string-based-registerinterface

Conversation

@bmagyar
Copy link
Copy Markdown
Member

@bmagyar bmagyar commented Jun 26, 2020

This PR adds actuator handles and interfaces for handling them.
It is all by using strings so it should support pretty much anything. The backing data structure is a DynamicJointState message which seems to be quite useful for this although a map-map structure may fare better, but I consider that an implementation improvement, the current code implements the functionality required by a transmission parser / loader which is coming soon.

There's extensive testing for all the basic cases.

What this PR doesn't have is refactoring all the functions using joint handles. If this approach is favoured, we could do that in a separate PR.

Since the role of handles are to remain pretty much the same this shouldn't conceptually conflict with #101 .

resolves #85

Copy link
Copy Markdown
Member

@jordan-palacios jordan-palacios left a comment

Choose a reason for hiding this comment

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

Create a test for the ActuatorHandle class itself too.

@destogl
Copy link
Copy Markdown
Member

destogl commented Jun 29, 2020

Hi @bmagyar,

I see the reasons you need this for further development on Transmissions, nevertheless, IMO, this functionality would be in the future part of the Actuator and Sensor-Components. So that component would create transmission class for themselves and use it.

Can this PR see as a temporerly solution? If so, I propose to rename ActuatorHandle to something elese so it does not interfere/confuse the implementation of components.

@bmagyar bmagyar force-pushed the string-based-registerinterface branch from 8f6d7cd to 0289900 Compare June 29, 2020 07:15
@bmagyar
Copy link
Copy Markdown
Member Author

bmagyar commented Jun 29, 2020

I wouldn't necessarily call it a temporary solution but rather a minimal first enabler version which could be overridden, redesigned to a full extent. How do you propose to rename it? I'd be happy for you to revamp these handles in the future but I wouldn't want to add them as something very different from actuators as that's exactly what they are... Perhaps something to pinpoint that they just handle single actuator interface values? ActuatorValueHandle?

@Karsten1987
Copy link
Copy Markdown
Contributor

I would hold this PR back until #101 is merged. I don't see the point of introducing a temporary interface which then has to be deleted afterwards.

@bmagyar
Copy link
Copy Markdown
Member Author

bmagyar commented Jun 30, 2020

When is #101 ready for a merge?
Can you outline how you'd implement the same thing with #101 ? Are you going to incorporate the logic from here in there?

@jordan-palacios
Copy link
Copy Markdown
Member

I would hold this PR back until #101 is merged. I don't see the point of introducing a temporary interface which then has to be deleted afterwards.

I disagree. This PR is ready now and will allow us to start working on all the Trasnmission related code: TrasnmissionInterface, TrasnmissionLoader, SimpleTransmission, etc.

I love the work being poured in #101 (I'm myself trying to catch up on it to weigh in) but as of now is probably weeks away from being ready to be part of the master branch.

@Karsten1987
Copy link
Copy Markdown
Contributor

This PR is ready now and will allow us to start working on all the Trasnmission related code: TrasnmissionInterface, TrasnmissionLoader, SimpleTransmission, etc.

I guess I am missing something here, but I don't see how this PR is related to transmissions? The interfaces added here are introducing design which is all fundamentally revoked by #101. The resource management is planned to be different, the memory allocation as well as composing multiple actuators into a "system" of actuators.

Are you going to incorporate the logic from here in there?

As mentioned above, I don't see which logic you mean. The resource management? That'll be addressed after #101 is merged - IMO. But asking the other way around, if we merge this before #101 are you incorporating this logic here into the new setup?

But that's just my take on this. Happy to be overruled ;-)

bmagyar added 3 commits July 2, 2020 07:57
Implement string-based interface and handle-handling using DynamicJointState message
@bmagyar bmagyar force-pushed the string-based-registerinterface branch from 0289900 to f3e96a0 Compare July 2, 2020 07:28
@bmagyar
Copy link
Copy Markdown
Member Author

bmagyar commented Jul 7, 2020

Following our meeting yesterday I am going ahead with merging this.

The CI has multiple issues at this point: failing to find control_msgs as it is looking up binary packages in eloquent and ignoring the .repos file (which is either in the wrong spot or is being removed (not sure about one bit on the CI messages)). I'm giving it another go, forcing foxy to be picked up for binaries but perhaps rolling would be more appropriate (can discuss later)

@bmagyar bmagyar force-pushed the string-based-registerinterface branch from 10543e8 to 122af64 Compare July 7, 2020 10:11
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #112 into master will decrease coverage by 1.75%.
The diff coverage is 38.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   47.68%   45.93%   -1.76%     
==========================================
  Files          24       23       -1     
  Lines         541      677     +136     
  Branches      247      345      +98     
==========================================
+ Hits          258      311      +53     
+ Misses         61       57       -4     
- Partials      222      309      +87     
Flag Coverage Δ
#unittests 45.93% <38.85%> (-1.76%) ⬇️
Impacted Files Coverage Δ
...face/include/hardware_interface/robot_hardware.hpp 50.00% <ø> (-50.00%) ⬇️
...ardware_interface/test/test_register_actuators.cpp 19.04% <19.04%> (ø)
hardware_interface/test/test_actuator_handle.cpp 30.00% <30.00%> (ø)
hardware_interface/src/robot_hardware.cpp 53.84% <53.33%> (+5.40%) ⬆️
...ace/include/hardware_interface/actuator_handle.hpp 78.57% <78.57%> (ø)
hardware_interface/test/test_macros.cpp 43.47% <0.00%> (-6.53%) ⬇️
...e_interface/test/test_robot_hardware_interface.cpp 45.88% <0.00%> (-4.12%) ⬇️
...ot_hardware/test/test_robot_hardware_interface.cpp 37.50% <0.00%> (-1.90%) ⬇️
controller_manager/test/test_load_controller.cpp 19.04% <0.00%> (-1.23%) ⬇️
... and 10 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 64b91a2...592fb95. Read the comment docs.

@bmagyar
Copy link
Copy Markdown
Member Author

bmagyar commented Jul 8, 2020

Apart from the codecov glitches, this PR is ready to go, merging...

@bmagyar bmagyar merged commit f4d33a7 into ros-controls:master Jul 8, 2020
destogl added a commit to b-robotized-forks/ros2_control that referenced this pull request Aug 11, 2022
* Adapt joint_trajectory_controller to resource_manager
* Fix JTC tests with resource_manager
* Remove resource_manager from tests, use hw interfaces directly
* Release interfaces on joint_trajectory_controller deactivate()
* Correct reserve size for joint_trajecotry_controller state interface.
* Remove constructor used only for testing
* Remove colcon ignore
* Add back enabled controllers to CI

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
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.

Implement registerInterface() following a string-based setup so it can be checked against when attempting to load transmissions

5 participants