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

Discussion-only PR for Volkswagen FPv2 #1427

Closed
wants to merge 2 commits into from

Conversation

jyoung8607
Copy link
Collaborator

@jyoung8607 jyoung8607 commented Apr 27, 2020

Do not merge, discussion only. CI expected to fail.

This is an example of minimally viable FPv2 for Volkswagen MQB. It adds support for different rx_addr offsets to the UDS and ISO-TP code in a way that tries to avoid affecting queries for other cars. Identification is done by querying the steering characteristic parameters from the EPS rack. A version of this does work on my development vehicle.

Further discussion: #1238 (comment)

TL;DR: I am not in favor of the steering-rack parameter ONLY approach to identification, but I'll implement it if that's what Comma wants. I still strongly advocate for using the VIN, backed up by UDS validation of versions, as the most correct and future-proof solution. It could be done in a way that only affects car port(s) that want to use the VIN as part of the identity mix.

Thoughts requested from @gregjhogan and @pd0wm based on our DM discussions.

@jyoung8607
Copy link
Collaborator Author

jyoung8607 commented Apr 27, 2020

This proposal would include a change to the Panda UDS library as follows:

def get_rx_addr_for_tx_addr(tx_addr, rx_offset=0x8):
  if tx_addr in FUNCTIONAL_ADDRS:
    return None

  if tx_addr < 0xFFF8:
    # Standard 11 bit response addr (add 8), unless overridden
    # Some manufacturers use different offsets
    return tx_addr + rx_offset

Just didn't want to carry around a second PR to Panda for discussion only.

@jyoung8607 jyoung8607 marked this pull request as draft April 27, 2020 18:55
@jyoung8607 jyoung8607 marked this pull request as ready for review April 28, 2020 00:46
@pd0wm
Copy link
Contributor

pd0wm commented Apr 28, 2020

As discussed:

All MQB cars have the same ecu fw versions, and they speak the same API. For openpilot there will be only one car. CAR.VW_MQB. The only place we need to distinguish between car models is in the get_params to get to correct wheelbase and tuning params. For now we'll use VIN there, if in the future that doesn't work we can very easily switch to something else (e.g. the EPS has an dataflash id). By solving it this way the FW query is the same as all other cars, and the VIN code is very contained.

@jyoung8607
Copy link
Collaborator Author

Withdrawing for now, per our discussion. Will take another run at this in late May/early June.

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

Successfully merging this pull request may close these issues.

2 participants