Duration Recorder cannot be implemented as an action server (intended for reuse)

[raghaprasad]

I ran into several roadblocks with duration recorder implementation recently. In light of these discoveries I believe its not possible to proceed as planned for duration recorder to be action server based.

Issue

Duration Recorder cannot be implemented as an action server (intended for reuse)

Root cause

Duration recorder uses rosbag::Recorder for generating the rosbags.

However, rosbag::Recorder has been designed to shutdown after generating the rosbag file.

bool Recorder::checkDuration(const ros::Time& t)
{
    if (options_.max_duration > ros::Duration(0))
    {
        if (t - start_time_ > options_.max_duration)
        {
            if (options_.split)
            {
                while (start_time_ + options_.max_duration < t)
                {
                    stopWriting();
                    split_count_++;
                    checkNumSplits();
                    start_time_ += options_.max_duration;
                    startWriting();
                }
            } else {
                ros::shutdown();
                return true;
            }
        }
    }
    return false;
}

This means any action needed to be taken post creation of the rosbag file, like uploading to s3 or cleaning up the generated files cannot be performed. ros::shutdown is async shutdown of the ros node and therefore any actions invoked after the shutdown are unlikely to succeed.

Callouts & action items

• Fix robag documentation: rosbag::recorder does not have clear documentation which meant that we missed this flaw during design
• Fix rosbag::recorder not to shutdown: a library that does not call ros::start should not be designed to call ros::shutdown . We need to propose a fix to rosbag for next version of ros and make sure this is not the case for ROS2 F

[AAlon]

I would consider implementing our own version of rosbag::recorder:

• The cost would be very low if we can simply copy the recorder.cpp/recorder.h files and change the few things we need to change.
• Maintenance pain shouldn't be an issue since those classes are very mature, were last modified years ago.
• We wouldn't need to constrain ourselves to the functionality that the existing recorder provides. It already happened in the past that we had to work around the limitations of that class instead of doing it the proper way (I think it was around the splits option making it continue recording forever? or was it something else?).

Doing a POC of copying & renaming that class and building everything together should be really quick, in the order of minutes. If we do that and it goes smoothly it seems to me like the best path forward.

[mm318]

Additionally, I think changing implementation is less risky than pivoting on the design/architecture of the system.

[raghaprasad]

Additionally, I think changing implementation is less risky than pivoting on the design/architecture of the system.

This might be true is a general context, but certainly not in this case. What we intend to do is to launch duration_recorder as a node instead of an action server.

[raghaprasad]

@AAlon This might actually work! In terms of things needed to be done :

• [x] Copy over recorder.cpp & recorder.h. License is BSD so its fine to do as long as we attribute it correctly @ryanewel
• [x] Verify if we can build after copying
• [x] Comment out ros::shutdown and replace with stopWriting
• [x] Run once to make sure max_duration is honored
• [x] Run once to make sure actual file created
• [x] Use Rosbag cli to check rosbag using info and replay
• [ ] Create an issue in github to improve codecoverage -- we will get to it afte private beta

[mm318]

After some non-trivial modifications to recorder.cpp (fixing cyclical shared_ptr references, increasing NodeHandle lifetimes, etc.), I believe we have a working solution.

EDIT: Please see https://github.com/aws-robotics/rosbag-uploader-ros1/pull/76. Lots of code and CMake file cleanup may be required.

[mm318]

@raghaprasad and @ryanewel, can you confirm that all that remains of this ticket is to improve code coverage?