Added Tesla safety changes. #132

zax123 · 2018-08-14T01:16:57Z

Strictly the safety changes to panda code to add support for pre-autopilot Tesla Model S with the EPAS harness and giraffe designed by @kalud and @zax123 installed.

rbiasini · 2018-08-14T05:53:37Z

board/safety/safety_tesla.h

+int tesla_ignition_started = 0;
+
+// interp function that holds extreme values
+float tesla_interpolate(struct lookup_t xy, float x)


just moved a generic interpolation function to safety.h I recommend using that one.

zax123 · 2018-08-14T11:58:11Z

OK, I removed the function from safety_tesla.h and I'm using the generic interpolate function in safety.h now, thanks!

rbiasini · 2018-08-15T22:32:32Z

board/safety.h

@@ -49,6 +49,7 @@ int controls_allowed = 0;
 #include "safety/safety_gm.h"
 #include "safety/safety_ford.h"
 #include "safety/safety_cadillac.h"
+#include "safety/safety_tesla.h"


move this inside #ifdef PANDA: legacy boards and pedal don't support floats.

rbiasini · 2018-08-15T22:32:48Z

board/safety.h

@@ -100,6 +102,7 @@ const safety_hook_config safety_hook_registry[] = {
  {SAFETY_GM, &gm_hooks},
  {SAFETY_FORD, &ford_hooks},
  {SAFETY_CADILLAC, &cadillac_hooks},
+  {SAFETY_TESLA, &tesla_hooks},


move this inside #ifdef PANDA: legacy boards and pedal don't support floats.

… don't support floats used in Tesla safety code

zax123 · 2018-08-15T23:11:20Z

Done, good eye for finding that!

rbiasini · 2018-08-15T23:17:28Z

board/safety.h

@@ -49,6 +49,9 @@ int controls_allowed = 0;
 #include "safety/safety_gm.h"
 #include "safety/safety_ford.h"
 #include "safety/safety_cadillac.h"
+#ifdef PANDA
+#include "safety/safety_tesla.h"


move this after https://github.com/commaai/panda/pull/132/files#diff-7b552a9c774a81e90fcf2183ff134492R47

This is done and will be in my next commit.

rbiasini · 2018-08-15T23:29:49Z

board/safety/safety_tesla.h

+    int hour = (to_push->RDLR & 0x1F000000) >> 24;
+    int minute = (to_push->RDHR & 0x3F00) >> 8;
+    int second = (to_push->RDLR & 0x3F0000) >> 16;
+    current_car_time = (hour * 3600) + (minute * 60) + second;


using car time from CAN seems unnecessary complicated and potentially unreliable. I recommend re-using:

uint32_t ts = TIM2->CNT; uint32_t ts_elapsed = get_ts_elapsed(ts, tesla_ts_lever_last);

I got rid of this section of code. It was going to be used if we only used double-pulls to activate openpilot. We now are activating on a single pull. Double-pull may activate other features, but safety-wise, we only need to monitor a single pull because nothing changes from a safety perspective when you pull twice.

rbiasini · 2018-08-15T23:38:39Z

board/safety/safety_tesla.h

+// 2m/s are added to be less restrictive
+const struct lookup_t TESLA_LOOKUP_ANGLE_RATE_UP = {
+    {2., 7., 17.},
+    {5., .8, .25}};


above 17m/s, this is twice than what is in toyota_safety_ipas.h. Isn't it too much?

I'm sorry for my ignorance, I didn't copy this piece of code. It looks like the difference is the .25 in Tesla, which is .15 in Toyota. Is that the part that concerns you?

So we have a couple engineers and coders who tested and tweaked the values in this code and I believe that that value was necessary in order to take even some modest curves at a decent speed. I don't think it's excessive. There are still some curves that the model can't take which I believe it should. I can definitely change this to .15 for upstreaming purposes, but I have a feeling users will likely change it back because of the turns it would disengage on. Definitely your call, please let me know.

ok, let's keep these numerical details last. It comes down to testing the max steer injection case:
https://medium.com/@comma_ai/how-to-write-a-car-port-for-openpilot-7ce0785eda84

rbiasini · 2018-08-16T00:08:35Z

board/safety/safety_tesla.h

+    eac_status = ((to_push->RDHR >> 21)) & 0x7;
+    // For human steering override we must not disable controls when eac_status == 0
+    // Additional safety: we could only allow eac_status == 0 when we have human steerign allowed
+    if ((controls_allowed == 1) && (eac_status != 0) && (eac_status != 1) && (eac_status != 2))


If we don't want to disable on user steer override (let's think about it), then I would remove this check. Does not seem safety relevant.
of course openpilot will not allow to engage when eac_status==3 (for example), but this check here seems unnecessary

Are you saying you don't want the redundant check inside safety_tesla that ensures the the EPAS is not in EAC_FAULT(3) status?

yeah, it would be safety relevant only if we want to disengage on user steer override.
I assume that if EAC is faulted, then it won't respond to steer commands.

Yes, as soon as EAC faults, it disables steer commands and the user must take control. However, there is no feedback to openpilot indicating this, so I'm thinking it would be good for us to set controls_allowed = 0 because then feedback gets back to OP and it can throw an alert on the screen and/or play an alert sound. What do you think?

rbiasini · 2018-08-16T00:11:54Z

board/safety/safety_tesla.h

+  //get latest steering wheel angle
+  if (addr == 0x00E)
+  {
+    int angle_meas_now = (int)((((to_push->RDLR & 0x3F) << 8) + ((to_push->RDLR >> 8) & 0xFF)) * 0.1 - 819.2);


we should just use floats and define a new sample_t struct that contains floats

It looks like this code was copied from the Toyota IPAS safety code. Did you want to make this generic somehow and move it into safety.h?

exactly. safety_toyota_ipas.h is a good starting point, but hasn't reached the level of safety we want. safety_tesla.h will be the first "approved" angle control safety logic.

rbiasini · 2018-08-16T00:30:47Z

board/safety/safety_tesla.h

+      {
+
+        // add 1 to not false trigger the violation
+        int delta_angle_up = (int)(interpolate(TESLA_LOOKUP_ANGLE_RATE_UP, tesla_speed) * 25. + 1.);


I think all of these should be floats

Yeah, so, we could change these to floats, but then the toyota_ipas safety code is different. Should we make them the same?

yes, I would be happy to generalize those functions and use them in both places

If you would be so kind to do that, I would appreciate it, just because I didn't write that code myself and would likely introduce more bugs trying to fix it.

yes, once the new functions are defined, I'll take care of using them in safety_toyota_ipas

Oh, so you'd like the Tesla team to generalize them and put them in safety.h using floats, or were you going to do that? Just want to be clear...

tesla team shoul'd do that :)

Better enclose Tesla-relevant code in ifdef PANDA due to use of floats

rbiasini · 2018-08-16T02:00:26Z

board/safety/safety_tesla.h

+        int delta_angle_down = (int)(interpolate(TESLA_LOOKUP_ANGLE_RATE_DOWN, tesla_speed) * 25. + 1.);
+        int highest_desired_angle = tesla_desired_angle_last + (tesla_desired_angle_last > 0 ? delta_angle_up : delta_angle_down);
+        int lowest_desired_angle = tesla_desired_angle_last - (tesla_desired_angle_last > 0 ? delta_angle_down : delta_angle_up);
+        int TESLA_MAX_ANGLE = (int)(interpolate(TESLA_LOOKUP_MAX_ANGLE, tesla_speed) + 1.);


we also need a simple check that ensures the rate limits are observed at each step (I only see the real time check). See safety_gm.h, for example.

rbiasini · 2018-08-16T02:03:07Z

board/safety/safety_tesla.h

+    {
+      // We should not be able to STEER under these conditions
+      // Other sending is fine (to allow human override)
+      steer_allowed = 0;


can't this be replaced by controls_allowed?

rbiasini · 2018-08-16T02:05:54Z

board/safety/safety_tesla.h

+        controls_allowed = 0;
+        puts("Steering commads disallowed");
+      }
+    }


what is blocking messages to be sent when controls_allowed == 0? Seems like this function would return True.

zax123 · 2018-08-26T13:48:42Z

Hi @rbiasini, we've made all the requested changes as per our conversations above. Can you please check on the new code and let me know if anything else is missing?

Thanks!

rbiasini · 2018-08-30T07:05:17Z

@zax123 , thanks. Will review within this week

rbiasini · 2018-09-04T00:09:16Z

board/safety/safety_tesla.h

+    }
+
+    // update array of samples
+    update_fsample(&tesla_angle_meas, angle_meas_now);


tesla_angle_meas is unused

rbiasini · 2018-09-04T00:14:10Z

board/safety/safety_tesla.h

+    {
+      // add 1 to not false trigger the violation
+      float delta_angle_up = interpolate(TESLA_LOOKUP_ANGLE_RATE_UP, tesla_speed) * 25. + 1.;
+      float delta_angle_down = interpolate(TESLA_LOOKUP_ANGLE_RATE_DOWN, tesla_speed) * 25. + 1.;


why the 25 factor? here the check should be done each step, right?

rbiasini · 2018-09-04T00:41:23Z

@zax123 , there is a minor conflict and a bug in the angle commanded rate limiter check (see my comment). Other than that it looks good.
My only worry is that the RT measured steering angle check will false trigger. I don't have a good solution in mind for this, except that maybe you can do the RT rate limiter check on the commanded angle (but I realize that checking the measured angle is more meaningful...).

Anyway, Once the merge conflict + bug are fixed, we can merge this PR. After that, you guys should write a regression test (see panda/tests/safety/) and we'll add the Tesla to the list of community supported cars (assuming the official branch uses this safety code :)). Finally, bounty will be paid!
Good job Tesla guys!

Do interpolation check at every step (not every 25) Change tesla safety constant number to 8, not 7

zax123 · 2018-09-18T21:01:59Z

Hi @rbiasini ,

Thanks for keeping this alive. I've made the necessary changes. For some reason, there's still a merge conflict with the SAFETY_TESLA number. It no longer conflicts with the SAFETY_HYUNDAI number.

The other changes are done. I'd just like it if you could confirm this is all ok before we move on to making the unit test file.

Thank you!

Rob

zax123 · 2018-09-18T21:02:21Z

PS. Congratulations on your promotion!!

Added Tesla safety changes.

a8cc5d6

rbiasini reviewed Aug 14, 2018

View reviewed changes

Use generic interpolate function from safety.h

92a1837

rbiasini reviewed Aug 15, 2018

View reviewed changes

Moved Tesla safety include to within IFDEF PANDA as legacy neo boards…

05306c5

… don't support floats used in Tesla safety code

rbiasini reviewed Aug 15, 2018

View reviewed changes

rbiasini reviewed Aug 16, 2018

View reviewed changes

Remove double-pull code and time tracking from CAN message.

20462ab

Better enclose Tesla-relevant code in ifdef PANDA due to use of floats

rbiasini reviewed Aug 16, 2018

View reviewed changes

Changes as per requests by @rbiasini from conversations in commaai#132

337db0f

rbiasini reviewed Sep 4, 2018

View reviewed changes

Robert Cotran added 3 commits September 18, 2018 16:48

Removed unused variable

9846b14

Do interpolation check at every step (not every 25) Change tesla safety constant number to 8, not 7

Add the HYUNDAI safety number

5f1365d

Remove unused struct and function (fsample)

c77d876

Merge branch 'master' into master

a0898ef

pd0wm added 2 commits October 4, 2018 17:19

fix build

3ce3493

Add stub gmlan implementations in safety tests

b213974

pd0wm merged commit 6385551 into commaai:master Oct 4, 2018

pd0wm mentioned this pull request Oct 8, 2018

Pull request for safety only changes for pre-autopilot Tesla Model S commaai/openpilot#296

Closed

rbiasini mentioned this pull request Nov 2, 2018

Support for Classic Tesla Model S #121

Closed

Added Tesla safety changes. #132

Added Tesla safety changes. #132

Conversation

zax123 commented Aug 14, 2018

Choose a reason for hiding this comment

zax123 commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zax123 commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbiasini Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbiasini Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbiasini Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zax123 commented Aug 26, 2018

rbiasini commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbiasini commented Sep 4, 2018

zax123 commented Sep 18, 2018

zax123 commented Sep 18, 2018

rbiasini Aug 15, 2018 •

edited

Loading

rbiasini Aug 15, 2018 •

edited

Loading

rbiasini Aug 16, 2018 •

edited

Loading