-
Notifications
You must be signed in to change notification settings - Fork 227
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
Updates for global severity threshold -> default severity threshold #130
Conversation
@@ -29,19 +29,19 @@ rclpy_logging_initialize(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args)) | |||
Py_RETURN_NONE; | |||
} | |||
|
|||
/// Get the global severity threshold of the logging system. | |||
/// Get the default severity threshold of the logging system. | |||
/** | |||
* \return severity | |||
*/ | |||
static PyObject * | |||
rclpy_logging_get_severity_threshold(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the name of this function be changed accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there's a conceptual adaptation of the rclpy use of rcutils logging coming in another rclpy PR, but this PR is just the bare minimum to keep it compiling if ros2/rcutils#57 gets merged (so the PRs don't block each other)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
INFO = 20 | ||
WARN = 30 | ||
ERROR = 40 | ||
FATAL = 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we actually get the values from rcutils/logging.h
rather than redefining them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we should, eventually. in my mind that would be done by moving the values into the rcutils.logging
python module so we can include them here, and empy template them into rcutils' logging.h. Since there's a lot going on in ros2/rcutils#57 already, I think it's best to put it off to a separate step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
closed in favour of #132 |
connects to ros2/rcutils#57
This is the minimum PR required to keep rclpy working with the changes in ros2/rcutils#57
It does not attempt to leverage any of the configuration of loggers (that'll be a separate PR)