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

Toyota: Add SECOC longitudinal control #1385

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

Conversation

chrispypatt
Copy link
Contributor

@chrispypatt chrispypatt commented Oct 18, 2024

  • Add counter for secoc acc messages like the other secoc messages
  • Add new acc message creation for second ACCEL_CMD
  • Remove SECOC openpilotLongitudinalControl disable

This will be accompanied by this related panda safety PR: commaai/panda#2061

@chrispypatt chrispypatt force-pushed the secoc-long branch 2 times, most recently from 745b551 to 848f074 Compare October 18, 2024 17:53
Copy link
Collaborator

@jyoung8607 jyoung8607 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term I'd like to clean up the SecOC message wrapping convention, but that's a separate refactor that we don't need to get into for this PR.

For development purposes we've gotten away with setting the accel value in both ACC control messages, but before we merge we need to match stock actuation behavior: always set zero/inactive acceleration in ACC_CONTROL, and only send the true accel in ACC_CONTROL_2. There are checks in commaai/panda#2072 to ensure this correct behavior.

Aside from that, LGTM.

@nelsonjchen
Copy link
Contributor

Just passing a Discord Link here to some of the chatter on the state of the PR:

Nov 21: "Well the mutation tests are failing so something seems wrong functionally. But I would also say my attempt at supporting two acc commands in the tests is a bit gross"

https://discord.com/channels/469524606043160576/905950538816978974/1309380353261174847

@chrispypatt chrispypatt marked this pull request as ready for review December 11, 2024 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car related to opendbc/car/ toyota
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants