From b79d385be2fc780521d025a3ddb422ac5baa0589 Mon Sep 17 00:00:00 2001 From: Devin Bonnie Date: Mon, 1 Jun 2020 12:44:10 -0700 Subject: [PATCH 1/5] Add check for invalid topic statistics publish period Signed-off-by: Devin Bonnie --- rclcpp/include/rclcpp/create_subscription.hpp | 5 ++++ .../include/rclcpp/subscription_options.hpp | 2 +- .../test_subscription_topic_statistics.cpp | 25 +++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/create_subscription.hpp b/rclcpp/include/rclcpp/create_subscription.hpp index 207bee4a71..2635a61dcd 100644 --- a/rclcpp/include/rclcpp/create_subscription.hpp +++ b/rclcpp/include/rclcpp/create_subscription.hpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -81,6 +82,10 @@ create_subscription( options, *node_topics->get_node_base_interface())) { + if (options.topic_stats_options.publish_period <= std::chrono::milliseconds(0)) { + throw std::invalid_argument("topic statistics publish period must be greater than 0"); + } + std::shared_ptr> publisher = create_publisher( node, diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 5088f35fb1..bd510fe29b 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -66,7 +66,7 @@ struct SubscriptionOptionsBase // Topic to which topic statistics get published when enabled. Defaults to /statistics. std::string publish_topic = "/statistics"; - // Topic statistics publication period in ms. Defaults to one minute. + // Topic statistics publication period in ms. Defaults to one second. std::chrono::milliseconds publish_period{std::chrono::seconds(1)}; }; diff --git a/rclcpp/test/topic_statistics/test_subscription_topic_statistics.cpp b/rclcpp/test/topic_statistics/test_subscription_topic_statistics.cpp index 3a7e90c025..355e6659b5 100644 --- a/rclcpp/test/topic_statistics/test_subscription_topic_statistics.cpp +++ b/rclcpp/test/topic_statistics/test_subscription_topic_statistics.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -223,6 +224,30 @@ class TestSubscriptionTopicStatisticsFixture : public ::testing::Test std::shared_ptr empty_subscriber; }; +TEST(TestSubscriptionTopicStatistics, test_invalid_publish_period) +{ + rclcpp::init(0 /* argc */, nullptr /* argv */); + + auto node = std::make_shared("test_period_node"); + + auto options = rclcpp::SubscriptionOptions(); + options.topic_stats_options.state = rclcpp::TopicStatisticsState::Enable; + options.topic_stats_options.publish_period = std::chrono::milliseconds(0); + + auto callback = [](Empty::UniquePtr msg) { + (void) msg; + }; + + ASSERT_THROW( + (node->create_subscription>( + "should_throw_invalid_arg", + rclcpp::QoS(rclcpp::KeepAll()), + callback, + options)), std::invalid_argument); + + rclcpp::shutdown(); +} + TEST_F(TestSubscriptionTopicStatisticsFixture, test_manual_construction) { // Manually create publisher tied to the node From 93a467667a6dca232a29e81c903136663e1b3064 Mon Sep 17 00:00:00 2001 From: Devin Bonnie Date: Tue, 2 Jun 2020 12:46:01 -0700 Subject: [PATCH 2/5] Update documentation Signed-off-by: Devin Bonnie --- rclcpp/include/rclcpp/create_subscription.hpp | 29 +++++++++++++++---- .../include/rclcpp/subscription_options.hpp | 3 +- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/rclcpp/include/rclcpp/create_subscription.hpp b/rclcpp/include/rclcpp/create_subscription.hpp index 2635a61dcd..360ee854f9 100644 --- a/rclcpp/include/rclcpp/create_subscription.hpp +++ b/rclcpp/include/rclcpp/create_subscription.hpp @@ -42,10 +42,26 @@ namespace rclcpp /// Create and return a subscription of the given MessageT type. /** - * The NodeT type only needs to have a method called get_node_topics_interface() - * which returns a shared_ptr to a NodeTopicsInterface, or be a - * NodeTopicsInterface pointer itself. - */ + * The NodeT type only needs to have a method called get_node_topics_interface() + * which returns a shared_ptr to a NodeTopicsInterface, or be a + * NodeTopicsInterface pointer itself. + * @tparam MessageT + * @tparam CallbackT + * @tparam AllocatorT + * @tparam CallbackMessageT + * @tparam SubscriptionT + * @tparam MessageMemoryStrategyT + * @tparam NodeT + * @param node + * @param topic_name + * @param qos + * @param callback + * @param options + * @param msg_mem_strat + * @return the created subscription + * @throws std::invalid argument if topic statistics is enabled and the publish period is + * less than or equal to zero. + */ template< typename MessageT, typename CallbackT, @@ -83,7 +99,10 @@ create_subscription( *node_topics->get_node_base_interface())) { if (options.topic_stats_options.publish_period <= std::chrono::milliseconds(0)) { - throw std::invalid_argument("topic statistics publish period must be greater than 0"); + throw std::invalid_argument( + "topic_stats_options.publish_period must be greater than 0, specified value of " + + std::to_string(options.topic_stats_options.publish_period.count()) + + " ms"); } std::shared_ptr> publisher = diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index bd510fe29b..44546b85b2 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -66,7 +66,8 @@ struct SubscriptionOptionsBase // Topic to which topic statistics get published when enabled. Defaults to /statistics. std::string publish_topic = "/statistics"; - // Topic statistics publication period in ms. Defaults to one second. + // Topic statistics publication period in ms. Defaults to one second. Only values greater + // than zero are allowed. std::chrono::milliseconds publish_period{std::chrono::seconds(1)}; }; From 73cb6b0ae90530b48acecb685ac0464e5610ac55 Mon Sep 17 00:00:00 2001 From: Devin Bonnie Date: Tue, 2 Jun 2020 16:01:05 -0700 Subject: [PATCH 3/5] Address review comments Signed-off-by: Devin Bonnie --- rclcpp/include/rclcpp/create_subscription.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/create_subscription.hpp b/rclcpp/include/rclcpp/create_subscription.hpp index 360ee854f9..99e680bd50 100644 --- a/rclcpp/include/rclcpp/create_subscription.hpp +++ b/rclcpp/include/rclcpp/create_subscription.hpp @@ -45,6 +45,7 @@ namespace rclcpp * The NodeT type only needs to have a method called get_node_topics_interface() * which returns a shared_ptr to a NodeTopicsInterface, or be a * NodeTopicsInterface pointer itself. + * * @tparam MessageT * @tparam CallbackT * @tparam AllocatorT @@ -59,7 +60,7 @@ namespace rclcpp * @param options * @param msg_mem_strat * @return the created subscription - * @throws std::invalid argument if topic statistics is enabled and the publish period is + * @throws std::invalid_argument if topic statistics is enabled and the publish period is * less than or equal to zero. */ template< From 844c63788f8683539e6e008454f0d24f9deaab49 Mon Sep 17 00:00:00 2001 From: Devin Bonnie Date: Tue, 2 Jun 2020 16:14:26 -0700 Subject: [PATCH 4/5] Address doc formatting comments Signed-off-by: Devin Bonnie --- rclcpp/include/rclcpp/create_subscription.hpp | 30 +++++++++---------- .../include/rclcpp/subscription_options.hpp | 4 +-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/rclcpp/include/rclcpp/create_subscription.hpp b/rclcpp/include/rclcpp/create_subscription.hpp index 99e680bd50..b949e20e8d 100644 --- a/rclcpp/include/rclcpp/create_subscription.hpp +++ b/rclcpp/include/rclcpp/create_subscription.hpp @@ -46,21 +46,21 @@ namespace rclcpp * which returns a shared_ptr to a NodeTopicsInterface, or be a * NodeTopicsInterface pointer itself. * - * @tparam MessageT - * @tparam CallbackT - * @tparam AllocatorT - * @tparam CallbackMessageT - * @tparam SubscriptionT - * @tparam MessageMemoryStrategyT - * @tparam NodeT - * @param node - * @param topic_name - * @param qos - * @param callback - * @param options - * @param msg_mem_strat - * @return the created subscription - * @throws std::invalid_argument if topic statistics is enabled and the publish period is + * \tparam MessageT + * \tparam CallbackT + * \tparam AllocatorT + * \tparam CallbackMessageT + * \tparam SubscriptionT + * \tparam MessageMemoryStrategyT + * \tparam NodeT + * \param node + * \param topic_name + * \param qos + * \param callback + * \param options + * \param msg_mem_strat + * \return the created subscription + * \throws std::invalid_argument if topic statistics is enabled and the publish period is * less than or equal to zero. */ template< diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 44546b85b2..ebf4331c4f 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -66,8 +66,8 @@ struct SubscriptionOptionsBase // Topic to which topic statistics get published when enabled. Defaults to /statistics. std::string publish_topic = "/statistics"; - // Topic statistics publication period in ms. Defaults to one second. Only values greater - // than zero are allowed. + // Topic statistics publication period in ms. Defaults to one second. + // Only values greater than zero are allowed. std::chrono::milliseconds publish_period{std::chrono::seconds(1)}; }; From 6ca32b20c8de453b6838516f2ee70e21f5f186ea Mon Sep 17 00:00:00 2001 From: Devin Bonnie Date: Wed, 3 Jun 2020 09:57:40 -0700 Subject: [PATCH 5/5] Update doc spacing Signed-off-by: Devin Bonnie --- rclcpp/include/rclcpp/create_subscription.hpp | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/rclcpp/include/rclcpp/create_subscription.hpp b/rclcpp/include/rclcpp/create_subscription.hpp index b949e20e8d..2eb24b48b5 100644 --- a/rclcpp/include/rclcpp/create_subscription.hpp +++ b/rclcpp/include/rclcpp/create_subscription.hpp @@ -42,27 +42,27 @@ namespace rclcpp /// Create and return a subscription of the given MessageT type. /** - * The NodeT type only needs to have a method called get_node_topics_interface() - * which returns a shared_ptr to a NodeTopicsInterface, or be a - * NodeTopicsInterface pointer itself. - * - * \tparam MessageT - * \tparam CallbackT - * \tparam AllocatorT - * \tparam CallbackMessageT - * \tparam SubscriptionT - * \tparam MessageMemoryStrategyT - * \tparam NodeT - * \param node - * \param topic_name - * \param qos - * \param callback - * \param options - * \param msg_mem_strat - * \return the created subscription - * \throws std::invalid_argument if topic statistics is enabled and the publish period is - * less than or equal to zero. - */ + * The NodeT type only needs to have a method called get_node_topics_interface() + * which returns a shared_ptr to a NodeTopicsInterface, or be a + * NodeTopicsInterface pointer itself. + * + * \tparam MessageT + * \tparam CallbackT + * \tparam AllocatorT + * \tparam CallbackMessageT + * \tparam SubscriptionT + * \tparam MessageMemoryStrategyT + * \tparam NodeT + * \param node + * \param topic_name + * \param qos + * \param callback + * \param options + * \param msg_mem_strat + * \return the created subscription + * \throws std::invalid_argument if topic statistics is enabled and the publish period is + * less than or equal to zero. + */ template< typename MessageT, typename CallbackT,