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

VW PQ: ACC_System & ACC_GRA_Anziege #769

Closed
wants to merge 2 commits into from

Conversation

CineXWorm
Copy link
Contributor

This merge request is about:

  • Fix scaling issues
  • Remove unused signals
  • Add value descriptions

@CineXWorm
Copy link
Contributor Author

@jyoung8607 I am pretty sure they would like your insight here.

Comment on lines +1077 to +1078
SG_ ACS_Checksum : 0|8@1+ (1,0) [0|1] "" XXX
SG_ ACS_Zaehler : 8|4@1+ (1,0) [0|1] "" XXX
Copy link
Contributor

Choose a reason for hiding this comment

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

We use all capital names for these so the parser can recognize which signals are counters/checksums

@jyoung8607
Copy link
Collaborator

To recap my Discord conversation with @jackking444: this PR does identify and fix a couple of signal bugs. They will have a minor impact on OP longitudinal output. Unfortunately this PR also introduces several new bugs, above and beyond just the renaming of the checksum and counter signal. So, this should not be merged as-is. I will file a superseding PR shortly.

@jyoung8607
Copy link
Collaborator

Following up to myself:

There are only two signals that needed to be fixed in ACC_System, fixes submitted in #791. There is only one signal (plus the message name) that needed to be fixed in ACC_GRA_Anzeige, fixes submitted in #792.

The rest of the proposed signal changes are invalid. This PR is superseded and can be closed.

@sshane
Copy link
Contributor

sshane commented Nov 11, 2023

The rest of the proposed signal changes are invalid. This PR is superseded and can be closed.

Closing per comment.

@sshane sshane closed this Nov 11, 2023
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