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: update msg definitions #87

Merged
merged 10 commits into from
May 9, 2024
Merged

feat: update msg definitions #87

merged 10 commits into from
May 9, 2024

Conversation

yukkysaito
Copy link
Collaborator

@yukkysaito yukkysaito commented Apr 25, 2024

Description

@youtalk is writing on @yukkysaito behalf.

This PR has resulted the review of autoware_msgs message definitions as part of the transition from autoware_auto_msgs to autoware_msgs.
Note that to ensure a smooth transition, TrafficSignal-related message definitions will remain during the transition period until June 1st, but will be removed at the end of the transition period.
https://github.com/orgs/autowarefoundation/discussions/3862#discussion-5671332

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.

yukkysaito and others added 4 commits April 25, 2024 19:39
Signed-off-by: Yukihito Saito <[email protected]>
…mmand-msg

feat!(autoware_vehicle_msgs): remove control mode command msg
…nematicState, and VehicleOdometry messages (#88)

feat!(autoware_vehicle_msgs): remove VehicleControlCommand, VehicleKinematicState, and VehicleOdometry messages

Signed-off-by: mitsudome-r <[email protected]>
@youtalk youtalk self-requested a review May 8, 2024 13:42
@youtalk youtalk changed the title Autoware msg feat: update msg definitions May 9, 2024
Signed-off-by: Yutaka Kondo <[email protected]>
@youtalk
Copy link
Member

youtalk commented May 9, 2024

I think I cannot fixed the clang-tidy-differential CI check. So I would like to ignore it...
https://github.com/autowarefoundation/autoware_msgs/actions/runs/9011448888/job/24759177882?pr=87

/usr/bin/git diff --diff-filter=D --name-only 224bd3a87fb539630f5a09409c809a81f8bb9118...4059e53f18c390b3a98a7620d6c6ff31a6019aba
autoware_planning_msgs/msg/PathPointWithLaneId.msg
autoware_planning_msgs/msg/PathWithLaneId.msg
autoware_vehicle_msgs/msg/ControlModeCommand.msg
autoware_vehicle_msgs/msg/VehicleControlCommand.msg
autoware_vehicle_msgs/msg/VehicleKinematicState.msg
autoware_vehicle_msgs/msg/VehicleOdometry.msg
Warning: No paths found using the specified patterns
Successfully created paths-output-file: /tmp/88c974fb-1275-4a36-b025-430116b8ac00.txt
Run bash $GITHUB_ACTION_PATH/get-changed-paths.sh
changed-files
  Resolving repository path: /__w/autoware_msgs/autoware_msgs/.
  Retrieving changes between 224bd3a[87](https://github.com/autowarefoundation/autoware_msgs/actions/runs/9011448888/job/24759177882?pr=87#step:6:91)fb539630f5a09409c809a81f8bb9118 (main) → 4059e53f18c390b3a98a7620d6c6ff31a6019aba (autoware_msg)
  Error: Failed to get changed files between: 224bd3a87fb539630f5a09409c809a81f8bb9118...4059e53f18c3[90](https://github.com/autowarefoundation/autoware_msgs/actions/runs/9011448888/job/24759177882?pr=87#step:6:94)b3a98a7620d6c6ff31a6019aba
  Error: Process completed with exit code 1.

@youtalk youtalk marked this pull request as ready for review May 9, 2024 02:43
@youtalk youtalk self-assigned this May 9, 2024
@xmfcx
Copy link
Collaborator

xmfcx commented May 9, 2024

Note that to ensure a smooth transition, TrafficSignal-related message definitions will remain during the transition period until June 1st, but will be removed at the end of the transition period.

I hope, we will be flexible to update these autoware_msgs message definitions in the future more freely, through version control and timed deprecation and transition processes.

@youtalk
Copy link
Member

youtalk commented May 9, 2024

That's right. Following the discussion bellow, the packages in the old autoware_common repository have now been managed using semantic versioning.
https://github.com/orgs/autowarefoundation/discussions/4671

I hope that this will make it easier to update and migrate message definitions.

Copy link
Member

@mitsudome-r mitsudome-r left a comment

Choose a reason for hiding this comment

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

LGTM

@youtalk
Copy link
Member

youtalk commented May 9, 2024

@xmfcx Please review again.

@xmfcx
Copy link
Collaborator

xmfcx commented May 9, 2024

@mitsudome-r could you give reasons behind removing the messages?

For example, why is autoware_vehicle_msgs/msg/VehicleControlCommand.msg removed and what will be used in its place?

@xmfcx
Copy link
Collaborator

xmfcx commented May 9, 2024

As for the messages added, for the initial versions, I won't make comments on them.

Because I feel like if I attempt to modify any part of these messages, the migration from _auto msgs will take much longer.

I would like to change or modify them gradually through message versions.

@xmfcx
Copy link
Collaborator

xmfcx commented May 9, 2024

And in general, it compiles and I don't know what to test it against so I cannot make much comments.

Feel free to merge it as you like.

But please don't prevent us from gradually changing these messages through planned deprecation and transitioning processes. Right now, all of these messages are messy like they were in _auto.

@yukkysaito
Copy link
Collaborator Author

For example, why is autoware_vehicle_msgs/msg/VehicleControlCommand.msg removed and what will be used in its place?

My understanding is that A is a remnant from the autoware.auto era and has been redefined here in core/universe. The one being removed this time is a remnant from the .auto era.

https://github.com/autowarefoundation/autoware_msgs/tree/15c62e7b55a215a01ca9aa60930a6987a8f4c839/autoware_control_msgs/msg

Of course, it would be possible to separate the PRs.
However, I think some drastic change from autoware_auto_msgs to autoware_msgs is inevitable, since autoware_msgs is to be defined as the official component interface of autoware, I think it is acceptable to erase the remnants of autoware_auto at this time to prevent it from being used as a formal component interface.
I would like to hear your opinions.

Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

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

@mitsudome-r could you give reasons behind removing the messages?

For example, why is autoware_vehicle_msgs/msg/VehicleControlCommand.msg removed and what will be used in its place?

Those are the ones that have been erased once because no one is using them at the moment. This is a refactoring to minimize message definition.
As for the other modifications, they only add message definitions. So they are backward compatible.

@youtalk youtalk merged commit 54c1dab into main May 9, 2024
12 of 13 checks passed
@youtalk youtalk deleted the autoware_msg branch May 9, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants