Unable to activate OP while brake hold active

[jsegill]

Describe the bug

When brake hold is enabled and in effect on a Toyota Corolla HB 2019, OP will not engage.

Before 0.8.11, OP would engage while in a brake hold with a lead car. Not allowing OP to engage while at a stop, makes stop and go traffic disengagements more frustrating. One cannot simply re-enable OP after making a correction resulting in a stop of the car. Also trying to re-enable OP with brake hold active, results in the brake hold being released with no OP engagement.

Stock Toyota system allows for ACC engagement while brake hold is active. Stock Toyota ACC requires a second push of the resume button to resume ACC functions.

I believe this stems from PR commaai/openpilot#22810 to fix issue Issue commaai/openpilot#22791 . Anyway to prevent the integral build up, mentioned in Issue commaai/openpilot#22791 so OP may be enabled again when brake hold is active?

What hardware does this issue affect?

comma three, comma two

Which car does this affect?

Toyotas; Toyota Corolla HB 2019

Provide a route where the issue occurs

99725b56a15b5510|2021-12-01--15-16-19

openpilot version

0.8.11

Additional info

No response

[jsegill]

Update: Discovered OP will activate when manually pushing the brake at a complete stop- this mirrors what OP used to do with the brake hold feature. While physically pushing the brake at a complete stop, the resume with another press of the resume button illuminates on the Toyota dash.

Seems like this behavior should be flipped... Activate OP while brake hold is active (car holding brake), not while brake is manually pushed.

[jsegill]

May have found a way to fix this car bug with the generous help of @cydia2020
PR soon?

[sshane]

Looked at the history and I don't see any clear reason why it was prevented. We allow it if not openpilot longitudinal control, so perhaps it was a way to avoid faults with openpilot longitudinal since we didn't want to spend the time to verify that this combination of modes worked properly.

We also used to disallow enabling without a lead with brake pressed, so perhaps this can be revisited. Not a priority at the moment though.

[jsegill]

Thank you @sshane for researching the issue and the history of why this may be happening. It's been confusing to find the root cause. Much appreciation!

[sshane]

@adeebshihadeh this is an openpilot design decision, do we want to allow enabling with auto hold again? The stock systems usually allow this

[adeebshihadeh]

I don't see why not? You just have to press resume first?

[sshane]

@jsegill can you open a new PR similar to #29057? Let's ensure the press resume to go alert for cruiseState.standstill is also shown

[jsegill]

Definitely!

[jsegill]

@sshane Can we enable for all vehicles w/ brakeHold? Won't each cars PCM give back a message if it's not supported natively? I won't be able to work on it much until next week...

[sshane]

Yes, just remove the alert that has a noEntry and show the resumeRequired alert instead. Transferred back since all changes need to be made in openpilot

[jsegill]

Yes, just remove the alert that has a noEntry and show the resumeRequired alert instead. Transferred back since all changes need to be made in openpilot

Like this? #94e7c240c4dfa49aa70ab8a5ed4a59cf62316dd3 Commit 94e7c24
[] 94e7c24

[sshane]

Yes, exactly

[jsegill]

I'll test Sunday and submit a PR if all goes well.

[sshane]

You can open the PR now, we need to test on a few brands here to make sure it all works

[jsegill]

Just got it in.

[jsegill]

Last PR submitted won't work. Will need further work.