openpilot
openpilot copied to clipboard
commaai
GM long: add pitch-compensation
(This, but without model-predicted pitch, and this is entirely contained in gm/carcontroller)
Description/justification
This PR adds pitch compensation to longitudinal control for GM cars, resulting in a large decrease in longitudinal error (versus the long plan) when pitch changes or when on an incline/decline.
Adds longitudinal pitch compensation to be used in cars that use gas/brake control, where the command specifies an amount of acceleration force rather than an amount of acceleration relative to the road. The implementation is contained in car controller, as a type of passive long feedforward, transparent to the controller.
To allow this to account for longitudinal actuator delay to allow for smoothing of the pitch signal, future model pitch is used to create a smoothed, predictive pitch.
Though not shown here, another benefit of this improvement is that if more than ACCEL_MAX acceleration is required to maintain speed up a steep hill, stock will not be able to maintain speed. With this improvement that is no longer the case.
actuators.accelPitchCompensated is used to determine the gas command, which specifies an acceleration force. The brake command on gm specifies acceleration relative to the ground, so the actuators.accel value is the correct value for friction brakes, even when going downhill.
To support this assertion, see that gas command does not match up with aEgo:
While brake command does match with aEgo
I also verified this extensively (though not with proper documentation) when developing "one pedal mode" in my openpilot fork, which involved artificially sending a constant brake command and observing the car's braking behavior. I found that the same brake command produces far more braking force on a decline, and far less (or none) on an incline.
However, when openpilot requests negative acceleration in order to stop behind a lead when going uphill, it would engage the friction brakes sooner (at the same time it would now, without this improvement), despite the negative gravitational acceleration, and resulting in more braking than in necessary, since it's being assisted by gravity. So you'd like for openpilot to postpone friction brake use when decelerating uphill, but it can become a problem when stopping, as the pitch-compensated acceleration may remain positive even though openpilot would like to stop, resulting in late stopping behind leads. The solution is to smoothly switch to non-pitch-compensated acceleration for determining brakes at low speed. I've been using this method on my fork for about a month and it keeps the efficiency and brake-avoidance benefits of pitch-compensation without introducing possible problems when stopping behind leads.
(Any issues caused by the gas and brake signals having different physical meanings will also be minimized by using the correct gas/brake threshold acceleration value)
This should also be used on other makes that specify acceleration force with gas or brake commands
Route: c11fcb510a549332|2022-08-19--09-03-54--0
Route: c11fcb510a549332|2022-08-19--09-03-54--0
Verification
Here we compare master (no pitch) with present and future pitch compensation on a couple steep steps of a hill, going up and down the steps. "Present pitch" is from the above route.
Pitch compensation cuts speed error in half, and lowers acceleration error by 13%.
Additionally, this eliminates the problem of MAX_ACCEL (2m/s^2) being insufficient for climbing steep hills; with pitch compensation op can accelerate up any hill the car can.
(In the future, when Comma has sufficiently verified the model predicted pitch, I'd like to include future pitch in order to compensate for long actuator delay, which, as you can see here, decreases speed and acceleration error by an additional 46% compared to when using instantaneous pitch, and is tunable depending on the value of long actuator delay. Compared to master, future pitch compensation reduces speed and acceleration error by 74% and 53% respectively, resulting in greater passenger comfort and improved vehicle efficiency.)
@HaraldSchafer would Bolt long be helpful in verifying this? I can try to get that merged
It's possible we need something similar to this for Toyotas to undo the accel compensation the PCM does, so we should try to make this as generic as possible.
So something in between this and the previous implementation that was unnecessarily couched inside of liveParams?
This isn't that large; blocking this on abstracting it out seems like premature optimization.
The additional calculation of accel_g in the else block necessary at all. If that's not there, then accel is only changed at the lower rate but logged at 100Hz, making the data appear choppy in logs.
With a view towards https://github.com/commaai/openpilot/issues/30341, second-order effects would change the calculation from pitch to sin(pitch) cos(roll). I can't say for sure if it's necessary, but freeway ramps, which can have pronounced pitch and roll, come to mind.
Thanks @simontheflutist.
Oops forgot to close this. This too will be superseded by #29659.
This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I'll get Shanes's proposed changes done and a new test route so that this can be merged
This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.
This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.
It looks like you didn't use one of the Pull Request templates. Please check the contributing docs. Also make sure that you didn't modify any of the checkboxes or headings within the template.
This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.