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

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

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
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...)

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
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

ros2bag/ros2bag/verb/play.py Outdated Show resolved Hide resolved
// 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

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 ={}?

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@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.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@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 ;)

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@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.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants