Skip to content

move interfaces into hardware_interface package#1

Merged
destogl merged 1 commit intodestogl:hardware_control_components-designfrom
ros-controls:hardware_control_components-design-kk
Jun 22, 2020
Merged

move interfaces into hardware_interface package#1
destogl merged 1 commit intodestogl:hardware_control_components-designfrom
ros-controls:hardware_control_components-design-kk

Conversation

@Karsten1987
Copy link
Copy Markdown

I am sorry, this change is pretty brutal, but I feel it makes more sense like this.

  • the interfaces for components were made as real interfaces with pure functions (= 0)
  • I shifted the logic for the urdf parser into the hardware interfaces as well. I could see that we put that into its own package or leave it inside there as-is.
  • the robot control components are somewhat default implementations of these interfaces and exported via pluginlib. No public headers are needed for this.
  • the linters are mostly run. There're some cpplint and test failures in the component parser.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Copy link
Copy Markdown
Owner

@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.

Hi Karsten,

This looks excellent, we got full flexibility and small include footprint! Thnks!

Before merging, I would like to fix the interfaces.

In your proposal, which class has a role of the "Container" which manages everything? Also how should hardware be loaded, or should hardware implement SensorInterface, ActuatorInterfce and SensorInterface directly?

@destogl destogl self-assigned this Jun 17, 2020
@Karsten1987
Copy link
Copy Markdown
Author

Just went through your comments again.

As for claiming resources, I am not sure that each individual package should take care of this. I'd rather have this at a central place (possibly the hardware interface) which manages all resources "globally".

@destogl
Copy link
Copy Markdown
Owner

destogl commented Jun 17, 2020

My observation regarding managing claims in each actuator is following:

CONS:

  • larger header files
  • addition member function calls

PROS:

  • no need for additional maps in container object
  • Actuators are self-contained
  • transparent interface - no functional influence from other classes (similar to previous one)
  • simpler management of Actuators in container object

@destogl
Copy link
Copy Markdown
Owner

destogl commented Jun 17, 2020

Thanks for the fast answer.

I also added a few comments and the only question is, if we extent the final classes with some shared functionality...

@Karsten1987
Copy link
Copy Markdown
Author

no need for additional maps in container object
Actuators are self-contained

These two points are pretty important for me and I think it's the right approach. That would also go along with the discussion about using InterfaceValues in these interfaces.

@Karsten1987
Copy link
Copy Markdown
Author

Karsten1987 commented Jun 18, 2020

I was thinking about the resource claiming a bit more. I haven't had time to prototype it, but essentially I'd like to implement a central resource manager within the hardware interface package. Each individual component (a single actuator, a single sensor or a complete system) has to implement a function in the style of get_claimed_resources() in which each component returns a list of resource names it tries to claim.
Upon loading, the hardware resource manager then verifies that every resource exists and can be claimed only once. That way, the only thing to do per component is to declare a list of resources.
Whenever the component is deactivated, the resources are being released and (re-)claimed when (re-)activated.

All that resource claiming would be specific to the actuators though. I don't really see a point of claiming a sensor resource. What do you think?

@destogl
Copy link
Copy Markdown
Owner

destogl commented Jun 18, 2020

All that resource claiming would be specific to the actuators though. I don't really see a point in claiming a sensor resource. What do you think?

I agree completely, this is only an issue when writing variables.

Each individual component (a single actuator, a single sensor or a complete system) has to implement a function in the style of get_claimed_resources() in which each component returns a list of resource names it tries to claim.

I imagined that each component has/posses it’s resources and it is therefore responsible for it. My idea was that only through components hardware can be accessed. Nevertheless, I understand and agree with you on this. We can have one Ressource manager who takes care about that.

That would also go along with the discussion about using InterfaceValues in these interfaces.

To my knowledge, it is the same if we use some kind of complex objects (e.g. maps) or vectors/arrays of doubles since we are just passing pointers to them. Am I right on this? If so I would rather provide more data and than users can decide if and how they use them.

@destogl
Copy link
Copy Markdown
Owner

destogl commented Jun 18, 2020

To summarize.

The main open questions are:

  • How resources (actuators) are claimed?
  • Which type of pointers should be used in read and write functions? (doubles or more complex data)
  • Do we need separate functions for Accessing actuators data from Hardware and Controllers? E.g. set_command (from controllers), set_current_value (from Hardware)

And TODO:

  • add constant with common interface types (for now: position, velocity, effort)

@Karsten1987
Copy link
Copy Markdown
Author

I imagined that each component has/posses it’s resources and it is therefore responsible for it. My idea was that only through components hardware can be accessed.

That is still true. The actual communication with the hardware is done through the components. The resource management is just purely virtual within the hardware interface. I don't think that a component should know much about its peers.

@destogl
Copy link
Copy Markdown
Owner

destogl commented Jun 20, 2020

The resource management is just purely virtual within the hardware interface. I don't think that a component should know much about its peers.

I meant actually, that actuators are only managing resources. Nevertheless this is not the issue currently. I will merge this until Monday so we can continue to work on the one stream of code.

I hope this is ok with you :)

@destogl destogl merged commit a9e1249 into destogl:hardware_control_components-design Jun 22, 2020
destogl added a commit to ros-controls/ros2_control_demos that referenced this pull request Jun 22, 2020
@Karsten1987 Karsten1987 deleted the hardware_control_components-design-kk branch November 11, 2020 05:11
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.

2 participants