openpilot icon indicating copy to clipboard operation
openpilot copied to clipboard

hyundai: ev6 - use good steering angle sensor

Open gregjhogan opened this issue 3 years ago • 20 comments

Our EV6 has a rack mounted power steering motor, and we have signals for two different steering angles. I have a theory that one sensor is on the steering column and the other is on the steering rack, and we are currently using the one on the steering column which looks like it may be a much worse signal (see data from d824e27e8c60172c|2022-09-15--09-57-55--6 below).

image

Not sure if we need to worry about some CAN-FD cars having a rack mounted power steering motor and other cars having a column mounted power steering motor.

Before merging:

  • [x] Decide if it is OK the logged angle rate is for a different sensor (@HaraldSchafer says that is fine)
  • [x] Determine if LAT_ACCEL_FACTOR should change (@HaraldSchafer can you take a look?)
  • [ ] Determine what platforms we should apply this change to (@adeebshihadeh can you look at data for other cars this change would currently affect and exclude them if necessary?)

gregjhogan avatar Sep 16 '22 00:09 gregjhogan

Do we care if the steering angle rate is for a different sensor than the steering angle?

gregjhogan avatar Sep 16 '22 00:09 gregjhogan

How do we re-analyze what should be used for LAT_ACCEL_FACTOR?

gregjhogan avatar Sep 16 '22 02:09 gregjhogan

Do you see a real difference between the acceleration derived from the steering angle and what the localizer says at high angles/accelerations?

sshane avatar Sep 16 '22 02:09 sshane

Do you see a real difference between the acceleration derived from the steering angle and what the localizer says?

I haven't looked at the data for that, but In the image of the above segment I see at one point the angle in one signal changing about 0.1 deg and the other changing almost 2 deg when they diverge and come back together.

gregjhogan avatar Sep 16 '22 02:09 gregjhogan

I have an hda1 ev6 and can check if I have two steering angles like your ev6 if that is helpful at all. Let me know what I should look for or post.

hoomoose avatar Sep 16 '22 05:09 hoomoose

The localizer seems to confirm this change is an improvement for our EV6 image

gregjhogan avatar Sep 16 '22 15:09 gregjhogan

Saw @hoomoose comment in the Discord that these changes completely eliminated ping pong in his HDA1 EV6 Wind, so I cherry-picked these two commits (https://github.com/commaai/openpilot/pull/25801/commits/4cde6afd36e22cb15d3544ec3d2410e34e0892a8, https://github.com/commaai/openpilot/pull/25801/commits/508c00fd0642a30251ab678a8daf6b249cca1604) on to my local copy of his HDA1 EV6 branch. Interestingly, I had the exact opposite experience - in my HDA1 EV6 Light, these two commits on top of that branch result in extreme oversteering. Are there more recent changes in master that this PR depends on?

mattwebbio avatar Sep 18 '22 03:09 mattwebbio

I also cherry picked those changes into my IONIQ 5 and drive IS now super smooth. Definitely an improvement!

ioniq5-cz avatar Sep 20 '22 11:09 ioniq5-cz

From a Tucson Hybrid route (36e10531feea61a4|2022-08-16--18-14-13), it appears MDPS->STEERING_ANGLE is always zero, however MDPS->STEERING_ANGLE_2 is set. It also looks nearly identical to the current STEERING_SENSORS->STEERING_ANGLE signal.

Screenshot from 2022-09-27 19-07-44 Screenshot from 2022-09-27 19-10-21 Screenshot from 2022-09-27 19-11-44

sshane avatar Sep 28 '22 02:09 sshane

On the ev6 STEERING_SENSORS->STEERING_ANGLE also looked extremely similar a lot of the time. It does seems like the Tuscon has a rack mounted EPS (R-MDPS).

gregjhogan avatar Sep 28 '22 05:09 gregjhogan

It looks like it can vary per-platform as well. We're going to need live detection like we do for Toyota with its more accurate angle sensor (unless STEERING_ANGLE_2 is a copy of STEERING_SENSOR's signal and we can just use that):

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=1526
platform='KIA EV6 2022', segment='22de8111a8c5463c|2022-07-29--13-34-49--6', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=1377
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--0', len(new_steering_angle)=5558, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=650
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--7', len(new_steering_angle)=6000, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=1814
platform='KIA EV6 2022', segment='22de8111a8c5463c|2022-07-29--13-34-49--1', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=1257
platform='KIA EV6 2022', segment='22de8111a8c5463c|2022-07-29--13-34-49--0', len(new_steering_angle)=5930, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=1369
platform='KIA EV6 2022', segment='22de8111a8c5463c|2022-07-29--13-52-45--2', len(new_steering_angle)=6000, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=140
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--4', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=72
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--5', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=846
platform='KIA EV6 2022', segment='22de8111a8c5463c|2022-07-29--13-34-49--5', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=97
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--3', len(new_steering_angle)=6000, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=52
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--6', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=362
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--2', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

set(new_steering_angle)={-0.0}, len(set(new_steering_angle))=1, len(set(new_steering_angle_2))=1280
platform='HYUNDAI TUCSON HYBRID 4TH GEN', segment='36e10531feea61a4|2022-08-16--18-14-13--1', len(new_steering_angle)=5999, new_steering_angle == new_steering_angle_2=False

sshane avatar Oct 05 '22 06:10 sshane

There are four different situations here:

  • Tucson Hybrid: MDPS->STEERING_ANGLE is not set, and MDPS->STEERING_ANGLE_2 is nearly identical to STEERING_SENSORS->STEERING_ANGLE (~0.05-0.1 degree angle error across 8 segments). We can use MDPS->STEERING_ANGLE_2 here.
  • EV6: MDPS->STEERING_ANGLE equals MDPS->STEERING_ANGLE_2 and has an average of 0.9 degrees of error across 154 segments (max segment error was 2.32 deg, min 0.18 deg).
  • IONIQ 5 (40 segments): MDPS->STEERING_ANGLE equals MDPS->STEERING_ANGLE_2 and has an average of 0.80 degrees of error across 40 segments (max segment error was 1.75 deg, min 0.25 deg).
  • IONIQ 5 (5 segments): All from one user, but this IONIQ 5 is similar to the TUCSON in that MDPS->STEERING_ANGLE is not set. Average angle error with STEERING_SENSORS->STEERING_ANGLE is 0.75 deg (max segment error was 1.1 deg, min 0.62 deg) The difference between the Tucson though is that the two signals have a higher error, and precision looks to be about the same for both. Toss up on whether either signal is more accurate to the localizer's accel so it's fine to use the new signal here: https://imgur.com/a/yS4K57y

sshane avatar Oct 05 '22 20:10 sshane

It looks like this new signal can be more noisy and diverge from the localizer accel compared to the previous signal. Since this isn't easy to merge anymore, going to leave this open for now.

EV6: https://imgur.com/a/V7lmbKU

sshane avatar Oct 05 '22 20:10 sshane

When I zoom in on a random place where the MDPS signal is noisy, the noise seems to have the same frequency as the motor torque output. I feel like the localizer may filter this level of noise out, and things like suspension dynamics may also hide changes in steering angle from the localizer at the level we are looking here, but maybe I am wrong. image

gregjhogan avatar Oct 06 '22 02:10 gregjhogan

Is there any chance you (Comma) would consider merging the torque values separate from the switch to MDPS? Using the MDPS steering angle sensor makes OP unusable for me on the EV6 Light (maybe I don't have it?), but without the torque settings in this PR I have pretty strong understeer.

Update - opened a PR: https://github.com/commaai/openpilot/pull/26076

mattwebbio avatar Oct 14 '22 15:10 mattwebbio

recent example where ping pong was reported that looks similar 5e31c7585bf1a946|2022-11-04--00-00-39--9 image

gregjhogan avatar Nov 04 '22 18:11 gregjhogan

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 Dec 07 '23 01:12 github-actions[bot]

@sshane what's the verdict on this?

@nuwandavek is it worth switching the signal for a single platform?

adeebshihadeh avatar Dec 07 '23 01:12 adeebshihadeh

Looked at the data from the 2022 Hyundai Tucson Hybrid and this signal has much less noise than the current one used in upstream.

sunnyhaibin avatar Dec 21 '23 05:12 sunnyhaibin

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 Jan 21 '24 01:01 github-actions[bot]

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

github-actions[bot] avatar Jan 29 '24 01:01 github-actions[bot]