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

Set rmw log severity #918

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

Set rmw log severity #918

Show file tree
Hide file tree
Changes from all commits
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
9 changes: 9 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,14 @@ 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

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

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

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_UNSUPPORTED == rmw_status) {
RCUTILS_SAFE_FWRITE_TO_STDERR("rmw_set_log_severity not supported by rmw implementation\n");
} else if (RMW_RET_OK != rmw_status) {
return RCL_RET_ERROR;
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
}
}

for (size_t i = 0; i < log_levels->num_logger_settings; ++i) {
rcutils_ret_t rcutils_status = rcutils_logging_set_logger_level(
Expand Down
37 changes: 37 additions & 0 deletions rcl/test/rcl/test_logging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@

#include "rcutils/logging_macros.h"

#include "rmw/rmw.h"

#include "../mocking_utils/patch.hpp"

// Define dummy comparison operators for rcutils_allocator_t type
Expand Down Expand Up @@ -195,6 +197,41 @@ TEST(TestLogging, test_failing_logger_level_configure) {
}
}

TEST(TestLogging, test_failing_rmw_set_log_severity) {
const char * argv[] = {
"test_logging", RCL_ROS_ARGS_FLAG,
RCL_LOG_LEVEL_FLAG, "debug"};
const int argc = sizeof(argv) / sizeof(argv[0]);
rcl_allocator_t default_allocator = rcl_get_default_allocator();
rcl_arguments_t global_arguments = rcl_get_zero_initialized_arguments();
ASSERT_EQ(RCL_RET_OK, rcl_parse_arguments(argc, argv, default_allocator, &global_arguments)) <<
rcl_get_error_string().str;
OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT(
{
EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&global_arguments)) << rcl_get_error_string().str;
});

{
auto mock = mocking_utils::patch_to_fail(
"lib:rcl", rmw_set_log_severity, "failed", RMW_RET_ERROR);
EXPECT_EQ(RCL_RET_ERROR, rcl_logging_configure(&global_arguments, &default_allocator));
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

EXPECT_EQ(RCL_RET_OK, rcl_logging_fini()) << rcl_get_error_string().str;
}

{
auto mock = mocking_utils::patch_to_fail(
"lib:rcl", rmw_set_log_severity, "not supported", RMW_RET_UNSUPPORTED);
EXPECT_EQ(RCL_RET_OK, rcl_logging_configure(&global_arguments, &default_allocator));
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

EXPECT_EQ(RCL_RET_OK, rcl_logging_fini()) << rcl_get_error_string().str;
}
}

TEST(TestLogging, test_failing_external_logging) {
const char * argv[] = {
"test_logging", RCL_ROS_ARGS_FLAG,
Expand Down