PlayerClock initial implementation - Player functionally unchanged #689

emersonknapp · 2021-03-24T20:30:57Z

Related Issues

Design: Design for rosbag2 handling of clock/simtime #675
Design extension: Add time conversion logic diagrams to time control design #700
Part of play - publish /clock topic #99
Part of [Epic] Rosbag2 playback time control and simulation time #696
Depends on Fix and clarify logic in test_play filter test #690 (fixes incorrect test)

Description

Adds new class PlayerClock with now() and sleep_until() with simple rate handling.
Adds tests for class
Removes time handling from Player in favor of configuring and using PlayerClock

Fixes #99 Design: #675 Depends on #689 Depends on #693 to expose to the CLI Add a `rosgraph_msgs/Clock` publisher to the `Player` - that uses `PlayerClock::now()` to publish current time. Signed-off-by: Emerson Knapp <[email protected]>

rosbag2_cpp/src/rosbag2_cpp/player_clock.cpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_cpp/src/rosbag2_cpp/player_clock.cpp

emersonknapp · 2021-03-26T17:48:39Z

Cross-platform sanity check

Gist: https://gist.githubusercontent.com/emersonknapp/56fa49315468fd07d734740da7975b4a/raw/b85678a7e3040cb2fef7dbccb3e6133c1431ee56/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests ros2bag rosbag2_py
TEST args: --packages-select rosbag2_transport rosbag2_tests ros2bag rosbag2_py
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8036

Linux
Linux-aarch64
macOS
Windows

Note: rclcpp warnings unrelated and should be fixed by ros2/rclcpp#1608

jikawa-az

Thanks for the documentation, diagrams, and test cases!

Fixes #99 Design: #675 Depends on #689 Depends on #693 to expose to the CLI Add a `rosgraph_msgs/Clock` publisher to the `Player` - that uses `PlayerClock::now()` to publish current time. Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-03-31T23:21:04Z

Gist: https://gist.githubusercontent.com/emersonknapp/6fb38f4384ee9db70d518f601711591a/raw/e3aa6275de0ca40c21d1a6860083e814c486933c/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2_py ros2bag rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2_py ros2bag rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8062

Linux
Linux-aarch64
macOS
Windows

david-prody

API looks good. my main question is how scenerio 3 from the design would be set up using this class.

rosbag2_cpp/include/rosbag2_cpp/player_clock.hpp

rosbag2_cpp/test/rosbag2_cpp/test_player_clock.cpp

rosbag2_transport/src/rosbag2_transport/player.cpp

emersonknapp · 2021-04-01T08:08:19Z

Linux
Linux-aarch64
macOS
Windows

Create new PlayerClock class with `now()`, `sleep_until()`, and rate handling, with tests. Removes time handling from `Player` in favor of using this new class. Signed-off-by: Emerson Knapp <[email protected]>

Signed-off-by: Emerson Knapp <[email protected]>

…lper functions to match design Signed-off-by: Emerson Knapp <[email protected]>

…nts to clock setup and move some logic out into helper function Signed-off-by: Emerson Knapp <[email protected]>

…implement - makes it obvious how SimTimeClock will be implemented Signed-off-by: Emerson Knapp <[email protected]>

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-04-02T19:07:31Z

Linux
Linux-aarch64
macOS
Windows

…nings Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-04-02T22:08:55Z

Windows: (all build warnings resolved - yellow now only for the Cyclone CMake warning)

Karsten1987

Generally lgtm, but I am finding the typedefs a bit wild. I'd actually stick to their original types if we don't plan on evolving them further into their own structs.

rosbag2_cpp/CMakeLists.txt

rosbag2_cpp/include/rosbag2_cpp/clocks/player_clock.hpp

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp

rosbag2_cpp/test/rosbag2_cpp/test_time_controller_clock.cpp

rosbag2_storage/include/rosbag2_storage/serialized_bag_message.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp

* Remove time point typedefs in favor of using the types directly. * Use condition_variable for waiting * Minor formatting updates Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-04-02T23:28:24Z

@Karsten1987 thanks for the feedback - I think it put a nice finishing touch on this diff. Now, fingers crossed for a green result :)

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp · 2021-04-03T01:17:23Z

Fixed float->double conversion warning

Windows:

^ Green - cmake warning is eclipse-cyclonedds/cyclonedds#741

MichaelOrlov · 2021-04-03T01:56:36Z

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp

+    return reference.steady + std::chrono::nanoseconds(diff_nanos);
+  }
+
+  std::mutex mutex;


It is bad practice to use names reserved in C++. It would be more safe if rename variable mutex .
Maybe ref_rate_mutex or time_control_mutex ?

MichaelOrlov · 2021-04-03T02:03:11Z

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp

+
+  std::mutex mutex;
+  std::condition_variable cv;
+  PlayerClock::NowFunction now_fn RCPPUTILS_TSA_GUARDED_BY(mutex);


If I am understand correctly we are going to assign now_fn only in constructor. In this case strict requirement to lock mutex is extra and not required.

MichaelOrlov · 2021-04-03T03:07:56Z

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp

+    return std::chrono::duration_cast<std::chrono::nanoseconds>(duration).count();
+  }
+
+  rcutils_time_point_value_t steady_to_ros(std::chrono::steady_clock::time_point steady_time)


@emersonknapp It would be nice to add REQUIRES(mutex) attribute to steady_to_ros function.
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#getting-started
However I am not sure if it possible to do with rclcpp abstraction layer.

MichaelOrlov · 2021-04-03T03:08:27Z

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp

+  }
+
+  std::chrono::steady_clock::time_point ros_to_steady(rcutils_time_point_value_t ros_time)
+  {


Similar. It would be nice to add REQUIRES(mutex) attribute to ros_to_steady function.

…os2#689) * Initial PlayerClock integration - functionality unchanged * Create PlayerClock, a pure virtual interface with `now()`, and `sleep_until()` for Player to use to control timing of message playback * TimeControllerClock implementation of PlayerClock * Removes time handling from `Player` in favor of using this new class Signed-off-by: Emerson Knapp <[email protected]>

* Design for rosbag2 handling of clock/simtime (ros2#675) * Add a design dock for rosbag2 handling of clock/simtime Signed-off-by: Emerson Knapp <[email protected]> * PlayerClock initial implementation - Player functionally unchanged (ros2#689) * Initial PlayerClock integration - functionality unchanged * Create PlayerClock, a pure virtual interface with `now()`, and `sleep_until()` for Player to use to control timing of message playback * TimeControllerClock implementation of PlayerClock * Removes time handling from `Player` in favor of using this new class Signed-off-by: Emerson Knapp <[email protected]> * /clock publisher in Player (ros2#695) * Add /clock publishing to Player Signed-off-by: Emerson Knapp <[email protected]> * fix for foxy abi Signed-off-by: mitsudome-r <[email protected]> * fix bug Signed-off-by: mitsudome-r <[email protected]> Co-authored-by: Emerson Knapp <[email protected]>

emersonknapp marked this pull request as ready for review March 25, 2021 00:19

emersonknapp requested a review from a team as a code owner March 25, 2021 00:19

emersonknapp requested review from thomas-moulard, piraka9011 and Karsten1987 and removed request for a team and thomas-moulard March 25, 2021 00:19

emersonknapp force-pushed the emersonknapp/player-clock branch 2 times, most recently from ef47e0e to 06398ad Compare March 25, 2021 01:00

emersonknapp requested a review from jikawa-az March 25, 2021 01:03

emersonknapp force-pushed the emersonknapp/player-clock branch from 06398ad to 3049f1a Compare March 25, 2021 18:31

This was referenced Mar 26, 2021

/clock publisher in Player #695

Merged

[Epic] Rosbag2 playback time control and simulation time #696

Closed

MichaelOrlov reviewed Mar 26, 2021

View reviewed changes

emersonknapp force-pushed the emersonknapp/player-clock branch from 6fe7642 to c5a0c06 Compare March 26, 2021 17:46

This was referenced Mar 30, 2021

Use lifecycle nodes for playback - implements mute/unmute #666

Closed

Add time conversion logic diagrams to time control design #700

Merged

emersonknapp force-pushed the emersonknapp/player-clock branch from 261fd11 to f06836e Compare March 30, 2021 20:22

jikawa-az approved these changes Mar 30, 2021

View reviewed changes

emersonknapp requested review from david-prody and removed request for piraka9011 March 30, 2021 21:00

emersonknapp force-pushed the emersonknapp/player-clock branch from f06836e to bc24c53 Compare March 31, 2021 18:15

emersonknapp mentioned this pull request Mar 31, 2021

Pause/resume PlayerClock #704

Merged

david-prody reviewed Apr 1, 2021

View reviewed changes

rosbag2_cpp/include/rosbag2_cpp/player_clock.hpp Outdated Show resolved Hide resolved

rosbag2_cpp/test/rosbag2_cpp/test_player_clock.cpp Outdated Show resolved Hide resolved

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved

emersonknapp requested a review from david-prody April 1, 2021 07:10

emersonknapp force-pushed the emersonknapp/player-clock branch from da412e2 to 5e1012c Compare April 1, 2021 20:50

Emerson Knapp added 7 commits April 2, 2021 00:59

Initial PlayerClock integration - functionality unchanged

59b6bff

Create new PlayerClock class with `now()`, `sleep_until()`, and rate handling, with tests. Removes time handling from `Player` in favor of using this new class. Signed-off-by: Emerson Knapp <[email protected]>

Return double for get_rate

3d3c9c8

Signed-off-by: Emerson Knapp <[email protected]>

Rename RealTime to SteadyTime

8b3a48a

Signed-off-by: Emerson Knapp <[email protected]>

Refactor to say ROSTime instead of PlayerTime, add time conversion he…

67fe510

…lper functions to match design Signed-off-by: Emerson Knapp <[email protected]>

Address review comments: clarify test variables, add clarifying comme…

6f7466d

…nts to clock setup and move some logic out into helper function Signed-off-by: Emerson Knapp <[email protected]>

Make PlayerClock a pure virtual interface for TimeControllerClock to …

bb59ff7

…implement - makes it obvious how SimTimeClock will be implemented Signed-off-by: Emerson Knapp <[email protected]>

Make sure to check rclcpp::ok while looping

20f37f3

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the emersonknapp/player-clock branch from 5e1012c to 20f37f3 Compare April 2, 2021 07:59

Move TimeControllerClock into separate file

3173b87

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the emersonknapp/player-clock branch from 05e2252 to 3173b87 Compare April 2, 2021 19:18

Explicitly cast from double to rmw_duration_value_t - fix windows war…

bffc11e

…nings Signed-off-by: Emerson Knapp <[email protected]>

Karsten1987 reviewed Apr 2, 2021

View reviewed changes

Karsten1987 approved these changes Apr 2, 2021

View reviewed changes

rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp Outdated Show resolved Hide resolved

emersonknapp force-pushed the emersonknapp/player-clock branch from 550d432 to 08e530d Compare April 2, 2021 23:14

Address review comments from Karsten

daadf5b

* Remove time point typedefs in favor of using the types directly. * Use condition_variable for waiting * Minor formatting updates Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the emersonknapp/player-clock branch from 08e530d to daadf5b Compare April 2, 2021 23:27

Fix windows warning by initializing to double

b668a92

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp merged commit 9ed9102 into master Apr 3, 2021

delete-merged-branch bot deleted the emersonknapp/player-clock branch April 3, 2021 02:42

MichaelOrlov reviewed Apr 3, 2021

View reviewed changes

emersonknapp mentioned this pull request Apr 5, 2021

Enable thread safety analysis for rosbag2_cpp and add annotations in TimeControllerClock #710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PlayerClock initial implementation - Player functionally unchanged #689

PlayerClock initial implementation - Player functionally unchanged #689

emersonknapp commented Mar 24, 2021 •

edited

Loading

emersonknapp commented Mar 26, 2021 •

edited

Loading

jikawa-az left a comment

emersonknapp commented Mar 31, 2021

david-prody left a comment

emersonknapp commented Apr 1, 2021 •

edited

Loading

emersonknapp commented Apr 2, 2021 •

edited

Loading

emersonknapp commented Apr 2, 2021 •

edited

Loading

Karsten1987 left a comment

emersonknapp commented Apr 2, 2021 •

edited

Loading

emersonknapp commented Apr 3, 2021 •

edited

Loading

MichaelOrlov Apr 3, 2021

MichaelOrlov Apr 3, 2021

MichaelOrlov Apr 3, 2021

MichaelOrlov Apr 3, 2021

PlayerClock initial implementation - Player functionally unchanged #689

PlayerClock initial implementation - Player functionally unchanged #689

Conversation

emersonknapp commented Mar 24, 2021 • edited Loading

Related Issues

Description

emersonknapp commented Mar 26, 2021 • edited Loading

jikawa-az left a comment

Choose a reason for hiding this comment

emersonknapp commented Mar 31, 2021

david-prody left a comment

Choose a reason for hiding this comment

emersonknapp commented Apr 1, 2021 • edited Loading

emersonknapp commented Apr 2, 2021 • edited Loading

emersonknapp commented Apr 2, 2021 • edited Loading

Karsten1987 left a comment

Choose a reason for hiding this comment

emersonknapp commented Apr 2, 2021 • edited Loading

emersonknapp commented Apr 3, 2021 • edited Loading

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

emersonknapp commented Mar 24, 2021 •

edited

Loading

emersonknapp commented Mar 26, 2021 •

edited

Loading

emersonknapp commented Apr 1, 2021 •

edited

Loading

emersonknapp commented Apr 2, 2021 •

edited

Loading

emersonknapp commented Apr 2, 2021 •

edited

Loading

emersonknapp commented Apr 2, 2021 •

edited

Loading

emersonknapp commented Apr 3, 2021 •

edited

Loading