-
Notifications
You must be signed in to change notification settings - Fork 242
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 all topics #30
Conversation
c63b58f
to
83d9a34
Compare
83d9a34
to
ebfb989
Compare
ebfb989
to
0eb18eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like having multiple copies of the logging file doesn't make sense. If there is something in the rcutils logging macros missing, we can address them upstream.
@@ -0,0 +1,61 @@ | |||
// Copyright 2018, Bosch Software Innovations GmbH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the need for this file? Is there something in rcutils which is missing?
rosbag2/include/rosbag2/rosbag2.hpp
Outdated
* Records topics to a bagfile. Subscription happens at startup time, hence the topics must | ||
* exist when "record" is called. | ||
* | ||
* @param file_name Name of the bagfile to write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we are using the notion of \param
for doxygen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
RCUTILS_LOG_ERROR_NAMED( | ||
"rosbag2", | ||
"failed to destroy serialized message: %s", rcl_get_error_string_safe()); | ||
ROSBAG2_LOG_ERROR_STREAM( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is still a problem with the allocator IMO.
So assume we create a serialized message using this subscription, keep it in memory and destroy the subscription before destroying the message.
The message now has an invalid allocator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a close look and I believe it should be ok as is. The content of the allocator gets copied inside rmw_serialized_message_init(...)
(see https://github.com/ros2/rcutils/blob/f04cc666dd06347050ff5c18b80079922e729d92/src/char_array.c#L50) so it should still be valid even after the pointer is destroyed (as long as its state is empty, of course, which seems to be the case for the default allocator, in the other case, even a delete
won't help).
We did a check by prematurely destroying the allocator (create new rcutils_allocator_t
, use it to initialize the serialized message and then immediately destroy it) and it ran perfectly and valgrind was also happy.
That said, I'd love to have a better way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. Please ignore my comment then.
rosbag2/src/rosbag2/rosbag2_node.cpp
Outdated
{ | ||
std::vector<std::string> sanitized_topic_names; | ||
for (const auto & topic_name : topic_names) { | ||
sanitized_topic_names.push_back(topic_name[0] != '/' ? "/" + topic_name : topic_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/ros2/rcl/blob/master/rcl/include/rcl/expand_topic_name.h
This function does exactly what you're looking for :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to use then. Thank you
rosbag2/src/rosbag2/rosbag2_node.cpp
Outdated
} | ||
} | ||
|
||
return reduce_multiple_types_to_one(filter_topics_with_wrong_types(filtered_topics_and_types)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really need to do this? This copies the complete map. Maybe we could use the first element of the type just by convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is done at startup time, it's not really a big overhead. What happens if there are multiple types and it's not a ros type? The subscription would fail, I guess, but isn't it easier to just leave it out?
rosbag2/src/rosbag2/rosbag2_node.cpp
Outdated
// This should be replaced by an auto-discovery system in the future | ||
std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
return reduce_multiple_types_to_one( | ||
filter_topics_with_wrong_types(this->get_topic_names_and_types())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the topic types returned from here are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I deleted our own filtering code, then. Thanks.
while (rclcpp::ok() && messages_stored_counter_[topic_name] < number_expected_messages) { | ||
publisher->publish(message->serialized_data); | ||
// We need to wait a bit, in order for record to process the messages | ||
std::this_thread::sleep_for(50ms); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this potentially dangerous to assume? What if the publisher rate is higher than this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just for regular functionality testing (no performance or the like), so I think it's fine to assume. Or what would be your preferred rationale?
@@ -0,0 +1,61 @@ | |||
// Copyright 2018, Bosch Software Innovations GmbH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a copy of the first file? Why is this necessary?
@@ -69,17 +66,17 @@ get_interface_instance( | |||
auto unmanaged_instance = class_loader->createUnmanagedInstance(storage_id); | |||
instance = std::shared_ptr<InterfaceT>(unmanaged_instance); | |||
} catch (const std::runtime_error & ex) { | |||
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, | |||
"unable to load instance of read write interface: %s", ex.what()); | |||
ROSBAG2_STORAGE_LOG_ERROR_STREAM( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see a reason for this.
@@ -0,0 +1,61 @@ | |||
// Copyright 2018, Bosch Software Innovations GmbH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the third copy of this file ?!
Regarding the logging: We basically copied that behaviour from rviz where we always had similar macros in
At least the first problem can't be addressed in rcutils as far as I can see. |
- correctly expand topic names using rcl - do not check type correctness (supposed to be done internally)
rosbag2/src/rosbag2/rosbag2_node.cpp
Outdated
}); | ||
} | ||
|
||
std::string Rosbag2Node::expand_topic_name(std::string topic_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this function take a const std::string &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolut.
RCUTILS_LOG_ERROR_NAMED( | ||
"rosbag2", | ||
"failed to destroy serialized message: %s", rcl_get_error_string_safe()); | ||
ROSBAG2_LOG_ERROR_STREAM( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. Please ignore my comment then.
…ackage (#48) * GH-111 Add package for converter plugins * GH-111 Add CDR converter plugin * GH-111 Add test for more primitives types * GH-116 Fix cdr converter after rebase on new converters interface * GH-116 Use rmw_serialize/rmw_deserialize directly in converter and link against rmw_fastrtps_cpp * Fix converter package.xml * Fix clang warnings * GH-30 Change interface to the same convention as rmw_(de)serialize * comply to new rcutils error handling API * use poco to load fastrtps * Update rosbag2_converter_default_plugins/src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp Co-Authored-By: Karsten1987 <karsten@osrfoundation.org>
- The TODO comments have been removed because they're no longer relevant: they have been discussed in the PR review
* GH-112 Open storage for reading handing in rmw_identifier * GH-113 Cleanup: better naming * GH-113 Introduce interface for StorageFactory to allow mocks in tests * GH-113 Add test for SequentialReader for using converters - Added mocks for storage and converters (and factories) * GH-113 Implement skeleton convert function - Use convert only if necessary (different input and output formats), converters are only loaded if really necessary. - Allocate_ros2_message is public to enable extensive tests for this function. - Helper function to get any typesupport by name - Helper function for empty ros2_message * GH-113 Implement allocate_ros2_message - Treats most messages already. - Some combinations of nested messages with arrays are still missing - Cleanup of DynamicArrayNested messages is failing - Main difficulty is the cleanup of the allocated ros2_message which needs to be done manually - The test_ros2_message is intended to be run with valgrind and there should be no leaks or problems with free! * GH-113 Fix DynamicArrayNested deallocation Swapping with empty container seems more stable than deleting the data pointer of the container. * GH-113 Add test for BoundedArrayNested deallocation * GH-113 Refactoring of deallocation code * GH-113 Fix string initialization in all types * GH-113 Fix vector<bool> initialization * GH-113 Add test for deallocation of topic name + Refactoring * GH-113 Minor refactoring of converter * GH-113 Make sure to throw an error if converters do not exist * GH-113 Delete superfluous imports * GH-113 Small fix for deleting vectors * GH-113 Fix build after rebase * GH-30 Minor refactoring - The TODO comments have been removed because they're no longer relevant: they have been discussed in the PR review * GH-30 Give an allocator as parameter to allocate_ros2_message() * GH-111 Add missing test dependencies for CDR converter test * GH-128 Extend message allocation test to also cover big strings - Big strings are not treated with small string optimization and need to be checked, too. * GH-128 Add tests for nested arrays * GH-128 always initialize vectors with a placement new * pass by ref * use new getter functions * consistent function naming * uncrustify * GH-30 Fix windows build * use visibility macros on all functions
This PR is based on #27
The goal of this PR is to allow to subscribe to all available topics. In addition, there are a few technical changes: