-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add new ros2 bag
Kilted features + updates
#5771
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
Add new ros2 bag
Kilted features + updates
#5771
Conversation
- These changes are meant to reflect the deprecation of positional topic arguments in favor of using --topics
- The bag directories are correctly explained in 3.1, but in subsequent sections it falls back to referring to the recordings as files. Adjust wording to explain that a bag directory is made - Change commands to capture the .mcap file inside the bag directory
- Add action and service info for ros2 bag info - Add playback controls and playback progress for ros2 bag play
- To make this progression logical, add an extra section where a recording is split into multiple bag files with -d arg in a bag directory called subset_separate - Later, use these separate files when explaining how to play all bag files from a single recording (just call ros2 bag play on the bag directory), as well as how to play multiple select files (using new arg -i!)
ros2 bag
Kilted Features + Updatesros2 bag
Kilted features + updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm with a few minor comments.
@christophebedard @MichaelOrlov either of you can review this?
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
- Use keyboard syntax where appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! 🚀
I want to clarify some sections to better reflect the feature and the goal behind them, but otherwise this looks good!
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
- For examples that involve calling ros2 bag info and ros2 bag play, have the example show it with playing the whole recording instead of individual files
- Change wording around splitting recording - Explain the use case of splitting recording
- Add example where there are two separate record commands, each recording a topic - Explain the use case for this separate recording - Add explanation of --message-order
@christophebedard I'm pretty new to open-source still so not sure what the cadence of these things are, but letting you know these changes should be ready for review, in case you were waiting for me to notify you. My only minor concern is that the "play multiple bags" section is a bit lengthy now based on the structure that we discussed, but let me know your thoughts. Thanks! |
Thank you! We're pretty busy, so a simple ping when the PR is ready for review again is helpful :)
I think it's good, and this way it's pretty self-contained. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to make sure that you have only 1 sentence per line (see CI): https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Contributing-To-ROS-2-Documentation.html#writing-pages
Other than that and a small fix, this looks good to me!
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
...rials/Beginner-CLI-Tools/Recording-And-Playing-Back-Data/Recording-And-Playing-Back-Data.rst
Outdated
Show resolved
Hide resolved
- Add/remove line breaks so that each sentence lives on its own line, adhering to the CI guidelines
4e33fbb
to
998f0b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR and for iterating!
(cherry picked from commit b81dc71) Co-authored-by: Harrison Chen <[email protected]>
Description
ros2 bag play
progress bar-i
argument to play multiple bags-i
explanation--topics
toros2 bag record
commands to reflect the deprecation of positional topic arguments-o
argument, if provided), and pathing to .mcap files now reflects thatros2 bag record
andros2 bag play
, so as not to confuse the reader when their output differsFixes #5550