Make use of time source type for throttling logs #183
Conversation
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
test/test_logging_macros.cpp
Outdated
| @@ -154,7 +154,7 @@ TEST_F(TestLoggingMacros, test_logging_throttle) { | |||
| auto is_before_throttle_end = | |||
| ((std::chrono::system_clock::now() - start) < throttle_time); | |||
| RCUTILS_LOG_ERROR_THROTTLE( | |||
| RCUTILS_STEADY_TIME, throttle_time.count(), first ? "first" : "other"); | |||
| rcutils_steady_time_now, throttle_time.count(), first ? "first" : "other"); | |||
There was a problem hiding this comment.
@BMarchi nit: why not keeping the function behind the macro? RCUTILS_STEADY_TIME reads better.
| @@ -54,7 +54,8 @@ | |||
| skipfirst_doc_lines = [ | |||
| 'The first log call is being ignored but all subsequent calls are being processed.'] | |||
| throttle_params = OrderedDict(( | |||
| ('time_source_type', 'The time source type of the time to be used'), | |||
| ('time_source_type', 'Function that returns rcutils_ret_t and expects a ' | |||
| 'rcutils_time_point_value_t pointer.'), | |||
There was a problem hiding this comment.
@BMarchi nit: I wonder if time_source makes more sense than time_source_type. @dirk-thomas thoughts?
There was a problem hiding this comment.
Well, "time source" has a very specific meaning in rclcpp and since this is not the same I would avoid using the same term for it. See my other comment for a suggestion.
There was a problem hiding this comment.
So should I call it then get_time_point_value?
rcutils/logging.py
Outdated
| @@ -54,7 +54,8 @@ | |||
| skipfirst_doc_lines = [ | |||
| 'The first log call is being ignored but all subsequent calls are being processed.'] | |||
| throttle_params = OrderedDict(( | |||
| ('time_source_type', 'The time source type of the time to be used'), | |||
| ('time_source_type', 'Function that returns rcutils_ret_t and expects a ' | |||
There was a problem hiding this comment.
How about renaming this parameter to match the new semantic, e.g. get_time_point_value?
There was a problem hiding this comment.
That goes against #183 (comment). Do we want to be explicit about the parameter being a function or do we want to hide it for the sake of API ergonomics? I'm slightly inclined towards the latter but it's not a strong stand.
There was a problem hiding this comment.
The function parameter is being used as a function:
if (time_source_type(&__rcutils_logging_now) != RCUTILS_RET_OK) {
There is no way you can just pass RCUTILS_STEADY_TIME (assuming it isn't a macro defining a function).
There was a problem hiding this comment.
That's precisely what I'm proposing, an alias for that function so that it reads better. But it's a detail and orthogonal to the actual macro parameter name.
There was a problem hiding this comment.
And that's what @BMarchi implemented in e1b4344. WDYT @dirk-thomas ?
There was a problem hiding this comment.
My previous comment still applies: it is not a time source so it should not be named time_source imo.
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
…mbinations Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
rcutils/logging.py
Outdated
| @@ -168,7 +169,7 @@ def __init__(self, *, params=None, args=None, doc_lines=None): | |||
| }, **name_args | |||
| }, | |||
| doc_lines=skipfirst_doc_lines + throttle_doc_lines + name_doc_lines)), | |||
| )) | |||
| )) | |||
There was a problem hiding this comment.
This looks like an unrelated and unnecessary change?
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
include/rcutils/time.h
Outdated
| @@ -39,6 +39,8 @@ extern "C" | |||
| #define RCUTILS_NS_TO_MS(nanoseconds) (nanoseconds / (1000LL * 1000LL)) | |||
| /// Convenience macro to convert nanoseconds to microseconds. | |||
| #define RCUTILS_NS_TO_US(nanoseconds) (nanoseconds / 1000LL) | |||
| /// Convenience macro to reference the function for system time. | |||
There was a problem hiding this comment.
@BMarchi nit: ref that function
Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Part of 797. Gives meaning to the time_source_type argument to throttle macros.