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

feat(control_validator): add hold and lpf #9120

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yuki-takagi-66
Copy link
Contributor

@yuki-takagi-66 yuki-takagi-66 commented Oct 18, 2024

Description

This PR add holding function and LPF for the velocity check.
in additon, velocity check is separated to two diags to trigger different MRM behavior.

Related links

launch PR: autowarefoundation/autoware_launch#1193

Parent Issue:

  • Link

How was this PR tested?

tier4 internal tests

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Signed-off-by: Yuki Takagi <[email protected]>
Signed-off-by: Yuki Takagi <[email protected]>
Signed-off-by: Yuki Takagi <[email protected]>
Signed-off-by: Yuki Takagi <[email protected]>
@github-actions github-actions bot added the component:control Vehicle control algorithms and mechanisms. (auto-assigned) label Oct 18, 2024
Copy link

github-actions bot commented Oct 18, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@yuki-takagi-66 yuki-takagi-66 changed the title feat(control validato): add hold and lpf feat(control validatot): add hold and lpf Oct 18, 2024
@yuki-takagi-66 yuki-takagi-66 marked this pull request as ready for review October 18, 2024 10:38
@yuki-takagi-66 yuki-takagi-66 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Oct 18, 2024
@kosuke55 kosuke55 changed the title feat(control validatot): add hold and lpf feat(control_validator): add hold and lpf Oct 19, 2024
@kyoichi-sugahara
Copy link
Contributor

@yuki-takagi-66
Please update newly introduced parameter description in README and add parameter in the yaml file in the package with default value.

@kyoichi-sugahara
Copy link
Contributor

@yuki-takagi-66
If there's no particular reason for removing the docstrings in this PR, it would be clearer to add them back in accordance with the changes. Or is it possible that they're being temporarily removed due to potential future changes?

Also, since I'm not familiar with the background of these changes, the processing seems fine to me. However, is it appropriate for me to approve without understanding the context or motivation behind these changes?

[
Signed-off-by: Yuki Takagi <[email protected]>
@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Oct 21, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.

Project coverage is 27.41%. Comparing base (dd6e677) to head (60df871).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...toware_control_validator/src/control_validator.cpp 34.61% 17 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9120   +/-   ##
=======================================
  Coverage   27.41%   27.41%           
=======================================
  Files        1302     1303    +1     
  Lines       95842    95894   +52     
  Branches    39128    39137    +9     
=======================================
+ Hits        26274    26289   +15     
- Misses      66912    66949   +37     
  Partials     2656     2656           
Flag Coverage Δ *Carryforward flag
differential 5.71% <34.61%> (?)
total 27.41% <ø> (+<0.01%) ⬆️ Carriedforward from dd6e677

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yuki-takagi-66
Copy link
Contributor Author

yuki-takagi-66 commented Oct 21, 2024

@kyoichi-sugahara
Thank you for the review.
I've update README and yaml in universe side.

I think we have no additional information to be added as docstrings from the function name and the arg names.
And we do not operate doxygen docs.
Therefore, I think we should not maintain the docstrings for this function, although we have option to maintain.

Also, since I'm not familiar with the background of these changes, the processing seems fine to me. However, is it appropriate for me to approve without understanding the context or motivation behind these changes?

Maybe it is not a sufficient description, LPF and hold is required to achieve the appropriate behavior along with MRM.
I've add some of descriptions.

Copy link
Contributor

@kyoichi-sugahara kyoichi-sugahara left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:control Vehicle control algorithms and mechanisms. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants