Skip to content

[Humble] Use Fast-DDS Waitsets instead of listeners (backport #619)#633

Merged
audrow merged 1 commit intoros2:humblefrom
eProsima:backport/619
Sep 7, 2022
Merged

[Humble] Use Fast-DDS Waitsets instead of listeners (backport #619)#633
audrow merged 1 commit intoros2:humblefrom
eProsima:backport/619

Conversation

@MiguelCompany
Copy link
Copy Markdown
Collaborator

@MiguelCompany MiguelCompany commented Aug 30, 2022

Signed-off-by: Miguel Company MiguelCompany@eprosima.com

@richiware @MiguelCompany
Use Fast-DDS Waitsets instead of listeners (backport ros2#619)
cdc9816 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany mentioned this pull request Aug 30, 2022
Merged
4 tasks
@MiguelCompany MiguelCompany changed the title Use Fast-DDS Waitsets instead of listeners (backport #619) [Humble] Use Fast-DDS Waitsets instead of listeners (backport #619) Aug 30, 2022
@fujitatomoya fujitatomoya requested a review from ivanpauno August 30, 2022 21:52
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Aug 30, 2022

CI(Full build and test with rmw_fastrtps only): https://gist.githubusercontent.com/fujitatomoya/635b829c55226943225a7669b61a0cfd/raw/c860c9ae967778906b6ad848cb82d783fae8e5ea/ros2.repos

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Aug 31, 2022

why do we need to bakcport this?
It's a huge change, I don't think it needs to be backported

@MiguelCompany
Copy link
Copy Markdown
Collaborator Author

MiguelCompany commented Sep 1, 2022

why do we need to bakcport this? It's a huge change, I don't think it needs to be backported

The change is big, yes. But it fixes several issues on the default RMW, like the ones listed here and here.

According to our testing, it also improves the overall behavior regarding discovery and services.

@alsora
Copy link
Copy Markdown
Contributor

alsora commented Sep 1, 2022

+1 on merging this.
If we don't backport it, a lot of users will likely have to build from sources and cherry-pick it as this fixes critical bugs.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Sep 1, 2022

a couple of CI failure are unrelated, ready to merge with 2nd review.

@ivanpauno ivanpauno added the enhancement New feature or request label Sep 1, 2022
@jefferyyjhsu jefferyyjhsu mentioned this pull request Sep 1, 2022
Merged
alsora added a commit to irobot-ros/rmw_fastrtps that referenced this pull request Sep 2, 2022
@alsora
Merge pull request #29 from jefferyyjhsu/irobot/humble
b5d5c93 
Update irobot/humble to include ros2#633
audrow
audrow approved these changes Sep 6, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

This is a large change. I don't think it breaks API for any of the rmw_ functions, so if I understand correctly, it looks okay to be backported into Humble.

@audrow
Copy link
Copy Markdown
Member

audrow commented Sep 6, 2022 •
edited
Loading

Here's another run of CI, just to make sure.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@audrow
Copy link
Copy Markdown
Member

audrow commented Sep 7, 2022

The Windows gets the same CI warning as in @fujitatomoya's previous run, so I'm going to merge.

@audrow audrow merged commit fe73970 into ros2:humble Sep 7, 2022
@MiguelCompany
Copy link
Copy Markdown
Collaborator Author

MiguelCompany commented Sep 13, 2022

@audrow When will this be available on binaries?

@MiguelCompany MiguelCompany deleted the backport/619 branch September 13, 2022 09:32
@benjinne benjinne mentioned this pull request Sep 26, 2022
Closed
@doisyg doisyg mentioned this pull request Oct 19, 2022
Closed
@clalancette clalancette mentioned this pull request Oct 25, 2022
Closed
@MiguelCompany
Copy link
Copy Markdown
Collaborator Author

MiguelCompany commented Oct 25, 2022

@audrow When will this be available on binaries?

@audrow @clalancette could a release of rwm_fastrps_cpp be done any time soon?

@hplatou hplatou mentioned this pull request Dec 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@audrow audrow audrow approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@clalancette clalancette Awaiting requested review from clalancette

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@MiguelCompany @fujitatomoya @ivanpauno @alsora @audrow @richiware