Skip to content

Conversation

@christophebedard
Copy link
Member

Closes #915

This makes rcl call rmw_set_log_severity if the default level is set, e.g. with --ros-args --log-level debug.

Signed-off-by: Christophe Bedard [email protected]

Signed-off-by: Christophe Bedard <[email protected]>
@christophebedard
Copy link
Member Author

Not sure if this is the best place to put it, but it feels like the simplest solution, and it's right next to rcutils_logging_set_default_logger_level so it does make sense.

@christophebedard
Copy link
Member Author

I've also added a test: 90ed0db

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.

@clalancette clalancette added the more-information-needed Further information is required label May 20, 2021
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:20
@Yadunund
Copy link
Member

I'd be interested in resurrecting the effort here, especially to address ros2/rmw_zenoh#423.

I'm in favor of having --log-level define the logging level at the RMW level as well since a lot of users are familiar with this arg and in fact, other tier-1 middlewares which rely on RCUTILS_LOG_XX macros seem to honor the --log-level arg. However in rmw_zenoh, we have custom logging macros and having rcl call rmw_set_log_severity would be a convenient way of changing the logging level of the custom logger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

more-information-needed Further information is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make rcl call rmw_set_log_severity

4 participants