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

Split Rosbag2Transport into Player and Recorder classes - first pass to enable further progress #721

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Split Rosbag2Transport into Player and Recorder classes - first pass to enable further progress #721

merged 1 commit into from
Apr 8, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 8, 2021 •
edited
Loading

Alternative, or precursor to #716

Only splits the Rosbag2Transport class into separate Recorder and Player classes. Does not create new headers, or change them into subclass of rclcpp::Node

Potential follow-ons after this PR:

  • Make Player and Recorder subclasses of rclcpp::Node
  • Move Player node-spinning out into the invoking context
  • Get rid of Rosbag2TransportTestFixture class, which no longer provides very much add-on value

Split Rosbag2Transport into Player and Recorder classes - first pass …
d5e9915 
…to enable further progress

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp requested a review from a team as a code owner April 8, 2021 18:35
@emersonknapp emersonknapp requested review from jaisontj and prajakta-gokhale and removed request for a team April 8, 2021 18:35
@emersonknapp emersonknapp marked this pull request as draft April 8, 2021 18:35
@emersonknapp emersonknapp changed the title Split Rosbag2Transport into Player and Recorder classes - first pass to enable further progress [Proposal] Split Rosbag2Transport into Player and Recorder classes - first pass to enable further progress Apr 8, 2021
emersonknapp
emersonknapp commented Apr 8, 2021
View reviewed changes
rosbag2_transport/src/rosbag2_transport/recorder.cpp
serialization_format_,
serialized_offered_qos_profiles_for_topic(topic_with_type.first)
});
{
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 don't know why, but uncrustify started complaining about this in my local env

@emersonknapp emersonknapp marked this pull request as ready for review April 8, 2021 18:39
This was referenced Apr 8, 2021
Closed
Closed
@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/6aa292debbe888752ba73469cea12a66/raw/289911b9f2bec145362cff4027bff646b78487d4/ros2.repos
BUILD args: --packages-up-to rosbag2_py rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_py rosbag2_transport rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8174

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

Karsten1987
Karsten1987 approved these changes Apr 8, 2021
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.

go ahead once CI turns green

@emersonknapp emersonknapp changed the title [Proposal] Split Rosbag2Transport into Player and Recorder classes - first pass to enable further progress Split Rosbag2Transport into Player and Recorder classes - first pass to enable further progress Apr 8, 2021
@emersonknapp emersonknapp merged commit fb7fc1c into master Apr 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/transport-split branch April 8, 2021 21:35
Kettenhoax pushed a commit to Kettenhoax/rosbag2 that referenced this pull request Apr 9, 2021
@emersonknapp
Split Rosbag2Transport into Player and Recorder classes - first pass …
b30f17a 
…to enable further progress (ros2#721)

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@clalancette
Copy link
Contributor

Just as an FYI, this change seems to have broken rosbag2_bag_v2_plugins: https://build.ros2.org/view/Rbin_uF64/job/Rbin_uF64__rosbag2_bag_v2_plugins__ubuntu_focal_amd64__binary/216/console

@emersonknapp
Copy link
Collaborator Author

Thanks for the note! I wish we had a way to include them in the ci_launcher runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
3 participants
@emersonknapp @clalancette @Karsten1987