Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport/Humble] Notification of significant events during bag recording and playback #1037

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

gbiggs
Copy link
Member

@gbiggs gbiggs commented Jul 4, 2022

Backports #908 to Humble.

…#908)

* Simple hacky prototype

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Improved prototype with more flexibility and event information

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Use the full relative file path for event info

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Remove no-longer required dependency

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Remove left-over merge

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Change event names

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add overrides to mocks

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add missing include

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Remove empty file

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Remove unnecessary virtual

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Correct formatting

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add missing doc

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Reformat documentation

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Change read event topic name

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Change read event topic name

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Simplify for loop

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Remove unnecessary blank line

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Change argument type to const

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add events handling to mocks

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add comment explaining 5-message split

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add integration tests for player and recorder

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add ability to check if an event has callbacks

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Shift event topic publishing to a separate thread

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Fix formatting

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Test SequentialReader event callbacks

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Test SequentialWriter event callbacks

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Document bag events

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Check if joinable before joining

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Correct spelling mistake

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Correct spelling mistake

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Reduce time wasted waiting for test to reach intended behaviour

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Address test flakiness

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Improve size check

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Use constant for split size in tests

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add missing header

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Allocate thread on the stack

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add getters for number of messages per 'file'

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Replace magic number

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Address unstable OSX build

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Make path in test cross-platform

Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Add missing virtual destructor

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Follow uncrustify suggestion

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Fix rebase errors

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Remove double include

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
@gbiggs gbiggs added the enhancement New feature or request label Jul 4, 2022
@gbiggs gbiggs requested a review from a team as a code owner July 4, 2022 22:56
@gbiggs gbiggs self-assigned this Jul 4, 2022
@gbiggs gbiggs requested review from MichaelOrlov and hidmic and removed request for a team July 4, 2022 22:56
@MichaelOrlov MichaelOrlov requested a review from audrow July 5, 2022 17:32
@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/6c1703180c5770c61d51d01a7b4f6539/raw/8c7b6492858213d297f35d2374807d7b894effaa/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_interfaces rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-above rosbag2_cpp rosbag2_interfaces rosbag2_transport rosbag2_tests rosbag2
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10474

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gbiggs Looks good to me.

@audrow This is new feature which we would like to backport to the Humble.
I've reviewed it. And I don't see any breaking API or ABI changes. We only adding new API.

@audrow
Copy link
Member

audrow commented Jul 6, 2022

Looks good to me. Are the PR job failures expected?

@MichaelOrlov
Copy link
Contributor

Looks good to me. Are the PR job failures expected?

Yes. It's known issue #862

@MichaelOrlov
Copy link
Contributor

@audrow Can we merge this PR now?

@audrow
Copy link
Member

audrow commented Jul 14, 2022

Great, one more CI run, just to see if anything has changed - if this is green, I'll merge it:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

@audrow Can we merge it now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants