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

Fix for volatile reliable [5425] #532

Merged
merged 4 commits into from
May 16, 2019
Merged

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented May 16, 2019

Found an issue (ros2/ros2#703) with late joining volatile readers on volatile writers.

Reproduction steps:

  1. Reliable volatile asynchronous publisher is created, and a call to write is performed.
  2. A matching reliable volatile subscriber is created.
  3. A new call to write is performed.

Expected result:
A call to the subscription listener is made shortly after the write call.

Observed result:
The call to the listener is made after a time that matches the heartbeat period.

Explanation:

  1. The subscriber may receive the initial heartbeat before discovering the publisher (thus ignoring it)
  2. The publisher has not received the pre-emptive ACKNACK, so it hasn't repeated the initial heartbeat.
  3. When the second write call is made, the publisher sends a DATA message for sequence number 2, followed by a GAP to indicate that it will not receive sequence number 1.
  4. (bug) After receiving the GAP, the subscriber was not checking it could notify the user of sequence number 2.

@MiguelCompany MiguelCompany changed the title Fix for volatile reliable Fix for volatile reliable [5425] May 16, 2019
@MiguelCompany MiguelCompany force-pushed the hotfix/volatile-reliable branch from bb250f7 to b6625ff Compare May 16, 2019 13:01
@dirk-thomas
Copy link
Contributor

I can confirm that this patch fixes the regression which made the time source tests in ROS 2 fail, see ros2/build_farmer#196. Running a full set of builds now to check for any other side effects: ros2/ros2#703 (comment)

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany
Copy link
Member Author

Wrong config on CI. Rebuilding

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@raquelalvarezbanos raquelalvarezbanos self-requested a review May 16, 2019 16:48
@dirk-thomas
Copy link
Contributor

The full builds of ROS 2 look very good. All the regressions seem to be resolved. 👍 to merge this fix.

Thank you for creating it that quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants