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

Revert "FPv2: fingerprint on all FW combinations" #25417

Merged
merged 8 commits into from
Aug 13, 2022

Conversation

sshane
Copy link
Contributor

@sshane sshane commented Aug 12, 2022

Reverts the inter-brand fingerprinting part of #25204, but still keeps around fingerprinting on multiple FW versions for an ECU from the same brand.

@sshane sshane added fingerprint car vehicle-specific labels Aug 12, 2022
@adeebshihadeh
Copy link
Contributor

Any reason this is a draft?

Comment on lines 246 to 247
fw_versions_dict[(addr, sub_addr)].add(fw.fwVersion)
return dict(fw_versions_dict)
fw_versions_dict[(addr, sub_addr)] = fw.fwVersion
return fw_versions_dict
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still want the reverted code here, as this will still be used for keeping around multiple responses from one ecu from different brand queries (some Hyundai ECUs respond differently to two different Hyundai queries for example).

Comment on lines +424 to +425
# Try to match using FW returned from this brand only
matches = match_fw_to_car_exact(build_fw_dict(car_fw))
Copy link
Contributor Author

@sshane sshane Aug 13, 2022

Choose a reason for hiding this comment

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

Goes from trying to match on all FW so far -> trying to match only on FW returned from this brand

Comment on lines +349 to +355
brands = get_interface_attr('FW_VERSIONS', ignore_none=True).keys()
for exact_match, match_func in exact_matches:
# TODO: For each brand, attempt to fingerprint using only FW returned from its queries
# For each brand, attempt to fingerprint using all FW returned from its queries
matches = set()
fw_versions_dict = build_fw_dict(fw_versions, filter_brand=None)
matches |= match_func(fw_versions_dict)
for brand in brands:
fw_versions_dict = build_fw_dict(fw_versions, filter_brand=brand)
matches |= match_func(fw_versions_dict)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is responsible for going from matching using all FW, to filtering FW down to each brand's queries and fingerprinting solely on that.

@sshane sshane marked this pull request as ready for review August 13, 2022 06:15
@sshane
Copy link
Contributor Author

sshane commented Aug 13, 2022

There were some good improvements baked into that PR so I only reverted what we need to ensure we only fingerprint using a specific brand's returned versions.

The main improvement this leaves in is making sure we try to fingerprint on all FW returned from the same ECU from multiple different brand queries (Hyundai is guilty of this).

This may cause some cars to fail to fingerprint on master, but I think we need that to happen to gather data to know what to remove.

selfdrive/car/fw_versions.py Outdated Show resolved Hide resolved
@sshane sshane merged commit 45cfcfa into master Aug 13, 2022
@sshane sshane deleted the revert-25204-fpv2-all-queries branch August 13, 2022 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car vehicle-specific fingerprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants