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

Use both pitch and roll angles in steering compensation #30341

Closed
simontheflutist opened this issue Oct 26, 2023 · 9 comments
Closed

Use both pitch and roll angles in steering compensation #30341

simontheflutist opened this issue Oct 26, 2023 · 9 comments
Assignees

Comments

@simontheflutist
Copy link

The roll compensation formula in the 0.8.13 blog post is

$$ \dot v = \ldots + g \theta $$

where $\theta$ is the car's roll angle (treated via a small-angle approximation). Considering that locationd provides both roll $\theta$ and pitch $\phi$, I would propose to replace this formula with

$$ \dot v = \ldots + g \cos \phi \sin \theta. $$

Linearization shows that $\cos \phi \sin \theta = \theta + o(|\phi| + |\theta|)$ for small angles $\phi$ and $\theta$, but it occurs to me that including second-order effects might help with further reducing the steering error that remains.

@simontheflutist
Copy link
Author

simontheflutist commented Oct 26, 2023

Here is a Python function to that compares my proposed compensation to the existing compensation in terms of grade and cross slope.

import numpy as np

def kinematic_orientation_compensation(cross_slope, grade):
    sin_theta = cross_slope / (1. + cross_slope**2)**0.5
    cos_phi = 1. / (1. + grade**2)**0.5
    
    # incumbent compensation factor
    theta = np.arcsin(sin_theta)
    # new compensation factor
    a = sin_theta * cos_phi
    
    print(f"linear compensation error is {(theta-a)/a * 100:.2f}%")
    print(f"which is {(theta - a) * 9.8:.3f} m/s/s")

For an extreme example, let's combine the following values:

>>> kinematic_orientation_compensation(cross_slope=0.1, grade=0.23)
linear compensation error is 2.78%
which is 0.026 m/s/s

If this acceleration went uncorrected for, say, 10 seconds, the horizontal drift would be 1.3 meters. Of course, the openpilot control loop (including the planner) would be able to soak up much of the error in due time. But it is always better not to overwork the integral controller.

twilsonco added a commit to twilsonco/openpilot that referenced this issue Oct 27, 2023
twilsonco added a commit to twilsonco/openpilot that referenced this issue Oct 27, 2023
twilsonco added a commit to twilsonco/openpilot that referenced this issue Oct 27, 2023
simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Oct 28, 2023
twilsonco added a commit to twilsonco/openpilot that referenced this issue Oct 28, 2023
@Verylukyguy
Copy link
Contributor

Verylukyguy commented Oct 28, 2023

If this acceleration went uncorrected for, say, 10 seconds, the horizontal drift would be 1.3 meters. Of course, the openpilot control loop (including the planner) would be able to soak up much of the error in due time. But it is always better not to overwork the integral controller.

It has been widely noted that on long smooth curves OpenPilot seems to "give up" at some point and if you touch the steering wheel for a second, OpenPilot "resets" and completes the curve on its own.

A couple years ago, @JasonJShuler saw this and said that is was likely a bug buried deep in OpenPilot from the beginning.
I wonder if this is it.

simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Oct 28, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
@simontheflutist
Copy link
Author

@twilsonco latest patch is a better implementation than feeding straight into control. This plugs into the existing orientation-for-control filtering strategy in paramsd, avoiding difficult questions of pitch noise.

@simontheflutist
Copy link
Author

It has been widely noted that on long smooth curves OpenPilot seems to "give up" at some point and if you touch the steering wheel for a second, OpenPIlot "resets" and completes the curve on its own.

What I am concerned about is the implementation of live parameter estimation in paramsd and torqued (e.g. observability issue?) Adaptive control is difficult to do right, and history is full of examples such as the MIT rule where a very reasonable controller failed in unexpected ways.

simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Oct 28, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
@Verylukyguy
Copy link
Contributor

@twilsonco latest patch is a better implementation than feeding straight into control. This plugs into the existing orientation-for-control filtering strategy in paramsd, avoiding difficult questions of pitch noise.

With you guys working together, I can't wait to see the results.

simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Oct 31, 2023
simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Oct 31, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Nov 1, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Nov 3, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
twilsonco added a commit to twilsonco/openpilot that referenced this issue Nov 3, 2023
twilsonco added a commit to twilsonco/openpilot that referenced this issue Nov 3, 2023
simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Nov 4, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
simontheflutist added a commit to simontheflutist/openpilot that referenced this issue Nov 4, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341
cydia2020 added a commit to cydia2020/dodgypilot that referenced this issue Nov 6, 2023
params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

commaai#30341

Co-Authored-By: simontheflutist <13023178+simontheflutist@users.noreply.github.com>
@nuwandavek
Copy link
Contributor

@simontheflutist this is conceptually in the correct direction. The bulk of the work, like with roll compensation, is in making sure the pitch value from the localizer can be completely trusted, on a large dataset. Roll from the localizer had a failure (from various causes) of once every thousand minutes or so, which had to be validated and addressed. I'm assuming it is the same for pitch. A few things like tapping the device I think cause pitch spikes, which need to be validated too. I'm adding this to my list of things to do, but may not get to it soon.

@simontheflutist
Copy link
Author

But is the pitch significantly worse than roll? If not, a paramsd feedforward would seem to benefit from the noise attenuation inherent to parameter kf.

@nuwandavek
Copy link
Contributor

It may not be significantly worse than roll (this needs to be verified though). But there are a few ways in which roll is different from pitch. For example, the device does not move/shake the same way in roll-direction than in pitch direction, when it is tapped, or when the car goes over potholes. This can lead to some sketchy and subtle lateral movement. But all of these things can be verified and handled.
If you're using this in your fork at the moment, I'd suggest you look at the above things. Also, you want to use the calibratedOrietationNED since there is non trivial pitch difference due to mounting, that we adjust for.

@nuwandavek
Copy link
Contributor

Not a priority at the moment. Will revisit later when appropriate.

@nuwandavek nuwandavek closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2023
cydia2020 added a commit to cydia2020/dodgypilot that referenced this issue Dec 4, 2023
commit 6479dbd
Author: Irene <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 18:16:14 2023 +1100

    Revert to comma device roll

    IMU roll is dodgy

commit 03f0e53
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 17:57:55 2023 +1100

    Update opendbc

commit 21f46f3
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 17:46:01 2023 +1100

    oops forgot the checks

commit 3bf3c27
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:50:08 2023 +1100

    Update cereal

commit 59d247e
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:49:32 2023 +1100

    use vehicle roll and pitch

commit 628318e
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:40:31 2023 +1100

    get pitch, roll, and yaw from vehicle IMU

commit 59cc9b1
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:35:50 2023 +1100

    Revert "sin(roll)cos(pitch) feedforward in paramsd."

    This reverts commit 96b5e10.

commit 35dd93b
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:35:43 2023 +1100

    Revert "try to use calibratedOrientationNED"

    This reverts commit 696ca81.

commit e43cf4a
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:35:40 2023 +1100

    Revert "don't touch roll_valid"

    This reverts commit 2ddb3b8.

commit 3ecff4e
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Dec 4 16:35:35 2023 +1100

    Revert "fix this"

    This reverts commit 06b5e83.

commit 06b5e83
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Tue Nov 28 15:06:46 2023 +1100

    fix this

commit e6496af
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Tue Nov 28 14:57:58 2023 +1100

    whitelist all toyotas

commit 2ddb3b8
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Sat Nov 11 10:57:29 2023 +1100

    don't touch roll_valid

commit 696ca81
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Sat Nov 11 10:54:43 2023 +1100

    try to use calibratedOrientationNED

commit 16f70d0
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Nov 6 17:15:26 2023 +1100

    oops

commit 8847fe3
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Nov 6 17:12:49 2023 +1100

    revert: build everything on device

commit 96b5e10
Author: cydia2020 <12470297+cydia2020@users.noreply.github.com>
Date:   Mon Nov 6 16:47:18 2023 +1100

    sin(roll)cos(pitch) feedforward in paramsd.

    params.roll is now reinterpreted as "gravity factor" rather than physical configuration, but this reinterpretation agrees perfectly with the existing linear model of acc = "roll" * g

    commaai#30341

    Co-Authored-By: simontheflutist <13023178+simontheflutist@users.noreply.github.com>

commit 98c270b
Author: Irene <12470297+cydia2020@users.noreply.github.com>
Date:   Tue Sep 26 14:34:20 2023 +1000

    hw: add back OnePlus support (#13)

    * Update launch_chffrplus.sh

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * Add libs section

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * fan controller

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * fix up fan controller

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * revert oneplus cleanup

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * properly restored camera_qcom.*

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * compile camerad on device only

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    * fix compilation and remove op3t event

    Co-Authored-By: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>

    ---------

    Co-authored-by: Internet of Tofu <107650540+InternetofTofu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants