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

Add recorder stop() API #1300

Merged
merged 4 commits into from
May 3, 2023
Merged

Conversation

MichaelOrlov
Copy link
Contributor

MichaelOrlov and others added 3 commits April 19, 2023 20:46
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: aditya <aditya@nimble.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov MichaelOrlov marked this pull request as ready for review April 20, 2023 07:03
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner April 20, 2023 07:03
@MichaelOrlov MichaelOrlov requested review from emersonknapp, hidmic and james-rms and removed request for a team April 20, 2023 07:03
@MichaelOrlov MichaelOrlov changed the title Add recorder close API Add recorder stop() API Apr 20, 2023
@james-rms
Copy link
Contributor

james-rms commented Apr 20, 2023

  • What happens to discovery state between stop() and record()? do the full set of topics from before closing get written to the new bag file?
  • I also wonder if we need pause at all. What if instead of adding stop() we just changed pause to close the bag?
  • We should also update rosbag2_py to wrap these methods.

@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented Apr 20, 2023

@james-rms I will try to answer on your questions:

  • What happens to discovery state between stop() and record()? do the full set of topics from before closing get written to the new bag file?

Discovery will be stopped and recreated with all subscriptions. If topics currently discoverable it will be added to the new bag if not it is not. Consider stop() operation as opposite to the record() operation. stop() will wrap everything up and ready for one more call for record().

  • I also wonder if we need pause at all. What if instead of adding stop() we just changed pause to close the bag?

In current design we need stop(), pause() and resume(). Pause mode just dropping messages from subscriptions. The stop() really stop recording and wrapping everything up. There are lot more above pause() and resume() API and it would be a major redesign in recorder by remapping them to the stop() and record() calls.

  • We should also update rosbag2_py to wrap these methods.

Sorry I missed it. I will add.
Update: Apparently there are nothing to wrap in rosbag2_py. We are not exposing rosbag2_cpp::writer class in python since it is cpp version nor exposing rosbag2_transport::recorder where relevant stop() API was added. Because we have rosbag2_py::Recorder with it's own business logic above rosbag2_transport::recorder and stop() API doesn't fit to it.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/52b8a6060eacf2b928a2eca23185c85d/raw/d2ec5fdbe4478354c04fabd5d70ccaee677623a9/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport rosbag2_cpp
TEST args: --packages-above rosbag2_transport rosbag2_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11981

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

@MichaelOrlov
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@MichaelOrlov MichaelOrlov merged commit a218e37 into rolling May 3, 2023
@delete-merged-branch delete-merged-branch bot deleted the morlov/add_recorder_close_api branch May 3, 2023 20:23
@MichaelOrlov
Copy link
Contributor Author

https://github.com/Mergifyio backport iron

@mergify
Copy link

mergify bot commented May 16, 2023

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 16, 2023
* Expose Writer::close() as public API and add Recorder::stop() API

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: aditya <aditya@nimble.ai>

* Move routines from recorder's destructor to RecorderImpl::stop()

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

* Add can_record_again_after_stop unit test in rosbag2_transport

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

* Update Doxygen comments for stop() and pause() API in recorder.hpp

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

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: aditya <aditya@nimble.ai>
(cherry picked from commit a218e37)
MichaelOrlov added a commit that referenced this pull request May 17, 2023
* Expose Writer::close() as public API and add Recorder::stop() API

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: aditya <aditya@nimble.ai>

* Move routines from recorder's destructor to RecorderImpl::stop()

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

* Add can_record_again_after_stop unit test in rosbag2_transport

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

* Update Doxygen comments for stop() and pause() API in recorder.hpp

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

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: aditya <aditya@nimble.ai>
(cherry picked from commit a218e37)

Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-05-18/31587/1

@MichaelOrlov MichaelOrlov mentioned this pull request Jun 1, 2023
3 tasks
MichaelOrlov added a commit that referenced this pull request Jun 13, 2023
* Expose Writer::close() as public API and add Recorder::stop() API

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: aditya <aditya@nimble.ai>

* Move routines from recorder's destructor to RecorderImpl::stop()

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

* Add can_record_again_after_stop unit test in rosbag2_transport

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

* Update Doxygen comments for stop() and pause() API in recorder.hpp

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
MichaelOrlov added a commit that referenced this pull request Jun 13, 2023
- Add Recorder::stop() API
- Move routines from recorder's destructor to RecorderImpl::stop()
- Add can_record_again_after_stop unit test in rosbag2_transport
- Update Doxygen comments for stop() and pause() API in recorder.hpp

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
emersonknapp pushed a commit that referenced this pull request Jun 13, 2023
- Add Recorder::stop() API
- Move routines from recorder's destructor to RecorderImpl::stop()
- Add can_record_again_after_stop unit test in rosbag2_transport
- Update Doxygen comments for stop() and pause() API in recorder.hpp

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants