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

Force steering cut through apply_toyota_steer_torque_limits #658

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@arne182
Copy link
Contributor

commented May 22, 2019

If there is a violation do not reduce steering down to 0 in one frame.

This causes a steering error to occur.

If the STEER_ERROR_MAX occurs the steering should slowly reduce before the panda registers a violation and also immediately reduces steering torque to 0.

I suggest increasing the panda STEER_ERROR_MAX to 375. This PR will be done in the Panda Repo

Force steering cut through apply_toyota_steer_torque_limits
If there is a violation do not reduce steering down to 0 in one frame. 

This causes a steering error to occur. 

If the STEER_ERROR_MAX occurs the steering should slowly reduce before the panda registers a violation and also immediately reduces steering torque to 0.

I suggest increasing the panda STEER_ERROR_MAX to 375. This PR will be done in the Panda Repo
@pd0wm

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Thanks!

Why did you lower STEER_ERROR_MAX to 349?

@arne182

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Just so that this triggers before the Panda violation does. It does not really matter as long as this value is lower. If you accept putting the panda value up to 375 we can leave this one at 350

@rbiasini

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Panda code and carcontroller code should match. No reason to have panda safety code less stringent.


apply_steer = apply_toyota_steer_torque_limits(apply_steer, self.last_steer, CS.steer_torque_motor, SteerLimitParams)
if apply_steer == 0 and self.last_steer == 0:
apply_steer_req = 0

This comment has been minimized.

Copy link
@rbiasini

rbiasini May 24, 2019

Contributor

you don't want panda to block messages after a user cancellation.
I'm fine with this change but the if not enable, then apply_torque = 0 should stay after apply_toyota_steer_torque_limits.

Makes sense?

@arne182

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

I understand you. Then you should correct the panda code violation to return steering torque and not a Boolean that cuts the steering torque immediately to 0 causing steering faults. This is easy to reproduce. Have steering wheel at about 450°. Engage openpilot and steer into the endstop. This causes the torque mismatch violation and the steering torque if reduced from 1500 to 0 in one frame will cause the steering to be unavailable.

@pd0wm

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

If controls allowed goes to false in the panda, you want to block steering messages immediately. It's not an issue if the EPS temporarily goes into a fault state after disengagement.

@pd0wm

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Can you split the enable check and the fault check, and apply the enable check after the rate limit?

If openpilot is not enabled we do not want to send any torque.

@rbiasini

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@pd0wm @arne182 , did you guys synch'd up on this?
@pd0wm and I spent quite some time trying to replicate unrecoverable faults caused by immediately dropping torque to zero but found no evidence that this would fix any issue. Must be something specific with your car model (we were on Prius)

@arne182

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@ShaneSmiskol has some on the Corolla and I on the rav4h. Will do some tests @pd0wm suggested so that you can see it.

@geohot

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@rbiasini What's the verdict on this. Can you close it or merge?

@geohot geohot added the bug label Jun 7, 2019

@arne182

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Tests not yet completed. Error in panda code was found making it more conservative. Waiting on @pd0wm s answer on Discord to how to perform tests correctly

@pd0wm

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@arne182 how did your tests go? Did you find any permanent faults that would not occur after making this change?

To summarize for other users reading this thread:

  • Currently when an EPS fault code appears we immediately cut the torque. Sending 0 torque is necessary to have the faults clear.
  • This technically violates the rate limit for winding down torque. If you would do this during normal operation, immediately setting torque to 0 could trigger a temporary EPS fault.
  • The current panda safety code does not block any messages when doing this.
  • From our testing we did not see any difference in how fast the fault code would go away when we apply the rate limit vs not applying it
  • Some fault codes seem to be permament, applying the rate limit also didn't seem to change this.
@arne182

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Will be testing this week. Results should be there by the weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.