Change log level for lifecycle_publisher#1715
Conversation
De-activating a lifecycle publisher while the function that was invoking `publish` is still running floods the log of useless warning messages. This requires to add a boolean check around the publish call, thus making useless the choice of a lifecycle publisher Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
|
I think having a visible warning might still be worth having, @Karsten1987 what do you think? We have logging macros like "throttled", "once", and even ones that have arbitrary predicates, e.g.: https://docs.ros2.org/foxy/api/rclcpp/logging_8hpp.html#a451bee77c253ec72f4984bb577ff818a Maybe we could warn throttled, and debug every time? (or something like that) |
|
Nice, I didn't know about those macros. |
|
Either way works for me, you can have your own flag to control when using this macro and a custom function or expression. |
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
fujitatomoya
left a comment
There was a problem hiding this comment.
I am not really inclined to have this dedicated flag to control print or not to break the API. @alsora do you have particular reason to use RCLCPP_WARN_FUNCTION?
how about,
- RCLCPP_DEBUG, then user can control the logging.
- RCLCPP_WARN_ONCE only once state is deactivated, then reset that once it becomes activated internally w/o changing API?
|
@fujitatomoya why is this breaking the API? |
|
@alsora sorry my bad, i though new parameter for the method, but just a constructor.
okay, i am good with this one too. |
wjwwood
left a comment
There was a problem hiding this comment.
lgtm
Just a note, this won't be backport-able since it will break ABI.
|
CI:
|
|
Looks like it doesn't compile. |
|
Looks like it tries to |
|
We can't coerce a lambda that captures into a function pointer... Unfortunately. So I'm not actually sure how to fix this... I think we need a new kind of macro that takes a function and a |
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
|
My bad, I only built the code without unit tests, but this file is just a header so it didn't actually got built. |
|
CI:
|
|
The cmake warning is probably due to this needing to be rebased. I'll wait for feedback on doc changes, and if you can rebase it at the same time I'd appreciate that. |
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
|
CI:
|
De-activating a lifecycle publisher while the function that was invoking
publishis still running floods the log of useless warning messages.This requires to add a boolean check around the publish call, thus making useless the choice of a lifecycle publisher over a regular publisher.