Add split by time to recording

[jhdcs]

This is my first attempt at adding splitting bags by time functionality to Rosbag 2's recording. Currently having some difficulty with adding the new functionality to the command line for testing.

[emersonknapp]

You're almost there - you're just missing an addition to the parsing command at line 125

See the string for parsing:

"ssssss|bbKKKObO"

That is the types of the arguments that are expected, s for string, b for bool etc, see https://docs.python.org/3/c-api/arg.html - from the looks of it you need an additional K in the middle for the unsigned long long you've added.

[jhdcs]

Thanks for the hint! I've added the missing K to the parse string, but I'm still getting an error saying "ros2: error: unrecognized arguments: --max-bag-duration". I think I'm probably still missing a spot that I need to add the command in...

[emersonknapp]

Taking a look - in order to help understand more easily in general, can you paste the exact command you ran when mentioning output?

And, a note that this feature will need tests before merging.

[emersonknapp]

When I build this and try it, I get this result -

-> /ros_ws $ ros2 bag record
Traceback (most recent call last):
  File "/ros_ws/install_x86_64/ros2cli/bin/ros2", line 11, in <module>
    load_entry_point('ros2cli==0.9.2', 'console_scripts', 'ros2')()
  File "/ros_ws/install_x86_64/ros2cli/lib/python3.8/site-packages/ros2cli/cli.py", line 39, in main
    add_subparsers_on_demand(
  File "/ros_ws/install_x86_64/ros2cli/lib/python3.8/site-packages/ros2cli/command/__init__.py", line 249, in add_subparsers_on_demand
    extension.add_arguments(
  File "/ros_ws/install_x86_64/ros2bag/lib/python3.8/site-packages/ros2bag/command/bag.py", line 26, in add_arguments
    add_subparsers_on_demand(
  File "/ros_ws/install_x86_64/ros2cli/lib/python3.8/site-packages/ros2cli/command/__init__.py", line 249, in add_subparsers_on_demand
    extension.add_arguments(
  File "/ros_ws/install_x86_64/ros2bag/lib/python3.8/site-packages/ros2bag/verb/record.py", line 63, in add_arguments
    parser.add_argment(
AttributeError: 'ArgumentParser' object has no attribute 'add_argment'

Based on your result, however, it sounds like either you didn't build up to ros2bag to get your new argument, or forgot to source install/setup.bash before running the command

[jhdcs]

Made a few changes based on your suggestions. Also implemented a test, but I'm not positive it's actually running correctly. It's not generating an error (that I can see), but still seems to be exiting early. Still investigating.

[jhdcs]

Finally found the problem with the test, and I'm actually a little embarrassed it took so long for me to find. Turns out I was trying to divide seconds by milliseconds. Last I checked, Physics doesn't agree with that. Anyways, converted the split time to milliseconds, and it seems to be running correctly now.

[jhdcs]

Can I get another review on this? I'm not sure why the checks are failing, are they more unreliable tests?

[jhdcs]

@emersonknapp Sorry for bothering you again, but now that Foxy has been released, could I get another review on this?

[emersonknapp]

This LGTM - thanks for the contribution. Sorry about the slow turnaround on the review.

[emersonknapp]

@emersonknapp - please run this CI job
Gist: https://gist.githubusercontent.com/emersonknapp/4b7201d9af882bbd754a32bf3d31b748/raw/e9836cd526e7df9cec412fe2d254f3f2e5d68463/ros2.repos
BUILD args: --packages-up-to rosbag2
TEST args: --packages-select-regex rosbag2 ros2bag
Job: ci_launcher

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows

[emersonknapp]

You'll need to rebase on master, this branch is pretty out of date

[jhdcs]

Rebase complete. We'll see what the checks say.

[Karsten1987]

This looks good to me. Pending CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

From the Windows CI output:

09:44:54 C:\ci\ws\src\jhdcs\rosbag2\rosbag2_cpp\src\rosbag2_cpp\writers\sequential_writer.cpp(255,42): error C2676: binary '-': 'std::chrono::system_clock::time_point' does not define this operator or a conversion to a type acceptable to the predefined operator [C:\ci\ws\build\rosbag2_cpp\rosbag2_cpp.vcxproj]

[jhdcs]

I think what's going on is that the compiler is treating std::chrono::high_resolution_clock in std::chrono::time_point<std::chrono::high_resolution_clock> within the BagMetadata struct as a system_clock in Linux, and as a steady_clock in Mac & Windows. Interesting quirk. I've replaced its clock with a std::chrono::system_clock to make sure it only gets treated the way I'm expecting. But the only way to tell is to try and build it on those OSes again.

Can I get another CI on this?

[Karsten1987]

It doesn't compile locally on my OSX:

--- stderr: rosbag2_storage
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosbag2/rosbag2_storage/src/rosbag2_storage/metadata_io.cpp:201:28: error: no viable overloaded '='
    metadata.starting_time = node["starting_time"]
    ~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/chrono:1326:28: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'time_point<std::__1::chrono::steady_clock, duration<[...], ratio<[...], 1000000000>>>' to 'const time_point<std::__1::chrono::system_clock, duration<[...], ratio<[...], 1000000>>>' for 1st argument
class _LIBCPP_TEMPLATE_VIS time_point
                           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/chrono:1326:28: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'time_point<std::__1::chrono::steady_clock, duration<[...], ratio<[...], 1000000000>>>' to 'time_point<std::__1::chrono::system_clock, duration<[...], ratio<[...], 1000000>>>' for 1st argument
In file included from /Users/karsten/workspace/ros2/ros2_master/src/ros2/rosbag2/rosbag2_storage/src/rosbag2_storage/metadata_io.cpp:35:
In file included from /Users/karsten/workspace/ros2/ros2_master/install/include/yaml-cpp/yaml.h:17:
/Users/karsten/workspace/ros2/ros2_master/install/include/yaml-cpp/node/impl.h:216:14: error: implicit instantiation of undefined template 'YAML::convert<std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1, 1000000> > > >'
  AssignData(convert<T>::encode(rhs));
             ^
/Users/karsten/workspace/ros2/ros2_master/install/include/yaml-cpp/node/impl.h:201:3: note: in instantiation of function template specialization 'YAML::Node::Assign<std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1, 1000000> > > >' requested here
  Assign(rhs);
  ^
/Users/karsten/workspace/ros2/ros2_master/src/ros2/rosbag2/rosbag2_storage/src/rosbag2_storage/metadata_io.cpp:184:27: note: in instantiation of function template specialization 'YAML::Node::operator=<std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1, 1000000> > > >' requested here
    node["starting_time"] = metadata.starting_time;
                          ^
/Users/karsten/workspace/ros2/ros2_master/install/include/yaml-cpp/node/node.h:142:8: note: template is declared here
struct convert;
       ^
In file included from /Users/karsten/workspace/ros2/ros2_master/src/ros2/rosbag2/rosbag2_storage/src/rosbag2_storage/metadata_io.cpp:35:
In file included from /Users/karsten/workspace/ros2/ros2_master/install/include/yaml-cpp/yaml.h:17:
/Users/karsten/workspace/ros2/ros2_master/install/include/yaml-cpp/node/impl.h:216:24: error: incomplete definition of type 'YAML::convert<std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1, 1000000> > > >'
  AssignData(convert<T>::encode(rhs));
             ~~~~~~~~~~^~
3 errors generated.

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows

[jhdcs]

I think this might have been that flaky test you were warning me about. I've applied Karsten's fix from elsewhere in the testing suite, hopefully that makes it more reliable.

Not sure about the test_qos_simple test, though. I don't remember having touched that test, and it seems to be trying to access a database that's in use by another test?

[jhdcs]

I'm still not finding what could be causing the test_qos_simple failure. Near as I can guess, it might be failing because the other tests are failing. Maybe.

@emersonknapp can I get another CI on this when you get a chance? Hopefully the updates to the windows testing code should fix it.

[Karsten1987]

A new round of CI:

• Linux
• Linux-aarch64
• macOS
• Windows

@emersonknapp can you take a look at this once more?

[emersonknapp]

Looks good to me!