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

ros2 bag convert now excludes messages not in [start_time;end_time] #1455

Merged
merged 14 commits into from
Sep 28, 2023

Conversation

pfavr2
Copy link
Contributor

@pfavr2 pfavr2 commented Sep 3, 2023

resolves #599

resolves ros2#599

Signed-off-by: Peter Favrholdt <ros2github@how.dk>
@pfavr2 pfavr2 requested a review from a team as a code owner September 3, 2023 14:44
@pfavr2 pfavr2 requested review from emersonknapp and jhdcs and removed request for a team September 3, 2023 14:44
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
@pfavr2
Copy link
Contributor Author

pfavr2 commented Sep 4, 2023

This is my first PR to ROS2. If it is not good enough I look forward to hearing your comments!

I hope to be able to contribute a lot more in the coming years. Best regards, Peter

@MichaelOrlov MichaelOrlov self-requested a review September 6, 2023 21:57
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.

@pfavr2 Thank you for your contribution.
I was thinking about the usability of this feature.
For the final user probably would be difficult to find what the exact timestamps in nanoseconds should be for cutting to put them in yaml config file for bag rewrite.
It would be much easier to operate with time offset or duration from the beginning of the bag for start and end time when configuring cutting interval.
Also curious about if the final user would like to have multiple intervals for bag rewrite.
It might make sense to define a vector or array of the start-end intervals.

Another point is that we would like to avoid extra dependencies in the bottom layers of the rosbag2.
To avoid bringing extra dependencies I would recommend using a plain uint64_t type for datestamps or durations similar to what we have already used for the max_bagfile_duration.

Also need to add e few unit tests to make sure that data stamps parsing properly from and to the yaml files and make sure that sequential_writer correctly respects start and end time.

@pfavr2
Copy link
Contributor Author

pfavr2 commented Sep 8, 2023

Thank you very much for the review. I'll look into the improvements soon!

I agree it would be nice to have multiple time formats for the yaml file and this could be added later.

Specifying the start and end time in nanoseconds are easy when using foxglove for previewing the original file. The time of the marker can be shown in seconds with nanoseconds precision. So copy-paste and delete the decimal point. One possibility would be to rename the start_time and end_time to start_time_ns and end_time_ns for the yaml file? Then later additional time formats can be added as well... would this suffice for now?

The ability to split into multiple output-files with different start_time and end_time is useful for our application of this feature. I think it could be useful for others (you don't have to use it if you only need a single output file - but it won't hurt to offer the possibility).

Thank you again for the review comments.

Best regards,

Peter

@MichaelOrlov
Copy link
Contributor

@pfavr2 Using nanoseconds representation and copy past and deleting decimal point from Foxglove studio - sounds like a reasonable approach.
For instance, if we have a time stamp from Foxglove studio 1666744557.446749846 seconds it could be easily transformed to the 1666744557446749846 nanoseconds by removing the dot from the middle.

And yes, please rename variables to the start_time_ns and end_time_ns for the yaml file.

When I was talking about the array of the start - end timestamps I meant that those chunks would be cut and inserted in one new file, not in the multiple files.
If you need to cut multiple chunks to each new file you can use a split example

$ ros2 bag convert -i bag1 -o out.yaml
# out.yaml
output_bags:
- uri: split1
  topics: [/topic1, /topic2]
- uri: split2
  topics: [/topic1, /topic2]

pfavr2 and others added 2 commits September 25, 2023 21:06
Moved timestamp check above Get TopicInformation
Added parsing test.

Signed-off-by: Peter Favrholdt <ros2github@how.dk>
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.

@pfavr2 We can avoid at least 3 type conversions to the std::chrono::time_point<std::chrono::high_resolution_clock>(timestamp) when checking if message in the accepting time range if we would be using uint32_t type for the start_time_ns and end_time_ns parameters and use native comparison for timestamps. It will be better from the performance point of view.
Because uint32_t is exactly half of the int64_t range and we wouldn't need a type cast for the message->time_stamp > start_time_ns comparison.

Or maybe even using int64_t type in this case 0 will be a valid range and we can default to the -1 to determine that start or end time limit is not set.

rosbag2_storage_mcap/CMakeLists.txt Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
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.

@pfavr2 Thanks for changing the type to the int64_t.
uint32_ t certainly is not enough, thanks for pointing it out.
Now it looks good! Almost ready to go.
Please remove rclcpp dependencies from the SQLite3 storage plugin.
We don't have a structural diagram with rules but we are trying to keep lower rosbag2 levels such as storage and storage plugins to be almost ROS agnostic to avoid cyclic dependencies if will be needed to use in other core packages.
Also usually we catch out on code reviews if someone adds unnecessary dependencies in any packages :)

rosbag2_storage_sqlite3/CMakeLists.txt Outdated Show resolved Hide resolved
rosbag2_storage_sqlite3/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Peter Favrholdt <ros2github@how.dk>
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.

LGTM with green CI.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/4af23e6b6151048a6b10b02c16815726/raw/e98144f2a24dd82b42d0c621a2137b2bf908c861/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_storage rosbag2_py
TEST args: --packages-above rosbag2_cpp rosbag2_storage rosbag2_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12716

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

@MichaelOrlov MichaelOrlov merged commit a80adfd into ros2:rolling Sep 28, 2023
14 checks passed
@Yuchen966
Copy link

Is this feature compatible with humble?

@MichaelOrlov
Copy link
Contributor

Unfortunately not, since it changes ABI.

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.

Ability to split already existing ros2 bags by time
3 participants