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

Pass parameters in the correct order to DDS_DataReader_read in rmw_co… #129

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

cwecht
Copy link
Contributor

@cwecht cwecht commented Jul 31, 2023

…nnextdds_count_unread_samples for micro.

The order of parameters is now consistent with the respective function call for Connext Pro.

…nnextdds_count_unread_samples for micro

Signed-off-by: Christopher Wecht <cwecht@mailbox.org>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this has always been wrong; even on Connext Pro, the API seems to be:

DDS_DataReader_read(DDS_DataReader, DDS_UntypedSampleSeq, DDS_SampleInfoSeq, DDS_Long, DDS_SampleStateMask, DDS_ViewStateMask, DDS_InstanceStateMask)

So this looks good to me. I'm going to run CI on it, but @asorbini I could really use a double-check here to make sure that we're doing the right thing with this change.

@clalancette
Copy link
Contributor

CI:

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

@clalancette
Copy link
Contributor

I'm going to go ahead and merge this since it seems right. If there are any follow-up comments we can deal with them later.

@clalancette clalancette merged commit 6b73258 into ros2:rolling Aug 3, 2023
1 check passed
@cwecht
Copy link
Contributor Author

cwecht commented Aug 4, 2023

This Fix really should be backported to humble.

@clalancette
Copy link
Contributor

This Fix really should be backported to humble.

We can backport it to Humble (and Iron), but I'd rather wait a bit here to see if anything pops up in Rolling first. While I don't think that will be the case, we did in fact change the bitmasks that are being passed. So I want to make sure we didn't break anything.

@cwecht
Copy link
Contributor Author

cwecht commented Aug 8, 2023

@clalancette please not that due to #127 no one will be ablt to test these changes on the rolling branch since it won't even compile. This patch (and #130 as well BTW) will just work on humble as there is not build issue.

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.

None yet

2 participants