-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: add check for keepout filter mask if it is not nullptr #5773
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
fix: add check for keepout filter mask if it is not nullptr #5773
Conversation
mini-1235
left a comment
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.
Initially, I chose the logging type RCLCPP_WARN_THROTTLE identical to the check that in KeepoutFilter::process. But this type does't work correctly due to issue: ros2/rclcpp#2587.
Alternatively, we can temporarily use:
RCLCPP_WARN, but it fills the terminal too quickly.
RCLCPP_WARN_ONCE, this type also does not work correctly (it does not print once), but at least it provides information and with less frequency than the first option.
Replace RCLCPP_WARN_ONCE with RCLCPP_WARN_THROTTLE, when the above issue will be solved.
If you agree with the current implementation, I will also change the warning type in Keepout Filter::process
I will leave the final decision to @SteveMacenski 😄
|
@AJedancov rebase for me. We did some change recently based on a Rolling sync that need to be reflected here for CI |
Signed-off-by: AJedancov <[email protected]>
Signed-off-by: AJedancov <[email protected]>
692f273 to
7270aae
Compare
Codecov Report❌ Patch coverage is
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
thanks much for this! |
…gation#5773) * fix: add check for filter mask if not nullptr Signed-off-by: AJedancov <[email protected]> * fix: replace with throttle warning Signed-off-by: AJedancov <[email protected]> --------- Signed-off-by: AJedancov <[email protected]> Signed-off-by: Christopher Thompson <[email protected]>
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Update filter mask:
Future work that may be required in bullet points
Initially, I chose the logging type
RCLCPP_WARN_THROTTLEidentical to the check that inKeepoutFilter::process. But this type does't work correctly due to issue: Issue with RCLCPP THROTTLE logging in the plugin based approach #2587.Alternatively, we can temporarily use:
RCLCPP_WARN, but it fills the terminal too quickly.RCLCPP_WARN_ONCE, this type also does not work correctly (it does not print once), but at least it provides information and with less frequency than the first option.Replace
RCLCPP_WARN_ONCEwithRCLCPP_WARN_THROTTLE, when the above issue will be solved.If you agree with the current implementation, I will also change the warning type in
Keepout Filter::process.For Maintainers:
backport-*.