search

ros2 / rcl

Library to support implementation of language specific ROS Client Libraries.
Apache License 2.0
130 stars 163 forks source link

Make rcl call `rmw_set_log_severity` #915

Open christophebedard opened 3 years ago

christophebedard commented 3 years ago

Feature request

Feature description

rmw_set_log_severity was added in:

It's implemented by the rmw implementations (Cyclone DDS, FastDDS, Connext), but it doesn't really seem to be used anywhere/by anything.

It would make sense to have rcl (e.g. rcl_logging_configure()?) call rmw_set_log_severity() along with its own logging level configuration. Is there any reason why these two shouldn't be connected?

Implementation considerations

TBD

fujitatomoya commented 3 years ago

i think it makes sense, probably setting default log level during Context::init?

christophebedard commented 3 years ago

assuming you mean rclcpp::Context::init, yeah it does call rcl_init and logging-related rcl_* functions.

I opened a simple draft PR just to have a concrete proposition: #918

thomasmoore-torc commented 2 days ago

Are there any plans to progress with resolving this issue? Getting logs out of the RMW while integrating ROS2 into target hardware has been a pain point for us. In the case of Fast DDS, getting logs currently require:

  1. Building Fast DDS with LOG_NO_INFO=OFF (and FASTDDS_ENFORCE_LOG_INFO=ON if a non-debug build)
  2. Add a call to rmw_set_log_severity(RMW_LOG_SEVERITY_INFO); to every process for which we want to be able to see Fast DDS info messages

The first step is not much of an issue for us as we are building Fast DDS as part of our pipeline and it's easy enough to create a build with those options set. However, the second step is the more obtrusive issue as we now have to start modifying source code, potentially in many locations.

I can see the reasoning for not necessarily wanting to couple the RCL and RMW log levels, but it would be nice to have some solution to enable RMW log messages. Here are a few options that could be considered:

  1. Add a new --rmw-log-level ROS parameter
  2. Add a new RMW_LOG_LEVEL environment variable

If there is a strong preference either way, then I can start working on an alternative to #918 to implement the preference.

christophebedard commented 2 days ago

Unfortunately, I think adding a new config option, such as a new CLI option or a new env var, may be even less well-received than the current proposal (#918).

We've been wanting to figure out logging by creating a coherent overall strategy for the past few years (see https://github.com/ros2/design/issues/314), which is why some of these small/one-off changes have been put on hold, but it hasn't really happened yet (https://github.com/ros2/design/pull/315). I can bring this issue up during the next ROS 2 PMC meeting, but it may get pushed back a bit.