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

CANParser: remove dependence on cereal #934

Merged
merged 19 commits into from
Jul 31, 2024

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Sep 5, 2023

resolve #825

@deanlee deanlee force-pushed the dont_depend_on_cereal branch 8 times, most recently from d62f9cc to 97e3e8f Compare September 5, 2023 08:31
can/common.h Show resolved Hide resolved
can/common.h Outdated Show resolved Hide resolved
@deanlee
Copy link
Contributor Author

deanlee commented Dec 19, 2023

So we still need to support parsing dynamic capnp in parser.cc if the DYNAMIC_CAPNP is defined?

Also parser_pyx.pyx still depends on cereal, do we need to move it to openpiot? Or just leave it as it is?

@adeebshihadeh
Copy link
Contributor

The goal of this is to make opendbc a real independent submodule,, and that means getting rid of the cereal dependence entirely.

I'm imagining we have a very fast function that converts from cereal's CAN struct to opendbc's. Then, we can wrap it in Cython for openpilot and PlotJuggler can also use it.

@deanlee
Copy link
Contributor Author

deanlee commented Dec 19, 2023

got it

@adeebshihadeh
Copy link
Contributor

@deanlee want to finish this one up?

@deanlee
Copy link
Contributor Author

deanlee commented Mar 25, 2024

y, but it might take a little while.

@deanlee deanlee force-pushed the dont_depend_on_cereal branch 6 times, most recently from 9cfb4e3 to 7ef09a4 Compare March 25, 2024 09:03
@deanlee
Copy link
Contributor Author

deanlee commented Mar 25, 2024

@adeebshihadeh : Done. there is no loss in performance.
PlotJuggler needs to be modified after this to use the new api.

@deanlee deanlee marked this pull request as ready for review March 25, 2024 09:23
@deanlee deanlee force-pushed the dont_depend_on_cereal branch 2 times, most recently from d7bdfbd to ab36998 Compare March 26, 2024 05:31
@deanlee
Copy link
Contributor Author

deanlee commented Mar 26, 2024

@adeebshihadeh removed all dependencies on cereal. the CAN frame uses the same format as CanPacker: [address, 0, dat, source]

@deanlee deanlee marked this pull request as draft May 6, 2024 02:48
@deanlee
Copy link
Contributor Author

deanlee commented Jun 8, 2024

@sshane the extra conversion steps make it (commaai/openpilot#32009) more than twice as slow as the master.

this branch

6000 CAN packets, 10 runs
263.47 mean ms, 367.93 max ms, 258.64 min ms, 37.38 std ms
0.0506 mean ms / CAN packet

master

6000 CAN packets, 10 runs
125.61 mean ms, 130.85 max ms, 118.84 min ms, 2.78 std ms
0.0209 mean ms / CAN packet

@sshane
Copy link
Contributor

sshane commented Jun 10, 2024

Seeing similar results here. I think this is acceptable, but would be nice to speed it up in the future. Before #1039 I'm seeing

6000 CAN packets, 10 runs
295.08 mean ms, 303.76 max ms, 292.93 min ms, 2.99 std ms
0.0492 mean ms / CAN packet

on this PR I'm seeing

6000 CAN packets, 10 runs
214.52 mean ms, 216.95 max ms, 212.54 min ms, 1.13 std ms
0.0358 mean ms / CAN packet

and on current master I'm seeing

6000 CAN packets, 10 runs
97.72 mean ms, 98.73 max ms, 96.72 min ms, 0.70 std ms
0.0163 mean ms / CAN packet

@deanlee deanlee marked this pull request as ready for review June 11, 2024 03:17
@sshane
Copy link
Contributor

sshane commented Jun 12, 2024

@deanlee is this ready now? Once it's in we can really start on commaai/openpilot#32630!

@sshane
Copy link
Contributor

sshane commented Jun 12, 2024

BTW here's a list of things we need to remove from selfdrive/car, do you know if we have Python logging in opendbc yet? commaai/openpilot#32726

@deanlee
Copy link
Contributor Author

deanlee commented Jun 12, 2024

Y, it's ready.

BTW here's a list of things we need to remove from selfdrive/car, do you know if we have Python logging in opendbc yet? https://github.com/commaai/openpilot/pull/32726BTW here's a list of things we need to remove from selfdrive/car, do you know if we have Python logging in opendbc yet? https://github.com/commaai/openpilot/pull/32726BTW here's a list of things we need to remove from selfdrive/car, do you know if we have Python logging in opendbc yet? commaai/openpilot#32726

There shouldn't be any, but I'm not very sure right now

can/parser.cc Show resolved Hide resolved
same capitalization
@sshane
Copy link
Contributor

sshane commented Jul 31, 2024

LGTM, thanks for the PR!

@sshane sshane changed the title CANParser: remove dependence on cereal CANParser: remove dependence on cereal Jul 31, 2024
@sshane sshane merged commit 39a5345 into commaai:master Jul 31, 2024
3 checks passed
@deanlee deanlee deleted the dont_depend_on_cereal branch July 31, 2024 07:12
@sshane
Copy link
Contributor

sshane commented Jul 31, 2024

@deanlee want to update PlotJuggler?

@deanlee
Copy link
Contributor Author

deanlee commented Aug 1, 2024

I keep encountering the following error when trying to clone the PlotJuggler:

Cloning into 'PlotJuggler'...
remote: Enumerating objects: 18341, done.
remote: Counting objects: 100% (4032/4032), done.
remote: Compressing objects: 100% (428/428), done.
error: RPC failed; curl 56 GnuTLS recv error (-9): Error decoding the received TLS packet.
fatal: the remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed

I'll attempt it again later.

martinl pushed a commit to martinl/opendbc that referenced this pull request Aug 29, 2024
* move car interfaces to opendbc

* Revert "move car interfaces to opendbc"

This reverts commit 0589ea0.

* remove dependence on cereal

* parse can strings in a c++ helper function

* remove all cereal

* reserve

* use itervalues

* rebase master

* cleanup

* keep the same style

* revert unrelated changes

* truly no cereal!

* same capitalization

same capitalization

* parser: declutter nanos usage (commaai#1067)

thought there'd be more

* revert more

---------

Co-authored-by: Shane Smiskol <[email protected]>
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.

Remove dependence on cereal
3 participants