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

IsoTpMessage: support ISO-TP w/ CAN FD #1524

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Conversation

sshane
Copy link
Contributor

@sshane sshane commented Jul 18, 2023

very much a draft just to make it work, wanted to have a CAN FD user test.

@sshane sshane marked this pull request as draft July 18, 2023 10:47
python/uds.py Show resolved Hide resolved
@coffee-cake-isaac
Copy link

Working on my F150 Lightning fork as well.

@blue-genie
Copy link

I have been running with this change in this PR, as is, for a few weeks - it fingerprints correctly every time - 2023 F-150 ICE

@sheaduncan
Copy link

@adeebshihadeh we have a lot of users on the Ford CAN-FD test branch that @coffee-cake-isaac created and continue to see successful fingerprints using this fix.

@sheaduncan
Copy link

@sshane, this is working for C3X users. However, we do have one Mach-E user who missed a FP and was returning Mazda ECUs.

This UDS fix works to get all the responses, but there may need to be some work on the FW version query on the OP side.

image
These VEHICLE_MANUFACTURER_ECU_SOFTWARE_NUMBER seemed to be shared between Ford and Mazda

@coffee-cake-isaac
Copy link

Also seem to have a bit of an issue with some HKG users no longer fingerprinting properly, either.

@sshane
Copy link
Contributor Author

sshane commented Jan 30, 2024

@coffee-cake-isaac what is the issue related to HKG?

@sshane
Copy link
Contributor Author

sshane commented Jan 30, 2024

@sshane, this is working for C3X users. However, we do have one Mach-E user who missed a FP and was returning Mazda ECUs.

This UDS fix works to get all the responses, but there may need to be some work on the FW version query on the OP side.

image
These VEHICLE_MANUFACTURER_ECU_SOFTWARE_NUMBER seemed to be shared between Ford and Mazda

Do you have a dongle ID for that user?

@coffee-cake-isaac
Copy link

coffee-cake-isaac commented Jan 30, 2024

@coffee-cake-isaac what is the issue related to HKG?

I don't actually know if this was related or not; Sunny's users were having issues when introducing this with Sunnypilot, but I think it was unrelated as we haven't heard anything since.

EDIT: I just confirmed this is still in SP and we haven't had fingerprint issues aside from the overlapping ones like myself and another Mach E have experienced.

@coffee-cake-isaac
Copy link

@sshane, this is working for C3X users. However, we do have one Mach-E user who missed a FP and was returning Mazda ECUs.
This UDS fix works to get all the responses, but there may need to be some work on the FW version query on the OP side.
image
These VEHICLE_MANUFACTURER_ECU_SOFTWARE_NUMBER seemed to be shared between Ford and Mazda

Do you have a dongle ID for that user?

Actually, since I've had some ECU upgrades done for a recall mine is doing it now, as well. Since most of my routes recently have been on a fork without this panda change, I have a route that shows this as well.

112e4d6e0cad05e1|2024-01-03--12-11-40

@sheaduncan
Copy link

@sshane, this is working for C3X users. However, we do have one Mach-E user who missed a FP and was returning Mazda ECUs.
This UDS fix works to get all the responses, but there may need to be some work on the FW version query on the OP side.
![image](https://private-user-images.githubusercontent.com/22713956/291014423-74d2d196-0821-4bc6-b87b-69321aa7bc04.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDY2MjI4ODYsIm5iZiI6MTcwNjYyMjU4NiwicGF0aCI6Ii8yMjcxMzk1Ni8yOTEwMTQ0MjMtNzRkMmQxOTYtMDgyMS00YmM2LWI4N2ItNjkzMjFhYTdiYzA0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMzAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTMwVDEzNDk0NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWU3OGM4OWRlNTIzMzc5YThlOTllYzU5MDI1NTdlZTdhZmE2NjVkNDVlYzQ1ZmEyZTEwZTY3Njg0OGFkOTljODUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N

@sshane, this is working for C3X users. However, we do have one Mach-E user who missed a FP and was returning Mazda ECUs.
This UDS fix works to get all the responses, but there may need to be some work on the FW version query on the OP side.
image
These VEHICLE_MANUFACTURER_ECU_SOFTWARE_NUMBER seemed to be shared between Ford and Mazda

Do you have a dongle ID for that user?

83a4e056c7072678
IMG_0277

@sshane
Copy link
Contributor Author

sshane commented Jan 31, 2024

112e4d6e0cad05e1|2024-01-03--12-11-40

That route looks fine as long as we get everything back from the Ford queries, if ECUs happen to respond to Mazda queries, that's okay

Edit: same with 83a4e056c7072678, EPS responds to both ford and mazda so during fingerprinting it will only look at Mazda

@sheaduncan
Copy link

112e4d6e0cad05e1|2024-01-03--12-11-40

That route looks fine as long as we get everything back from the Ford queries, if ECUs happen to respond to Mazda queries, that's okay

Edit: same with 83a4e056c7072678, EPS responds to both ford and mazda so during fingerprinting it will only look at Mazda

Strange, not sure why they're failing to fingerprint then. Those FW versions are in Ford's listed FW versions.

@coffee-cake-isaac
Copy link

Yeah, they all come back but it says "Car Unrecognized".

@sshane
Copy link
Contributor Author

sshane commented Jan 31, 2024

Which route? All recent mock routes (not many) for 83a4e056c7072678 fingerprint on master.

Comment on lines 490 to +491
elif rx_data[0] >> 4 == ISOTP_FRAME_TYPE.FIRST:
# should work with CAN FD. maybe a check for max 62 bytes from the spec?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sheaduncan can you get a route where you cause a query longer than the single frame can hold, so we can see the first and consecutive frame action?

Copy link

@sheaduncan sheaduncan Feb 7, 2024

Choose a reason for hiding this comment

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

This is with a fresh copy of master and pointing to this Panda Branch (isotp-canfd-support):
09136c309ba9461d/2024-02-07--08-51-50

This is with a fresh copy of master with current Panda master:
09136c309ba9461d/2024-02-07--14-42-56

Choose a reason for hiding this comment

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

2 more routes:

Red Panda in Aux port of C3 and USB Power into Main
09136c309ba9461d/2024-02-07--17-31-57

Red Panda in Aux port and OBD2 Power in Main
09136c309ba9461d/2024-02-07--17-36-38

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sheaduncan can you try this branch and upload the rlog? It won't fingerprint. https://github.com/commaai/openpilot/tree/ford-test

Choose a reason for hiding this comment

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

@sshane None of these fingerprinted for that branch. Getting Hyundai responses and getting new FW extended versions.

09136c309ba9461d/00000000--7340e1444b
09136c309ba9461d/00000001--578b507ed2
09136c309ba9461d/00000002--b138639015
09136c309ba9461d/00000003--7e95bc66d8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's what I was looking for. I'll check it out when I have time.

Revert "handle can fd first frames"

This reverts commit 0dcc048.
@sheaduncan
Copy link

What's needed to help get this PR merged?

@northportio
Copy link

status update?

@sshane
Copy link
Contributor Author

sshane commented May 28, 2024

When we officially support CAN FD Ford (we are looking into a vehicle to purchase), we will put the time into verifying and merging this!

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

Successfully merging this pull request may close these issues.

6 participants