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

Add spin_and_wait_for_matched to PublicationManager and update test c… #797

Merged
merged 5 commits into from Jun 30, 2021
Merged

Add spin_and_wait_for_matched to PublicationManager and update test c… #797

merged 5 commits into from Jun 30, 2021

Conversation

Barry-Xu-2018
Copy link
Contributor

Address #790

@Barry-Xu-2018
Add spin_and_wait_for_matched to PublicationManager and update test c…
3bf491d 
…odes

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 requested a review from a team as a code owner June 28, 2021 07:57
@Barry-Xu-2018 Barry-Xu-2018 requested review from MichaelOrlov and piraka9011 and removed request for a team June 28, 2021 07:57
@Barry-Xu-2018 Barry-Xu-2018 mentioned this pull request Jun 28, 2021
Closed
@MichaelOrlov
Copy link
Contributor

@Barry-Xu-2018 I am kind of confused about your implementation and this is not what I expected to see.
Why do you need to spin node for PublicationManager?
It does make sense to spin node only for subscription.

@Barry-Xu-2018
Copy link
Contributor Author

@MichaelOrlov

I am kind of confused about your implementation and this is not what I expected to see.
Why do you need to spin node for PublicationManager?
It does make sense to spin node only for subscription.

According to your suggestion in #790 (comment)

For the Problem 1 related to the discovery it would be more appropriate to add wait_for_matched(..) API for the PublicationManager. Similar as made spin_and_wait_for_matched in SubscriptionManager.

My understanding is to add similar API like spin_and_wait_for_matched in SubscriptionManager to PublicationManager.
Spin node is to confirm the number of connected subscription (from rosbag) for specified publisher.

@Barry-Xu-2018
Address review comments
9ea3391 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@MichaelOrlov
Copy link
Contributor

@Barry-Xu-2018 It seems you misinterpreted my suggestion. I intentionally mentioned wait_for_matched(..) API without word spin. Also mentioned similar as spin_and_wait_for_matched(..). I meant that similar doesn't means "the same as".

Anyway, thank you for your PR, I will take a look more closely on it since you have addressed some review comments.

@Barry-Xu-2018
Copy link
Contributor Author

@MichaelOrlov

It seems you misinterpreted my suggestion. I intentionally mentioned wait_for_matched(..) API without word spin. Also mentioned similar as spin_and_wait_for_matched(..). I meant that similar doesn't means "the same as".

Sorry for my misunderstanding.
Spin is unnecessary.

Anyway, thank you for your PR, I will take a look more closely on it since you have addressed some review comments.

Please wait a moment for the review.
I will update codes to remove spin.

@Barry-Xu-2018
Wait without spin
0ee7cd4 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Contributor Author

@MichaelOrlov @emersonknapp

Addressed your comments.
Please review again.

MichaelOrlov
MichaelOrlov reviewed Jun 29, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@Barry-Xu-2018 LGTM among a few nitpicks.

rosbag2_test_common/include/rosbag2_test_common/publication_manager.hpp Outdated Show resolved Hide resolved
rosbag2_test_common/include/rosbag2_test_common/publication_manager.hpp Outdated Show resolved Hide resolved
@Barry-Xu-2018
Remove unused codes and adjust default sleep time
1d11108 
Signed-off-by: Barry Xu <barry.xu@sony.com>
MichaelOrlov
MichaelOrlov approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@Barry-Xu-2018 All looks good now.
Thank you for your contribution.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/8f92e1829d0071ceb0844bff7ad53c52/raw/e7160639b7b34096e84da97c39d90e499def5c70/ros2.repos

BUILD args: --packages-up-to rosbag2_test_common rosbag2_tests
TEST args: --packages-select rosbag2_test_common rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8641/

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

@Barry-Xu-2018
Fix wrong description of return value
787a0b8 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@MichaelOrlov MichaelOrlov merged commit 252e241 into ros2:master Jun 30, 2021
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@Barry-Xu-2018
Add spin_and_wait_for_matched to PublicationManager and update test c… (
4580a8b 
ros2#797)

* Add spin_and_wait_for_matched to PublicationManager and update test codes

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Address review comments

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Wait without spin

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Remove unused codes and adjust default sleep time

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Fix wrong description of return value

Signed-off-by: Barry Xu <barry.xu@sony.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@Barry-Xu-2018 @YXL76
Add spin_and_wait_for_matched to PublicationManager and update test c… (
f302329 
ros2#797)

* Add spin_and_wait_for_matched to PublicationManager and update test codes

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Address review comments

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Wait without spin

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Remove unused codes and adjust default sleep time

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Fix wrong description of return value

Signed-off-by: Barry Xu <barry.xu@sony.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@Barry-Xu-2018 @YXL76
Add spin_and_wait_for_matched to PublicationManager and update test c… (
b5eee3b 
ros2#797)

* Add spin_and_wait_for_matched to PublicationManager and update test codes

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Address review comments

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Wait without spin

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Remove unused codes and adjust default sleep time

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Fix wrong description of return value

Signed-off-by: Barry Xu <barry.xu@sony.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@camm73 @YXL76
Add spin_and_wait_for_matched to PublicationManager and update test c… (
96cd8bf 
ros2#797)

* Add spin_and_wait_for_matched to PublicationManager and update test codes

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Address review comments

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Wait without spin

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Remove unused codes and adjust default sleep time

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Fix wrong description of return value

Signed-off-by: Barry Xu <barry.xu@sony.com>
YXL76 pushed a commit to YXL76/rosbag2 that referenced this pull request Nov 18, 2022
@Barry-Xu-2018 @YXL76
Add spin_and_wait_for_matched to PublicationManager and update test c… (
a66400b 
ros2#797)

* Add spin_and_wait_for_matched to PublicationManager and update test codes

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Address review comments

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Wait without spin

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Remove unused codes and adjust default sleep time

Signed-off-by: Barry Xu <barry.xu@sony.com>

* Fix wrong description of return value

Signed-off-by: Barry Xu <barry.xu@sony.com>
@YXL76 YXL76 mentioned this pull request Nov 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp left review comments

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@piraka9011 piraka9011 Awaiting requested review from piraka9011

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@Barry-Xu-2018 @MichaelOrlov @emersonknapp