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

update signature for added pub/sub options #46

Closed
wants to merge 1 commit into from

Conversation

eboasson
Copy link
Collaborator

@eboasson eboasson commented Oct 9, 2019

This relates to ros2/rmw#187.

Before merging, it'd probably be best to make a "dashing" branch.

Signed-off-by: Erik Boasson eb@ilities.com

Signed-off-by: Erik Boasson <eb@ilities.com>
@dirk-thomas
Copy link
Member

I would suggest holding off on branch and get ros2/rmw#188 (comment) resolved first. Than you should be easily able to keep supporting both ROS distros from the same branch.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Looks right to me.

@rotu
Copy link
Collaborator

rotu commented Oct 11, 2019

I wasn't able to find a solution with type reflection or SFINAE to make it cross-compatible, but I think I found a decent enough alternative using #ifdefs:

eboasson#1

@rotu
Copy link
Collaborator

rotu commented Oct 12, 2019

I figured out how to do this cleanly with SFINAE! I prefer this approach, but I'll leave you guys to choose.

eboasson#2

A few tricks:

  1. The extern "C" linkage specifier only needs to be on the function declaration in the rmw.h header and will link the matching overload.
  2. I had to declare the new types (e.g. rmw_subscription_options_t) to be template parameters, so that they don't trigger an unknown type name.
  3. Where I used new properties of existing classes, I had to replace the class name (e.g. rmw_subscription_t) with a template argument so it views it as a substitution failure instead of a missing member.

@eboasson
Copy link
Collaborator Author

@rotu, on macOS it breaks 😞 ... but now there is ros2/rmw#188 and that's a clean solution.

@rotu
Copy link
Collaborator

rotu commented Oct 17, 2019

Good to know. What’s the failure on MacOS?

@eboasson
Copy link
Collaborator Author

I get this:

This error state is being overwritten:

  'failed to resolve symbol 'rmw_create_publisher' in shared library '/Users/erik/ros2_ws/install/rmw_cyclonedds_cpp/lib/librmw_cyclonedds_cpp.dylib', at /Users/erik/ros2_ws/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:163'

with this new error message:

  'failed to resolve symbol 'rmw_create_subscription' in shared library '/Users/erik/ros2_ws/install/rmw_cyclonedds_cpp/lib/librmw_cyclonedds_cpp.dylib', at /Users/erik/ros2_ws/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:163'

And checking the contents of the library with nm /Users/erik/ros2_ws/install/rmw_cyclonedds_cpp/lib/librmw_cyclonedds_cpp.dylib | grep -E 'rmw_create_(publisher|subscriber)' I only see scary mangled names:

0000000000005950 T __Z20rmw_create_publisherPK10rmw_node_tPK29rosidl_message_type_support_tPKcPK17rmw_qos_profile_t
00000000000425a0 d __ZZ20rmw_create_publisherPK10rmw_node_tPK29rosidl_message_type_support_tPKcPK17rmw_qos_profile_tE26__rcutils_logging_location

@eboasson
Copy link
Collaborator Author

Closing because #51 sorted it out

@eboasson eboasson closed this Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants