Skip to content

Toyota: Add SECOC long support#2061

Closed
chrispypatt wants to merge 23 commits intocommaai:masterfrom
chrispypatt:secoc-long
Closed

Toyota: Add SECOC long support#2061
chrispypatt wants to merge 23 commits intocommaai:masterfrom
chrispypatt:secoc-long

Conversation

@chrispypatt
Copy link
Copy Markdown

@chrispypatt chrispypatt commented Oct 18, 2024

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

Accompanies: commaai/opendbc#1385

@chrispypatt chrispypatt changed the title Toyota: Add SECOC Toyota: Add SECOC long support Oct 18, 2024
@jyoung8607
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Author

chrispypatt commented Nov 1, 2024

@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
Copy link
Copy Markdown
Collaborator

jyoung8607 commented Nov 2, 2024

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
Copy link
Copy Markdown
Author

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

@chrispypatt chrispypatt force-pushed the secoc-long branch 4 times, most recently from 9d53389 to eac8676 Compare November 5, 2024 04:03
chrispypatt and others added 2 commits December 2, 2024 22:31
Co-authored-by: PenitentTangent2401 <57408008+PenitentTangent2401@users.noreply.github.com>
@chrispypatt chrispypatt marked this pull request as ready for review December 11, 2024 02:21
@chrispypatt
Copy link
Copy Markdown
Author

@sunnyhaibin the tests are now failing after removing the 0x411 and 0x750 in test_diagnostics. That test mentions testers and uses the 0x750 message. Am I right to think that test is not applicable?

@chrispypatt
Copy link
Copy Markdown
Author

@jyoung8607 what else needs to be done here? I know you don't like the tests... can I either get some guidance on what would be preferred improvements for these tests or if we are fine backing out the common test changes I made to add an _accel_msg_2 and tackle that improvement in another PR?

What needs to be completed before this can be merged?

@sshane
Copy link
Copy Markdown
Contributor

sshane commented Feb 20, 2025

We've moved the car safety code into opendbc, please rebase and re-open your PR there!

@sshane sshane closed this Feb 20, 2025
@chrispypatt chrispypatt deleted the secoc-long branch February 20, 2025 04:24
@chrispypatt
Copy link
Copy Markdown
Author

chrispypatt commented Feb 20, 2025

Thanks for the heads up @sshane, I moved the safety code to my existing opendbc PR commaai/opendbc#1385

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.

5 participants