-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Refactor CAN Parser #1046
Refactor CAN Parser #1046
Conversation
8e3077a
to
0e4d010
Compare
a33fa27
to
7e3f62a
Compare
👀 batman@workstation-shane:~/openpilot/selfdrive/debug$ ./check_can_parser_performance.py
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10/10 [00:01<00:00, 7.92it/s]
6000 CAN packets, 10 runs
99.36 mean ms, 99.86 max ms, 98.82 min ms, 0.33 std ms
0.0166 mean ms / CAN packet
batman@workstation-shane:~/openpilot/selfdrive/debug$ ./check_can_parser_performance.py
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10/10 [00:00<00:00, 20.73it/s]
6000 CAN packets, 10 runs
23.19 mean ms, 23.77 max ms, 22.84 min ms, 0.25 std ms
0.0039 mean ms / CAN packet |
Yes, this test should be accurate. The test results from my master version are outdated. |
ccc7f95
to
27fce80
Compare
9a8920a
to
6d455a9
Compare
4b8a441
to
de1e9ae
Compare
Nice, you mean we can pass in a dictionary with bus as the key, or similar? |
This comment was marked as resolved.
This comment was marked as resolved.
y, or may something like |
c49fae1
to
4a31d80
Compare
4a31d80
to
ff068a1
Compare
ff068a1
to
b67430f
Compare
Thanks for contributing to opendbc! In order for us to review your PR as quickly as possible, check the following:
|
This PR is too complicated. Closed. Some functions have been moved to PR #1382. |
This PR refactors the existing code by shifting most of the logic to C++, significantly simplifying the update process, improving performance, and resolving issues related to the mixed update logic that was previously split between Cython and C++. The parser_pyx.pyx now functions as a streamlined interface wrapper around the C++ functions.
Key Changes:
__init__
method to initialize value dictionaries, has been removed. This led to unnecessary complexity and has now been streamlined.By centralizing most of the logic in C++, this PR simplifies the Update Logic, Reduces the complexity of the code by removing redundant operations and unnecessary data copying.The new implementation is more efficient, as demonstrated by performance comparisons.
Issues Resolved:
This PR resolves multiple issues, and the test cases have been updated to catch them:
Performance Comparison:
master:
this branch:
Additional Notes:
PlotJuggler
will be broken after this PR because data is now read directly fromMessageState
instead of being copied usingquery_latest
. However, it is quite easy to modifyPlotJuggler
to use the new API.