Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Rosbag2 Snapshot

## Context

The following design handles functionality of keeping data from topics in buffer of specified size or from specific time duration and saving them after trigger.

Terms:
* "Snapshot" - save buffered data from topics to disk as rosbag
* "Trigger" - an invocation letting a node know when to save rosbag to disk
* "Recorder" - rosbag2 node which records data right after program start until killed

## Goal

Rosbag in ROS1 had functionality of creating a [snapshot](https://github.com/ros/rosbag_snapshot). Snapshotter was a program which subscribed to topics and buffered messages until a `trigger` for snapshot was sent. As in Rosbag2 we want to support the following features:
* Save data to rosbag on trigger
* Enable triggering buffer save to disk with service call
* Node and buffer configuration
* Enable user to specify `cli` arguments which could be shared with `ros2 bag record` / `Recorder` node or similar to them (dependent on design). User should be able to configure buffer size (maximum memory per topic) and duration (maximum time difference between newest and oldest message), subscribed topics (list of topics or all currently available topics) and output rosbag filename.
* Pause and resume buffering process
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this feature very well - my understanding for the intention of this feature is:

  • rosbag2 is always keeping a circular buffer of the latest information - but not writing anything to disk
  • when we trigger a snapshot - dump the entire buffer to disk

Why does a user want to pause buffering? This sounds to me like it makes rosbag2 just drop data, such that you wouldn't be able to take a snapshot of it.

* Pausing refers to stopping buffering new messages until resumed or write is triggered.
* Resuming continues the process of buffering after pause. Resuming would clear all buffers.


## Proposal

As thinking of this design in the past `rosbag_snapshot` was written as different package than `rosbag`. It was defined this way not to break the existent API of `rosbag`. Taken this into consideration it could be done so this time too. However it would require duplicating a large part of code which is already written and being optimized as a part of `rosbag2` repository. So as not being sure which way would be the most appropriate to implement I propose multiple approaches.

### Outside rosbag2

First approach implements `ros2 bag snapshot` as a different package. Mostly it requires rewriting `Recorder` node completly. Completly means in this case diving into implementation of `Writer`, `SequentialWriter` and `BaseWriterInterface` to copy and adjust writing to disk algorithms.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that this makes sense, and I would honestly remove it from the proposal - you might add a section at the end called "alternate approaches considered" - but top-level I like to see a design being a single proposal of what should be implemented, not a selection of choices.

I think everyone would agree that this is a useful feature for rosbag2 to do - somebody could implement something in a separate repository/package without doing any design here - I think our goal is to determine a design that does fit into rosbag2 directly.


Pros:
* Design flexibility

Cons:
* Many new functions would be redefined or even redundant
* Breaking the initial desing of writers/readers in favor of putting more effort to custom mechanism
* Not being part of `rosbag2` and `ros2bag` (cli incompatibility)

### Writer Interfaces changes + `Recorder` modifications

Second approach would base on making changes to `Recorder`, writers and their interfaces. This approach bases on adding service to `Recorder` which handles triggering buffer save. Although current design of writers bases on `BaseWriterInterface` which would need to be changed to enable some kind of `save()/flush()` functionality. I believe this is not an elegant approach but I haven't find other way to trigger buffer save from `Recorder`, through `Writer` to `SequentialWriter`. For sure it requires implementing new `SnapshotWriter` since it needs to take care of buffering the messages from subscribers instead of passing them straight to `MessageCache` and `CacheConsumer`.

Pros:
* Maintaining snapshot inside `ros2 bag snapshot ...` (or even `ros2 bag record snapshot ...`) and `Recorder` functionality
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that this makes sense as a separate verb. I would think that the circular-buffer + snapshot approach should be considered a behavioral modification of recording, just like rosbag2 play --loop

I'd like to see a specific section for "User Interface" - how do I interact with this feature (without knowing how it is built)?

I think maybe something like the following:

# Recording is invoked with this feature via an extra option to "record"
ros2 bag record --buffer-snapshot

# Snapshot is exposed as a Service on the Recorder node (so it can be triggered remotely)
ros2 service call /rosbag2_recorder rosbag2_interfaces/Snapshot

# C++ API
* RecordingOptions has a new value "buffer_snapshot"
* Recorder has a new API `Recorder::snapshot`

* Reuse most of already written code (producer-consumer in save, abstract interfacing between writers)

Cons:
* `BaseWriterInterface` redefinition (adding at least one pure abstract function)
* May require many changes in current implementation of the above mentioned classes

### Custom `Recorder` implementation

Last but not least it is possible to write new alternative to `Recorder` node inside `rosbag2_transport` package. `Snapshoter` would implement functionalities of subscribing the messages, buffering them at high level node and most probably passing data straight to `MessageCache` and `CacheConsumer`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the Recorder actually won't know much about this behavior. The SequentialWriter has the message_cache_ and is the entity that will probably implement this behavior. That's the sort of proposal I'm looking for here, like "what owns what functionality, logically". The Recorder itself I think should just pass through configuration, its responsibility is the transport layer, e.g. Subscriptions, and the Writer should handle behavior around writing.


Pros:
* Creating independent piece of code
* Adding new cli phrase and leaving package-wise compatibility inside `rosbag2` repository

Cons:
* Many new functions would be redefined or even redundant
* Mixing high and low level of abstraction in one piece of code
* Feels like putting much effort than it is needed