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.

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

Set rmw log severity #918

Draft
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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
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