When using sim time, wait for /clock before beginning recording

[emersonknapp]

Fixes #1276
Retry at #1354 after reverting

When using sim-time, check that /clock has actually been received before writing any messages into the bag, to avoid the time jump that then happens on first /clock message, given that initial messages are written with a timestamp of 0.
This potentially drops some messages that it could have written, but is more correct, in that those messages are "out of time" and have no temporal standing in simulated time.

[MichaelOrlov]

@emersonknapp While implementation looks good to me, the failure on the record_all_with_sim_time Is a big surprise for me.

20: [ RUN      ] RecordIntegrationTestFixture.record_all_with_sim_time
20: stdin is not a terminal device. Keyboard handling disabled.[INFO] [1686360701.266425264] [rosbag2_recorder]: Press SPACE for pausing/resuming
20: [INFO] [1686360701.268967062] [rosbag2_recorder]: Event publisher thread: Starting
20: [INFO] [1686360701.270181670] [rosbag2_recorder]: Listening for topics...
20: [INFO] [1686360701.270365093] [rosbag2_recorder]: use_sim_time set, waiting for /clock before starting recording...
20: /tmp/ws/src/rosbag2/rosbag2_transport/test/rosbag2_transport/test_record_all_use_sim_time.cpp:64: Failure
20: Value of: pub_manager.wait_for_matched(string_topic.c_str())
20:   Actual: false
20: Expected: true
20: [INFO] [1686360711.281632076] [rosbag2_recorder]: Event publisher thread: Exiting
20: [  FAILED  ] RecordIntegrationTestFixture.record_all_with_sim_time (10034 ms)
20: [----------] 1 test from RecordIntegrationTestFixture (10034 ms total)

It looks like our check for the

if (node->get_clock()->wait_until_started(record_options_.topic_polling_interval))

doesn't work as we expected.

[emersonknapp]

The test failure was a side effect of changing the discovery behavior. The test was treating /clock like any other topic to record.

• Recorder was waiting for first /clock message before creating subscriptions
• Test was waiting for both /clock and /string_topic publishers to have matched subscriptions before publishing messages

The above two conditions created a deadlock. I've rewritten the test to publish /clock constantly, and only worry about recording /string_topic.

[MichaelOrlov]

@emersonknapp LGTM. Thanks for the PR!

[emersonknapp]

Pulls: #1378
Gist: https://gist.githubusercontent.com/emersonknapp/36579bb645001b58e67310f066d1f600/raw/a112d00e3d040cabccbf7c01f10c5bade79bd4bb/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12208

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

@Mergifyio backport iron humble

[mergify]

backport iron humble

✅ Backports have been created

• #1391 When using sim time, wait for /clock before beginning recording (backport #1378) has been created for branch iron
• #1392 When using sim time, wait for /clock before beginning recording (backport #1378) has been created for branch humble but encountered conflicts

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-06-15/32038/1