commaai
Toyota: Add SECOC longitudinal control
DBC:
- Add counter for secoc acc messages like the other secoc messages
- Add new acc message creation for second ACCEL_CMD
- Remove SECOC
openpilotLongitudinalControldisable
Safety:
Creates a SECOC variant of common long messages. This is basically a copy of the TOYOTA_COMMON_LONG_TX_MSGS but:
swapped TOYOTA_COMMON_TX_MSGS for TOYOTA_COMMON_SECOC_TX_MSGS added a the new secoc acc command address 0x183
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
Confirmed that the RAV4 Prime does camera ACC like the other TSS2 camera ACC. Radar tracks are able to be parsed with toyota_tss2_adas:
- https://streamable.com/1278ih
- https://streamable.com/wq3icy
No need to disable radar to achieve openpilot longitudinal with this car.
41ba5b181f29435d/0000007b--edfe81ecc8
Is that something I can control? If so do you have advice on where to start? I just learned tsk radar worked out of the box when sunny dug into it. So not sure what I'd be looking for.
Is that an issue that would block getting this merged?
@sshane have you had a chance to look at my questions above?
Also, how can we get this merged? It's been open for several months and there are a lot of TSK users who have been running this on various forks. Is there something needed to move this forward?
@sshane I moved the safety code from https://github.com/commaai/panda/pull/2061 here as requested. Mutation tests are timing out since I synced with master - before I applied my changes from that other PR. Is this a known issue?
@sshane Just synced with master and tests are now passing. Do you have any feedback for this PR?
@jyoung8607 can you re-review?
Anything here that is blocking us from getting it merged?
@jyoung8607 @sshane I replaced the route with one with logs and just resolved some merge conflicts. Anything else you guys want changed or are things looking good?
@jyoung8607 @sshane, I just synced with master again fixing merge conflicts. Please let me know if there is any suggestions/blockers on getting this merged.
Radar points do look unusually sparse, especially with cars off to the side
@sshane I am looking into this, I think I maybe see what you are saying. I compared my route 41ba5b181f29435d/00000001--e3ae76382f with the SECOC Sienna right below it in routes.py 8bfb000e03b2a257/00000004--f9eee5f52e. Mine was not as active/full of data points. Is that what you were seeing?
@sshane I grabbed screenshots from the route I added for this PR and other recent routes. Are the points I'm seeing not expected? I compared with a few other secoc and non-secoc vehicles and didn't see any big differences. My routes seem to be showing all the cars and some objects in frame.
@sshane @adeebshihadeh @jyoung8607 FYI I recently updated this PR with safety changes and test fixes after noticing the Yaris secoc route was failing with this code.
The route is now passing after removing assumptions that all secoc vehicles support long.
Are there any other fixes or suggestions any of you have to get this merged?
Thank you for your work on this @chrispypatt ! Is this the only PR outstanding before we can mainline Toyota Long control to the official nightly-dev channel?
@GaganBhat yep once this gets merged OP dev branches will support Toyota secoc long
Awesome! @sshane @adeebshihadeh @jyoung8607 Are there any other changes needed here (apart from a rebase / conflict resolution) prior to being able to merge?
This Toyota long support from @chrispypatt has been in-use for many model years now on Sunnypilot with great results and feedback from the community. Would love to get it mainlined.
@sshane @jyoung8607 I just resolved some merge conflicts w/ master and have updated my branch. Any feedback or discussion around my previous questions would be appreciated.
Good news! Sounds like it's on Jason's radar https://discord.com/channels/469524606043160576/1184981830714261604/1400183941515972830 so for now we need to be patient 🙂
Rebased.
I think the car port and safety code are basically okay, but we have to figure out a better way to run the accel tests. We can't hack up the common tests like that. Can you try doing it with inheritance instead?
Something like this in the base classes:
def _accel_msg_835(self, accel, cancel_req=0):
values = {"ACCEL_CMD": accel, "CANCEL_REQ": cancel_req}
return self.packer.make_can_msg_panda("ACC_CONTROL", 0, values)
def _accel_msg_387(self, accel):
values = {"ACCEL_CMD": accel}
return self.packer.make_can_msg_panda("ACC_CONTROL_2", 0, values)
Then in the non-SecOC cases:
def _accel_msg(self, accel):
return self._accel_msg_835(accel)
And in the SecOC cases:
def _accel_msg(self, accel):
return self._accel_msg_387(accel)
def test_835_actuation_blocked(self, stock_longitudinal: bool = True):
"""
For SecOC cars, longitudinal acceleration must be sent in ACC_CONTROL_2, but all other ACC
data remains in ACC_CONTROL. Verify no actuation is sent via ACC_CONTROL.
"""
(the same as test_acc_cancel but should_tx = accel == self.INACTIVE_ACCEL)
Also tweak test_acc_cancel to always call _accel_msg_835 instead of _accel_msg.
I started to do this myself but hadn't settled on how to juggle the existing classes, figured you could try.
Sounds good I can take a stab at it. My time is a bit more limited ATM.
should_tx = accel == self.INACTIVE_ACCEL
I couldn't get this exactly to work and had to follow how test_acc_cancel is implemented using should_tx = np.isclose(accel, self.INACTIVE_ACCEL, atol=0.0001) due to FP errors in comparing small floats.
But otherwise, @jyoung8607 I really like your suggestion. It's much cleaner than my previous implementation.
LGTM pending comma review due to safety code changes.
Since it's behind ALLOW_DEBUG, you're good to merge safety changes.
Thank you for working on this @chrispypatt !! Awesome to see this merged.