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

Recording - limited buffer with triggerable snapshot #663

Closed
d-walsh opened this issue Feb 18, 2021 · 11 comments · Fixed by #851
Closed

Recording - limited buffer with triggerable snapshot #663

d-walsh opened this issue Feb 18, 2021 · 11 comments · Fixed by #851
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@d-walsh
Copy link

d-walsh commented Feb 18, 2021

Description

The snapshot capability was added to the ROS1 version of rosbag and it would be great to support this use case with ROS2. https://github.com/ros/rosbag_snapshot

Feature high level:

  • Don't record to disk, instead keep a full buffer of a configurable size
  • On a trigger (probably a Service call), dump the contents of the buffer to disk

This allows for capturing data around events of interest without having to store the data for an entire recording duration, saving on disk usage for resource limited use case.

Implementation Suggestions

Testing Suggestions

@d-walsh d-walsh added the enhancement New feature or request label Feb 18, 2021
@emersonknapp
Copy link
Collaborator

Really cool feature 👍

@emersonknapp emersonknapp added the help wanted Extra attention is needed label Feb 18, 2021
@emersonknapp
Copy link
Collaborator

Would like to have more details of the feature on this ticket itself. Putting to bottom of backlog for now.

@emersonknapp emersonknapp changed the title Support rosbag snapshot Recording - limited buffer with triggerable snapshot Mar 19, 2021
@kielczykowski
Copy link

Hey, it feels like i can give a hand with implementing this feature.

Would it be more suitable to open PR and new discussion there or continue designing solution in this thread?

@emersonknapp
Copy link
Collaborator

emersonknapp commented Apr 14, 2021

As with most features of any significant weight, I'd prefer a design proposal first before looking at an implementation (feels slower - but slow is smooth and smooth is fast). You could open up a PR with a new document in the folder https://github.com/ros2/rosbag2/tree/master/docs/design - then we could tie this feature request ticket to that design.

There's room for flexibility, of course, if you think it's faster to throw up a sample implementation than to write a small doc - but generally I find it easier to review the doc, that way any disagreement over definitions or behaviors can be hashed out before a lot of time is lost on making it work well enough for a proof-of-concept.

Luckily, we're getting to a place where it should be a lot easier to implement something like this. The Recorder has been converted to be a rclcpp::Node which will make it easier to expose new APIs via services (I am currently implementing the same idea for playback control on Player)

In a design, I would be looking for specifics on expected user inputs / disk outputs. It doesn't have to focus too heavily on the exact implementation details, but pseudocode API or diagrams with an idea of how they tie to existing structures would help make the review go smoothly. Feel free to join the Tooling WG meetings if you want to have open discussion.

@JWhitleyWork
Copy link

@d-walsh Sorry for resurrecting this zombie thread but please see https://github.com/gaia-platform/rosbag2_snapshot.

@emersonknapp
Copy link
Collaborator

I'll also note that this feature now exists in the core - which you can observe in the above merged PRs. I'd love to see any diff between that feature and your package contributed to the core, so that you don't have to maintain a custom wrapper.

@JWhitleyWork
Copy link

@emersonknapp I am apparently blind but thanks for pointing this out! Is there documentation for usage?

@emersonknapp
Copy link
Collaborator

You know what - that made me realize there isn't! I'll look into adding some now.

You can see the option in ros2 bag record --help - but we should at least have a mention in the README.

A more comprehensive documentation page is definitely needed but that's a little ways out, I think.

@JWhitleyWork
Copy link

Thanks! I'll give it a look and see how it compares.

@CourchesneA
Copy link

I have tested the snapshot feature that was merged into the main branch and it looks like it is working great. However it lacks the option to provide the duration of the snapshot instead of the buffer size, which seems to be supported by @JWhitleyWork's implementation. We will test that implementation in the following days, it is also great that it is in a separate package, which removes the need to build and run a rolling distribution to get this feature

@JWhitleyWork
Copy link

@CourchesneA Thanks for the info! The timestamps were not correct on my node but that is being fixed with gaia-platform/rosbag2_snapshot#5 (please feel free to test/approve if you have time). We will also be adding the ability to trigger snapshots which run into the future (stop time > now()) very soon. Let us know if you have any issues!

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

Successfully merging a pull request may close this issue.

5 participants