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

major refactor to hide the dds implementation #17

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@wjwwood
Member

wjwwood commented Mar 26, 2014

I set out to refactor the prototype so that packages using rclcpp and std_msgs, e.g. rclcpp_examples, will not have to include any headers which contain any kind of DDS symbols.

To this end I have restructured the package layout:

dependency_change

In this way, all DDS specific code is either in the genidlcpp generated cpp files or in implementation headers in the new package rclcpp_impl_ospl. The implementation headers in rclcpp_impl_ospl are included by the generated cpp files from genidlcpp and the cpp files in rclcpp. That way there is no transitive dependency on any DDS headers for message generators (like std_msgs) or rclcpp. In order to access the DDS headers you would need to depend on rclcpp_impl_ospl. The DDS types are still generated in genidlcpp, so a further useful abstraction would probably be to have a genrclcpp_impl_ospl which generates the code which uses both ROS msg types and DDS idl types and then genidlcpp would only be responsible for generating C++ code from the idl files.

Pros of this design:

  • DDS symbols are hidden in installed headers from message generation packages (like std_msgs)
  • DDS symbols are hidden from rclcpp
  • Publisher and Subscription objects are no longer template types (this could be achieved separately)
  • Compilation of end-user code is faster because majority of generation and compilation happens on message generation packages
  • Lends itself to abstracting the DDS implementation away

Cons of the design:

  • More code is generated in the message generator packages rather than with the compiler via templates
  • More code is compiled in the message generator packages, causing them to take longer to compile and be larger to distribute

To help address the first Con I believe we can move the majority of the code in the msg_pubsub.cpp file into the rclcpp_impl_ospl package as templates in headers and use a similar pattern that was used previously with the dds_impl::DDSTypeResolver struct. This will not move where the code is compiled (in std_msgs), but it will change the location of the code and move it out of a EmPy template file which is more difficult to work in.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood Mar 27, 2014

Member

This has been rebased against master, so it merges, but it also apparently broke some stuff on the rebase. I'll update the pr soon with fixes.

Member

wjwwood commented Mar 27, 2014

This has been rebased against master, so it merges, but it also apparently broke some stuff on the rebase. I'll update the pr soon with fixes.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood Mar 27, 2014

Member

Ok, req/res is working now too.

Member

wjwwood commented Mar 27, 2014

Ok, req/res is working now too.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood Mar 27, 2014

Member

Rebased on master too.

Member

wjwwood commented Mar 27, 2014

Rebased on master too.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Apr 1, 2014

Member

I read through the code changes and I do see the advantages achieved with this refactoring and consider them very valuable. But I am particularly worried about the amount of "client library code" embedded in the message packages (especially https://github.com/osrf/ros_dds/pull/17/files#diff-6).

Imo we are heading into a direction which is against the goal to keep those parts modular and with low coupling. If any of the client library code should be modified or bug fixed that would affect every single message package. Actually it turns around the dependency direction - message packages now define crucial parts of the client library and actually depend on their specific interface.

I do see the need that the message packages need to provide more functionality than the plain convert_ros_message_to_dds() functions currently available. But I would suggest that we identify that exact bare-minimum API and keep it conceptionally completely separate from the client library. It should allow us to create different message packages against different vendors while keeping the client library logic separate and in a higher level package.

Member

dirk-thomas commented Apr 1, 2014

I read through the code changes and I do see the advantages achieved with this refactoring and consider them very valuable. But I am particularly worried about the amount of "client library code" embedded in the message packages (especially https://github.com/osrf/ros_dds/pull/17/files#diff-6).

Imo we are heading into a direction which is against the goal to keep those parts modular and with low coupling. If any of the client library code should be modified or bug fixed that would affect every single message package. Actually it turns around the dependency direction - message packages now define crucial parts of the client library and actually depend on their specific interface.

I do see the need that the message packages need to provide more functionality than the plain convert_ros_message_to_dds() functions currently available. But I would suggest that we identify that exact bare-minimum API and keep it conceptionally completely separate from the client library. It should allow us to create different message packages against different vendors while keeping the client library logic separate and in a higher level package.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood Apr 1, 2014

Member

I agree, and I think we could move the majority of the code in https://github.com/osrf/ros_dds/pull/17/files#diff-6 into rclcpp_impl_ospl as templated headers and put something like DDSTypeResolver back in genidlcpp. However, from what I can imagine, this only really changes where that chunk of code is stored and doesn't change the dependency structure between packages, where the code is built, or improve the run time or build time greatly. It would make the message generation packages more generic, but I am worried that we will have to have a DDS implementation specific version of each of the message generators anyways.

Member

wjwwood commented Apr 1, 2014

I agree, and I think we could move the majority of the code in https://github.com/osrf/ros_dds/pull/17/files#diff-6 into rclcpp_impl_ospl as templated headers and put something like DDSTypeResolver back in genidlcpp. However, from what I can imagine, this only really changes where that chunk of code is stored and doesn't change the dependency structure between packages, where the code is built, or improve the run time or build time greatly. It would make the message generation packages more generic, but I am worried that we will have to have a DDS implementation specific version of each of the message generators anyways.

@esteve

This comment has been minimized.

Show comment
Hide comment
@esteve

esteve May 5, 2014

Contributor

I like the split between generic code and vendor specific implementations in this branch, I must say that it was a bit hard for me to read a diff this big though, so I'd rather merge it now (after rebasing on master) before another month passes. I'm currently making changes to the Publisher/Subscription code, so it's best to merge this branch now and I'll port my changes.

@wjwwood let me know if I can help with rebasing this branch on master.

Contributor

esteve commented May 5, 2014

I like the split between generic code and vendor specific implementations in this branch, I must say that it was a bit hard for me to read a diff this big though, so I'd rather merge it now (after rebasing on master) before another month passes. I'm currently making changes to the Publisher/Subscription code, so it's best to merge this branch now and I'll port my changes.

@wjwwood let me know if I can help with rebasing this branch on master.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood May 5, 2014

Member

@esteve I was waiting on the merge of the parameters branch.

Member

wjwwood commented May 5, 2014

@esteve I was waiting on the merge of the parameters branch.

@esteve

This comment has been minimized.

Show comment
Hide comment
@esteve

esteve May 5, 2014

Contributor

@wjwwood was about to merge that one... you're quick :-)

Contributor

esteve commented May 5, 2014

@wjwwood was about to merge that one... you're quick :-)

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood May 5, 2014

Member

Rebasing this is turning out to be super complex, mostly because since rclcpp now depends on message generation (for parameter support) it now introduces a circular dependency. So basically we would need to break out parameters into a different package.

I think it is better, at this point, to not merge this and just keep it in mind for next iteration. This will take some significant time and should be improved anyways to better separate message generation and implementation code.

Member

wjwwood commented May 5, 2014

Rebasing this is turning out to be super complex, mostly because since rclcpp now depends on message generation (for parameter support) it now introduces a circular dependency. So basically we would need to break out parameters into a different package.

I think it is better, at this point, to not merge this and just keep it in mind for next iteration. This will take some significant time and should be improved anyways to better separate message generation and implementation code.

@wjwwood

This comment has been minimized.

Show comment
Hide comment
@wjwwood

wjwwood May 5, 2014

Member

I would vote to close this and leave the branch as a reference.

Thoughts?

Member

wjwwood commented May 5, 2014

I would vote to close this and leave the branch as a reference.

Thoughts?

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas May 5, 2014

Member

I absolutely agree with your conclusion.

Member

dirk-thomas commented May 5, 2014

I absolutely agree with your conclusion.

@esteve

This comment has been minimized.

Show comment
Hide comment
@esteve

esteve May 6, 2014

Contributor

Sounds good to me too.

Contributor

esteve commented May 6, 2014

Sounds good to me too.

@wjwwood wjwwood closed this May 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment