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 <[email protected]>

* Improved prototype with more flexibility and event information

Signed-off-by: Geoffrey Biggs <[email protected]>

* Use the full relative file path for event info

Signed-off-by: Geoffrey Biggs <[email protected]>

* Remove no-longer required dependency

Signed-off-by: Geoffrey Biggs <[email protected]>

* Remove left-over merge

Signed-off-by: Geoffrey Biggs <[email protected]>

* Change event names

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add overrides to mocks

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add missing include

Signed-off-by: Geoffrey Biggs <[email protected]>

* Remove empty file

Signed-off-by: Geoffrey Biggs <[email protected]>

* Remove unnecessary virtual

Signed-off-by: Geoffrey Biggs <[email protected]>

* Correct formatting

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add missing doc

Signed-off-by: Geoffrey Biggs <[email protected]>

* Reformat documentation

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Change read event topic name

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Change read event topic name

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Simplify for loop

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Remove unnecessary blank line

Signed-off-by: Geoffrey Biggs <[email protected]>

* Change argument type to const

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add events handling to mocks

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add comment explaining 5-message split

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add integration tests for player and recorder

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add ability to check if an event has callbacks

Signed-off-by: Geoffrey Biggs <[email protected]>

* Shift event topic publishing to a separate thread

Signed-off-by: Geoffrey Biggs <[email protected]>

* Fix formatting

Signed-off-by: Geoffrey Biggs <[email protected]>

* Test SequentialReader event callbacks

Signed-off-by: Geoffrey Biggs <[email protected]>

* Test SequentialWriter event callbacks

Signed-off-by: Geoffrey Biggs <[email protected]>

* Document bag events

Signed-off-by: Geoffrey Biggs <[email protected]>

* Check if joinable before joining

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Correct spelling mistake

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Correct spelling mistake

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Reduce time wasted waiting for test to reach intended behaviour

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Address test flakiness

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Improve size check

Co-authored-by: Michael Orlov <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Use constant for split size in tests

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add missing header

Signed-off-by: Geoffrey Biggs <[email protected]>

* Allocate thread on the stack

Signed-off-by: Geoffrey Biggs <[email protected]>

* Add getters for number of messages per 'file'

Signed-off-by: Geoffrey Biggs <[email protected]>

* Replace magic number

Signed-off-by: Geoffrey Biggs <[email protected]>

* Address unstable OSX build

Signed-off-by: Geoffrey Biggs <[email protected]>

* Make path in test cross-platform

Co-authored-by: Emerson Knapp <[email protected]>
Signed-off-by: Geoffrey Biggs <[email protected]>

* Add missing virtual destructor

Signed-off-by: Geoffrey Biggs <[email protected]>

* Follow uncrustify suggestion

Signed-off-by: Geoffrey Biggs <[email protected]>

* Fix rebase errors

Signed-off-by: Geoffrey Biggs <[email protected]>

* Remove double include

Signed-off-by: Geoffrey Biggs <[email protected]>

Co-authored-by: Michael Orlov <[email protected]>
Co-authored-by: Emerson Knapp <[email protected]>
@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