-
Notifications
You must be signed in to change notification settings - Fork 640
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
refactor(ekf_localizer): add a function to check the delay step #2657
refactor(ekf_localizer): add a function to check the delay step #2657
Conversation
Codecov ReportBase: 11.55% // Head: 11.55% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2657 +/- ##
=======================================
Coverage 11.55% 11.55%
=======================================
Files 1309 1309
Lines 91321 91413 +92
Branches 24220 24287 +67
=======================================
+ Hits 10549 10564 +15
- Misses 69729 69791 +62
- Partials 11043 11058 +15
*This pull request uses carry forward flags. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -0,0 +1,25 @@ | |||
// Copyright 2022 Autoware Foundation |
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.
I will approve after the conflict is resolved!
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.
Resolved the conflicts in cdb8d15
#include <string> | ||
|
||
std::string poseDelayStepWarningMessage( | ||
const double delay_step, const double extend_state_step, const double ekf_dt); |
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.
It's better to make extended_state_step
as int
, given that it is defined so in hyper_parameters.hpp
.
const double delay_step, const double extend_state_step, const double ekf_dt); | |
const double delay_step, const int extend_state_step, const double ekf_dt); |
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.
Resolved in 2777ae9
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.
LGTM
…warefoundation#2657) * Add a function to check delay step * Apply the delay step checker to twist * Test the delay step cheker
…warefoundation#2657) * Add a function to check delay step * Apply the delay step checker to twist * Test the delay step cheker
Description
Checking the delay step in the callback function is redundant. I added a function to do this and improved the readability.
Note that the warning message is modified to reduce the number of arguments.
Related links
Tests performed
Smoke tests to check the delay step boundary
Notes for reviewers
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.