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

Use types in hardware interface from ros2_control in local namespace #339

Merged
merged 3 commits into from
May 3, 2022

Conversation

destogl
Copy link
Contributor

@destogl destogl commented Apr 14, 2022

ros2_control moved some of the global usings to the local namespace. The change is compatible with galactic and rolling. It correct builds from the source on rolling but does not break anything.

ros-controls/ros2_control#673

@destogl destogl requested a review from fmauch April 14, 2022 08:54
fmauch
fmauch previously approved these changes Apr 14, 2022
Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

Makes sense and shouldn't do any harm.

@fmauch
Copy link
Contributor

fmauch commented Apr 14, 2022

But wait a second. Should this be necessary at all?

We include hardware_interface/system_interface.hpp, which defines hardware_interface::CallbackReturn. Wouldn't it be better to reuse that instead of redefining it ourselves? As we are implementing the virtual methods from hardware_interface::SystemInterface it would only make sense to use its return type definition.

@fmauch fmauch dismissed their stale review April 14, 2022 09:19

I was having a second thought.

@destogl
Copy link
Contributor Author

destogl commented Apr 15, 2022

We include hardware_interface/system_interface.hpp, which defines hardware_interface::CallbackReturn. Wouldn't it be better to reuse that instead of redefining it ourselves? As we are implementing the virtual methods from hardware_interface::SystemInterface it would only make sense to use its return type definition.

You are right! This is much better! Thanks for the idea!!!

@destogl
Copy link
Contributor Author

destogl commented Apr 15, 2022

@fmauch updated! This may be failing on galactic though.

@destogl destogl requested a review from fmauch April 15, 2022 08:21
@fmauch
Copy link
Contributor

fmauch commented Apr 15, 2022

Thanks @destogl. That might be the time to actually branch out for galactic...

Let's finish a couple of outstanding PRs, then branch out and merge this.

@fmauch fmauch added the humble Relevant for humble and higher ROS versions label Apr 15, 2022
@fmauch fmauch mentioned this pull request Apr 19, 2022
4 tasks
@destogl destogl changed the title Correct compiling on rolling after ros2_control changes Use types in hardware interface from ros2_control in local namespace Apr 28, 2022
Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

rebased, still aprove

@fmauch fmauch merged commit 89409da into main May 3, 2022
@fmauch fmauch deleted the add-using-into-namespace branch May 3, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
humble Relevant for humble and higher ROS versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants