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

Implement rclcpp-specific logging macros [taking name not object] #389

Merged
merged 20 commits into from
Nov 15, 2017

Conversation

dhood
Copy link
Member

@dhood dhood commented Oct 24, 2017

Requires ros2/ament_cmake_ros#2

These are wrappers around rcutils' logging macros (see details below) that inject the package name as the default name/prefix name, based on the decision in ros2/demos#184

@dhood dhood added the in progress Actively being worked on (Kanban column) label Oct 24, 2017
@dhood dhood self-assigned this Oct 24, 2017
COMMAND ${PYTHON_EXECUTABLE} ARGS -c "${python_code}"
DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/logging.hpp.em.watch"
COMMENT "Expanding logging.hpp.em"
VERBATIM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird single space indentation here.

@mikaelarguedas
Copy link
Member

I discussed this briefly with @wjwwood recently, I think it would be valuable to decide how we want the use facing macros to be named before starting using them on the higher level packages.

  1. One option was to use e.g. RCLCPP_INFO to have a direct correlation between the package declaring the macro and the macro name. This would require slightly more work when porting from ROS 1 that would be to #define ROS_X RCLCPP_X to keep the same code. A header providing such defines could also be provided by rclcpp to reduce the amount of extra code on the user side

  2. Another option is to use ROS_X as this PR does, with the downside of not knowing where the macro comes from as ROS1 users have been complaining about for a while.

@dhood
Copy link
Member Author

dhood commented Nov 14, 2017

Updated based on our discussion yesterday:

  • No "named" variant of logging macros: force a logging context to be specified.
  • No "unnamed" variants either: there is no default logger being injected behind the scenes like there was in ROS 1.
  • No ROS prefix is being injected either.
  • The context is temporarily being specified as the logger name (logger objects out of scope for this PR).

Regarding the code:

  • RCLCPP is being used as the prefix e.g. RCLCPP_INFO, so that users know where the macro's defined.
  • The template re-uses the rcutils logic for generating doc strings so the macro arguments are documented.
  • "Throttle" is excluded from this PR because it will require a custom rclcpp-time-specific implementation (as opposed to just wrapping the rcutils implementation) which I consider a followup enhancement.
  • FYI, the rcutils named macros take the name at the end of the feature argument list, while these macros take the name as the first argument, so there's some parameter re-shuffling.
  • I have created RCLCPP-specific versions of e.g. RCUTILS_LOG_MIN_SEVERITY_DEBUG so that rclcpp macros can be compiled out independently to rcutils macros being compiled out (and can be completely compiled out once they do more work than just forwarding to rcutils).
  • I have not created RCLCPP-specific versions of e.g. RCUTILS_LOG_SEVERITY_INFO because as I see it users should not need to interact with those severities themselves in order to use log macros.

Gist with the generated logging.hpp contents:
https://gist.github.com/dhood/c97a36fc5db0cbacc87e9426437c8a84#file-logging-hpp

CI including new tests (looks like they're currently grouped with rcutils' logging tests so it's difficult to tell them apart, but they're there)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dhood dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 14, 2017
@dhood dhood mentioned this pull request Nov 14, 2017
14 tasks
@dhood dhood changed the title Implement rclcpp-specific logging macros Implement rclcpp-specific logging macros [taking name not object] Nov 15, 2017
@dhood dhood merged commit 24f3970 into master Nov 15, 2017
@dhood dhood deleted the logging_macros branch November 15, 2017 22:14
@dhood dhood removed the in review Waiting for review (Kanban column) label Nov 15, 2017
#define RCLCPP_LOG_MIN_SEVERITY_WARN 2
#define RCLCPP_LOG_MIN_SEVERITY_ERROR 3
#define RCLCPP_LOG_MIN_SEVERITY_FATAL 4
#define RCLCPP_LOG_MIN_SEVERITY_NONE 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the tests in this PR covering this ?

I'm surprised that these levels don't match the severity enums defined in rcutils, (https://github.com/ros2/rcutils/blob/a2f722530c5e39eecfeb62b8c40db99bf9ae2a5e/include/rcutils/logging.h#L123-L128) is it on purpose ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've misinterpreted their purpose, they're for compiling out macros, not setting severity level of messages/loggers. My comment above is relevant here: "I have created RCLCPP-specific versions of e.g. RCUTILS_LOG_MIN_SEVERITY_DEBUG so that rclcpp macros can be compiled out independently to rcutils macros being compiled out (and can be completely compiled out once they do more work than just forwarding to rcutils)."

Because of the compile-time nature there are no tests for them, but I've tested it manually (that was how I discovered the need for ros2/rcutils#60)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood the purpose, the fact that we define different enums values for them still puzzles me though (and yeah I indeed missed ros2/rcutils#60 which introduce the same "error" at the rcutils level IMO).
If a user want to add a custom severity, (let's say between debug and info for example), he can do it thanks to the values of rcutils:
RCUTILS_LOG_SEVERITY_VERBOSE_MYSEVERITY = 15.
but cannot compile only those out because these enums don't leave any room between the levels. But now he cannot just compile out by using the same level he defined in rcutils, he has to find something between RCLCPP_LOG_MIN_SEVERITY_DEBUG and RCLCPP_LOG_MIN_SEVERITY_INFO.
How do we expect users to do this ? by defining:

#define RCLCPP_LOG_MIN_SEVERITY_MYSEVERITY RCUTILS_LOG_SEVERITY_MYSEVERITY/10

or with hardcoded floating point values ?

#define RCLCPP_LOG_MIN_SEVERITY_MYSEVERITY 1.5

It seems more user friendly to allow them to use the same values rather than a different one at each level, but maybe there is a technical constraint I missed here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning was that, while a user can add their own severity level and call to the log functions with their custom level, it's not straightforward for them to extend the logging macros to have ones for their severity (even with the values matching the enum values). Since we need the min severity to be a preprocessor directive, we can't directly use the enum values (i.e. #define RCLCPP_LOG_MIN_SEVERITY_MYSEVERITY RCUTILS_LOG_SEVERITY_MYSEVERITY would not work for the purpose of compiling out the macros, which is what prompted ros2/rcutils#60). So, simply to avoid the need for factoring out the values used by the severity enum (10, 20, etc) into preprocessor directives, the values for the min severities are independent to the severity values themselves because there are other limiting factors on users creating custom severity macros anyway.

It could be done though, definitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants