PlayerClock initial implementation - Player functionally unchanged by emersonknapp · Pull Request #689 · ros2/rosbag2

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…lper functions to match design Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…nts to clock setup and move some logic out into helper function Signed-off-by: Emerson Knapp <eknapp@amazon.com>

…implement - makes it obvious how SimTimeClock will be implemented Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

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

Linux
Linux-aarch64
macOS
Windows

…nings Signed-off-by: Emerson Knapp <eknapp@amazon.com>

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

* Design for rosbag2 handling of clock/simtime (ros2#675) * Add a design dock for rosbag2 handling of clock/simtime Signed-off-by: Emerson Knapp <eknapp@amazon.com> * 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 <eknapp@amazon.com> * /clock publisher in Player (ros2#695) * Add /clock publishing to Player Signed-off-by: Emerson Knapp <eknapp@amazon.com> * fix for foxy abi Signed-off-by: mitsudome-r <ryohsuke.mitsudome@tier4.jp> * fix bug Signed-off-by: mitsudome-r <ryohsuke.mitsudome@tier4.jp> Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>

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 Karsten1987, piraka9011 and thomas-moulard 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 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 <eknapp@amazon.com>

Return double for get_rate

3d3c9c8

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Rename RealTime to SteadyTime

8b3a48a

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

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

67fe510

…lper functions to match design Signed-off-by: Emerson Knapp <eknapp@amazon.com>

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 <eknapp@amazon.com>

Make PlayerClock a pure virtual interface for TimeControllerClock to …

bb59ff7

…implement - makes it obvious how SimTimeClock will be implemented Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Make sure to check rclcpp::ok while looping

20f37f3

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

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 <eknapp@amazon.com>

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

Conversation

emersonknapp commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emersonknapp commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jikawa-az left a comment

Choose a reason for hiding this comment

Uh oh!

emersonknapp commented Mar 31, 2021

Uh oh!

david-prody left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emersonknapp commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emersonknapp commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emersonknapp commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karsten1987 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emersonknapp commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emersonknapp commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

MichaelOrlov Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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