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.

Sign up for GitHub

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

Jump to bottom

Support matched/unmatched event #101

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Support matched/unmatched event #101

merged 1 commit into from
Mar 22, 2023

Conversation

Barry-Xu-2018
Copy link
Collaborator

Address ros2/rmw#330

@Barry-Xu-2018 Barry-Xu-2018 mentioned this pull request Feb 15, 2023
Closed
11 tasks
fujitatomoya
fujitatomoya approved these changes Feb 26, 2023
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

overall looks good to me.

@fujitatomoya
Copy link
Collaborator

@asorbini we would like to request another review on this.

Related to new feature, ros2/rmw#330 (comment)

@fujitatomoya
Copy link
Collaborator

@asorbini could you take a look at this?

fujitatomoya
fujitatomoya reviewed Mar 8, 2023
View reviewed changes
rmw_connextdds_common/src/common/rmw_impl_waitset_std.cpp Outdated
{
this->status_matched = *status;

if (status->current_count_change == 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Barry-Xu-2018
CC: @asorbini

current_count_change will be either 1 or -1 always? otherwise, we will miss the event. as far as i see the specification, this could be 2 or -2 for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the processing likes Fastdds.
@asorbini please help to confirm this since I have no source codes.

@fujitatomoya fujitatomoya mentioned this pull request Mar 8, 2023
Merged
@mjcarroll
Copy link
Member

@Barry-Xu-2018 this has some conflicts now.

@Barry-Xu-2018
Copy link
Collaborator Author

@mjcarroll

Thanks for notification.

this has some conflicts now.

Yes. I found it and fix it now.
Besides, I found it is hard to do rebase since the implementation of inconsistent topic was merged.
I have to find another do 'rebase'.

@Barry-Xu-2018
Copy link
Collaborator Author

Barry-Xu-2018 commented Mar 16, 2023 •
edited
Loading

Now interface is changed. We only use matched event in rmw layer (ros2/rmw#331).
So code changes are quite significant.
Besides, merged implementation of inconsistent topic leads to many conflicts with old commits while doing rebase.
So I will merge all commits in this PR to one commit and then do rebase.

@Barry-Xu-2018
Support matched event
4a94ebc 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-matched-unmatched-event-support branch from 12de24e to 4a94ebc Compare March 16, 2023 02:58
@fujitatomoya fujitatomoya mentioned this pull request Mar 18, 2023
Merged
@fujitatomoya
Copy link
Collaborator

@asorbini friendly ping, we would like you to review on this one.

@mjcarroll mjcarroll merged commit 364c44c into ros2:rolling Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@asorbini asorbini Awaiting requested review from asorbini

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Barry-Xu-2018 @fujitatomoya @mjcarroll