From a84a33bde7919dcbddc9ce77e7f16e160cc317a6 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Thu, 2 Apr 2020 20:24:24 +0000 Subject: [PATCH 01/13] Override Subscriber QoS Signed-off-by: Anas Abou Allaban --- .../rosbag2_test_common/publisher_manager.hpp | 7 ++- .../rosbag2_transport/record_options.hpp | 1 + .../src/rosbag2_transport/recorder.cpp | 21 +++----- .../src/rosbag2_transport/recorder.hpp | 14 +++++ .../test/rosbag2_transport/test_record.cpp | 53 +++++++++++++++++-- 5 files changed, 77 insertions(+), 19 deletions(-) diff --git a/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp b/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp index 80d63fbdbf..635edf5ea4 100644 --- a/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp +++ b/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp @@ -75,13 +75,16 @@ class PublisherManager template void add_publisher( - const std::string & topic_name, std::shared_ptr message, size_t expected_messages = 0) + const std::string & topic_name, + std::shared_ptr message, + size_t expected_messages = 0, + const rclcpp::QoS & qos = rclcpp::QoS(10)) { auto node_name = std::string("publisher") + std::to_string(counter_++); auto publisher_node = std::make_shared( node_name, rclcpp::NodeOptions().start_parameter_event_publisher(false).enable_rosout(false)); - auto publisher = publisher_node->create_publisher(topic_name, 10); + auto publisher = publisher_node->create_publisher(topic_name, qos); publisher_nodes_.push_back(publisher_node); publishers_.push_back( diff --git a/rosbag2_transport/include/rosbag2_transport/record_options.hpp b/rosbag2_transport/include/rosbag2_transport/record_options.hpp index e53d9a1d31..20c72130ff 100644 --- a/rosbag2_transport/include/rosbag2_transport/record_options.hpp +++ b/rosbag2_transport/include/rosbag2_transport/record_options.hpp @@ -32,6 +32,7 @@ struct RecordOptions std::string node_prefix = ""; std::string compression_mode = ""; std::string compression_format = ""; + std::string qos_profiles = ""; bool include_hidden_topics = false; }; diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index f0cf9ce0c1..414f063609 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -31,19 +31,6 @@ #include "qos.hpp" #include "rosbag2_node.hpp" -#ifdef _WIN32 -// This is necessary because of a bug in yaml-cpp's cmake -#define YAML_CPP_DLL -// This is necessary because yaml-cpp does not always use dllimport/dllexport consistently -# pragma warning(push) -# pragma warning(disable:4251) -# pragma warning(disable:4275) -#endif -#include "yaml-cpp/yaml.h" -#ifdef _WIN32 -# pragma warning(pop) -#endif - namespace { bool all_qos_same(const std::vector & values) @@ -67,6 +54,7 @@ Recorder::Recorder(std::shared_ptr writer, std::shared_ptr< void Recorder::record(const RecordOptions & record_options) { + qos_profile_overrides_ = YAML::Load(record_options.qos_profiles); if (record_options.rmw_serialization_format.empty()) { throw std::runtime_error("No serialization format specified!"); } @@ -182,10 +170,15 @@ std::shared_ptr Recorder::create_subscription( const std::string & topic_name, const std::string & topic_type, const rclcpp::QoS & qos) { + auto subscription_qos = Rosbag2QoS(); + if (qos_profile_overrides_[topic_name]) { + subscription_qos = qos_profile_overrides_[topic_name].as(); + ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); + } auto subscription = node_->create_generic_subscription( topic_name, topic_type, - qos, + subscription_qos, [this, topic_name](std::shared_ptr message) { auto bag_message = std::make_shared(); bag_message->serialized_data = message; diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.hpp b/rosbag2_transport/src/rosbag2_transport/recorder.hpp index 25c842978e..f5fb1d9eea 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.hpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.hpp @@ -23,6 +23,19 @@ #include #include +#ifdef _WIN32 +// This is necessary because of a bug in yaml-cpp's cmake +#define YAML_CPP_DLL +// This is necessary because yaml-cpp does not always use dllimport/dllexport consistently +# pragma warning(push) +# pragma warning(disable:4251) +# pragma warning(disable:4275) +#endif +#include "yaml-cpp/yaml.h" +#ifdef _WIN32 +# pragma warning(pop) +#endif + #include "rclcpp/qos.hpp" #include "rosbag2_cpp/writer.hpp" @@ -101,6 +114,7 @@ class Recorder std::unordered_map> subscriptions_; std::unordered_set topics_warned_about_incompatibility_; std::string serialization_format_; + YAML::Node qos_profile_overrides_; }; } // namespace rosbag2_transport diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index ced11aec86..27e90697dc 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -20,6 +20,7 @@ #include "rclcpp/rclcpp.hpp" +#include "../../src/rosbag2_transport/qos.hpp" #include "rosbag2_transport/rosbag2_transport.hpp" #include "test_msgs/msg/arrays.hpp" @@ -100,8 +101,7 @@ TEST_F(RecordIntegrationTestFixture, qos_is_stored_in_metadata) )); } -TEST_F(RecordIntegrationTestFixture, records_sensor_data) -{ +TEST_F(RecordIntegrationTestFixture, records_sensor_data) { using clock = std::chrono::system_clock; using namespace std::chrono_literals; @@ -118,7 +118,7 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) publisher->publish(msg); } ); - MockSequentialWriter & writer = + MockSequentialWriter &writer = static_cast(writer_->get_implementation_handle()); auto start = clock::now(); @@ -141,3 +141,50 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) EXPECT_FALSE(recorded_messages.empty()); } #endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION + +TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) +{ + auto strict_msg = std::make_shared(); + strict_msg->string_value = "strict"; + std::string strict_topic = "/strict_topic"; + std::string relaxed_topic = "/relaxed_topic"; + + rosbag2_transport::RecordOptions record_options = + {false, false, {strict_topic, relaxed_topic}, "rmw_format", 100ms}; + // 0 means system default for all options + record_options.qos_profiles = + "/strict_topic:\n" + " history: 2\n" // Keep All + " depth: 0\n" + " reliability: 2\n" // Best Effort + " durability: 2\n" // Volatile + " deadline:\n" + " sec: 0\n" + " nsec: 0\n" + " lifespan:\n" + " sec: 0\n" + " nsec: 0\n" + " nsec: 0\n" + " liveliness: 0\n" + " liveliness_lease_duration:\n" + " sec: 0\n" + " nsec: 0\n" + " avoid_ros_namespace_conventions: false\n"; + + // Create two publishers on the same topic with different QoS profiles. + // If no override is specified, then the recorder cannot see any published messages. + auto profile1 = rosbag2_transport::Rosbag2QoS().best_effort().durability_volatile(); + auto profile2 = rosbag2_transport::Rosbag2QoS().best_effort().transient_local(); + pub_man_.add_publisher(strict_topic, strict_msg, 3, profile1); + pub_man_.add_publisher(strict_topic, strict_msg, 3, profile2); + + start_recording(record_options); + run_publishers(); + stop_recording(); + + MockSequentialWriter & writer = + static_cast(writer_->get_implementation_handle()); + auto recorded_messages = writer.get_messages(); + + ASSERT_GE(recorded_messages.size(), 0u); +} From a0aa4b67e22b5c4897b9f67577a2146e36d09832 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Thu, 2 Apr 2020 22:49:06 +0000 Subject: [PATCH 02/13] Fix merge conflict Signed-off-by: Anas Abou Allaban --- rosbag2_transport/src/rosbag2_transport/recorder.cpp | 2 +- rosbag2_transport/test/rosbag2_transport/test_record.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index 414f063609..4fe4e5081d 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -170,7 +170,7 @@ std::shared_ptr Recorder::create_subscription( const std::string & topic_name, const std::string & topic_type, const rclcpp::QoS & qos) { - auto subscription_qos = Rosbag2QoS(); + Rosbag2QoS subscription_qos(qos); if (qos_profile_overrides_[topic_name]) { subscription_qos = qos_profile_overrides_[topic_name].as(); ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 27e90697dc..60a35e79ab 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -118,7 +118,7 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) { publisher->publish(msg); } ); - MockSequentialWriter &writer = + MockSequentialWriter & writer = static_cast(writer_->get_implementation_handle()); auto start = clock::now(); From db47c2bc4e1a6157ecef0458bef48d452dfbcdf5 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Thu, 2 Apr 2020 22:53:40 +0000 Subject: [PATCH 03/13] Remove extra topic Signed-off-by: Anas Abou Allaban --- rosbag2_transport/test/rosbag2_transport/test_record.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 60a35e79ab..7fe04436be 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -147,10 +147,9 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) auto strict_msg = std::make_shared(); strict_msg->string_value = "strict"; std::string strict_topic = "/strict_topic"; - std::string relaxed_topic = "/relaxed_topic"; rosbag2_transport::RecordOptions record_options = - {false, false, {strict_topic, relaxed_topic}, "rmw_format", 100ms}; + {false, false, {strict_topic}, "rmw_format", 100ms}; // 0 means system default for all options record_options.qos_profiles = "/strict_topic:\n" From ecab1145107117810ddfdc14731be3e4142e64ed Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Fri, 3 Apr 2020 00:19:46 +0000 Subject: [PATCH 04/13] Auto and brace init Signed-off-by: Anas Abou Allaban --- .../include/rosbag2_test_common/publisher_manager.hpp | 2 +- rosbag2_transport/test/rosbag2_transport/test_record.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp b/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp index 635edf5ea4..3baa19cc52 100644 --- a/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp +++ b/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp @@ -78,7 +78,7 @@ class PublisherManager const std::string & topic_name, std::shared_ptr message, size_t expected_messages = 0, - const rclcpp::QoS & qos = rclcpp::QoS(10)) + const rclcpp::QoS & qos = rclcpp::QoS{10}) { auto node_name = std::string("publisher") + std::to_string(counter_++); auto publisher_node = std::make_shared( diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 7fe04436be..7207009478 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -172,8 +172,8 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) // Create two publishers on the same topic with different QoS profiles. // If no override is specified, then the recorder cannot see any published messages. - auto profile1 = rosbag2_transport::Rosbag2QoS().best_effort().durability_volatile(); - auto profile2 = rosbag2_transport::Rosbag2QoS().best_effort().transient_local(); + auto profile1 = rosbag2_transport::Rosbag2QoS{}.best_effort().durability_volatile(); + auto profile2 = rosbag2_transport::Rosbag2QoS{}.best_effort().transient_local(); pub_man_.add_publisher(strict_topic, strict_msg, 3, profile1); pub_man_.add_publisher(strict_topic, strict_msg, 3, profile2); @@ -181,7 +181,7 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) run_publishers(); stop_recording(); - MockSequentialWriter & writer = + auto & writer = static_cast(writer_->get_implementation_handle()); auto recorded_messages = writer.get_messages(); From a93de075f13d6f21ddd00efcd1a06f8fa1cb1d88 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Fri, 3 Apr 2020 13:56:54 +0000 Subject: [PATCH 05/13] Switch override to map, move override logic Signed-off-by: Anas Abou Allaban --- .../rosbag2_transport/record_options.hpp | 5 +++- .../src/rosbag2_transport/recorder.cpp | 16 +++++------ .../src/rosbag2_transport/recorder.hpp | 2 +- .../test/rosbag2_transport/test_record.cpp | 28 ++++++------------- 4 files changed, 21 insertions(+), 30 deletions(-) diff --git a/rosbag2_transport/include/rosbag2_transport/record_options.hpp b/rosbag2_transport/include/rosbag2_transport/record_options.hpp index 20c72130ff..adb0573c2b 100644 --- a/rosbag2_transport/include/rosbag2_transport/record_options.hpp +++ b/rosbag2_transport/include/rosbag2_transport/record_options.hpp @@ -17,8 +17,11 @@ #include #include +#include #include +#include "rclcpp/rclcpp.hpp" + namespace rosbag2_transport { struct RecordOptions @@ -32,7 +35,7 @@ struct RecordOptions std::string node_prefix = ""; std::string compression_mode = ""; std::string compression_format = ""; - std::string qos_profiles = ""; + std::unordered_map topic_qos_profile_overrides{}; bool include_hidden_topics = false; }; diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index 4fe4e5081d..0daa129453 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -54,7 +54,7 @@ Recorder::Recorder(std::shared_ptr writer, std::shared_ptr< void Recorder::record(const RecordOptions & record_options) { - qos_profile_overrides_ = YAML::Load(record_options.qos_profiles); + topic_qos_profile_overrides_ = record_options.topic_qos_profile_overrides; if (record_options.rmw_serialization_format.empty()) { throw std::runtime_error("No serialization format specified!"); } @@ -170,15 +170,10 @@ std::shared_ptr Recorder::create_subscription( const std::string & topic_name, const std::string & topic_type, const rclcpp::QoS & qos) { - Rosbag2QoS subscription_qos(qos); - if (qos_profile_overrides_[topic_name]) { - subscription_qos = qos_profile_overrides_[topic_name].as(); - ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); - } auto subscription = node_->create_generic_subscription( topic_name, topic_type, - subscription_qos, + qos, [this, topic_name](std::shared_ptr message) { auto bag_message = std::make_shared(); bag_message->serialized_data = message; @@ -213,6 +208,11 @@ std::string Recorder::serialized_offered_qos_profiles_for_topic(const std::strin rclcpp::QoS Recorder::common_qos_or_fallback(const std::string & topic_name) { + rclcpp::QoS subscription_qos{10}; + if (topic_qos_profile_overrides_.count(topic_name)) { + subscription_qos = topic_qos_profile_overrides_.at(topic_name); + ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); + } auto endpoints = node_->get_publishers_info_by_topic(topic_name); if (!endpoints.empty() && all_qos_same(endpoints)) { return Rosbag2QoS(endpoints[0].qos_profile()).default_history(); @@ -222,7 +222,7 @@ rclcpp::QoS Recorder::common_qos_or_fallback(const std::string & topic_name) "Cannot determine what QoS to request, falling back to default QoS profile." ); topics_warned_about_incompatibility_.insert(topic_name); - return Rosbag2QoS{}; + return subscription_qos; } void Recorder::warn_if_new_qos_for_subscribed_topic(const std::string & topic_name) diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.hpp b/rosbag2_transport/src/rosbag2_transport/recorder.hpp index f5fb1d9eea..052a775597 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.hpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.hpp @@ -114,7 +114,7 @@ class Recorder std::unordered_map> subscriptions_; std::unordered_set topics_warned_about_incompatibility_; std::string serialization_format_; - YAML::Node qos_profile_overrides_; + std::unordered_map topic_qos_profile_overrides_; }; } // namespace rosbag2_transport diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 7207009478..3253f34328 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include "rclcpp/rclcpp.hpp" @@ -24,7 +25,6 @@ #include "rosbag2_transport/rosbag2_transport.hpp" #include "test_msgs/msg/arrays.hpp" -#include "test_msgs/msg/basic_types.hpp" #include "test_msgs/message_fixtures.hpp" #include "record_integration_fixture.hpp" @@ -140,7 +140,6 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) { EXPECT_EQ(recorded_topics.size(), 1u); EXPECT_FALSE(recorded_messages.empty()); } -#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) { @@ -151,24 +150,12 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) rosbag2_transport::RecordOptions record_options = {false, false, {strict_topic}, "rmw_format", 100ms}; // 0 means system default for all options - record_options.qos_profiles = - "/strict_topic:\n" - " history: 2\n" // Keep All - " depth: 0\n" - " reliability: 2\n" // Best Effort - " durability: 2\n" // Volatile - " deadline:\n" - " sec: 0\n" - " nsec: 0\n" - " lifespan:\n" - " sec: 0\n" - " nsec: 0\n" - " nsec: 0\n" - " liveliness: 0\n" - " liveliness_lease_duration:\n" - " sec: 0\n" - " nsec: 0\n" - " avoid_ros_namespace_conventions: false\n"; + const auto profile_override = rclcpp::QoS{10} + .best_effort().durability_volatile().keep_all().avoid_ros_namespace_conventions(false); + std::unordered_map topic_qos_profile_overrides = { + {"/strict_topic", profile_override} + }; + record_options.topic_qos_profile_overrides = topic_qos_profile_overrides; // Create two publishers on the same topic with different QoS profiles. // If no override is specified, then the recorder cannot see any published messages. @@ -187,3 +174,4 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) ASSERT_GE(recorded_messages.size(), 0u); } +#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION From 73f406d3aa7be71d8ae74c7a518a1f54c259211e Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Fri, 3 Apr 2020 14:05:39 +0000 Subject: [PATCH 06/13] Rename subscription f-n and update docs Signed-off-by: Anas Abou Allaban --- .../src/rosbag2_transport/recorder.cpp | 6 +++--- .../src/rosbag2_transport/recorder.hpp | 17 ++++++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index 0daa129453..b4fd043739 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -150,9 +150,9 @@ void Recorder::subscribe_topic(const rosbag2_storage::TopicMetadata & topic) // that callback called before we reached out the line: writer_->create_topic(topic) writer_->create_topic(topic); - // TODO(emersonknapp) re-enable common_qos_or_fallback once the cyclone situation is resolved + // TODO(emersonknapp) re-enable subscription_qos_for_topic once the cyclone situation is resolved #ifdef ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION - Rosbag2QoS subscription_qos{common_qos_or_fallback(topic.name)}; + Rosbag2QoS subscription_qos{subscription_qos_for_topic(topic.name)}; #else Rosbag2QoS subscription_qos{}; #endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION @@ -206,7 +206,7 @@ std::string Recorder::serialized_offered_qos_profiles_for_topic(const std::strin return YAML::Dump(offered_qos_profiles); } -rclcpp::QoS Recorder::common_qos_or_fallback(const std::string & topic_name) +rclcpp::QoS Recorder::subscription_qos_for_topic(const std::string & topic_name) { rclcpp::QoS subscription_qos{10}; if (topic_qos_profile_overrides_.count(topic_name)) { diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.hpp b/rosbag2_transport/src/rosbag2_transport/recorder.hpp index 052a775597..fa8f7f12bf 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.hpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.hpp @@ -98,11 +98,18 @@ class Recorder void record_messages() const; - /** Find the QoS profile that should be used for subscribing. - * If all currently offered QoS Profiles for a topic are the same, return that profile. - * Otherwise, print a warning and return a fallback value. - */ - rclcpp::QoS common_qos_or_fallback(const std::string & topic_name); + /** + * Find the QoS profile that should be used for subscribing. + * + * Profiles are prioritized by: + * 1. The override specified in the record_options, if one exists for the topic. + * 2. The common offered QoS profile. + * 2. A fallback QoS profile if the offered and requested profiles are different. + * + * \param topic_name The full name of the topic, with namespace (ex. /arm/joint_status). + * \return The QoS profile to be used for subscribing. + */ + rclcpp::QoS subscription_qos_for_topic(const std::string & topic_name); // Serialize all currently offered QoS profiles for a topic into a YAML list. std::string serialized_offered_qos_profiles_for_topic(const std::string & topic_name); From 64fac9366bb2228d611fcc4692a497fb22f3ecf2 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Fri, 3 Apr 2020 14:08:36 +0000 Subject: [PATCH 07/13] Add qos sub flag Signed-off-by: Anas Abou Allaban --- rosbag2_transport/CMakeLists.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index f5fafca5f7..36b7899c34 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -15,6 +15,9 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") add_compile_options(-Wall -Wextra -Wpedantic -Werror) endif() +# TODO(piraka9011) Remove once testing is finished +add_definitions(-DROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION) + # Windows supplies macros for min and max by default. We should only use min and max from stl if(WIN32) add_definitions(-DNOMINMAX) From 5c0c12caef129d7250499377c0484ee122cf0498 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Fri, 3 Apr 2020 22:17:11 +0000 Subject: [PATCH 08/13] Move ifdefs, update comments, fix include paths Signed-off-by: Anas Abou Allaban --- .../src/rosbag2_transport/recorder.cpp | 8 +++----- .../src/rosbag2_transport/recorder.hpp | 18 +++--------------- .../test/rosbag2_transport/test_record.cpp | 4 ++-- 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index b4fd043739..cb72a57b08 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -150,12 +150,7 @@ void Recorder::subscribe_topic(const rosbag2_storage::TopicMetadata & topic) // that callback called before we reached out the line: writer_->create_topic(topic) writer_->create_topic(topic); - // TODO(emersonknapp) re-enable subscription_qos_for_topic once the cyclone situation is resolved - #ifdef ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION Rosbag2QoS subscription_qos{subscription_qos_for_topic(topic.name)}; - #else - Rosbag2QoS subscription_qos{}; - #endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION auto subscription = create_subscription(topic.name, topic.type, subscription_qos); if (subscription) { subscriptions_.insert({topic.name, subscription}); @@ -213,6 +208,8 @@ rclcpp::QoS Recorder::subscription_qos_for_topic(const std::string & topic_name) subscription_qos = topic_qos_profile_overrides_.at(topic_name); ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); } + // TODO(emersonknapp) re-enable subscription_qos_for_topic once the cyclone situation is resolved +#ifdef ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION auto endpoints = node_->get_publishers_info_by_topic(topic_name); if (!endpoints.empty() && all_qos_same(endpoints)) { return Rosbag2QoS(endpoints[0].qos_profile()).default_history(); @@ -222,6 +219,7 @@ rclcpp::QoS Recorder::subscription_qos_for_topic(const std::string & topic_name) "Cannot determine what QoS to request, falling back to default QoS profile." ); topics_warned_about_incompatibility_.insert(topic_name); +#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION return subscription_qos; } diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.hpp b/rosbag2_transport/src/rosbag2_transport/recorder.hpp index fa8f7f12bf..a00dfe5547 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.hpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.hpp @@ -23,19 +23,6 @@ #include #include -#ifdef _WIN32 -// This is necessary because of a bug in yaml-cpp's cmake -#define YAML_CPP_DLL -// This is necessary because yaml-cpp does not always use dllimport/dllexport consistently -# pragma warning(push) -# pragma warning(disable:4251) -# pragma warning(disable:4275) -#endif -#include "yaml-cpp/yaml.h" -#ifdef _WIN32 -# pragma warning(pop) -#endif - #include "rclcpp/qos.hpp" #include "rosbag2_cpp/writer.hpp" @@ -103,8 +90,9 @@ class Recorder * * Profiles are prioritized by: * 1. The override specified in the record_options, if one exists for the topic. - * 2. The common offered QoS profile. - * 2. A fallback QoS profile if the offered and requested profiles are different. + * 2. The publisher's offered QoS profile. + * If all current publishers are offering the exact same compatibility profile. + * 3. The default Rosbag2QoS profile, if the above conditions are not met. * * \param topic_name The full name of the topic, with namespace (ex. /arm/joint_status). * \return The QoS profile to be used for subscribing. diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 3253f34328..413c806ed0 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -21,7 +21,7 @@ #include "rclcpp/rclcpp.hpp" -#include "../../src/rosbag2_transport/qos.hpp" +#include "qos.hpp" #include "rosbag2_transport/rosbag2_transport.hpp" #include "test_msgs/msg/arrays.hpp" @@ -140,6 +140,7 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) { EXPECT_EQ(recorded_topics.size(), 1u); EXPECT_FALSE(recorded_messages.empty()); } +#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) { @@ -174,4 +175,3 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) ASSERT_GE(recorded_messages.size(), 0u); } -#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION From e463ed935c900af46ff678afdd1d69323c285c81 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Mon, 6 Apr 2020 18:35:19 +0000 Subject: [PATCH 09/13] Fix merge conflicts Signed-off-by: Anas Abou Allaban --- rosbag2_transport/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index 36b7899c34..753877891c 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -155,7 +155,8 @@ function(create_tests_for_rmw_implementation) test/rosbag2_transport/test_record.cpp ${SKIP_TEST} LINK_LIBS rosbag2_transport - AMENT_DEPS test_msgs rosbag2_test_common) + AMENT_DEPS test_msgs rosbag2_test_common + INCLUDE_DIRS src/rosbag2_transport) rosbag2_transport_add_gmock(test_play test/rosbag2_transport/test_play.cpp From e6f7ee28b0d07d5d2988f07af6a2a517971488fe Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Mon, 6 Apr 2020 19:01:07 +0000 Subject: [PATCH 10/13] Remove magic numbers, use KeepAll history Signed-off-by: Anas Abou Allaban --- .../include/rosbag2_test_common/publisher_manager.hpp | 2 +- rosbag2_transport/src/rosbag2_transport/recorder.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp b/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp index 3baa19cc52..f5c376b03b 100644 --- a/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp +++ b/rosbag2_test_common/include/rosbag2_test_common/publisher_manager.hpp @@ -78,7 +78,7 @@ class PublisherManager const std::string & topic_name, std::shared_ptr message, size_t expected_messages = 0, - const rclcpp::QoS & qos = rclcpp::QoS{10}) + const rclcpp::QoS & qos = rclcpp::QoS{rclcpp::KeepAll()}) { auto node_name = std::string("publisher") + std::to_string(counter_++); auto publisher_node = std::make_shared( diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index cb72a57b08..47ded97090 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -203,7 +203,7 @@ std::string Recorder::serialized_offered_qos_profiles_for_topic(const std::strin rclcpp::QoS Recorder::subscription_qos_for_topic(const std::string & topic_name) { - rclcpp::QoS subscription_qos{10}; + rclcpp::QoS subscription_qos{rclcpp::KeepAll()}; if (topic_qos_profile_overrides_.count(topic_name)) { subscription_qos = topic_qos_profile_overrides_.at(topic_name); ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); From 422ffbe68ef6820f28a9201fe646f04641b34468 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Mon, 6 Apr 2020 19:40:01 +0000 Subject: [PATCH 11/13] Alpha sort, remove extra numbers, comments Signed-off-by: Anas Abou Allaban --- rosbag2_transport/CMakeLists.txt | 6 +++--- .../test/rosbag2_transport/test_record.cpp | 18 ++++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index 753877891c..de2335df2a 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -153,10 +153,10 @@ function(create_tests_for_rmw_implementation) rosbag2_transport_add_gmock(test_record test/rosbag2_transport/test_record.cpp - ${SKIP_TEST} - LINK_LIBS rosbag2_transport AMENT_DEPS test_msgs rosbag2_test_common - INCLUDE_DIRS src/rosbag2_transport) + INCLUDE_DIRS src + LINK_LIBS rosbag2_transport + ${SKIP_TEST}) rosbag2_transport_add_gmock(test_play test/rosbag2_transport/test_play.cpp diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 413c806ed0..473c0f69b4 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -21,7 +21,7 @@ #include "rclcpp/rclcpp.hpp" -#include "qos.hpp" +#include "rosbag2_transport/qos.hpp" #include "rosbag2_transport/rosbag2_transport.hpp" #include "test_msgs/msg/arrays.hpp" @@ -144,17 +144,17 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) { TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) { + const auto num_expected_msgs = 3; auto strict_msg = std::make_shared(); strict_msg->string_value = "strict"; - std::string strict_topic = "/strict_topic"; + const auto strict_topic = "/strict_topic"; rosbag2_transport::RecordOptions record_options = {false, false, {strict_topic}, "rmw_format", 100ms}; - // 0 means system default for all options - const auto profile_override = rclcpp::QoS{10} - .best_effort().durability_volatile().keep_all().avoid_ros_namespace_conventions(false); + const auto profile_override = rclcpp::QoS{rclcpp::KeepAll()} + .best_effort().durability_volatile().avoid_ros_namespace_conventions(false); std::unordered_map topic_qos_profile_overrides = { - {"/strict_topic", profile_override} + {strict_topic, profile_override} }; record_options.topic_qos_profile_overrides = topic_qos_profile_overrides; @@ -162,8 +162,10 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) // If no override is specified, then the recorder cannot see any published messages. auto profile1 = rosbag2_transport::Rosbag2QoS{}.best_effort().durability_volatile(); auto profile2 = rosbag2_transport::Rosbag2QoS{}.best_effort().transient_local(); - pub_man_.add_publisher(strict_topic, strict_msg, 3, profile1); - pub_man_.add_publisher(strict_topic, strict_msg, 3, profile2); + pub_man_.add_publisher( + strict_topic, strict_msg, num_expected_msgs, profile1); + pub_man_.add_publisher( + strict_topic, strict_msg, num_expected_msgs, profile2); start_recording(record_options); run_publishers(); From ee80ffdb0041f63d0d726a2eb640da6301540d2f Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Tue, 7 Apr 2020 08:41:48 -0400 Subject: [PATCH 12/13] Clean up Signed-off-by: Anas Abou Allaban --- rosbag2_transport/CMakeLists.txt | 3 --- .../src/rosbag2_transport/recorder.cpp | 20 +++++++++++++++++-- .../src/rosbag2_transport/recorder.hpp | 4 ++-- .../test/rosbag2_transport/test_record.cpp | 8 ++++---- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index de2335df2a..cb00ee96c2 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -15,9 +15,6 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") add_compile_options(-Wall -Wextra -Wpedantic -Werror) endif() -# TODO(piraka9011) Remove once testing is finished -add_definitions(-DROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION) - # Windows supplies macros for min and max by default. We should only use min and max from stl if(WIN32) add_definitions(-DNOMINMAX) diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.cpp b/rosbag2_transport/src/rosbag2_transport/recorder.cpp index 47ded97090..09f69d0b7e 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.cpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.cpp @@ -31,8 +31,23 @@ #include "qos.hpp" #include "rosbag2_node.hpp" +#ifdef _WIN32 +// This is necessary because of a bug in yaml-cpp's cmake +#define YAML_CPP_DLL +// This is necessary because yaml-cpp does not always use dllimport/dllexport consistently +# pragma warning(push) +# pragma warning(disable:4251) +# pragma warning(disable:4275) +#endif +#include "yaml-cpp/yaml.h" +#ifdef _WIN32 +# pragma warning(pop) +#endif + namespace { +// TODO(emersonknapp) re-enable subscription_qos_for_topic once the cyclone situation is resolved +#ifdef ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION bool all_qos_same(const std::vector & values) { auto adjacent_different_elements_it = std::adjacent_find( @@ -45,6 +60,7 @@ bool all_qos_same(const std::vector & values) // No adjacent elements were different, so all are the same. return adjacent_different_elements_it == values.end(); } +#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION } // unnamed namespace namespace rosbag2_transport @@ -209,7 +225,7 @@ rclcpp::QoS Recorder::subscription_qos_for_topic(const std::string & topic_name) ROSBAG2_TRANSPORT_LOG_INFO_STREAM("Overriding subscription profile for " << topic_name); } // TODO(emersonknapp) re-enable subscription_qos_for_topic once the cyclone situation is resolved -#ifdef ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION + #ifdef ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION auto endpoints = node_->get_publishers_info_by_topic(topic_name); if (!endpoints.empty() && all_qos_same(endpoints)) { return Rosbag2QoS(endpoints[0].qos_profile()).default_history(); @@ -219,7 +235,7 @@ rclcpp::QoS Recorder::subscription_qos_for_topic(const std::string & topic_name) "Cannot determine what QoS to request, falling back to default QoS profile." ); topics_warned_about_incompatibility_.insert(topic_name); -#endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION + #endif // ROSBAG2_ENABLE_ADAPTIVE_QOS_SUBSCRIPTION return subscription_qos; } diff --git a/rosbag2_transport/src/rosbag2_transport/recorder.hpp b/rosbag2_transport/src/rosbag2_transport/recorder.hpp index a00dfe5547..2311b5839f 100644 --- a/rosbag2_transport/src/rosbag2_transport/recorder.hpp +++ b/rosbag2_transport/src/rosbag2_transport/recorder.hpp @@ -90,8 +90,8 @@ class Recorder * * Profiles are prioritized by: * 1. The override specified in the record_options, if one exists for the topic. - * 2. The publisher's offered QoS profile. - * If all current publishers are offering the exact same compatibility profile. + * 2. The publisher's offered QoS profile if all current publishers are offering the exact same + * compatibility profile. * 3. The default Rosbag2QoS profile, if the above conditions are not met. * * \param topic_name The full name of the topic, with namespace (ex. /arm/joint_status). diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 473c0f69b4..0e2301596d 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -144,7 +144,7 @@ TEST_F(RecordIntegrationTestFixture, records_sensor_data) { TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) { - const auto num_expected_msgs = 3; + const auto num_msgs = 3; auto strict_msg = std::make_shared(); strict_msg->string_value = "strict"; const auto strict_topic = "/strict_topic"; @@ -158,14 +158,14 @@ TEST_F(RecordIntegrationTestFixture, topic_qos_overrides) }; record_options.topic_qos_profile_overrides = topic_qos_profile_overrides; - // Create two publishers on the same topic with different QoS profiles. + // Create two BEST_EFFORT publishers on the same topic with different Durability policies. // If no override is specified, then the recorder cannot see any published messages. auto profile1 = rosbag2_transport::Rosbag2QoS{}.best_effort().durability_volatile(); auto profile2 = rosbag2_transport::Rosbag2QoS{}.best_effort().transient_local(); pub_man_.add_publisher( - strict_topic, strict_msg, num_expected_msgs, profile1); + strict_topic, strict_msg, num_msgs, profile1); pub_man_.add_publisher( - strict_topic, strict_msg, num_expected_msgs, profile2); + strict_topic, strict_msg, num_msgs, profile2); start_recording(record_options); run_publishers(); From 398cc7fa517d8fcfe61656fc5b1dc73e65d5dee4 Mon Sep 17 00:00:00 2001 From: Anas Abou Allaban Date: Tue, 7 Apr 2020 08:45:47 -0400 Subject: [PATCH 13/13] Fix include dirs Signed-off-by: Anas Abou Allaban --- rosbag2_transport/CMakeLists.txt | 2 +- rosbag2_transport/test/rosbag2_transport/test_record.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index cb00ee96c2..4cd64a0386 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -151,7 +151,7 @@ function(create_tests_for_rmw_implementation) rosbag2_transport_add_gmock(test_record test/rosbag2_transport/test_record.cpp AMENT_DEPS test_msgs rosbag2_test_common - INCLUDE_DIRS src + INCLUDE_DIRS $ LINK_LIBS rosbag2_transport ${SKIP_TEST}) diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index 0e2301596d..efe1a75c54 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -21,7 +21,7 @@ #include "rclcpp/rclcpp.hpp" -#include "rosbag2_transport/qos.hpp" +#include "qos.hpp" #include "rosbag2_transport/rosbag2_transport.hpp" #include "test_msgs/msg/arrays.hpp"