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: add vehicle, control and localization calibration tools #22

Merged
merged 16 commits into from
Jun 11, 2024

Conversation

yabuta
Copy link
Contributor

@yabuta yabuta commented Apr 18, 2024

Description

I move vehicle, control and localization calibration tools from CalibrationTools repojitory in TIER IV space.(discussion)

CalibrationTools repojitory PR

I checked colconb build is success and tools launch test.
While stop_accel_evaluator is mentenance now, only build check

deviation_estimator: OK
stop_accel_evaluator: COLCON IGNORE
vehicle_cmd_analyzer: OK
parameter_estimator: OK
pitch_checker: OK
time_delay_estimator: OK

Related links

Tests performed

Notes for reviewers

Interface changes

Effects on system behavior

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.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

Copy link
Contributor

@SakodaShintaro SakodaShintaro left a comment

Choose a reason for hiding this comment

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

Looks Good To Me

@SakodaShintaro
Copy link
Contributor

Oh, some CI checks are failed.
It doesn't seem to be related to Localization, but I'll check again once it's fixed.

@yabuta
Copy link
Contributor Author

yabuta commented May 9, 2024

@SakodaShintaro
In pre-commit, there are errors in deviation_evaluator.
Pre-commit say it needs space after ':', is it OK to add space?

localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:46:46: E231 missing whitespace after ':'
            f"No error larger than {threshold:.3f} [m] observed. Provide more data on deviation_evaluator."
                                             ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:55:48: E231 missing whitespace after ':'
        print(f"  [OK] lowerbound = {lowerbound:.3f} < {threshold:.3f} = threshold [m]")
                                               ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:55:66: E231 missing whitespace after ':'
        print(f"  [OK] lowerbound = {lowerbound:.3f} < {threshold:.3f} = threshold [m]")
                                                                 ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:57:48: E231 missing whitespace after ':'
        print(f"  [NG] lowerbound = {lowerbound:.3f} > {threshold:.3f} = threshold [m]")
                                               ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:57:66: E231 missing whitespace after ':'
        print(f"  [NG] lowerbound = {lowerbound:.3f} > {threshold:.3f} = threshold [m]")
                                                                 ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:62:40: E231 missing whitespace after ':'
        print(f"  [OK] recall = {recall:.3f} > {recall_threshold:.3f}")
                                       ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:62:65: E231 missing whitespace after ':'
        print(f"  [OK] recall = {recall:.3f} > {recall_threshold:.3f}")
                                                                ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:64:40: E231 missing whitespace after ':'
        print(f"  [NG] recall = {recall:.3f} < {recall_threshold:.3f}")
                                       ^
localization/deviation_estimation_tools/deviation_evaluator/scripts/deviation_evaluation_visualizer.py:64:65: E231 missing whitespace after ':'
        print(f"  [NG] recall = {recall:.3f} < {recall_threshold:.3f}")
                                                                ^
planning/planning_debug_tools/scripts/perception_replayer/utils.py:149:51: E231 missing whitespace after ':'
            print(f"Time for {name}: {elapsed_time:.2f} ms")
                                                  ^
10    E231 missing whitespace after ':'

@SakodaShintaro
Copy link
Contributor

@yabuta
It's a little strange that the linter gives a warning for ':' in a string.
I had the same problem before, and it was a bug in flake8, so when I updated the version, the warning disappeared.

autowarefoundation/autoware_launch#970

@SakodaShintaro
Copy link
Contributor

@yabuta
I think this pull request will fix it. If this doesn't fix it, I'll change the code.
#24

@yabuta
Copy link
Contributor Author

yabuta commented May 9, 2024

@SakodaShintaro
Thanks!
I think it would be better to have mitsudome-san check it out.

@yabuta
Copy link
Contributor Author

yabuta commented May 9, 2024

@SakodaShintaro
In other hand, spell check error occurs in deviation_evaluator and others.
In deviation_evaluator, error words is minoda, koji and covs.Can you help to fix this error?

(Why ROS2 is error... I will check. )

@SakodaShintaro
Copy link
Contributor

@yabuta
I made a pull request to fix the localization part. The merge destination is this branch.
#25

SakodaShintaro and others added 3 commits May 9, 2024 18:07
@yabuta
Copy link
Contributor Author

yabuta commented May 19, 2024

@mitsudome-r Please approve if you look OK.

@SakodaShintaro
Copy link
Contributor

What is the status of this pull request?

Also, I noticed that some packages use autoware_auto_control_msgs::msg::AckermannControlCommand, autoware_auto_vehicle_msgs::msg::VelocityReport , ... etc.
Should this pull request be merged and then fixed in another pull request?

@yabuta
Copy link
Contributor Author

yabuta commented Jun 7, 2024

@SakodaShintaro I created changing msgs PR.
After changing msgs PR is merged this branch, I want to merge this PR to main.
I check whether individual tools go on now. Please wait for this check.

@SakodaShintaro
Copy link
Contributor

@yabuta
Thank you very much.
I'm not in a hurry so I'll wait 👍

@yabuta
Copy link
Contributor Author

yabuta commented Jun 11, 2024

@SakodaShintaro I create PR. Please check deviation_estimator.
#42

* merge

* delete autoware_auto

* style(pre-commit): autofix

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@yabuta yabuta merged commit 7a4ff19 into main Jun 11, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants