Use rclcpp::NodeOptions during node construction #73

mm318 · 2020-01-15T22:38:58Z

In order to support composeable nodes, the configurations for the node need to come from the rclcpp::NodeOptions as opposed to from C++ parameters in code.

mm318 · 2020-01-15T22:39:16Z

Still need to fix some minor unit test failures.

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp

system_metrics_collector/src/system_metrics_collector/main.cpp

thomas-moulard

SGTM - a few nits.

Why do we need to rename the topic? I think we can remove kTestTopic and use the real topic, no?

system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.hpp

system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.cpp

system_metrics_collector/src/system_metrics_collector/linux_memory_measurement_node.cpp

system_metrics_collector/src/system_metrics_collector/linux_memory_measurement_node.hpp

system_metrics_collector/src/system_metrics_collector/linux_process_cpu_measurement_node.cpp

dabonnie · 2020-01-15T23:54:26Z

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp

@@ -104,7 +122,7 @@ std::string PeriodicMeasurementNode::GetStatusString() const
  std::stringstream ss;
  ss << "name=" << get_name() <<
    ", measurement_period=" << std::to_string(measurement_period_.count()) << "ms" <<
-    ", publishing_topic=" << publishing_topic_ <<
+    ", publishing_topic=" << ((publisher_ == nullptr) ? "" : publisher_->get_topic_name()) <<


The .hpp still has publishing_topic_. Why tie displaying the topic to the node state?

If displaying the topic doesn't depend on the node state, then when the user does topic name remapping, the output is technically wrong.

Could this be simplified to publisher_ ? publisher_->get_topic_name() : ""

dabonnie · 2020-01-15T23:55:08Z

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.hpp

  /**
   * The period used to publish measurement data
   */
-  const std::chrono::milliseconds publish_period_;
+  std::chrono::milliseconds publish_period_;


Is this used anymore?

The value is coming from the parameters and stored here. Is your idea that the parameters values should be read at Start() and not stored, so that each Stop()->Start() cycle might have updated/different parameter values?

Sorry, this is the wrong line. I meant for line 107 (std::string publishing_topic_;). Mainly because there is a change with the status string that does not used publishing_topic_.

Okay, I removed the publishing_topic_ member variable.

system_metrics_collector/test/system_metrics_collector/test_linux_memory_measurement.cpp

...trics_collector/test/system_metrics_collector/test_linux_process_memory_measurement_node.cpp

dabonnie · 2020-01-15T23:57:28Z

system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp

@@ -130,8 +142,17 @@ constexpr std::chrono::milliseconds PeriodicMeasurementTestFixure::kDontPublishD

 TEST_F(PeriodicMeasurementTestFixure, Sanity) {
  ASSERT_NE(test_periodic_measurer_, nullptr);
+
+  ASSERT_EQ("name=test_periodic_node, measurement_period=50ms,"
+    " publishing_topic=, publish_period=500ms, started=false,"


An empty publishing_topic isn't indicative of what was actually set. I'm not a fan of this change. If we want to tie it to publisher state then I would only display when started.

Okay, changed this test to call Start() on the measurement node beforehand.

dabonnie · 2020-01-15T23:59:39Z

Why do we need to rename the topic? I think we can remove kTestTopic and use the real topic, no?

I agree with this, since it seems redundant to test the ROS2 parameter code.

mm318 · 2020-01-16T17:44:26Z

Why do we need to rename the topic? I think we can remove kTestTopic and use the real topic, no?

I have removed the remapping of the publishing topic to kTestTopic.

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp

..._metrics_collector/test/system_metrics_collector/test_linux_process_cpu_measurement_node.cpp

system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp

dabonnie · 2020-01-16T18:05:26Z

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp

@@ -68,7 +75,8 @@ bool PeriodicMeasurementNode::SetupStart()
    measurement_period_, [this]() {this->PerformPeriodicMeasurement();});

  if (publisher_ == nullptr) {
-    publisher_ = create_publisher<MetricsMessage>(publishing_topic_, 10 /*history_depth*/);
+    publisher_ = create_publisher<MetricsMessage>(collector_node_constants::kStatisticsTopicName,


Oh I see what you're doing now. Why aren't we keeping the member? This PR is actually removing functionality where we could pull the publishing topic out of NodeOptions. The work required seems minimal and I don't see why we can't do it, as a read only parameter, here.

This PR is actually removing functionality where we could pull the publishing topic out of NodeOptions.

I was going to do that, too, but this is what @thomas-moulard suggested under on the same rationale as the node name.

I took @thomas-moulard 's comment to mean we don't need to set and test a custom topic name in test. I'm going to follow up with him as we've discussed allowing a user to configure the topic name.

This was discussed in the team chatroom. Topic name will be configured via remapping (--ros-args --remap system_metrics:=custom_topic_name).

--ros-args --remap

Got it, found this explanation: https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#example

system_metrics_collector/src/system_metrics_collector/linux_cpu_collector.cpp

system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.hpp

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp

piraka9011

This LGTM

zmichaels11 · 2020-01-16T22:00:25Z

system_metrics_collector/src/system_metrics_collector/constants.hpp

@@ -25,10 +26,11 @@ namespace collector_node_constants
 constexpr const char kStatisticsTopicName[] = "system_metrics";

 constexpr const char kCollectPeriodParam[] = "measurement_period";
-constexpr const int64_t kDefaultCollectPeriodInMs = 1000;   // 1 second
+constexpr const std::chrono::milliseconds kDefaultCollectPeriod{1000};    // 1 second


Why is this still in milliseconds when the delay is now 1 second?

I need to get the count() in milliseconds at some point.

system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.hpp

system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp

dabonnie · 2020-01-17T00:12:28Z

Build successful but codecov still broken. FYI @ros2/aws-oncall

mm318 added 3 commits January 15, 2020 13:19

convert node constructors to use rclcpp::NodeOptions

f4ad662

fix some tests

c13910b

fix linter issues

27309ec

mm318 commented Jan 15, 2020

View reviewed changes

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Outdated Show resolved Hide resolved

mm318 commented Jan 15, 2020

View reviewed changes

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Outdated Show resolved Hide resolved

fix some unit test failures

e82645e

thomas-moulard reviewed Jan 15, 2020

View reviewed changes

system_metrics_collector/src/system_metrics_collector/main.cpp Outdated Show resolved Hide resolved

thomas-moulard reviewed Jan 15, 2020

View reviewed changes

dabonnie suggested changes Jan 15, 2020

View reviewed changes

ahcorde mentioned this pull request Jan 16, 2020

Added publish period attribute as a parameter #71

Closed

address PR comments

caa445f

mm318 requested review from dabonnie and thomas-moulard January 16, 2020 17:44

piraka9011 reviewed Jan 16, 2020

View reviewed changes

system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp Show resolved Hide resolved

piraka9011 reviewed Jan 16, 2020

View reviewed changes

..._metrics_collector/test/system_metrics_collector/test_linux_process_cpu_measurement_node.cpp Outdated Show resolved Hide resolved

piraka9011 reviewed Jan 16, 2020

View reviewed changes

system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp Outdated Show resolved Hide resolved

piraka9011 reviewed Jan 16, 2020

View reviewed changes

system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp Outdated Show resolved Hide resolved

dabonnie reviewed Jan 16, 2020

View reviewed changes

zmichaels11 reviewed Jan 16, 2020

View reviewed changes

mm318 added 2 commits January 16, 2020 10:38

address more PR comments

d7cc0fa

move constructor parameter validation tests to separate unit test cases

7a419c1

piraka9011 approved these changes Jan 16, 2020

View reviewed changes

zmichaels11 reviewed Jan 16, 2020

View reviewed changes

dabonnie reviewed Jan 16, 2020

View reviewed changes

system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.hpp Outdated Show resolved Hide resolved

dabonnie reviewed Jan 16, 2020

View reviewed changes

system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.hpp Outdated Show resolved Hide resolved

dabonnie reviewed Jan 16, 2020

View reviewed changes

system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp Outdated Show resolved Hide resolved

dabonnie approved these changes Jan 16, 2020

View reviewed changes

address PR comments

9d770d2

dabonnie approved these changes Jan 16, 2020

View reviewed changes

dabonnie merged commit f7e368c into master Jan 17, 2020

dabonnie deleted the miaofei/use-node-options branch January 17, 2020 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rclcpp::NodeOptions during node construction #73

Use rclcpp::NodeOptions during node construction #73

mm318 commented Jan 15, 2020

mm318 commented Jan 15, 2020

thomas-moulard left a comment

dabonnie Jan 15, 2020

mm318 Jan 16, 2020

piraka9011 Jan 16, 2020

mm318 Jan 16, 2020

dabonnie Jan 15, 2020

mm318 Jan 16, 2020

dabonnie Jan 16, 2020

mm318 Jan 16, 2020

dabonnie Jan 15, 2020

mm318 Jan 16, 2020

dabonnie commented Jan 15, 2020

mm318 commented Jan 16, 2020

dabonnie Jan 16, 2020

mm318 Jan 16, 2020

dabonnie Jan 16, 2020

mm318 Jan 16, 2020 •

edited

dabonnie Jan 16, 2020

piraka9011 left a comment

zmichaels11 Jan 16, 2020

mm318 Jan 16, 2020

dabonnie commented Jan 17, 2020

Use rclcpp::NodeOptions during node construction #73

Use rclcpp::NodeOptions during node construction #73

Conversation

mm318 commented Jan 15, 2020

mm318 commented Jan 15, 2020

thomas-moulard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabonnie commented Jan 15, 2020

mm318 commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm318 Jan 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piraka9011 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabonnie commented Jan 17, 2020

mm318 Jan 16, 2020 •

edited