Conversation
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.
Minor changes for documentation and a minor revert.
system_metrics_collector/src/system_metrics_collector/linux_cpu_measurement_node.hpp
Show resolved
Hide resolved
system_metrics_collector/src/system_metrics_collector/linux_memory_measurement_node.hpp
Outdated
Show resolved
Hide resolved
system_metrics_collector/src/system_metrics_collector/periodic_measurement_node.cpp
Outdated
Show resolved
Hide resolved
if (!publish_timer_ && | ||
publish_period_ != PeriodicMeasurementNode::DEFAULT_PUBLISH_WINDOW) | ||
{ | ||
if (!publish_timer_ && publish_period_ > std::chrono::milliseconds(0)) { |
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.
Checking in the start method is too late: the publish period (and measurement period) should be checked upon construction.
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 unit tests are currently deliberately passing in invalid publish_period
s to avoid this Collector::clearCurrentMeasurements()
call.
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.
Then why not make zero valid and document as such? We only have an issue if the period is strictly less than zero.
@@ -88,7 +83,7 @@ std::string PeriodicMeasurementNode::getStatusString() const | |||
", measurement_period=" << std::to_string(measurement_period_.count()) << "ms" << | |||
", publishing_topic=" << publishing_topic_ << | |||
", publish_period=" << | |||
(publish_period_ != PeriodicMeasurementNode::DEFAULT_PUBLISH_WINDOW ? | |||
(publish_period_ > std::chrono::milliseconds(0) ? |
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.
If done in construction then this is not needed.
Remove the
DEFAULT_PUBLISH_WINDOW
sentinel value as the publishing period should not be optional anymore.