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

Jump to bottom

[Logging] Clarify duration arg description in macros #359

Merged
merged 1 commit into from
May 17, 2022
Merged

[Logging] Clarify duration arg description in macros #359

merged 1 commit into from
May 17, 2022

Conversation

aprotyas
Copy link
Member

Quoting from ros2/rclcpp#1929:

Currently RCLCPP_*_THROTTLE (and the RCUTILS_LOG_*_THROTTLE_NAMED which they cal) arbitrarily accept the duration as an integral value of milliseconds. This is highly confusing, especially since it is not documented anywhere but deep in the source, in RCUTILS_LOG_CONDITION_THROTTLE_BEFORE, where it calls:

static rcutils_duration_value_t __rcutils_logging_duration = RCUTILS_MS_TO_NS((rcutils_duration_value_t)duration);

The expectation that the duration argument to the logging macros will be an integral value in milliseconds is not explicit at all, and as such the documentation for this argument should be clarified.

Signed-off-by: Abrar Rahman Protyasha aprotyas@u.rochester.edu

@aprotyas
Clarify duration arg description in logging macros
567a688 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
clalancette
clalancette approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This is a nice improvement, thanks @aprotyas ! Will run CI on it next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit 8da2aac into master May 17, 2022
@delete-merged-branch delete-merged-branch bot deleted the aprotyas/clarify_duration_doc branch May 17, 2022 20:41
@aprotyas
Copy link
Member Author

Thanks for the reviews and for merging the PR! I'll open backports to Humble, Galactic, and Foxy since it's backwards compatible a nice QoL improvement on the status quo.

@aprotyas
Copy link
Member Author

@mergify backport humble galactic foxy

mergify bot pushed a commit that referenced this pull request May 18, 2022
@aprotyas @mergify
Clarify duration arg description in logging macros (#359)
b8328ea 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
(cherry picked from commit 8da2aac)
@mergify mergify bot mentioned this pull request May 18, 2022
Merged
mergify bot pushed a commit that referenced this pull request May 18, 2022
@aprotyas @mergify
Clarify duration arg description in logging macros (#359)
ce72355 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
(cherry picked from commit 8da2aac)
@mergify mergify bot mentioned this pull request May 18, 2022
Merged
@mergify
Copy link

mergify bot commented May 18, 2022

backport humble galactic foxy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 18, 2022
@aprotyas @mergify
Clarify duration arg description in logging macros (#359)
79f3421 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
(cherry picked from commit 8da2aac)
@mergify mergify bot mentioned this pull request May 18, 2022
Merged
clalancette pushed a commit that referenced this pull request May 27, 2022
@mergify @aprotyas
Clarify duration arg description in logging macros (#359) (#362)
3d5b2e9 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
(cherry picked from commit 8da2aac)

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
clalancette pushed a commit that referenced this pull request May 27, 2022
@mergify @aprotyas
Clarify duration arg description in logging macros (#359) (#361)
b0ef2a5 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
(cherry picked from commit 8da2aac)

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
clalancette pushed a commit that referenced this pull request May 27, 2022
@mergify @aprotyas
Clarify duration arg description in logging macros (#359) (#360)
6555533 
The expectation that the `duration` argument to the logging macros will
be an integral value in milliseconds is not explicit at all, and as such
the documentation for this argument should be clarified.

Prior to this change, this information could only be found inside
`RCUTILS_LOG_CONDITION_THROTTLE_BEFORE`, which is inconvenient.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
(cherry picked from commit 8da2aac)

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
@fujitatomoya fujitatomoya mentioned this pull request Oct 28, 2022
Open
@danzimmerman danzimmerman mentioned this pull request Oct 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aprotyas @clalancette @fujitatomoya