-
Notifications
You must be signed in to change notification settings - Fork 506
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
Fix Servo logging frequency #457
Conversation
Codecov Report
@@ Coverage Diff @@
## main #457 +/- ##
=======================================
Coverage 51.43% 51.43%
=======================================
Files 223 223
Lines 23339 23339
=======================================
Hits 12002 12002
Misses 11337 11337
Continue to review full report at Codecov.
|
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 looks reasonable and good to me.
@@ -50,7 +50,7 @@ | |||
using namespace std::chrono_literals; // for s, ms, etc. | |||
|
|||
static const rclcpp::Logger LOGGER = rclcpp::get_logger("moveit_servo.servo_calcs"); | |||
constexpr auto ROS_LOG_THROTTLE_PERIOD = std::chrono::nanoseconds(30ms).count(); | |||
constexpr size_t ROS_LOG_THROTTLE_PERIOD = 3000; // milliseconds |
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.
Huh, what's the motivation to stop using std::chrono
and count
?
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 just think 3000; // milliseconds
is more readable than std::chrono::...whatever
. I would have to google how to use std::chrono properly.
Will change if you feel strongly though.
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.
How about 3000ms
(https://en.cppreference.com/w/cpp/chrono/operator%22%22ms)?
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.
In general, I prefer expressiveness through types and not comments (though I like comments).
The issue becomes, it's immediately cast to a size_t
which definitely doesn't enforce milliseconds.
I would prefer the use of chrono
but won't be a stickler for it here because that type safety should really be propagated to point of use (which I guess is a `ros::Duration, so that should really be the type here.
@henningkayser he'd have to do constexpr size_t ROS_LOG_THROTTLE_PERIOD = (3000ms).count();
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.
ROS_LOG_THROTTLE_PERIOD
doesn't need to be a size_t
, it could be an rclcpp::Duration
that returns nanoseconds()
to the logging call. I find it a little odd that the logging macro doesn't support the actual duration type.
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.
Absolutely, though I assumed increasing the magnitude of the change would have been out of scope for this PR. My guess is that ROS_INFO
just wraps printf
and so has a limited type set that it can support. ROS_INFO_STREAM
uses the insertion operator so could print more interesting things.
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 changed it to std::chrono::milliseconds(3000).count();
00d2611
to
e2f4683
Compare
A small fix to correct logger frequencies.
Prior to this, the messages only printed once.
main branch, message only prints once: (This is a gif, but nothing changes)
With this PR, prints every 3s: