Skip to content
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.

Sign up for GitHub

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

Make use of time source type for throttling logs #183

Merged
merged 6 commits into from
Oct 21, 2019
Merged

Make use of time source type for throttling logs #183

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77d0072
Make use of time source type for throttling logs
BMarchi Oct 1, 2019
e1b4344
Change macro parameter name
BMarchi Oct 2, 2019
82f76c1
Change time_source signature and add method to reconstruct feature co…
BMarchi Oct 4, 2019
62ab5e2
Revert changes
BMarchi Oct 4, 2019
bbe16af
Remove extra tab
BMarchi Oct 7, 2019
0815d74
Add link to function in macro
BMarchi Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/rcutils/time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BMarchi nit: ref that function

#define RCUTILS_STEADY_TIME rcutils_steady_time_now
wjwwood marked this conversation as resolved.
Show resolved Hide resolved

/// A single point in time, measured in nanoseconds since the Unix epoch.
typedef int64_t rcutils_time_point_value_t;
Expand Down
7 changes: 4 additions & 3 deletions rcutils/logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,12 @@
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'),
('get_time_point_value', 'Function that returns rcutils_ret_t and expects a '
'rcutils_time_point_value_t pointer.'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BMarchi nit: I wonder if time_source makes more sense than time_source_type. @dirk-thomas thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@BMarchi BMarchi Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I call it then get_time_point_value?

('duration', 'The duration of the throttle interval'),
))
throttle_args = {
'condition_before': 'RCUTILS_LOG_CONDITION_THROTTLE_BEFORE(time_source_type, duration)',
'condition_before': 'RCUTILS_LOG_CONDITION_THROTTLE_BEFORE(get_time_point_value, duration)',
'condition_after': 'RCUTILS_LOG_CONDITION_THROTTLE_AFTER'}
throttle_doc_lines = [
'Log calls are being ignored if the last logged message is not longer ago than the specified '
Expand Down Expand Up @@ -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)),
))
))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an unrelated and unnecessary change?



def get_macro_parameters(feature_combination):
Expand Down
4 changes: 2 additions & 2 deletions resource/logging_macros.h.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@ typedef bool (* RclLogFilter)();
* \def RCUTILS_LOG_CONDITION_THROTTLE_BEFORE
* A macro initializing and checking the `throttle` condition.
*/
#define RCUTILS_LOG_CONDITION_THROTTLE_BEFORE(time_source_type, duration) { \
#define RCUTILS_LOG_CONDITION_THROTTLE_BEFORE(get_time_point_value, duration) { \
static rcutils_duration_value_t __rcutils_logging_duration = RCUTILS_MS_TO_NS((rcutils_duration_value_t)duration); \
static rcutils_time_point_value_t __rcutils_logging_last_logged = 0; \
rcutils_time_point_value_t __rcutils_logging_now = 0; \
bool __rcutils_logging_condition = true; \
if (rcutils_steady_time_now(&__rcutils_logging_now) != RCUTILS_RET_OK) { \
if (get_time_point_value(&__rcutils_logging_now) != RCUTILS_RET_OK) { \
rcutils_log( \
&__rcutils_logging_location, RCUTILS_LOG_SEVERITY_ERROR, "", \
"%s() at %s:%d getting current steady time failed\n", \
Expand Down