search

commaai / panda

code powering the comma.ai panda
MIT License
1.54k stars 783 forks source link

Toyota: Add SECOC long support #2061

Open chrispypatt opened 1 month ago

chrispypatt commented 1 month ago

Creates a SECOC variant of common long messages. This is basically a copy of the TOYOTA_COMMON_LONG_TX_MSGS but:

  1. swapped TOYOTA_COMMON_TX_MSGS for TOYOTA_COMMON_SECOC_TX_MSGS
  2. added a the new secoc acc command address 0x183

This is still a WIP.

Accompanies: https://github.com/commaai/opendbc/pull/1385

jyoung8607 commented 3 weeks ago

@chrispypatt I can't directly push to your branch, so I opened another one at #2072.

I think that PR should work for our purposes, but it requires a tweak to opendbc (I'll comment over there) and some thought put into a cleaner common test. I'll add another comment about why shortly.

If you are comfortable with tackling the TODOs with a bit of guidance, please feel free to lift the contents of that PR into yours and run with it. Otherwise I'll get back to it when I can, but it may be a little while.

chrispypatt commented 3 weeks ago

@chrispypatt I can't directly push to your branch, so I opened another one at #2072.

I think that PR should work for our purposes, but it requires a tweak to opendbc (I'll comment over there) and some thought put into a cleaner common test. I'll add another comment about why shortly.

If you are comfortable with tackling the TODOs with a bit of guidance, please feel free to lift the contents of that PR into yours and run with it. Otherwise I'll get back to it when I can, but it may be a little while.

Thanks @jyoung8607 i will try tackling the TODOs you mentioned. Is there just a permission issue that doesn’t allow you to push to my branch? I’m happy giving you permissions if needed.

jyoung8607 commented 3 weeks ago

Thanks @jyoung8607 i will try tacking the TODOs you mentioned. Is there just a permission issue that doesn’t allow you to push to my branch? I’m happy giving you permissions if needed.

Nothing on your end. For understandable reasons, comma doesn't allow external contributors to commit directly to Panda safety code, so the usual GitHub mechanism for pushing to PR branches doesn't work between us.

I added one more cleanup to my PR which you can cherry pick, and I decided to leave the radar diagnostic thing alone for now. The only remaining TODO is cleaning up the test if we can. It's functional, but it would be nice if we can cleanly refactor the common tests to allow for testing two different accel messages with two different sets of allow conditions. VW need this as well as Toyota.

chrispypatt commented 2 weeks ago

Sounds good. I will take a look at refactoring that common test