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

ROSBAG: add snapshot subcommand #1414

Closed
wants to merge 1 commit into from

Conversation

kev-the-dev
Copy link
Contributor

Description

Addresses #1399 by adding a new subcommand, rosbag snapshot which acts similarly to the deprecated rosrecord -s command but with additional features. It seemed easier to me to implement this as a new subcommand rather than trying to add this to rosbag record without breaking API/ABI (I can still try that approach if it is preferred). It subscribes to topics and maintains a buffer of recent messages like a dash cam. This is useful in live testing where unexpected events can occur which would be useful to have data on but the opportunity is missed if rosbag record was not running (disk space limits make always running rosbag record impracticable). Instead, users may run snapshot in the background and save data from the recent past to disk as needed.

Usage

rosbag snapshot can be configured through command line flags and with ROS params for more granular control. By default, the command will run in server mode (buffering data). When certain flags are used, program will act as a client by requesting that the server write data to disk or freezing the buffer to preserve interesting data until a user can decide what to write.

CLI usage

$ rosrun rosbag snapshot -h
Usage: rosbag snapshot [options] [topic1 topic2 ...]

Buffer recent messages until triggered to write or trigger an already running instance.

Options:
  -h [ --help ]                produce help message
  -t [ --trigger-write ]       Write buffer of selected topcis to a bag file
  -p [ --pause ]               Stop buffering new messages until resumed or 
                               write is triggered
  -r [ --resume ]              Resume buffering new messages, writing over 
                               older messages as needed
  -s [ --size ] arg (=-1)      Maximum memory per topic to use in buffering in 
                               MB. Default: no limit
  -d [ --duration ] arg (=30)  Maximum difference between newest and oldest 
                               buffered message per topic in seconds. Default: 
                               30
  -o [ --output-prefix ] arg   When in trigger write mode, prepend PREFIX to 
                               name of writting bag file
  -O [ --output-filename ] arg When in trigger write mode, exact name of 
                               written bag file
  --topic arg                  Topic to buffer. If triggering write, write only
                               these topics instead of all buffered topics.

Hold a buffer of the last 30 seconds of data from selected topics until triggered to write
rosbag snapshot -d 30 /tf /odom /camera/image_color /camera/camera_info /velodyne_points
Buffer the most recent gigabyte of the following topics in the camera namespace
ROS_NAMESPACE=camera rosbag snapshot -s 1000 image_rect_color camera_info

Example launch file

<launch>
  <node name="snapshot" pkg="rosbag" type="snapshot" args="">
    <rosparam>
        default_duration_limit: 1  # Maximum time difference between newest and oldest message, seconds, overrides -d flag
        default_memory_limit: 0.1  # Maximum memory used by messages in each topic's buffer, MB, override -s flag
        topics:
            - test1                # Inherit defaults
            - test2:               # Override duration limit, inherit memory limit
                duration: 2
            - test3:               # Override both limits
                duration: -1       # Negative means no duration limit
                memory: 0.00
    </rosparam>
  </node>
</launch>

Client examples

Write all buffered data to <datatime>.bag
rosbag snapshot -t

Write buffered data from selected topics to new_lighting<datatime>.bag
rosbag snapshot -t -o new_lighting /camera/image_raw /camera/camera_info

Write all buffered data to /home/user/crashed_into_wall.bag
rosbag snapshot -t -O /home/user/crashed_into_wall

Pause buffering of new data, holding current buffer in memory until -t or -r is used
rosbag snapshot -p

Resume buffering new data
rosbag snapshot -r

Call trigger service manually, specifying absolute window start and stop time for written data

$ rosservice call /trigger_snapshot "filename: 'short_control_instability.bag'
topics:
- '/tf'
- '/odom'
- '/wrench'
- '/motors/cmd'
start_time:
  secs: 62516
  nsecs: 0
stop_time:
  secs: 62528
  nsecs: 0"

View status of buffered data (useful for future tools/GUI)

$ rostopic echo /snapshot_status 
topics: 
  - 
    topic: "/test"
    node_pub: ''
    node_sub: "/snapshot_1527185221120605085"
    window_start: 
      secs: 62516
      nsecs: 761000000
    window_stop: 
      secs: 62521
      nsecs: 984000000
    delivered_msgs: 524
    dropped_msgs: 0
    traffic: 2096
    period_mean: 
      secs: 0
      nsecs:         0
    period_stddev: 
      secs: 0
      nsecs:         0
    period_max: 
      secs: 0
      nsecs:         0
    stamp_age_mean: 
      secs: 0
      nsecs:         0
    stamp_age_stddev: 
      secs: 0
      nsecs:         0
    stamp_age_max: 
      secs: 0
      nsecs:         0
enabled: True
---

#!/usr/bin/env python
# Software License Agreement (BSD License)
#
# Copyright (c) 2008, Open Source Robotics Foundation, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018

@scpeters
Copy link
Contributor

scpeters commented Jun 6, 2018

The spelling of snapshoter.h and snapshoter.cpp seems odd to me; it would feel more natural with a double-t, as in snapshotter.h and snapshotter.cpp.

Though, I would actually just use snapshot.h and snapshot.cpp for the library code and use snapshot_main.cpp for the executable.

@scpeters
Copy link
Contributor

Though, I would actually just use snapshot.h and snapshot.cpp for the library code and use snapshot_main.cpp for the executable.

@ironmig do you have any thoughts about this comment?

@kev-the-dev
Copy link
Contributor Author

That makes more sense to me, but it may be preferred to follow the convention established for that package (play.cpp player.cpp record.cpp recorder.cpp)

@scpeters
Copy link
Contributor

Ah, that makes sense. I had seen the python file rosbag_main.py, but there is more of a pattern in the c++ code.

@paulbovbel
Copy link
Contributor

Thanks for this feature @ironmig, it's very useful!

Would the ros_comm maintainers consider merging it into melodic?

@dirk-thomas
Copy link
Member

This needs to be rebased to resolve merge conflicts.

@DLu
Copy link
Contributor

DLu commented Feb 7, 2020

@kev-the-dev I'd be happy to if you don't have time.

@kev-the-dev
Copy link
Contributor Author

I'm happy to see this through assuming it doesn't require a significant redesign. I rebased and squashed it. I'll look into the CI failures to see if they're trivial

@dirk-thomas
Copy link
Member

You can ignore the Noetic related CI results for now. Since not all dependencies of this repo have been released yet they are expected to fail.

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

I am not sure that this 1.5k lines contribution should be merged into this repository. It adds another message (which will implicitly be part of ros_core) as well as a new command which will implicitly be maintained by me (since you won't be easily able to watch for incoming issues related to it, create patches, and do releases on demand).

Therefore I am leaning towards suggesting to move the proposed implementation into a separate repository.

<name>rosbag_msgs</name>
<version>1.0.0</version>
<description>Service and message definitions for rosbag</description>
<maintainer email="dthomas@osrfoundation.org">Dirk Thomas</maintainer>
Copy link
Member

Choose a reason for hiding this comment

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

Your name should be listed here.

<description>Service and message definitions for rosbag</description>
<maintainer email="dthomas@osrfoundation.org">Dirk Thomas</maintainer>
<license>BSD</license>
<author>Kevin Allen</author>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@kev-the-dev
Copy link
Contributor Author

kev-the-dev commented Feb 8, 2020

That's fair, it is a lot to ask to maintain this much new code in ros_comm. Another package is probably the best option. It might be a few weeks/months, but I'll start on that when I get the time

@paulbovbel
Copy link
Contributor

We've been using this command for ~1 year at Locus, thanks for the contribution @kev-the-dev.

@arneboe
Copy link

arneboe commented Mar 4, 2020

There is a memory leak in Snapshotter::subscribe.
You are binding topicCB using boost::bind with queue as additional parameter.
Therefore the subscriber holds a shared_ptr to the queue.
Storing the subscriber inside the queue (queue->setSubscriber(sub);) essentially makes the queue hold a shared_ptr to itself and thus it will never be deleted.
Never deleting the subscriber will in turn stop a whole bunch of ros internals from getting shutdown properly.

A possible fix (albeit not a very nice one) is.

Snapshotter::~Snapshotter()
{
    for (std::pair<const std::string, boost::shared_ptr<MessageQueue>>& buffer : buffers_) 
    {
      buffer.second->sub_->shutdown();
    }
}

@DLu
Copy link
Contributor

DLu commented Apr 3, 2020

I am working on a refactor of this contribution to work as an independent package. Would it be possible to get https://github.com/ros/rosbag_snapshot willed into existence?

@tfoote
Copy link
Member

tfoote commented Apr 10, 2020

@DLu I'd suggest you get it going locally and then we can transfer it into the organization.

@dirk-thomas
Copy link
Member

Assuming this feature will land in its own package I will go ahead and close this ticket.

@DLu
Copy link
Contributor

DLu commented Jun 11, 2020

@tfoote After much ado, the package is now available: https://github.com/locusrobotics/rosbag_snapshot

@DLu
Copy link
Contributor

DLu commented Jun 11, 2020

@tfoote Can you get it forked over at ros/rosbag_snapshot? Also, is there a proper organization for the release repository for something in ros/?

@tfoote
Copy link
Member

tfoote commented Jun 18, 2020

We can fork it, but usually we prefer to transfer it so that we can keep the history of issues and PRs etc. This one doesn't have much history so that's not a big problem. One other thing about a fork is that apparently github search only searches the parent fork so if development is going to diverge it's better to transfer, or you have to request to "break the fork" from github customer service.

If you transfer it to me I can then transfer it into the organization.

For the gbp I can create one for you at in https://github.com/ros-gbp: https://github.com/ros-gbp/rosbag_snapshot-release

@DLu
Copy link
Contributor

DLu commented Jun 18, 2020

I have requested the transfer. (You can only transfer from an organization to yourself, so I had to transfer it to DLu first, then to you)

@tfoote
Copy link
Member

tfoote commented Jun 18, 2020

It's now here: https://github.com/ros/rosbag_snapshot

@ros-discourse
Copy link

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

https://discourse.ros.org/t/rosbag-snapshot/15324/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants