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

Expose topic filter to command line (addresses #342) #363

Merged
merged 34 commits into from
Apr 15, 2020
Merged

Expose topic filter to command line (addresses #342) #363

merged 34 commits into from
Apr 15, 2020

Conversation

mabelzhang
Copy link
Contributor

Addresses #342. Exposes reader topic filter to command line playback.

The test added in test_rosbag2_play_end_to_end.cpp depends on #362.
Usage:

ros2 bag play cdr_test --topics /test_topic
ros2 bag play --topics /array_topic -- cdr_test

@mabelzhang
add filter for selecting topics to read
bf71f78 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
add StorageFilter
fa96e5c 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
add safety check for storage pointer; add reset_filter
7d16787 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
Merge remote-tracking branch 'upstream/master' into read_topic_filter
585dddf
@mabelzhang
add set_filter and reset_filter to Reader
d04c3f9 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
fix mock sequential reader for rosbag2_transport test
e3de1e5 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
add topic filter to transport play options
81a0abc 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
add selective topics to cmd line
cf3182f 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
merge from upstream master
9fd6240 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
fix cpplint
3349b63 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
Merge remote-tracking branch 'upstream/master' into transport_read_to…
5ac292d 
…pic_filter
@mabelzhang
end to end read test
902b974 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
fix cmd line args
39234a6 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
add play end to end test for filter topic
c0a2188 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
remove read test
74e28c2 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
remove read test in CMakeList
31c9212 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
fix flake8
e837a6c 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang
address PR comments
7e7eb9b 
Signed-off-by: Mabel Zhang <[email protected]>
piraka9011
piraka9011 approved these changes Apr 10, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

One more request, but overall LGTM pending green CI (ament_export warning is fine...)

@mabelzhang
merge from master
988a390 
Signed-off-by: Mabel Zhang <[email protected]>
Karsten1987
Karsten1987 approved these changes Apr 13, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

rosbag2_transport/include/rosbag2_transport/play_options.hpp Outdated
// Topic names to whitelist when playing a bag. Only messages matching these
// specified topics will be played. If list is empty, the filter is ignored
// and all messages are played.
std::vector<std::string> topics_to_filter = std::vector<std::string>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
std::vector<std::string> topics_to_filter = std::vector<std::string>();
std::vector<std::string> topics_to_filter ={};

Copy link
Collaborator

Choose a reason for hiding this comment

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

std::vector<std::string> topics_to_filter{}

Copy link
Contributor

@piraka9011 piraka9011 Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Or just std::vector<std::string> topics_to_filter{};

Copy link
Collaborator

Choose a reason for hiding this comment

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

just adapting to the style below for the std::unordered_map ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, so I already changed to {} without the = so now they're inconsistent. Do we prefer changing the line below too, or change back to ={}?

@mabelzhang
Copy link
Contributor Author

My local workspace is very broken. I need to fix it before I can compile locally before trying CI again.

@mabelzhang
remove line break
5bd47bc 
Signed-off-by: Mabel Zhang <[email protected]>
@emersonknapp
Copy link
Collaborator

I recommend a docker-based workflow. Here's the Dockerfile I personally use https://github.com/emersonknapp/ros2containerbuild/blob/master/Dockerfile and use via

rocker \
  --x11 \
  --network host \
  --oyr-run-arg "--name SOMETHING --rm -v $PWD:/ws -w /ws" \
  candleends/ros2dev

Workspace always clean and portable :)

@mabelzhang
Copy link
Contributor Author

Yeah... I do develop in Docker containers normally, but the one I've been using is built from eloquent source, and with the recent volume of changes, I'm not able to just pull a few packages from master branch and compile. So I'm making a new Docker image, probably not unlike yours, that pulls ros2.repos from the master branch instead.

@emersonknapp
Copy link
Collaborator

I did not intend to close this... no idea how i did

@emersonknapp emersonknapp reopened this Apr 14, 2020
@mabelzhang
Copy link
Contributor Author

Darn. I thought there was no more work for me ;)

@mabelzhang
Copy link
Contributor Author

There are two CI test failures on Linux that I don't understand, projectroot.test_record_all__rmw_fastrtps_cpp and rosbag2_transport.RecordIntegrationTestFixture.published_messages_from_multiple_topics_are_recorded. They're printing binary bites. Still waiting for macOS and Windows.

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

@mabelzhang
Copy link
Contributor Author

macOS has the same test failures as Linux, and Windows has some record test failures.

@emersonknapp
Copy link
Collaborator

See #376 (comment) - my current hypothesis is that there is a bug in Fast-RTPS right now...

@mabelzhang
Copy link
Contributor Author

No more test failures in Linux and macOS. Waiting for Windows

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

@mabelzhang
Copy link
Contributor Author

Windows tests look okay too. Only the known record split bag failures.

@mabelzhang
alpha order
ff5de2f 
Signed-off-by: Mabel Zhang <[email protected]>
@mabelzhang mabelzhang merged commit d68745c into ros2:master Apr 15, 2020
@mabelzhang mabelzhang deleted the transport_read_topic_filter branch April 15, 2020 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

@Karsten1987 Karsten1987 Karsten1987 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@mabelzhang @emersonknapp @Karsten1987 @piraka9011