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

Interface composition revised #285

Conversation

mathias-luedtke
Copy link
Contributor

As reported in PR2/pr2_mechanism#330 the changes introduced in #235 break hardware interfaces without default contructors.

This PR relaxes this by requiring the default constructor only for interfaces derived from hardware_interface::HardwareResourceManager.

In addition I have cleaned-up the code a little bit and got rid of some warnings.
This breaks the API of CheckIsResourceManager.

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.

Sweet! 👍

@mathias-luedtke
Copy link
Contributor Author

I don't know if CheckIsResourceManager::value is used by someone else, but the header is internal though.

@bmagyar
Copy link
Member

bmagyar commented Oct 24, 2017

The cmake suggests those files are not getting installed although they are available to other packages from within a source-based installation. I think it is safe to remove.

@mathias-luedtke
Copy link
Contributor Author

mathias-luedtke commented Oct 24, 2017

The cmake suggests those files are not getting installed

They get installed! The others need them to be included.
I could just omit the las commit, but mark it as deprecated.
Update: If we keep them we won't get rid of the warnings.

In general the combined HW feature was not designed properly for reuse and maintenance :-/

@bmagyar
Copy link
Member

bmagyar commented Oct 24, 2017

Ah...I thought the internal folder wasn't within include/hardware_interface

What do you think about moving those header files into include/hardware_interface_internal so they can truly be internal-only. That'd partially patch up the reuse/maintenance aspect.
How much of it could we hide?

I wouldn't worry much about some hacky code breaking because those files are already marked internal and also pretty specific to this implementation.

@mathias-luedtke
Copy link
Contributor Author

What do you think about moving those header files into include/hardware_interface_internal so they can truly be internal-only.

These are internal API files that are used (and implicitly exposed) by the public API.
But the internal prefix in the path should make clear that it is not meant to be public (=stable) and is subject to changes at any time.

We could move CheckIsResourceManager into the internal namespace to emphasize this.

@mathias-luedtke mathias-luedtke changed the title [WIP] Interface composition revised Interface composition revised Oct 24, 2017
@bmagyar
Copy link
Member

bmagyar commented Oct 24, 2017

The internal namespace is a good idea. Could you make that change please?

@mathias-luedtke
Copy link
Contributor Author

done, this is an API break ;)

@bmagyar
Copy link
Member

bmagyar commented Oct 27, 2017

I approved, you merge ;)

@mathias-luedtke mathias-luedtke merged commit cc7a812 into ros-controls:kinetic-devel Oct 27, 2017
@mathias-luedtke mathias-luedtke deleted the fix/interface-composition branch October 27, 2017 18:44
k-okada added a commit to k-okada/pr2_mechanism that referenced this pull request Feb 13, 2018
ros-controls/ros_control#285 (which is included in hardware_interface 0.13.0) allow us to use class `hardware_interface::HardwareResourceManager` API
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