Closed
Conversation
dirk-thomas
approved these changes
Oct 23, 2017
| printf("add_two_ints_client was interrupted while waiting for the service. Exiting.\n"); | ||
| // This will have <package_name>.<node_name> as the logger name. | ||
| ROS_INFO_NAMED(node->get_name(), | ||
| "add_two_ints_client was interrupted while waiting for the service. Exiting.\n"); |
Member
There was a problem hiding this comment.
The trailing \n should be obsolete.
Same below.
| ss << "Incoming request" << std::endl; | ||
| ss << "a: " << request->a << " b: " << request->b << std::endl; | ||
| // No stream log macro yet. This will get <package_name> as the logger name. | ||
| ROS_INFO("%s", ss.str().c_str()) |
Member
There was a problem hiding this comment.
This could be reduced to just: ROS_INFO(ss.str().c_str())
Member
Author
|
I've realised that the reason for the "ros" prefix for roscpp could be from the log4cxx backend requiring the notion of a root logger. Since rcutils doesn't impose that in ros2/rcutils#57, that doesn't apply to ros2, so we can remove it. |
Member
|
I have a few questions about the approach:
|
Member
Author
|
closing in favour of design discussions elsewhere |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed usage for logging macro wrappers provided by rclcpp (not considering logger objects at the moment)
The purpose for wrapping the rcutils macros in the first place is to inject default names and/or name suffixes to log calls.
For comparison, ROS 1's rosconsole does the following:
rqt_gui_py_nodeexposes loggers likePyQt5.uic.propertiesThe proposal for ROS 2 is to do something similar by setting a default name/prefix, but also provide ROS_INFO_FULLNAMED that allows users to provide the full logger name if they want to.
Concrete proposals:
Reasoning:
Speaking with a couple of people it seems that we don't feel that having the "ros" prefix for the default name/prefix is that necessary, but that a default of some sort is still needed, so we can just go with "<package_name>". IMO if a user calls ROS_INFO_NAMED in the same file as ROS_INFO they should go to the same parent logger (different child for the named one), suggesting that whatever the default name is it should continue to be prepended even in the case of ROS_INFO_NAMED.
However, most people I spoke to were surprised that the package name was always being prepended without a way to override it, so we can also add ROS_INFO_FULLNAMED, which doesn't do any name modification, it just passes it through to rcutils.
Examples when you might want to use ROS_INFO_FULLNAMED:
This PR has some usage examples.