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

HKG: Car Port for Kia Sportage Plug-in Hybrid 2023 #30575

Closed

Conversation

sunnyhaibin
Copy link
Contributor

@sunnyhaibin sunnyhaibin commented Dec 2, 2023

Checklist

  • added entry to CarInfo in selfdrive/car/*/values.py and ran selfdrive/car/docs.py to generate new docs
  • test route added to routes.py
  • route with stock system: 428412ad82894ba3|2023-10-27--15-44-21
  • route with openpilot: 428412ad82894ba3|2023-12-09--17-01-47
  • harness Type: Hyundai N
  • architecture: CAN-FD

Link to all physical parts during installation

Thanks to the community 2022 Kia Sportage Plug-in Hybrid owner dhimmels.

@sshane
Copy link
Contributor

sshane commented Dec 5, 2023

@dhimmel @sunnyhaibin any reason not to combine with the current hybrid platform? We should get the HKG port into a state where this is just a fingerprint.

@sshane sshane added car port car vehicle-specific hyundai labels Dec 5, 2023
@sunnyhaibin
Copy link
Contributor Author

@dhimmel @sunnyhaibin any reason not to combine with the current hybrid platform? We should get the HKG port into a state where this is just a fingerprint.

Initially, the car seemed to do SCC from the radar, but the latest route confirmed it's doing SCC from the camera.

@sshane
Copy link
Contributor

sshane commented Dec 9, 2023

Great, then let's combine if possible

@sshane
Copy link
Contributor

sshane commented Dec 9, 2023

@dhimmel let us know when you can get a test route, just need to drive around for 10 minutes, ideally engaging with the system!

@dhimmel
Copy link

dhimmel commented Dec 10, 2023

let us know when you can get a test route, just need to drive around for 10 minutes, ideally engaging with the system!

@sshane I have this route from yesterday with id 428412ad82894ba3|2023-12-09--17-01-47--32. It might be too long (1 hour) but has several periods with OpenPilot engaged. Let me know if something shorter would be preferable for the tests?

@sshane
Copy link
Contributor

sshane commented Dec 12, 2023

I see we have been splitting up the hybrid and plug-in hybrids because of the different masses; we should start using the info in the hvac FW version soon, which splits ICE, hybrid and plug-in hybrid. It seems to be available on all the variants of the Sorento too, so we can combine a couple platforms with this.

For now though, we can merge as a separate platform as that will take some large data verification.

@sshane
Copy link
Contributor

sshane commented Dec 12, 2023

@dhimmel can you put your device on WiFi and upload a short 10 minute route with some engagement? Then we can merge!

selfdrive/car/hyundai/interface.py Outdated Show resolved Hide resolved
selfdrive/car/hyundai/interface.py Outdated Show resolved Hide resolved
selfdrive/car/hyundai/values.py Outdated Show resolved Hide resolved
selfdrive/car/hyundai/values.py Outdated Show resolved Hide resolved
@sshane sshane marked this pull request as ready for review December 12, 2023 00:09
@@ -116,6 +116,7 @@ class CarTestRoute(NamedTuple):
CarTestRoute("6a42c1197b2a8179|2023-09-21--10-23-44", HYUNDAI.KIA_OPTIMA_H_G4_FL),
CarTestRoute("c75a59efa0ecd502|2021-03-11--20-52-55", HYUNDAI.KIA_SELTOS),
CarTestRoute("b3537035ffe6a7d6|2022-10-17--15-23-49", HYUNDAI.KIA_SPORTAGE_HYBRID_5TH_GEN),
CarTestRoute("428412ad82894ba3|2023-12-09--17-01-47", HYUNDAI.KIA_SPORTAGE_PHEV_5TH_GEN, segment=8),
Copy link

Choose a reason for hiding this comment

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

can you put your device on WiFi and upload a short 10 minute route with some engagement? Then we can merge!

@sshane I see you've added 428412ad82894ba3|2023-12-09--17-01-47 segment 8 for testing. Would you still like a different route? I can upload another shorter route in the next 2 days if that is helpful

Copy link
Contributor

@sshane sshane Dec 13, 2023

Choose a reason for hiding this comment

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

That's fine, but can you get a route where you test the torque/max lateral acceleration it will apply?

Copy link

Choose a reason for hiding this comment

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

can you get a route where you test the torque/max lateral acceleration it will apply

Next time I'm near highway, I will try the highway interchange or exit suggestion to get the out of torque warning.

In the meantime, in the 428412ad82894ba3|2023-12-17--17-23-36--6 route at 17:28:13 – 4, the road took a sharp turn and the OpenPilot oversteered. Not sure if this was due to poor visibility at night with rain and oncoming headlights though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pull and try again? It should not be oversteering as it was.

Copy link

Choose a reason for hiding this comment

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

Can you pull and try again?

What's been holding me up is that even though the C3X is configured to use the kia-sportage-phev-2023-port branch, it doesn't find the latest commit even when I manually push check. It still says "up to date" but shows a stale commit 42a8a6d as the current version.

Copy link

@dhimmel dhimmel Dec 29, 2023

Choose a reason for hiding this comment

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

it doesn't find the latest commit even when I manually push check

Ah I think I understand the issue. I think my comma is checking the commaai/openpilot:kia-sportage-phev-2023-port branch rather than the sunnyhaibin/openpilot-1:kia-sportage-phev-2023-port branch.

I will install from my fork that I sync and report back using https://smiskol.com/fork/dhimmel/kia-sportage-phev-2023-port

Copy link

Choose a reason for hiding this comment

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

Can you pull and try again? It should not be oversteering as it was.

Okay, here are logs based on d44e81c: 428412ad82894ba3|2023-12-30--13-15-10--8 and route. I didn't reach max lateral torque on this route, so let me know if we need to capture a highway exit.

Copy link

Choose a reason for hiding this comment

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

@sshane tagging for re-review with the the latest logs in this thread.

Copy link

Choose a reason for hiding this comment

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

My experience has been that it still oversteers slightly on long curves. Installed from your fork

Copy link

@dhimmel dhimmel Feb 18, 2024

Choose a reason for hiding this comment

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

Okay, so @xtian and I find that there is a bit of oversteering on sharp turns with the 2023 Kia Sportage PHEV as of d44e81c.

@Wafflezzbutt reports the same on the 2024 Kia Sportage PHEV (with additional fingerprints added).

I do feel like it overcorrects sometimes. It seems to "lean left" especially around turns, and then over correct back

So @sshane / @sunnyhaibin my questions are:

  1. is the oversteering issue also present with Hybrid and Gas models of the Kia Sportage 2023+? If so I wonder if the best approach is to merge and then fix all models.
  2. Independent of 1, how do we go about fixing the oversteeting?

@sshane
Copy link
Contributor

sshane commented Dec 13, 2023

Your car also has a very high lateral accel factor (similar to #30025), and your car consistently overshoots the desired lateral accel, we will need to figure this out before we can merge:

image

image

@royjr royjr mentioned this pull request Dec 20, 2023
20 tasks
@dhimmel
Copy link

dhimmel commented Dec 20, 2023

I have the same model and am trying to install a 3X. Can I ask where you placed the camera connector harness?

Responding to this question on discord here for easier retreivability. The following picture shows where I mounted the Comma 3x alongside the rearview mirror assembly:

image

@@ -27,7 +27,7 @@ def __init__(self, CP):
self.STEER_STEP = 1 # 100 Hz

if CP.carFingerprint in CANFD_CAR:
self.STEER_MAX = 270
self.STEER_MAX = 170
Copy link

Choose a reason for hiding this comment

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

The change in this value will end up applying to many more cars than the 2023 Sportage PHEV, right? Just curious whether the logs and your investigation uncovered a general enhancement?

Copy link
Contributor

@sshane sshane Jan 31, 2024

Choose a reason for hiding this comment

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

Yes, I only did that to test if it fixed the problems. How is it after that change?

@Wafflezzbutt
Copy link

Wafflezzbutt commented Feb 11, 2024

I have a 2024 Sportage PHEV that has very few changes from the 2023. If there is anything I can contribute to add it to the compatibility list

'carFw': [{'address': 2000, 'brand': 'hyundai', 'bus': 0, 'ecu': 'fwdRadar', 'fwVersion': b'\xf1\x00NQ5__ 1.01 1.03 99110-CH000 ', 'logging': False, 'obdMultiplexing': True, 'request': [b'"\xf1\x00'], 'responseAddress': 2008, 'subAddress': 0}, {'address': 1988, 'brand': 'hyundai', 'bus': 0, 'ecu': 'fwdCamera', 'fwVersion': b'\xf1\x00NQ5 FR_CMR AT USA LHD 1.00 1.00 99211-P1070 690', 'logging': False, 'obdMultiplexing': True, 'request': [b'"\xf1\x00'], 'responseAddress': 1996, 'subAddress': 0}, {'address': 1975, 'brand': 'hyundai', 'bus': 0, 'ecu': 'cornerRadar', 'fwVersion': b'\xf1\x00NQ5 BCW RR 1.00 , 1.00 (\x81\x98\x83#\x02u\x06Y', 'logging': True, 'obdMultiplexing': True, 'request': [b'"\xf1\x00'], 'responseAddress': 1983, 'subAddress': 0}, {'address': 1971, 'brand': 'hyundai', 'bus': 0, 'ecu': 'hvac', 'fwVersion': b'\xf1\x00NQ5PH 97255-CH100UNIT-HEATER CONTROL 1.03NQ5PHEV DATC 1.2 1.01', 'logging': True, 'obdMultiplexing': True, 'request': [b'"\xf1\x00'], 'responseAddress': 1979, 'subAddress': 0}, {'address': 1969, 'brand': 'hyundai', 'bus': 0, 'ecu': 'parkingAdas', 'fwVersion': b'\xf1\x10NQ5 ADAS_PRK ANL 1.01 1.06 99910-CH330', 'logging': True, 'obdMultiplexing': True, 'request': [b'"\xf1\x10'], 'responseAddress': 1977, 'subAddress': 0}],

THESE SEEM TO WORK FINE:

CAR.KIA_SPORTAGE_5TH_GEN: {
(Ecu.fwdRadar, 0x7D0, None): [b'\xf1\x00NQ5__               1.01 1.03 99110-CH000         ',],
(Ecu.fwdCamera, 0x7C4, None): [b'\xf1\x00NQ5 FR_CMR AT USA LHD 1.00 1.00 99211-P1070 690',],  
(Ecu.cornerRadar, 0x7B7, None): [b'\xf1\x00NQ5 BCW RR 1.00 , 1.00 (\x81\x98\x83#\x02u\x06Y',],
(Ecu.hvac, 0x7B3, None): [b'\xf1\x00NQ5PH 97255-CH100UNIT-HEATER CONTROL 1.03NQ5PHEV DATC 1.2 1.01',],
(Ecu.parkingAdas, 0x7B1, None): [b'\xf1\x10NQ5  ADAS_PRK ANL 1.01 1.06 99910-CH330',],

},

I haven't had a ton of time to test it but it drove around my neighborhood fine

@Wafflezzbutt
Copy link

https://useradmin.comma.ai/?onebox=ad5d1c08bcc42d1a%7C2024-02-15--17-19-23

I do feel like it overcorrects sometimes. It seems to "lean left" especially around turns, and then over correct back

@xtian
Copy link

xtian commented Mar 12, 2024

Is this PR blocked on needing additional route data?

@Wafflezzbutt
Copy link

While The turning radius issue seems to be less of a problem since the 0.9.6 release, I think there may be an issue with how heavy the car is. The PHEV model is 5300 lbs compared to the 3500 on the regular sportage. In experimental mode it seems to have trouble braking if there is a downward slope and it occasionally gets too close to the car in front of it when coming to a complete stop

Here is a drive today where this should be obvious:
https://useradmin.comma.ai/?onebox=ad5d1c08bcc42d1a%7C2024-03-12--17-40-15

Especially:
Segment 3
Segment 6
Segment 10

@xtian
Copy link

xtian commented Apr 11, 2024

I get a “Driver’s grasp not detected” alert from the stock ACC relatively often while using @dhimmel’s fork. Are others experiencing this?

I also frequently get a “Consider taking a break” notice while on short drives. Not sure if that’s related.

Copy link
Contributor

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@xtian
Copy link

xtian commented Jul 29, 2024

Is there anything I can do to help this get merged?

@sshane
Copy link
Contributor

sshane commented Aug 20, 2024

We've moved the car interfacing code to our opendbc repository, which is now the new home for car ports and fingerprints. Please re-open your pull request against opendbc at your convenience by using this command below. This will transform all changes under selfdrive/car/ to opendbc_repo/opendbc/car/. Make sure you have initialized submodules beforehand and have checked out the latest opendbc commit.

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.

@sshane sshane closed this Aug 20, 2024
@dhimmel
Copy link

dhimmel commented Aug 20, 2024

@xtian or @Wafflezzbutt would either of you like to pick up the mantle and migrate the PR to opendbc? I'd usually jump at the opportunity to contribute to such an impactfull open source library, but am a bit pressed for time these days. Happy to support with any evaluation and test drive work.

@xtian
Copy link

xtian commented Aug 20, 2024

I might be able to, but first I'd need to know why this PR couldn't be merged after almost nine months.

@dhimmel
Copy link

dhimmel commented Aug 20, 2024

@xtian my understanding as an onlooker and not a maintainer is that this PR languished because of the oversteering issue and that the original author @sunnyhaibin was occupied with other work. But it looks like @sunnyhaibin has opened commaai/opendbc#1138 and #33341, so we should direct any assistance efforts to those PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants