When using sim time, wait for /clock before beginning recording #1378
Conversation
There was a problem hiding this comment.
@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.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
…ry after stop() Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
8d5624b to
786f648
Compare
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
|
The test failure was a side effect of changing the discovery behavior. The test was treating
The above two conditions created a deadlock. I've rewritten the test to publish |
|
Pulls: #1378
|
|
@Mergifyio backport iron humble |
✅ Backports have been created
|
* When using sim time, wait for /clock before beginning recording * Disable use-sim-time && no-discovery combo, allow re-enabling discovery after stop() Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit d52ddbb)
* When using sim time, wait for /clock before beginning recording * Disable use-sim-time && no-discovery combo, allow re-enabling discovery after stop() Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit d52ddbb) # Conflicts: # ros2bag/ros2bag/verb/record.py # rosbag2_transport/src/rosbag2_transport/recorder.cpp
|
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 |
Fixes #1276
Retry at #1354 after reverting
When using sim-time, check that
/clockhas actually been received before writing any messages into the bag, to avoid the time jump that then happens on first/clockmessage, 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.