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

Record and play multiple topics #27

Merged

Conversation

Projects
None yet
5 participants
@anhosi
Copy link
Contributor

commented Aug 29, 2018

This PR is based on PR #26.

This extends both the recording and replaying to support multiple topics at the same time.

  • The required meta data (topic and type) is also stored in the storage

Current limitation:

  • Same as in #26 the topics to record must be present at start. Discovery for new topics will be added later.
    Known issue
  • The after_write_action argument of Rosbag2::record is used only for the tests. We should be able to remove it soon, but will do so in a separate PR.

@tfoote tfoote added the in review label Aug 29, 2018

This was referenced Aug 30, 2018

@Martin-Idel-SI Martin-Idel-SI force-pushed the bsinno:feature/record_and_play_multiple_topics branch from 3936546 to b9e6b22 Sep 4, 2018

src/rosbag2/typesupport_helpers.cpp
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
if(TARGET rosbag2_typesupport_helpers_test)
ament_target_dependencies(rosbag2_typesupport_helpers_test rcl Poco ament_index_cpp

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

alphabet

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 5, 2018

Author Contributor

Fixed.

src/rosbag2/rosbag2_node.cpp
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
if(TARGET rosbag2_rosbag_node_test)
ament_target_dependencies(rosbag2_rosbag_node_test rclcpp Poco ament_index_cpp std_msgs)

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

alphabet

{
return borrow_serialized_message(0);
}

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

double whitespace?

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 5, 2018

Author Contributor

Fixed.

// without the sleep_for() many messages are lost.
std::this_thread::sleep_for(std::chrono::milliseconds(500));
publisher->publish(message->serialized_data.get());
std::this_thread::sleep_for(std::chrono::milliseconds(50));

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

shouldn't they be published according to their timestamp?

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 4, 2018

Author Contributor

Yes and they will (in the future). We are currently working on that but it is not in scope for this PR.

}

void Rosbag2::play(const std::string & file_name, const std::string & topic_name)
void Rosbag2::play(const std::string & file_name)
{
rosbag2_storage::StorageFactory factory;
auto storage = factory.open_read_only(file_name, "sqlite3");

if (storage) {

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

What happens in the else case?

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 5, 2018

Author Contributor

Nothing 😁. The storage factory already complains if the storage cannot be loaded.

If we want to have more elaborate error handling I would postpone that until the rosbag2_transport has been introduced as we would have a CLI integration then. Then it would be easier to think about feedback to the user.

#include <memory>
#include <string>

#include "rcutils/types.h"

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

alphabet

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 5, 2018

Author Contributor

fixed.

size_t expected_messages_number, std::string topic)
{
auto node = rclcpp::Node::make_shared("node_" + topic);
std::vector<typename T::_data_type> messages;

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

This maybe deserves a bit of documentation. The _data_type field is only available for primitives messages types whose single field is called data. Otherwise the variable is called like the field name:
https://github.com/ros2/rosidl/blob/master/rosidl_generator_cpp/resource/msg__struct.hpp.em#L199-L204

So this test can not easily be transformed into a complex msg type.

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 4, 2018

Author Contributor

For this particular test it is sufficient. But I agree that we probably should add a test for a complex type. That way we can probably replace the std_msgs test dependency with test_msgs which looks less surprising.

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 5, 2018

Author Contributor

We changed the read_integration_test to use also complex messages from test_msgs.

});
subscriptions_.push_back(subscription);

while (messages_received < expected_messages_number) {

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

can this resolve in an endless loop in case there is no network connection etc? This should maybe timeout

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 4, 2018

Author Contributor

In principle yes, but the test framework takes care of that so we do not need to "clutter" the test with timeout handling.

std::shared_ptr<rosbag2_storage::SerializedBagMessage> serialize_message(std::string message)
template<typename MessageT>
std::shared_ptr<rosbag2_storage::SerializedBagMessage> serialize_message(
std::string topic, typename MessageT::_data_type message)

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

some comment as above. I believe _data_type is not defined for every message type.

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 5, 2018

Author Contributor

You are right. This is still used by the write_integration_test which is massively rewritten in #31. So I would prefer to address this in #31 to avoid an unecessary rebase.

auto statement = database_->prepare_statement("SELECT name, type FROM topics ORDER BY id;");
auto query_results = statement->execute_query<std::string, std::string>();

for (auto result : query_results) {

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 4, 2018

Contributor

should there be a notification or similar in case no results for found for this query?

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 4, 2018

Author Contributor

This could only happen if the bag is empty, i.e. contains no messages. Here the result will simply be empty which should not hurt.
I am not sure how we want to handle empty bags. Currently it would be opened, the play would initialize some things, loop over an empty list and shutdown => no harm there. So I propose to discuss this on a new issue.

botteroa-si and others added some commits Aug 24, 2018

@anhosi anhosi force-pushed the bsinno:feature/record_and_play_multiple_topics branch from b9e6b22 to 3eaee5b Sep 5, 2018

class Rosbag2
{
public:
ROSBAG2_PUBLIC
void record(
const std::string & file_name,
const std::string & topic_name,
std::function<void(void)> after_write_action = nullptr);
std::vector<std::string> topic_names,

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 5, 2018

Contributor

const std::vector<> &

void Rosbag2::record(
const std::string & file_name,
const std::string & topic_name,
std::function<void(void)> after_write_action)
std::vector<std::string> topic_names,

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 5, 2018

Contributor

const ref

@Karsten1987 Karsten1987 merged commit c23d188 into ros2:master Sep 5, 2018

@Karsten1987 Karsten1987 removed the in review label Sep 5, 2018

@anhosi anhosi deleted the bsinno:feature/record_and_play_multiple_topics branch Sep 5, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Nov 26, 2018

ros2GH-27 Implement discovery for new topics after startup
- this allows new topics to be discovered also after startup.
- as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup
- each time the recorder subscribes to a new topic, this will be logged to console

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Nov 26, 2018

ros2GH-27 Provide polling frequency as option
- make option available in RecordOptions
- set to 100ms for now

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Nov 26, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Nov 26, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Nov 30, 2018

ros2GH-27 Implement discovery for new topics after startup
- this allows new topics to be discovered also after startup.
- as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup
- each time the recorder subscribes to a new topic, this will be logged to console

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Nov 30, 2018

ros2GH-27 Provide polling frequency as option
- make option available in RecordOptions
- set to 100ms for now

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Nov 30, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Nov 30, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Nov 30, 2018

botteroa-si pushed a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Refactor subscribing mechanism
- use set for subscribed topics instead of vector
- perform the asynchronous call only when discovery is needed

botteroa-si pushed a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

botteroa-si added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Implement discovery for new topics after startup
- this allows new topics to be discovered also after startup.
- as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup
- each time the recorder subscribes to a new topic, this will be logged to console

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Provide polling frequency as option
- make option available in RecordOptions
- set to 100ms for now

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Refactor subscribing mechanism
- use set for subscribed topics instead of vector
- perform the asynchronous call only when discovery is needed

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

anhosi added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Refactor recorder
- naming of methods
- emphasize similarity between first subscription and discovery loop

Karsten1987 pushed a commit that referenced this pull request Dec 7, 2018

Auto discovery of new topics (#63)
* GH-27 Implement discovery for new topics after startup

- this allows new topics to be discovered also after startup.
- as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup
- each time the recorder subscribes to a new topic, this will be logged to console

* GH-27 Provide polling frequency as option

- make option available in RecordOptions
- set to 100ms for now

* GH-27 Stop topic polling if subscription setup is complete

* GH-27 Minor refactoring for readability

* GH-27 Fix build after rebase

* GH-145 add no-discovery option to ros2 bag record

* GH-27 Refactor subscribing mechanism

- use set for subscribed topics instead of vector
- perform the asynchronous call only when discovery is needed

* GH-27 Expose polling-interval as cli option

* GH-27 Minor refactoring

* GH-27 Refactor recorder

- naming of methods
- emphasize similarity between first subscription and discovery loop

* small touch ups

* remove launch from function name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.