-
Notifications
You must be signed in to change notification settings - Fork 34
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
Indicate missing support for unique network flows #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anamud, thank you for your submission!
I left a couple of comments to make sure this code works with both rmw_connextdds
and rmw_connextddsmicro
, and with older versions of ROS2.
Once those are addressed, this PR will be ready to be merged, although it will need to be held off until ros2/rmw#294 is merged.
rmw_connextdds_common/src/common/rmw_get_network_flow_endpoints.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates @anamud!
The PR looks good now. I think two feature flags is a bit overkill, but I'm going to get rid of them altogether soon, so we can leave that as is.
At this point, I would only wait for ros2/rmw#294 to be merged.
I created #15 to get rid of all "feature flags" in Apologies for the back and forth, but the recent streak of PRs made me realize getting rid of these flags had more priority than I thought. |
@asorbini: Thanks for the review. I understand the rationale to get rid of the flags. I prefer to wait until the PR is merged before rebasing. |
#15 was merged, so you can rebase from |
d0479ed
to
bf0b455
Compare
rmw_connextdds_common/src/common/rmw_network_flow_endpoints.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
It would be good to run CI with the other changed repositories, but I'm assuming that will happen as part of merging of (one of the) other PRs (let me know and I can also start some jobs).
In general, the PR can be merged once ros2/rmw#294 is merged.
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
737b132
to
668f47b
Compare
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
@asorbini @wjwwood I removed a conflicting linkage error reported by the CI system. Please confirm that that change is correct. I am unable to test this PR out properly due to lack of a licensed connextdds system at my side. Can you please suggest an alternative test method? Since the PRs related to unique network flows are now approved, I would appreciate it if you can start a CI job at your side. Here is an updated ros2 rolling repository list that I use for testing. |
Hi @anamud, the change looks good to me, I think it's fine to have these function have C++ linkage, apologies for not spotting that in my earlier reviews. About the licensing issue: I'm not a legal expert and this is not legal advice, but maybe this ROS 2 development work counts towards "research purposes" covered by the "non-commercial license" included in Debian package Here's some CI jobs to validate the changes using your list: |
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
This PR indicates missing support in rmw_connextdds for unique network flows for publishers and subscribers in communicating nodes. Such indication is required for graceful error handling in the ROS2 RMW and above layers.
Associated pull-requests:
Initial contributions stem from Ericsson and eProsima.
Signed-off-by: Ananya Muddukrishna ananya.x.muddukrishna@ericsson.com