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.

Sign up for GitHub

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

Jump to bottom

VW MQB: Refactored CRC calculation #1236

Merged
merged 15 commits into from
Sep 13, 2024
Merged

VW MQB: Refactored CRC calculation #1236

merged 15 commits into from
Sep 13, 2024

Conversation

JaCzekanski
Copy link
Contributor

As discussed in #1016, this change get's rid of duplicate constants for messages that use single hex for all counter values.

Do not merge before #1235

@github-actions github-actions bot added the can related to CAN tools, aka opendbc/can/ label Sep 11, 2024
@JaCzekanski JaCzekanski mentioned this pull request Sep 11, 2024
Merged
@JaCzekanski
VW MQB: Refactor CRC constants
6a22559 
Big switch statement in volkswagen_mqb_checksum was refactored with map of arrays.
With this approach there's no need for specifying all 16 byte values if constant is the same for all of them.
@jyoung8607 jyoung8607 self-assigned this Sep 12, 2024
@jyoung8607
Copy link
Collaborator

@JaCzekanski In the abstract, I'm pretty happy with this. Your tests validated it worked within opendbc, and I did some crude profiling with 06d8f46 to verify there was no performance impact.

I was quite surprised to see it breaks when openpilot uses it, just for the CAN messages that didn't get full value initialization. I'm puzzled why this works in opendbc but not in openpilot. All is fine with 98b3385.

Do you have any insight here? I'm guessing there's been evolution in std::array over the years and some part of the openpilot/panda toolchain doesn't like it. I'm actually happy with this PR as it stands, we can keep the explicit array initialization and still get 99% of the value I hoped for, but would like to solve the mystery if we can.

@jyoung8607 jyoung8607 marked this pull request as draft September 12, 2024 20:50
@jyoung8607 jyoung8607 marked this pull request as ready for review September 12, 2024 20:50
@JaCzekanski
Copy link
Contributor Author

@jyoung8607 Yeah, it looks like it doesn't work the way intended it to: https://godbolt.org/z/9xeEd44Gc
I'm fine with leaving all values filled - slightly less neat, but still more condense compared to the previous implementation.

@jyoung8607
Copy link
Collaborator

Validation:

  • Simple profiling with 06d8f46, performance is equivalent or very slightly faster
  • New CI coverage from VW MQB: Added unittests for CRC calculation #1235, all checks pass
  • Local testing with pytest selfdrive/car/tests, all checks pass
  • Local testing with selfdrive/tests/process_replay/test_processes.py, no changes

@jyoung8607 jyoung8607 merged commit 0994697 into commaai:master Sep 13, 2024
3 checks passed
adurham added a commit to adurham/opendbc that referenced this pull request Sep 20, 2024
@adurham
Squashed commit of the following:
3058b7e 
commit c8beb97
Author: Shane Smiskol <[email protected]>
Date:   Mon Sep 16 17:30:34 2024 -0700

    Toyota: add brake force signal for hybrids (commaai#1247)

    add brake force signal

commit de025ae
Author: Shane Smiskol <[email protected]>
Date:   Mon Sep 16 17:05:56 2024 -0700

    Toyota: add drive force signal for hybrids (commaai#1246)

    * add new signal

    * set minmax

    * better comments

    * clarify

    * add FDRVREAL for hybrids

    * forgot scaling factor

commit cb92fc8
Author: Shane Smiskol <[email protected]>
Date:   Mon Sep 16 16:15:40 2024 -0700

    Toyota: add another acceleration request signal (commaai#1245)

    * add new signal

    * set minmax

    * better comments

    * clarify

commit 6f08c8a
Author: Shane Smiskol <[email protected]>
Date:   Mon Sep 16 14:48:20 2024 -0700

    Toyota: add ENG2F41 (commaai#1244)

    add ENG2F41

commit 57ebdbf
Author: Jakub Czekański <[email protected]>
Date:   Sun Sep 15 17:17:09 2024 +0200

    VW MQB: Keep CAN IDs in order (commaai#1243)

    ACC_02 had address in decimal format and was out of order.

commit 811ed77
Author: Jakub Czekański <[email protected]>
Date:   Sat Sep 14 00:04:34 2024 +0200

    VW MQB: Add missing ESP_33 messages and tests (commaai#1242)

    Added new CRC without coverage - oversight from rebasing commaai#1016

commit d55feec
Author: Jakub Czekański <[email protected]>
Date:   Fri Sep 13 18:22:34 2024 +0200

    VW MQB: Add CRC for Airbag_01, ESP_02, ESP10, ESP33, Licht_Anf_01 (commaai#1016)

    VW MQB: Add crc for Airbag_01, ESP_02, ESP_10, ESP_33 and Licht_Anf_01

commit 0994697
Author: Jakub Czekański <[email protected]>
Date:   Fri Sep 13 16:43:16 2024 +0200

    VW MQB: Refactored CRC calculation (commaai#1236)

    * VW MQB: Refactor CRC constants

    Big switch statement in volkswagen_mqb_checksum was refactored with map of arrays.
    With this approach there's no need for specifying all 16 byte values if constant is the same for all of them.

    * temporary, repeat packing step 1M times

    * follow comma convention for include order

    * comma indent convention, tighten message annotations

    * whitespace

    * cleanup

    * more comma indent convention

    * Revert "temporary, repeat packing step 1M times"

    This reverts commit 06d8f46.

    * comment touch-up

    * beautify spacing

    * codespell says you're gonna have to Google it

    * fully initialize all data ID arrays

    * clarify array name as part of VW support

    * fix comment typo

    ---------

    Co-authored-by: Jason Young <[email protected]>

commit 2b85882
Author: Adeeb Shihadeh <[email protected]>
Date:   Thu Sep 12 17:03:18 2024 -0700

    toyota doesn't have gas

commit 56c7afa
Author: Shane Smiskol <[email protected]>
Date:   Thu Sep 12 16:55:06 2024 -0700

    rm longitudinal profiles (commaai#1240)

    rm

commit b43d300
Author: Shane Smiskol <[email protected]>
Date:   Thu Sep 12 16:36:04 2024 -0700

    fix spacing for test_checksums (commaai#1239)

commit 751404a
Author: Jakub Czekański <[email protected]>
Date:   Thu Sep 12 21:02:08 2024 +0200

    VW MQB: Added unittests for CRC calculation (commaai#1235)

    * VW MQB: Added unittests for CRC calculation

    Some of the messages are either missing fields that causes serialization to skip bits
    or CRC/BZ fields aren't picked up by packers leading to invalid values.

    * VW MQB: Add missing fields in Getriebe_11

    Test case is reenabled and now passes

    * EV_Gearshift -> Motor_EV_01, enable test

    * skip Getribe_11 testing with a note about why

    * reenable all others, except SWA_01

    * turns out we can still test Getriebe_11

    * New test data for SWA_01, re-enable test

    * no longer a need for test skipping

    * no longer a need for custom tests

    * more concise

    * prefix with VW for clarity

    ---------

    Co-authored-by: Jason Young <[email protected]>

commit 4b31c18
Author: Jason Young <[email protected]>
Date:   Thu Sep 12 14:00:50 2024 -0400

    VW MQB: Various message and signal updates (commaai#1238)

    * VehicleSpeed -> ESP_08

    * ESP_21 updates

    * Motor_20 updates

    * SWA_01 updates

    * ESP_20 updates

    * Getriebe_11 updates

commit d8aba58
Author: Jason Young <[email protected]>
Date:   Thu Sep 12 12:39:20 2024 -0400

    VW MQB: EV_Gearshift -> Motor_EV_01 (commaai#1237)

    * DBC updates

    * update comment in CRC function

    * updates to carstate and interface

    * update shifter VAL lookup

commit c43cd3a
Author: Shane Smiskol <[email protected]>
Date:   Tue Sep 10 19:07:24 2024 -0700

    Nidec: add longitudinal control comment

    This name is confusion

commit 3ff9057
Author: Shane Smiskol <[email protected]>
Date:   Tue Sep 10 15:20:34 2024 -0700

    Test all torque cars' lateral limits (commaai#1232)

    * test more models!

    * Impreza 2020: reach max in ~0.8s instead of ~0.6

    * leave todo for gen2, not safety critical since jerk is under threshold (1000/40*2/100=0.5s)

    * fix honda

commit ef7102a
Author: Adeeb Shihadeh <[email protected]>
Date:   Sat Sep 7 16:32:53 2024 -0700

    Toyota: AEB actuation setup (commaai#1227)

    * try this

    * cleanup

    * lil more

    ---------

    Co-authored-by: Shane Smiskol <[email protected]>

commit c0a9ab5
Author: Shane Smiskol <[email protected]>
Date:   Thu Sep 5 15:45:42 2024 -0700

    Move radar delay to carParams (commaai#1224)

    move radar delay to CP

commit 3dde383
Author: Shane Smiskol <[email protected]>
Date:   Thu Sep 5 14:15:40 2024 -0700

    Radar interface getter (commaai#1220)

    * ret it now

    * don't forget

    * better

commit 86be858
Author: Jason Young <[email protected]>
Date:   Thu Sep 5 13:23:46 2024 -0400

    Hongqi: Add DBC for Hongqi HS5 (commaai#580)

commit 1994800
Author: dkiiv <[email protected]>
Date:   Thu Sep 5 12:47:35 2024 -0400

    VW PQ: Volkswagen Jetta 6th Gen (commaai#1208)

    VW PQ: add fingerprint for new car model

commit 6619c18
Author: Maxime Desroches <[email protected]>
Date:   Wed Sep 4 14:48:02 2024 -0700

    distutils comment (commaai#1222)

    comment

commit 1e6e045
Author: Maxime Desroches <[email protected]>
Date:   Wed Sep 4 14:20:59 2024 -0700

    add setuptools for python3.12 (commaai#1221)

    setuptools

commit ebb6d22
Author: Shane Smiskol <[email protected]>
Date:   Wed Sep 4 14:11:29 2024 -0700

    Revert "pytest: add default fixture option for pytest-asyncio"

    This reverts commit 81b081b.

commit 81b081b
Author: Shane Smiskol <[email protected]>
Date:   Wed Sep 4 13:59:50 2024 -0700

    pytest: add default fixture option for pytest-asyncio

    commaai/openpilot#33442

commit 936bd83
Author: Shane Smiskol <[email protected]>
Date:   Wed Sep 4 13:59:21 2024 -0700

    Kia EV6: change non-US entry back to Southeast Asia

    A EV6 user in AUS has a Kia EV6 GT 2023 with no adas ecu, so they needed the without HDA II harness instead

commit ff198e9
Author: Shane Smiskol <[email protected]>
Date:   Wed Sep 4 12:55:50 2024 -0700

    GM: add TODO for feature sets

commit 8a4b246
Author: Shane Smiskol <[email protected]>
Date:   Wed Sep 4 12:53:49 2024 -0700

    Reapply "GM: Car Port for 2019 Chevy Volt" (commaai#1218) (commaai#1219)

    * Reapply "GM: Car Port for 2019 Chevy Volt" (commaai#1218)

    This reverts commit 1c19fd0.

    * fix safety mismatch

commit f383e5c
Author: Shane Smiskol <[email protected]>
Date:   Tue Sep 3 19:11:31 2024 -0700

    fix interfaces usage

commit 1c19fd0
Author: Shane Smiskol <[email protected]>
Date:   Tue Sep 3 19:06:36 2024 -0700

    Revert "GM: Car Port for 2019 Chevy Volt" (commaai#1218)

    Revert "GM: Car Port for 2019 Chevy Volt (commaai#1210)"

    This reverts commit ca3b5c4.

commit e1318f2
Author: Shane Smiskol <[email protected]>
Date:   Tue Sep 3 15:53:06 2024 -0700

    interfaces returns RadarInterface (commaai#1217)

    interfaces returns radarinterface

commit ca3b5c4
Author: garrettpall <[email protected]>
Date:   Tue Sep 3 16:37:42 2024 -0400

    GM: Car Port for 2019 Chevy Volt (commaai#1210)

    * Add 2019 Volt Support

    * Add Route

    * Add Package

    I think LKA is part of Driver Confidence II Package and ACC is it's own add one

commit 7bf0ecd
Author: Adeeb Shihadeh <[email protected]>
Date:   Mon Sep 2 11:24:05 2024 -0700

    fix typo

commit 727f5c1
Author: Adeeb Shihadeh <[email protected]>
Date:   Mon Sep 2 11:16:28 2024 -0700

    add roadmpa

commit 6de42e3
Author: Adeeb Shihadeh <[email protected]>
Date:   Sun Sep 1 15:08:49 2024 -0700

    remove unused

commit 48ab55f
Author: Adeeb Shihadeh <[email protected]>
Date:   Sun Sep 1 15:03:30 2024 -0700

    panda_runner: fixup safety mode setting

commit b4d39b9
Author: Adeeb Shihadeh <[email protected]>
Date:   Sat Aug 31 16:00:02 2024 -0700

    Toyota: more ACC-related definitions (commaai#1215)

commit 26dfa1d
Author: Alexandre Nobuharu Sato <[email protected]>
Date:   Sat Aug 31 15:41:11 2024 -0300

    ci: Bot comment only for first timers (commaai#1213)

    * ci: Bot comment only for first timers and collapsible sections for UI screenshots

    Added also implement collapsible sections for UI screenshots #33415 <commaai/openpilot#33415>

    * fix

    * Update auto_pr_review.yaml

commit 227908e
Author: garrettpall <[email protected]>
Date:   Sat Aug 31 13:28:03 2024 -0400

    GM: Car Port for XT4 2023 (commaai#1177)

    * GM: Car Port for XT4 2023

    * Remove Comment to Match

    * Update opendbc/car/gm/values.py

    ---------

    Co-authored-by: Adeeb Shihadeh <[email protected]>

commit 732bf8f
Author: Adeeb Shihadeh <[email protected]>
Date:   Fri Aug 30 21:54:15 2024 -0700

    longitudinal maneuvers fixups (commaai#1209)

    * corolla updates

    * lil more

    * lil more

    * fix

    ---------

    Co-authored-by: Comma Device <[email protected]>

commit f99bb69
Author: Adeeb Shihadeh <[email protected]>
Date:   Fri Aug 30 20:04:46 2024 -0700

    longitudinal maneuvers: proper ratekeeping

commit c928f6b
Author: Adeeb Shihadeh <[email protected]>
Date:   Fri Aug 30 16:45:10 2024 -0700

    longitudinal maneuvers: add gas/brake step responses (commaai#1207)

    * longitudinal maneuvers: add gas/brake step responses

    * corolla updates

    * initial speed

    * try beep

    * beep

    * corolla updates

    * add vego

    * enable all

    ---------

    Co-authored-by: Comma Device <[email protected]>

commit f2fa755
Author: Adeeb Shihadeh <[email protected]>
Date:   Fri Aug 30 13:48:13 2024 -0700

    longitudinal profile runner (commaai#1197)

    * long profiles

    * start with creep

    * lil cleanup

    * corolla updates

    * cleanup

    * 2s

    * plot is a little nicer

    * strict mode

    * cleanup

    * unused

    * fix that

    ---------

    Co-authored-by: Comma Device <[email protected]>

commit cbad7f0
Author: Adeeb Shihadeh <[email protected]>
Date:   Thu Aug 29 15:12:44 2024 -0700

    simple joystick example (commaai#1200)

    * setup runner

    * port over joystickd

    * fix mypy

    * set accel and steer

commit a1b95d7
Author: Shane Smiskol <[email protected]>
Date:   Thu Aug 29 12:21:25 2024 -0700

    Honda: distance bars always cycle 1, 2, 3 (commaai#1201)

    1 is 1

commit c0aa729
Author: Adeeb Shihadeh <[email protected]>
Date:   Wed Aug 28 19:28:52 2024 -0700

    new optional syntax

commit 6cb7851
Author: Adeeb Shihadeh <[email protected]>
Date:   Wed Aug 28 19:27:13 2024 -0700

    car: default now_nanos value

commit aa1d21b
Author: Adeeb Shihadeh <[email protected]>
Date:   Wed Aug 28 17:51:06 2024 -0700

    ignore uv.lock for now

commit 3a8a864
Author: Shane Smiskol <[email protected]>
Date:   Tue Aug 27 16:10:41 2024 -0700

    Toyota: add a VSC message (commaai#1190)

    * add a vsc message

    * rename

    * rename

    * add comments

    * rename

    * rename

commit c9650ff
Author: Irene <[email protected]>
Date:   Wed Aug 28 08:47:46 2024 +1000

    Toyota: add misc adas/toggle signal definitions (commaai#1076)

    * Toyota: add signals

    * run generator

    * better name

    * engine running signal

commit 7009381
Author: Shane Smiskol <[email protected]>
Date:   Mon Aug 26 22:26:32 2024 -0700

    Revert "GM: implement allow throttle/brakes" (commaai#1187)

    Revert "GM: implement allow throttle/brakes (commaai#1185)"

    This reverts commit 8e20376.

commit 8e20376
Author: Shane Smiskol <[email protected]>
Date:   Mon Aug 26 21:47:18 2024 -0700

    GM: implement allow throttle/brakes (commaai#1185)

    implement allow gnb for GM

commit 46790af
Author: Adeeb Shihadeh <[email protected]>
Date:   Mon Aug 26 17:04:32 2024 -0700

    CI should use the defaults

commit 81fcc40
Author: Adeeb Shihadeh <[email protected]>
Date:   Mon Aug 26 17:02:38 2024 -0700

    openpilot isn't required for a car port PR (commaai#1183)

    * move test routes here

    * check for missing

    * no shebang

commit f6f0674
Author: Shane Smiskol <[email protected]>
Date:   Wed Aug 21 00:58:28 2024 -0700

    tests: some speed optimizations (commaai#1181)

    * mock sleep via monotonic

    * rm

    * no need to generate 8 buses

    * this doesn't actually make a big diff

    * lower loop

    * can reduce

    * here too

    * Revert "here too"

    This reverts commit 06b4cad.

    * Revert "can reduce"

    This reverts commit 55cae02.

commit 06c51c1
Author: Shane Smiskol <[email protected]>
Date:   Wed Aug 21 00:26:32 2024 -0700

    add test_car_interfaces.py (commaai#1178)

    * add test_car_interfaces.py

    * rm op stuff

    * fix

    * justsee

    * optimize get_fuzzy_car_interface_args a bit

    * Revert "optimize get_fuzzy_car_interface_args a bit"

    This reverts commit ba4d07f.

    * lower examples for now

    * sheesh

    * revert time

commit c02e83d
Author: Shane Smiskol <[email protected]>
Date:   Tue Aug 20 23:36:08 2024 -0700

    speedup CI (commaai#1179)

    * no setup python

    * replace with uv

    * test

    * test

    * test

    * test

    * test

    * test

    * test

    * test

    * test

    * test

    * test

    * test

    * slightly faster

    * this doesnt do anything without seen ecus

    * test

    * is durations slow?

    * not now

    * test in another pr

    * same to static

    * static

    * test

    * test

    * test

    * the action used to cache as well

    * test

commit 7d6f7cb
Author: Shane Smiskol <[email protected]>
Date:   Tue Aug 20 15:45:43 2024 -0700

    add banned apis

commit 3229aa2
Author: Shane Smiskol <[email protected]>
Date:   Tue Aug 20 15:41:30 2024 -0700

    add test_lateral_limits.py

commit fa1d7e7
Author: Shane Smiskol <[email protected]>
Date:   Mon Aug 19 17:59:36 2024 -0700

    fixup auto labeler (commaai#1089)

    * test

    * test

    * test

    * make sure to detect subdirectories like tests/

    * test

    * this should work

    * this should work

    * wtf is this

    * test

    * clean up

    * test

    * test

    * clean up

    * clean up

commit ff1c8b4
Author: Adeeb Shihadeh <[email protected]>
Date:   Mon Aug 19 15:18:50 2024 -0700

    add opendbc/can/ to LIBPATH

commit dc4de1a
Author: Shane Smiskol <[email protected]>
Date:   Mon Aug 19 14:34:32 2024 -0700

    rename car label

commit 8866aa7
Author: Adeeb Shihadeh <[email protected]>
Date:   Sun Aug 18 12:08:53 2024 -0700

    this was getting installed from panda

commit 5ed7a83
Author: Adeeb Shihadeh <[email protected]>
Date:   Sun Aug 18 11:32:03 2024 -0700

    more robust libdbc linking

commit d377af6
Author: Adeeb Shihadeh <[email protected]>
Date:   Sun Aug 18 11:01:09 2024 -0700

    pyyaml is unused

commit 74e042d
Author: Dean Lee <[email protected]>
Date:   Mon Aug 19 00:24:50 2024 +0800

    Remove Static Library from Build Script (commaai#1084)

    remove static lib

commit 357e43f
Author: Adeeb Shihadeh <[email protected]>
Date:   Sat Aug 17 12:42:16 2024 -0700

    simple README (commaai#1083)

    * init

    * Update README.md

commit 17e21fe
Author: Adeeb Shihadeh <[email protected]>
Date:   Sat Aug 17 12:16:30 2024 -0700

    CI: auto PR review (commaai#1082)

commit 3537404
Author: Adeeb Shihadeh <[email protected]>
Date:   Sat Aug 17 12:04:46 2024 -0700

    no docker (commaai#1081)

    * cleanup dependencies

    * all

    * install

    * less

    * try this

    * slimmm

    * skip docker

    * silence that one

    * disable for now

    * e

    * fix build

    * bring that back

    * not ready yet

commit 0a9727a
Author: Adeeb Shihadeh <[email protected]>
Date:   Sat Aug 17 10:28:49 2024 -0700

    one minute ci (commaai#1080)

    * speeeeed

    * version

    * disable

    * cppcheck

    * pip install

    * no update?

    * timeout

commit 9a53f08
Author: Shane Smiskol <[email protected]>
Date:   Sat Aug 17 00:38:33 2024 -0700

    move selfdrive/car to opendbc (commaai#1049)

    * move most of /car

    * rename selfdrive.car imports to opendbc.car

    * move some car tests

    move some car tests

    * fix car tests

    * fix actions?

    * add panda ignore to pytest

    * need these ignores from openpilot

    * fix tests for outside pip install (openpilot/local)

    forgot

commit 132b1a6
Author: Shane Smiskol <[email protected]>
Date:   Fri Aug 16 22:24:44 2024 -0700

    Bump pre-commit hooks, more coverage (similar to openpilot) (commaai#1078)

    * pytest defaults

    * move some stuff to pyproject

    * bump codespell

    * more like openpilot

commit f4d077b
Author: Shane Smiskol <[email protected]>
Date:   Wed Aug 7 17:30:23 2024 -0500

    CANPacker: msg type is tuple (commaai#1077)

    not mutable list

commit 1e9f853
Author: Adeeb Shihadeh <[email protected]>
Date:   Thu Aug 1 23:17:16 2024 -0700

    fix ruff warnings

commit 5c16f7e
Author: Shane Smiskol <[email protected]>
Date:   Thu Aug 1 16:40:51 2024 -0700

    define project (commaai#1073)

    * define project

    * spacing

    * cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jyoung8607 jyoung8607

Labels
can related to CAN tools, aka opendbc/can/
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JaCzekanski @jyoung8607