Skip to content

Support matched/unmatched event#101

Merged
mjcarroll merged 1 commit intoros2:rollingfrom
Barry-Xu-2018:topic-matched-unmatched-event-support
Mar 22, 2023
Merged

Support matched/unmatched event#101
mjcarroll merged 1 commit intoros2:rollingfrom
Barry-Xu-2018:topic-matched-unmatched-event-support

Conversation

@Barry-Xu-2018
Copy link
Copy Markdown
Collaborator

@Barry-Xu-2018 Barry-Xu-2018 commented Feb 15, 2023

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
Copy Markdown
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
Copy Markdown
Collaborator

fujitatomoya commented Feb 26, 2023

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

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

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Mar 3, 2023

@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
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya Mar 8, 2023

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
Copy Markdown
Collaborator Author

@Barry-Xu-2018 Barry-Xu-2018 Mar 9, 2023

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
Copy Markdown
Member

mjcarroll commented Mar 15, 2023

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

@Barry-Xu-2018
Copy link
Copy Markdown
Collaborator Author

Barry-Xu-2018 commented Mar 16, 2023

@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
Copy Markdown
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
Copy Markdown
Collaborator

fujitatomoya commented Mar 21, 2023

@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