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

refactor(autoware_auto_control_msgs): replace autoware_auto_control_msgs with autoware_control_msgs #3677

Closed
wants to merge 3 commits into from

Conversation

xmfcx
Copy link
Contributor

@xmfcx xmfcx commented May 11, 2023

Description

Closes: #3674

🤖 Generated by Copilot at 4fa17ed

This pull request replaces the use of AckermannControlCommand and AckermannLateralCommand messages from autoware_auto_control_msgs with the new generic Control and Lateral messages from autoware_control_msgs in various control modules and tools. This simplifies the control interface, avoids unnecessary conversions, and ensures compatibility with the latest control command definition. The changes affect the source code, header files, package dependencies, and documentation of the tier4_control_rviz_plugin, control_performance_analysis, joy_controller, mpc_lateral_controller, and autonomous_emergency_braking packages.

Related links

Tests performed

Notes for reviewers

Interface changes

🤖 Generated by Copilot at 4fa17ed

  • Replace autoware_auto_control_msgs with autoware_control_msgs to unify the control message interface across different controllers (link, link, link, link, link)
  • Change the control command message type from AckermannControlCommand to Control and update the corresponding fields, namespaces, headers, publishers, subscribers, buffers, and functions in tier4_control_rviz_plugin, control_performance_analysis, and joy_controller (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Change the lateral control command message type from AckermannLateralCommand to Lateral and update the corresponding fields, namespaces, headers, publishers, subscribers, buffers, and functions in mpc_lateral_controller (link, link, link, link, link, link, link, link, link, link, link)
  • Update the README files for tier4_control_rviz_plugin, control_performance_analysis, joy_controller, and mpc_lateral_controller to reflect the new message types and fields for control command and lateral control (link, link, link, link)

Effects on system behavior

Shouldn't change the system behavior.

Fun

🤖 Generated by Copilot at 4fa17ed

Control replaces
AckermannControlCommand
Simpler interface

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.

@xmfcx xmfcx added component:planning Route planning, decision-making, and navigation. (auto-assigned) component:control Vehicle control algorithms and mechanisms. (auto-assigned) component:vehicle Vehicle-specific implementations, drivers, packages. (auto-assigned) labels May 11, 2023
@xmfcx xmfcx requested a review from mkuri as a code owner May 11, 2023 14:58
@xmfcx xmfcx self-assigned this May 11, 2023
@xmfcx xmfcx requested a review from a team as a code owner May 11, 2023 14:58
@xmfcx xmfcx marked this pull request as draft May 11, 2023 14:58
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:system System design and integration. (auto-assigned) and removed component:planning Route planning, decision-making, and navigation. (auto-assigned) component:control Vehicle control algorithms and mechanisms. (auto-assigned) component:vehicle Vehicle-specific implementations, drivers, packages. (auto-assigned) labels May 11, 2023
@xmfcx
Copy link
Contributor Author

xmfcx commented May 11, 2023

Side tasks:

Remarks:

  • It wasn't being used in planning_test_utils, so I've removed its references.

@github-actions github-actions bot added the component:control Vehicle control algorithms and mechanisms. (auto-assigned) label May 11, 2023
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Patch coverage: 26.77% and no project coverage change.

Comparison is base (c9280db) 15.23% compared to head (be36eb1) 15.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3677   +/-   ##
=======================================
  Coverage   15.23%   15.23%           
=======================================
  Files        1468     1468           
  Lines      101783   101785    +2     
  Branches    31405    31405           
=======================================
+ Hits        15503    15507    +4     
+ Misses      69417    69416    -1     
+ Partials    16863    16862    -1     
Flag Coverage Δ *Carryforward flag
differential 18.34% <26.77%> (?)
total 15.21% <ø> (-0.02%) ⬇️ Carriedforward from c9280db

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

Impacted Files Coverage Δ
...ontrol_rviz_plugin/src/tools/manual_controller.cpp 0.00% <0.00%> (ø)
...analysis/src/control_performance_analysis_core.cpp 0.00% <0.00%> (ø)
...analysis/src/control_performance_analysis_node.cpp 0.00% <0.00%> (ø)
...troller/src/joy_controller/joy_controller_node.cpp 0.00% <0.00%> (ø)
..._controller/include/mpc_lateral_controller/mpc.hpp 63.15% <ø> (ø)
.../mpc_lateral_controller/mpc_lateral_controller.hpp 66.66% <ø> (ø)
...lude/mpc_lateral_controller/steering_predictor.hpp 100.00% <ø> (ø)
control/mpc_lateral_controller/src/mpc.cpp 49.86% <ø> (ø)
.../mpc_lateral_controller/src/steering_predictor.cpp 52.27% <ø> (ø)
control/mpc_lateral_controller/test/test_mpc.cpp 51.31% <ø> (ø)
... and 27 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot added component:planning Route planning, decision-making, and navigation. (auto-assigned) component:vehicle Vehicle-specific implementations, drivers, packages. (auto-assigned) labels May 11, 2023
@xmfcx xmfcx force-pushed the refactor-control-msgs branch 2 times, most recently from e6ab610 to 3ab893d Compare May 16, 2023 11:01
@github-actions github-actions bot added component:common Common packages from the autoware-common repository. (auto-assigned) component:simulation Virtual environment setups and simulations. (auto-assigned) component:tools Utility and debugging software. (auto-assigned) labels May 16, 2023
@xmfcx xmfcx added this to the 2023 May - Jun Milestone milestone May 16, 2023
@xmfcx
Copy link
Contributor Author

xmfcx commented May 16, 2023

Tested with the Planning simulation demo and it works fine.

Screenshot from 2023-05-16 16-41-38

@xmfcx xmfcx marked this pull request as ready for review May 16, 2023 13:48
@xmfcx
Copy link
Contributor Author

xmfcx commented May 17, 2023

I've added 2 missing PRs too, now everything should work together.

(I've previously missed them because I didn't do vcs status src on all repos)

@xmfcx
Copy link
Contributor Author

xmfcx commented May 17, 2023

I have had to squash and re-base while fixing the conflicts with the 8fcac0e

Compiled and tested with the planning simulator again to make sure it works.

@xmfcx
Copy link
Contributor Author

xmfcx commented May 17, 2023

It'd be great if we can merge these as soon as possible to avoid potential future merge conflict errors.

@takayuki5168
Copy link
Contributor

@xmfcx pre-commit failed.
@TakaHoribe Could you review the PR.

@TakaHoribe
Copy link
Contributor

@xmfcx Thank you!!! It looks good, but I thought the output of the trajectory_follower would be the ControlHolizon msg.
I don’t think we have to discuss right now though.

If you don’t have a large concern, I’d like to merge this PR.

@xmfcx
Copy link
Contributor Author

xmfcx commented May 18, 2023

@xmfcx Thank you!!! It looks good, but I thought the output of the trajectory_follower would be the ControlHorizon msg. I don’t think we have to discuss right now though.

If you don’t have a large concern, I’d like to merge this PR.

Thanks for reviewing, for now, I've just refactored and made minimal changes to just adapt the existing code.

To use the new messages, we can open up new PRs over this.

I am waiting for @mitsudome-r to ask the web.auto team to confirm if these changes won't break anything there.

But I think @isamu-takagi can confirm too, since he is working on the ad_api tasks.

@xmfcx
Copy link
Contributor Author

xmfcx commented May 18, 2023

@xmfcx pre-commit failed. @TakaHoribe Could you review the PR.

@takayuki5168 I've added the missing header, now it should be fine:

@takayuki5168
Copy link
Contributor

@xmfcx @TakaHoribe
I found that If we merge this PR now, the scenario simulator will not work well with the latest autoware.universe, which I think is critical.
Don't you apply the same change to the scenario simulator as well?
https://github.com/tier4/scenario_simulator_v2

@xmfcx
Copy link
Contributor Author

xmfcx commented May 18, 2023

I found that If we merge this PR now, the scenario simulator will not work well with the latest autoware.universe, which I think is critical. Don't you apply the same change to the scenario simulator as well? https://github.com/tier4/scenario_simulator_v2

@takayuki5168 Thanks for noticing, I've just talked to @mitsudome-r about this, he mentioned https://github.com/tier4/scenario_simulator_v2 and AWSIM both won't work. He will talk to internal team to find a resolution.

I will create a branch on scenario simulator to perform refactoring anyways.
Same with AWSIM.

@xmfcx
Copy link
Contributor Author

xmfcx commented May 26, 2023

We can merge this once the following issue is resolved:

Please feel free to add your comments on the issue.

@xmfcx xmfcx requested a review from brkay54 as a code owner June 20, 2023 15:30
@github-actions github-actions bot added the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Jun 20, 2023
…sgs with autoware_control_msgs

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@github-actions github-actions bot removed the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Jul 5, 2023
pre-commit-ci bot and others added 2 commits July 5, 2023 16:49
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@xmfcx xmfcx marked this pull request as draft July 5, 2023 16:57
@xmfcx xmfcx marked this pull request as ready for review July 6, 2023 08:25
@xmfcx
Copy link
Contributor Author

xmfcx commented Jul 6, 2023

Confirmed that planning simulator works with all the PRs combined.
Screenshot from 2023-07-06 12-04-39

@stale
Copy link

stale bot commented Sep 6, 2023

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Sep 6, 2023
@xmfcx
Copy link
Contributor Author

xmfcx commented Mar 7, 2024

I'm closing all of my message migration issues and PRs.

@xmfcx xmfcx closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) component:control Vehicle control algorithms and mechanisms. (auto-assigned) component:planning Route planning, decision-making, and navigation. (auto-assigned) component:simulation Virtual environment setups and simulations. (auto-assigned) component:system System design and integration. (auto-assigned) component:tools Utility and debugging software. (auto-assigned) component:vehicle Vehicle-specific implementations, drivers, packages. (auto-assigned) status:stale Inactive or outdated issues. (auto-assigned) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(autoware_auto_control_msgs): replace autoware_auto_control_msgs with autoware_control_msgs
4 participants