Add set_read_order reader API

[emersonknapp]

Depends on #51
Depends on ros2/rosbag2#1083

[MichaelOrlov]

@emersonknapp Now in mcap we have two timestamps for each message.

1. timestamp when message was send
2. timestamp when message was received or logged in.

For a while they are fulfilled both with received timestamp. However please consider to add both timestamps options in reader order.
e.g.

rosbag2_storage::ReadOrder::TimestampReceived
rosbag2_storage::ReadOrder::TimestampSend

We can throw some " feature unimplemented" exception for the case rosbag2_storage::ReadOrder::TimestampSend: for a while

[jhurliman]

@MichaelOrlov there is no index on the publish time, we would need to make a new version of the MCAP spec to support that. Since it cannot be properly supported with current MCAP I would suggest not adding an enum option that leads users to believe it might start working at some point.

[MichaelOrlov]

@jhurliman In case of timestamp I tend to agree that it make sense to have only one enum value for received and sent timestamp.
Latter on we can provide some flag in storage_options to what exact timestamp to use for indexing.
However I would recommend to handle API design on rosbag2_storage level with more thorough approach.
It will be better to have some temporarily unimplemented cases rather than changing API in future causing braking ABI changes and inability to port them in other previously issued distros.

@emersonknapp After some consideration it looks like we will need reverse direction for all enum values in ReadOrder enum.
And this reverse direction is not going to cause creation of the new indexes it's just reverse iterator in storage view.
I think it will be more reasonable to create separate boolean argument aka reverse_direction for void MCAPStorage::set_read_order(rosbag2_storage::ReadOrder read_order, bool reverse_direction) API.
Thoughts?

[emersonknapp]

@MichaelOrlov @james-rms @jhurliman re-requesting review now that ros2/rosbag2#1083 is merged

[emersonknapp]

Gist: https://gist.githubusercontent.com/emersonknapp/dd8525d484b3a202eccb140d466d6024/raw/ad6e7e56bc4b85a164c3cfb1588e08dfc4a4d102/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap
TEST args: --packages-above rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10963

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Implementation looks good to me.
Although need to fix failure on Windows build.

[clalancette]

Just as an FYI, I rebased this onto the latest so that I can use this as the basis for something else.

[MichaelOrlov]

@clalancette We have MS Windows build failure in this PR with message path exceeds the OS max path limit. The fully qualified file name must be less than 260 characters.

11:04:49 c:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(382,5): error MSB3491: Could not write lines to file "rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_cpp.dir\Release\rosbag2_.E861DA9D.tlog\rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_cpp.lastbuildstate". Path: rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_cpp.dir\Release\rosbag2_.E861DA9D.tlog\rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_cpp.lastbuildstate exceeds the OS max path limit. The fully qualified file name must be less than 260 characters. [C:\ci\ws\build\rosbag2_storage_mcap_test_fixture_interfaces\rosbag2_storage_mcap_test_fixture_interfaces__rosidl_typesupport_fastrtps_cpp.vcxproj]

May be you have came across with the similar issue and have some advice how to fix it?

[clalancette]

May be you have came across with the similar issue and have some advice how to fix it?

So it is tricky. The easiest thing to do is to rename your package to have a shorter name. It may also be possible to enable long paths, as described in https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation, but that comes with some tradeoffs (and would have to be a change to the infrastructure).

[MichaelOrlov]

@clalancette Thanks for the advice.
cc: @emersonknapp

[emersonknapp]

Gist: https://gist.githubusercontent.com/emersonknapp/0ac509a1c6b43db9058f067b849a44cb/raw/ad6e7e56bc4b85a164c3cfb1588e08dfc4a4d102/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap
TEST args: --packages-above rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10981

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

I tried to go an alternate route of adding the test messages into a test-subpackage, not to be exposed as a released package, but it opened up some complexity with the message_definition_cache being able to find message definitions from installed package. I think the package rename is the most reliable solution for now, though I don't like it.

[MichaelOrlov]

LGTM with green CI.

[emersonknapp]

Gist: https://gist.githubusercontent.com/emersonknapp/d213d28d034b03ceb7a7f6cb26df5e56/raw/ad6e7e56bc4b85a164c3cfb1588e08dfc4a4d102/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap
TEST args: --packages-above rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10984

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

Weird error on the windows

CVTRES : fatal error CVT1107: 'C:\Program Files\rti_connext_dds-6.0.1\lib\x64Win64VS2017\nddscored.lib' is corrupt [C:\ci\ws\build\rmw_connextdds_common\rmw_connextdds_common_pro.vcxproj]

Trying again for sanity

• Windows

[clalancette]

Weird error on the windows

Those have been popping up, and we haven't been able to track them down. They are some kind of infrastructure issue. Your second one failed in the same way. I kicked it off again:

• Windows

[emersonknapp]

I am now realizing that I don't know if we have run Windows CI on this package before. It is not (yet) part of any released variant or ros2/ros2/ros2.repos, so these issues may not have come up. If the following build fails (against main), I'm going to back out the package rename, move forward with the merge, and flag Windows CI fix for followup.