Skip to content

Set rmw log severity #918

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

Draft
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Draft

Set rmw log severity #918

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c187fcf
Set rmw log severity
christophebedard May 2, 2021
e8a3487
Handle unsupported rmw_set_log_severity and print error
christophebedard May 3, 2021
90ed0db
Add rmw_set_log_severity failure test
christophebedard May 3, 2021
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
7 changes: 7 additions & 0 deletions rcl/src/rcl/logging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ extern "C"
#include "rcl/macros.h"
#include "rcutils/logging.h"
#include "rcutils/time.h"
#include "rmw/rmw.h"

#define RCL_LOGGING_MAX_OUTPUT_FUNCS (4)

Expand Down Expand Up @@ -76,6 +77,12 @@ rcl_logging_configure_with_output_handler(
if (log_levels) {
default_level = (int)log_levels->default_logger_level;
rcutils_logging_set_default_logger_level(default_level);
if (RCUTILS_LOG_SEVERITY_UNSET != default_level) {
rmw_ret_t rmw_status = rmw_set_log_severity((rmw_log_severity_t)default_level);
Copy link
Contributor

@clalancette clalancette May 3, 2021

Choose a reason for hiding this comment

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

What you are doing here makes a certain sense; if we are changing the default logging level, we should change it everywhere.

On the other hand, I'm not sure what semantic we really want --log-level to have. As it stands right now, the semantic is more-or-less "core ROS 2 code". If we add this code in, it becomes "core ROS 2 code + RMW backend code". I'm not entirely sure that's what we want to do.

I think this would be helped by spelling out what the log-levels mean in a more rigorous way, probably as part of ros2/design#314 .

Copy link
Member Author

@christophebedard christophebedard May 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand. The fact that rmw implementations don't support the "unset" logging level/state definitely highlights this.

Are there any plans to tackle that soon-ish after Galactic? I can't commit to doing ros2/design#314, but I can probably help, and I can certainly implement whatever is in the design document for this.

Copy link
Contributor

@clalancette clalancette May 3, 2021

Choose a reason for hiding this comment

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

Are there any plans to tackle that soon-ish after Galactic?

The honest answer is that I don't know right now. We've been concentrating on Galactic, so we don't have a roadmap for H-Turtle yet. I do hope it is something that the ROS 2 community gets to for H-Turtle, as I think it is important.

Copy link
Member Author

@christophebedard christophebedard May 3, 2021

Choose a reason for hiding this comment

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

Got it. I'll see what I can do (in a few weeks) then.

I'll leave this draft here for now.

if (RMW_RET_OK != rmw_status) {
return RCL_RET_ERROR;
}
}

for (size_t i = 0; i < log_levels->num_logger_settings; ++i) {
rcutils_ret_t rcutils_status = rcutils_logging_set_logger_level(
Expand Down