/clock publisher in Player #695

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>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

/clock publisher in Player #695

/clock publisher in Player #695

emersonknapp commented Mar 26, 2021 •

edited

Loading

Karsten1987 left a comment •

edited

Loading

Karsten1987 commented Apr 6, 2021

emersonknapp commented Apr 6, 2021 •

edited

Loading

emersonknapp commented Apr 6, 2021 •

edited

Loading

Karsten1987 left a comment

emersonknapp commented Apr 7, 2021

emersonknapp commented Apr 7, 2021

emersonknapp commented Apr 7, 2021

emersonknapp commented Apr 7, 2021 •

edited

Loading

emersonknapp commented Apr 7, 2021

/clock publisher in Player #695

/clock publisher in Player #695

Conversation

emersonknapp commented Mar 26, 2021 • edited Loading

Karsten1987 left a comment • edited Loading

Choose a reason for hiding this comment

Karsten1987 commented Apr 6, 2021

emersonknapp commented Apr 6, 2021 • edited Loading

emersonknapp commented Apr 6, 2021 • edited Loading

Karsten1987 left a comment

Choose a reason for hiding this comment

emersonknapp commented Apr 7, 2021

emersonknapp commented Apr 7, 2021

emersonknapp commented Apr 7, 2021

emersonknapp commented Apr 7, 2021 • edited Loading

emersonknapp commented Apr 7, 2021

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