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

Indicate missing support for unique network flows #13

Merged
merged 11 commits into from
Apr 5, 2021

Conversation

anamud
Copy link

@anamud anamud commented Mar 25, 2021

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

Copy link
Collaborator

@asorbini asorbini left a 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.

Copy link
Collaborator

@asorbini asorbini left a 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.

@asorbini
Copy link
Collaborator

I created #15 to get rid of all "feature flags" in master. If you want, I think you could rebase your changes on asorbini/rolling-no-ff (or rebase them from master, once that PR is merged).

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.

@anamud
Copy link
Author

anamud commented Mar 29, 2021

I created #15 to get rid of all "feature flags" in master. If you want, I think you could rebase your changes on asorbini/rolling-no-ff (or rebase them from master, once that PR is merged).

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.

@asorbini
Copy link
Collaborator

#15 was merged, so you can rebase from master now and remove the feature flags

@anamud
Copy link
Author

anamud commented Mar 30, 2021

#15 was merged, so you can rebase from master now and remove the feature flags

Thanks, @asorbini for the notice. I have now rebased to master and updated the PR. I removed conditional compilation altogether assuming a rolling release model.

Copy link
Collaborator

@asorbini asorbini left a 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.

Ananya Muddukrishna added 9 commits April 1, 2021 12:04
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>
Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
@anamud
Copy link
Author

anamud commented Apr 2, 2021

@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.

@asorbini
Copy link
Collaborator

asorbini commented Apr 3, 2021

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 rti-connext-dds-5.3.1. If that is the case, than you could also submit a request for a permanent "reserach and non-commercial use" license from this RTI webpage and have access to Connext for other platforms too. This would also give you access to newer versions than 5.3.1.

Here's some CI jobs to validate the changes using your list:

  • Linux: Build Status
  • macOS: Build Status
  • Windows: Build Status

Signed-off-by: Ananya Muddukrishna <ananya.x.muddukrishna@ericsson.com>
@wjwwood wjwwood merged commit 6f00b1e into ros2:master Apr 5, 2021
@asorbini asorbini added the galactic PR pertaining the Galactic release label Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
galactic PR pertaining the Galactic release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants