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.

Sign up for GitHub

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

Jump to bottom

Add jump/seek API for Player class #826

Merged
merged 10 commits into from
Sep 10, 2021
Merged

Add jump/seek API for Player class #826

merged 10 commits into from
Sep 10, 2021

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov changed the title Add jump/seek API for the Player class Add jump/seek API for Player class Jul 28, 2021
@MichaelOrlov
Protect reader with mutex
016f2d7 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Add seek API to the Player class
e943710 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov force-pushed the michael_orlov/add-seek-api-to-the-player-class branch from 3cef05b to e943710 Compare July 31, 2021 00:51
@emersonknapp emersonknapp self-requested a review August 3, 2021 23:07
@MichaelOrlov
Add unit tests for player seek API with mock reader
addbe0a 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Fix copyright in test_play_next.cpp
ececda4 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Modify unit tests to cover cases when message queue size smaller than…
7280b2a 
… number of messages in bag.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Move mock_player to the separate header file
b7639cb 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Segregate repeating variables in unit tests to the RosBag2PlaySeekTes…
2e54c85 
…tFixture class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Add checks in unit tests for seek backward and forward with exact tim…
3f7169a 
…estamp

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov marked this pull request as ready for review August 13, 2021 07:59
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner August 13, 2021 07:59
@MichaelOrlov MichaelOrlov requested review from zmichaels11 and Karsten1987 and removed request for a team August 13, 2021 07:59
@MichaelOrlov
Copy link
Contributor Author

I am considering to add one more unit test and probably switch to the real rosbag2 file instead of mock reader.

@MichaelOrlov
Properly handle seek operation from the end of the bag and add unit t…
fa260d3 
…ests for these cases

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Replace MockReader with real bag for tests
07e33e9 
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov force-pushed the michael_orlov/add-seek-api-to-the-player-class branch from ac3d8e1 to 07e33e9 Compare August 17, 2021 09:34
@MichaelOrlov
Copy link
Contributor Author

Ready for review.

@emersonknapp
Copy link
Collaborator

Thanks for the update. I was on vacation last week but will review it this week

emersonknapp
emersonknapp approved these changes Aug 31, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Overall this looks fine - a few non-blocking comments that you can address if you like

rosbag2_transport/src/rosbag2_transport/player.cpp Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Show resolved Hide resolved
@MichaelOrlov
Copy link
Contributor Author

Gist:
https://gist.githubusercontent.com/MichaelOrlov/ad810b7bb85445a829ac57a03dcc14c5/raw/f87ec0131d7459819d794fa0f9ec69a0f161c505/ros2.repos
BUILD args: --packages-up-to rosbag2_transport
TEST args: --packages-select rosbag2_transport
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8969

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

@MichaelOrlov
Copy link
Contributor Author

@emersonknapp CI fails with errors in rmw_cyclonedds_cpp package which I didn't touch.

--- stderr: rmw_cyclonedds_cpp
19:52:53 /home/jenkins-agent/workspace/ci_linux-aarch64/ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp: In function ‘CddsPublisher* create_cdds_publisher(dds_entity_t, dds_entity_t, const rosidl_message_type_support_t*, const char*, const rmw_qos_profile_t*)’:
19:52:53 /home/jenkins-agent/workspace/ci_linux-aarch64/ws/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:2039:22: error: ‘dds_is_loan_available’ was not declared in this scope; did you mean ‘is_loan_available’?
19:52:53  2039 |     is_fixed_type && dds_is_loan_available(pub->enth);
19:52:53       |                      ^~~~~~~~~~~~~~~~~~~~~
19:52:53       |                      is_loan_available

Should I modify ros2.repos to refer to a some specific version of the cyclonedds?

@emersonknapp
Copy link
Collaborator

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

@MichaelOrlov
Copy link
Contributor Author

Rerunning Windows build. It seems some unrelevant issue that CI script can't delete temp file from TEMP folder.

  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit ef40da9 into master Sep 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the michael_orlov/add-seek-api-to-the-player-class branch September 10, 2021 18:38
WJaworskiRobotec pushed a commit to RobotecAI/rosbag2 that referenced this pull request Sep 12, 2021
@MichaelOrlov @WJaworskiRobotec
Add jump/seek API for Player class (ros2#826)
b966ddb 
* Protect reader with mutex

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add seek API to the Player class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add unit tests for player seek API with mock reader

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Fix copyright in test_play_next.cpp

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Modify unit tests to cover cases when message queue size smaller than number of messages in bag.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Move mock_player to the separate header file

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Segregate repeating variables in unit tests to the RosBag2PlaySeekTestFixture class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add checks in unit tests for seek backward and forward with exact timestamp

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Properly handle seek operation from the end of the bag and add unit tests for these cases

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Replace MockReader with real bag for tests

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Wojciech Jaworski <wojciech.jaworski@robotec.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@zmichaels11 zmichaels11 Awaiting requested review from zmichaels11

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add jump/seek API for the Player class
2 participants
@MichaelOrlov @emersonknapp