/clock publisher in Player by emersonknapp · Pull Request #695 · ros2/rosbag2

emersonknapp · 2021-03-26T01:39:05Z

Closes #99
Design: #675

Add a rosgraph_msgs/Clock publisher to the Player - that uses PlayerClock::now() to publish current time.

Expose this publish frequency to the CLI

Karsten1987

this generally looks good to me. As mentioned in the comments, I don't think we can so easily just fire up an executor in the play function, but we can address this in a follow up PR.

Testing is indeed a bit tricky, but maybe an end-to-end test in which you test that you get at least a non-zero amount of /clock callbacks and all these timestamps are in the appropriate time range?

rosbag2_transport/include/rosbag2_transport/play_options.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

Karsten1987 · 2021-04-06T22:36:54Z

@emersonknapp can we officially put this up for review and run CI on it? I've also noticed a few ROSBAG2_TRANSPORT_LOG... macros on your branch. Does this branch have to be rebased?

emersonknapp · 2021-04-06T22:44:59Z

Does this branch have to be rebased?

It looks like it!

can we officially put this up for review and run CI on it?

I was just waiting to have a reasonable test - which it looks like I have now. I'll push it in a couple minutes.

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

emersonknapp · 2021-04-06T22:57:20Z

Gist: https://gist.githubusercontent.com/emersonknapp/8785419a3ef05c024b54657b89f76d54/raw/d318e3e26f06cb8534b480ec3dffcf7dcb64e942/ros2.repos
BUILD args: --packages-up-to ros2bag rosbag2_py rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select ros2bag rosbag2_py rosbag2_transport rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8143

Linux
Linux-aarch64
macOS
Windows

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

Karsten1987

lgtm, I just had one remark towards the test. I am ok to move on with the rest.

rosbag2_transport/include/rosbag2_transport/play_options.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/test/rosbag2_transport/test_play_publish_clock.cpp

emersonknapp · 2021-04-07T01:07:13Z

Linux
Linux-aarch64
macOS
Windows

Also check that rate is respected Signed-off-by: Emerson Knapp <eknapp@amazon.com>

emersonknapp · 2021-04-07T02:31:43Z

Not checking received times (weirdly slow on Windows) - instead checking the deltas between the contents of the clock messages, which should be more reliable. Also checking on playback rate being reflected in these values.

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2021-04-07T19:28:11Z

Not sure exactly why this test works so poorly on OSX/Win - trying a OSX build locally to investigate

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

emersonknapp · 2021-04-07T20:58:05Z

Quick directed test

Windows:
OSX:

rosbag2_py/src/rosbag2_py/_transport.cpp

rosbag2_transport/include/rosbag2_transport/play_options.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

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

emersonknapp · 2021-04-07T23:34:30Z

Linux
Linux-aarch64
macOS
Windows

* Add /clock publishing to Player 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 mentioned this pull request Mar 26, 2021

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

Closed

50 tasks

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

emersonknapp mentioned this pull request Mar 31, 2021

Add time conversion logic diagrams to time control design #700

Merged

emersonknapp force-pushed the emersonknapp/player-clock-publish branch from c563f42 to 009c60d Compare March 31, 2021 11:14

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

emersonknapp force-pushed the emersonknapp/player-clock-publish branch 2 times, most recently from 7492fa2 to 9e5ffec Compare April 1, 2021 07:17

emersonknapp changed the title ~~Clock publisher in Player~~ [WIP] /clock publisher in Player Apr 1, 2021

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

emersonknapp force-pushed the emersonknapp/player-clock-publish branch 2 times, most recently from ce6db18 to 4b35b13 Compare April 2, 2021 21:58

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

emersonknapp force-pushed the emersonknapp/player-clock-publish branch 2 times, most recently from e69696b to 478285a Compare April 3, 2021 01:14

delete-merged-branch bot deleted the branch master April 3, 2021 02:42

emersonknapp force-pushed the emersonknapp/player-clock-publish branch 2 times, most recently from 9eb778e to 2001d5f Compare April 5, 2021 22:05

emersonknapp changed the base branch from emersonknapp/player-clock to emersonknapp/playerclock-thread-safety-tweaks April 5, 2021 22:06

emersonknapp force-pushed the emersonknapp/player-clock-publish branch from 2001d5f to daa6f04 Compare April 5, 2021 22:09

emersonknapp force-pushed the emersonknapp/playerclock-thread-safety-tweaks branch from cb3386f to e2ab944 Compare April 5, 2021 23:50

delete-merged-branch bot deleted the branch master April 6, 2021 01:22

emersonknapp force-pushed the emersonknapp/player-clock-publish branch from daa6f04 to 55502f9 Compare April 6, 2021 01:29

emersonknapp changed the base branch from emersonknapp/playerclock-thread-safety-tweaks to emersonknapp/player-clock-max-spin April 6, 2021 01:32

emersonknapp force-pushed the emersonknapp/player-clock-max-spin branch 4 times, most recently from 9e76353 to 8f21997 Compare April 6, 2021 02:48

emersonknapp force-pushed the emersonknapp/player-clock-publish branch 2 times, most recently from cad4618 to bddf622 Compare April 6, 2021 18:22

emersonknapp removed the request for review from piraka9011 April 6, 2021 20:52

Karsten1987 reviewed Apr 6, 2021

View reviewed changes

Emerson Knapp added 2 commits April 6, 2021 15:50

Add /clock publishing to Player

2cb7131

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

Add a test for clock publishing

a37da03

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

emersonknapp force-pushed the emersonknapp/player-clock-publish branch from 302a763 to a37da03 Compare April 6, 2021 22:50

emersonknapp marked this pull request as ready for review April 6, 2021 22:50

Fix linters

5c36ca2

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

Karsten1987 approved these changes Apr 6, 2021

View reviewed changes

rosbag2_transport/include/rosbag2_transport/play_options.hpp Outdated Show resolved Hide resolved

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

rosbag2_transport/test/rosbag2_transport/test_play_publish_clock.cpp Outdated Show resolved Hide resolved

emersonknapp force-pushed the emersonknapp/player-clock-publish branch 2 times, most recently from 53c111b to 6faa696 Compare April 7, 2021 01:06

More flexible test for clock publishing

7a896d5

Also check that rate is respected Signed-off-by: Emerson Knapp <eknapp@amazon.com>

emersonknapp force-pushed the emersonknapp/player-clock-publish branch from 6faa696 to 7a896d5 Compare April 7, 2021 02:30

Tweak test

b23492b

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

MichaelOrlov requested changes Apr 7, 2021

View reviewed changes

Emerson Knapp added 3 commits April 7, 2021 15:10

CLI optional freq for clock pub

131f19d

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

Move spinner out to rosbag2_transport

3d1a94a

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

Loosen the tests for buildfarm windows

a2823fd

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

emersonknapp merged commit 07c05da into master Apr 8, 2021

delete-merged-branch bot deleted the emersonknapp/player-clock-publish branch April 8, 2021 00:27

Kettenhoax pushed a commit to Kettenhoax/rosbag2 that referenced this pull request Apr 9, 2021

/clock publisher in Player (ros2#695)

ca5c1e5

* Add /clock publishing to Player Signed-off-by: Emerson Knapp <eknapp@amazon.com>

mitsudome-r pushed a commit to mitsudome-r/rosbag2 that referenced this pull request Apr 10, 2021

/clock publisher in Player (ros2#695)

628cd00

* Add /clock publishing to Player Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Conversation

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

Uh oh!

Uh oh!

Karsten1987 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Karsten1987 commented Apr 6, 2021

Uh oh!

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

Uh oh!

Uh oh!

emersonknapp commented Apr 6, 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!

emersonknapp commented Apr 7, 2021

Uh oh!

emersonknapp commented Apr 7, 2021

Uh oh!

emersonknapp commented Apr 7, 2021

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emersonknapp commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emersonknapp commented Mar 26, 2021 •

edited

Loading

Karsten1987 left a comment •

edited

Loading

emersonknapp commented Apr 6, 2021 •

edited

Loading

emersonknapp commented Apr 6, 2021 •

edited

Loading

emersonknapp commented Apr 7, 2021 •

edited

Loading