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

Refactor CAN Parser #1046

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Refactor CAN Parser #1046

wants to merge 7 commits into from

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented May 26, 2024

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:

  1. Removed query_latest() Function: The query_latest() function, which previously copied updated values to Cython and was invoked with empty CAN strings in Cython's __init__ method to initialize value dictionaries, has been removed. This led to unnecessary complexity and has now been streamlined.
  2. Exposed MessageState to Cython: MessageState is now directly accessible from Cython, allowing values to be read directly from the C++ MessageState object without the need for intermediate copying. This change streamlines data access, reduces overhead, and minimizes complexity and potential bugs.
  3. Enhanced Message Tracking: Updates are now tracked using and returning a set of updated addresses in MessageState::update, ensuring comprehensive capture of every update, independent of can frame's timestamp.

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:

6000 CAN packets, 10 runs
327.69 mean ms, 334.36 max ms, 321.73 min ms, 3.74 std ms
0.0546 mean ms / CAN packet

this branch:

6000 CAN packets, 10 runs
113.71 mean ms, 125.21 max ms, 109.85 min ms, 4.42 std ms
0.0189 mean ms / CAN packet

Additional Notes:

PlotJuggler will be broken after this PR because data is now read directly from MessageState instead of being copied using query_latest. However, it is quite easy to modify PlotJuggler to use the new API.

@deanlee deanlee force-pushed the refactor_parser branch 5 times, most recently from 8e3077a to 0e4d010 Compare May 27, 2024 05:24
@deanlee deanlee marked this pull request as ready for review May 27, 2024 05:39
@deanlee deanlee force-pushed the refactor_parser branch 3 times, most recently from a33fa27 to 7e3f62a Compare May 27, 2024 16:29
@sshane
Copy link
Contributor

sshane commented Jun 6, 2024

👀

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

@deanlee
Copy link
Contributor Author

deanlee commented Jun 6, 2024

👀

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.

@deanlee deanlee force-pushed the refactor_parser branch 3 times, most recently from ccc7f95 to 27fce80 Compare June 6, 2024 13:57
@deanlee deanlee marked this pull request as draft June 6, 2024 14:48
@deanlee deanlee force-pushed the refactor_parser branch 4 times, most recently from 9a8920a to 6d455a9 Compare June 6, 2024 20:09
@deanlee deanlee marked this pull request as ready for review June 6, 2024 20:23
@deanlee deanlee force-pushed the refactor_parser branch 4 times, most recently from 4b8a441 to de1e9ae Compare June 8, 2024 02:27
@sshane
Copy link
Contributor

sshane commented Jun 8, 2024

After this PR, we can simplify car/interface by using only one CanParser. With a single call to update_string for parsing all messages, there's no need to repeatedly feed identical can_strings to multiple instances of CanParser's update_string method for message filtering. This straightforward approach greatly enhances car/interface and parsing efficiency.

Nice, you mean we can pass in a dictionary with bus as the key, or similar?

@sshane

This comment was marked as resolved.

@deanlee
Copy link
Contributor Author

deanlee commented Jun 8, 2024

After this PR, we can simplify car/interface by using only one CanParser. With a single call to update_string for parsing all messages, there's no need to repeatedly feed identical can_strings to multiple instances of CanParser's update_string method for message filtering. This straightforward approach greatly enhances car/interface and parsing efficiency.

Nice, you mean we can pass in a dictionary with bus as the key, or similar?

y, or may something like {bus: [(msg1, freq1), (msg2, freq2)...], bus2: [...]}

@deanlee deanlee marked this pull request as draft July 31, 2024 05:59
@deanlee deanlee changed the title Refactor CAN Parser to boost performance Refactor CAN Parser Aug 9, 2024
@deanlee deanlee marked this pull request as ready for review August 9, 2024 08:22
@github-actions github-actions bot added the can related to CAN tools, aka opendbc/can/ label Aug 20, 2024
Copy link
Contributor

Thanks for contributing to opendbc! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • include a route or your device' dongle ID if relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can related to CAN tools, aka opendbc/can/ cleanup enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants