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

Proposed changes to ADAS signals spec. in compliance with SAE J3016 #660

Closed
wants to merge 2 commits into from

Conversation

hongkicha
Copy link
Contributor

@hongkicha hongkicha commented Sep 25, 2023

Hi, I'm Hongki CHA, the Sponsor of SAE J3016.

I'd like to propose some changes to better comply with the latest SAE J3016.

I welcome further discussion.

Signed-off-by: Hongki CHA [email protected]

@@ -1,3 +1,4 @@
Signed-off-by: Hongki CHA <[email protected]>
Copy link
Collaborator

@erikbosch erikbosch Sep 25, 2023

Choose a reason for hiding this comment

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

Signed off shall be in commit, not in file.

(Do a git commit --amend -s on every commit in PR)

(This line also causes a build error)

@SebastianSchildt
Copy link
Collaborator

Content-wise I support this change. Thanks @hongkicha making this aligned better to the specific definitions from J3016.

This also seems a candidate for backporting to the 4.x, potentially with the exclusion of the new SAE_5_DISENGAGING state, that has been added.

@erikbosch
Copy link
Collaborator

Meeting notes:

  • Please review
  • Sebastian: Makese sensor, maybe backport but adding SAE_5_disengage only to major/master

@erikbosch erikbosch added the Status:Review Please review/discuss contents label Sep 26, 2023
@erikbosch
Copy link
Collaborator

Hi @hongkicha.

There are some errors in Continuous Integration (CI) for your PRs. There are some guidelines at https://github.com/COVESA/vehicle_signal_specification/blob/master/BUILD.md that possibly could help. Feel free to reach our if you need help understanding the CI errors.

@hongkicha
Copy link
Contributor Author

@erikbosch @SebastianSchildt Hi, this is Hongki. Thanks for your comments. We're having our Thanksgiving holiday in Korea until Tuesday.
Considering the signed-off, I use GitHub Chrome directly, so I'm not sure how/where I can enter the command line. If necessary, I can set up my computer after I get back to my office.
I'm attending next week's COVESA AMM, and if possible, we can meet there and talk about how to proceed.

@erikbosch
Copy link
Collaborator

@hongkicha -looking forward to meeting you. I believe there are some limitations if just using the Github web UI to create or modify commits compared to creating/modifying a commit on a local machine

@erikbosch
Copy link
Collaborator

Meeting notes:

  • CI errors to be fixed
  • Apart from that no blocker for merging for 5.0

@erikbosch
Copy link
Collaborator

Merge notes:

  • Erik to assist making CI check success, then ok to merge

@erikbosch erikbosch mentioned this pull request Oct 23, 2023
@erikbosch
Copy link
Collaborator

Merged by #672

@erikbosch erikbosch closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Review Please review/discuss contents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants