search

OpenDDS / rmw_opendds

ROS-2 support for OpenDDS
Apache License 2.0
7 stars 10 forks source link

rmw client and service #28

Closed lij-oci closed 4 years ago

mitza-oci commented 4 years ago

@lij-oci this PR's title is "rmw service". Do you want the client changes here (if so, change the title), or do you want to keep them separate?

@mitza-oci Yeah, you are right. Since service and client share the same rosidl_service_type_support_t and some helper functions, changing one often needs to change the other. So I will change the title to "rmw client and service".

mitza-oci commented 4 years ago

@lij-oci I posted a comment here yesterday, please reply

mitza-oci commented 4 years ago

@lij-oci this PR's title is "rmw service". Do you want the client changes here (if so, change the title), or do you want to keep them separate?

@mitza-oci Yeah, you are right. Since service and client share the same rosidl_service_type_support_t and some helper functions, changing one often needs to change the other. So I will change the title to "rmw client and service".

Please don't edit github comments to reply. It doesn't send email notifications for those.

adamsj-ros commented 4 years ago

@mitza-oci Do you want me to follow up with your requested changes or are you going to have an opportunity to check them out?

mitza-oci commented 4 years ago

@mitza-oci Do you want me to follow up with your requested changes or are you going to have an opportunity to check them out?

I think a lot has changed on this branch since I reviewed (not just responses to reviews but increased scope). So I'll leave it up to you to determine if you want to go ahead and merge it.

adamsj-ros commented 4 years ago

So I'll leave it up to you to determine if you want to go ahead and merge it

@lij-oci Since you're intimately familiar with this code, could you go back through the items marked resolved above and make sure you have a meaningful comment on how you resolved it. I'm looking for a little more than "done" if possible. Especially important and helpful for me would be to add responses to all Adam's comments for these items now marked resolved. Thanks.

lij-oci commented 4 years ago

@adamsj-oci, @mitza-oci Those earlier few changes I wrote "done" are simple changes and Adam advised me to use the resolved button instead of writing "done". Now the important thing here is that I would like Adam to review rmw_client.cpp. It's similar to rmw_service, and shares the same type support interface. They should be merged together to make this work.