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

[pfavr2]

resolves #599

[pfavr2]

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]

@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]

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]

@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]

[MichaelOrlov]

@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.

[MichaelOrlov]

@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 :)

[MichaelOrlov]

LGTM with green CI.

[MichaelOrlov]

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
• Linux-aarch64
• Windows

[Yuchen966]

Is this feature compatible with humble?

[MichaelOrlov]

Unfortunately not, since it changes ABI.