-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
GM: Set new longitudinal scaling #31932
Conversation
Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:
|
This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity. |
I have validated these changes on the commaCarSegments dataset, see here. I was a bit unsure what exactly to be looking for, but in my tests nothing looks awry. I was unable to validate a few of the platforms since they have no segments with valid carParams. |
@Verylukyguy can you drive on this? I'd like to see just a few routes considering the zero gas is just very slightly different (previous value was -6 in new scale). And a camera-ACC user as well. Then we can merge |
@@ -33,27 +33,26 @@ class CarControllerParams: | |||
|
|||
def __init__(self, CP): | |||
# Gas/brake lookups | |||
self.ZERO_GAS = 2048 # Coasting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for future reference, this is the conversion:
((2048 << 3) | (1 << 15) | (1 << 17)) * 0.125 - 22534
dc7716b32bf25574/00000000--b86195eae6/0 |
route with this branch and stock ACC |
I am loading it now and I will test after work! |
2018 GMC Acadia: |
what else do we need to have this merged? POC version currently in FrogPilot with the corrected limit (which need this PR first) makes OP Long very usable |
We've moved the car interfacing code to our PR_NUMBER=33045
curl -L https://github.com/commaai/openpilot/pull/$PR_NUMBER.patch | sed -e 's/selfdrive\/car/opendbc_repo\/opendbc\/car/g' | git apply -v --reject Simply replace the PR number with your own. Once done, add the files, fix any conflicts, and open a new PR. Alternatively, you may start a new PR from scratch if that is easier for you. |
Description
See commaai/opendbc#951 for a description of the issue. See commaai/panda#1905, commaai/opendbc#1025 for submodule PRs.
Trivial changes: scaled all values according to the formula
f(x) = x + (4096*5) - 22534
which is the new scaling defined in the DBC file.Nontrivial change: using the above formula,
f(ZERO_GAS) == -6
. I figured that since this is quite close to 0, it is likely that our previous value of 2048 was a close but incorrect guess at the true zero value. I removed the reference toZERO_GAS
and used 0 instead.Verification
Marking WIP to validate with the segments dataset as suggested by @sshane