My Car tried to Kill me today (1973 Beetle +SDU Conversion), Potential Cruise Control Bug ?

[Pete9008]

Did I read that right, is there currently no throttle specific timeout, will any received CAN message keep it live?

Edit - just checked the code and you're right! I'm lost for words!

[Bigpie]

Any mapped can message received will keep it alive, yes.

My VCU messages wasn't getting though, intermittently, so canio (brake pedal) and throttle. So throttle stuck at last received value and brake not killing it. Not ideal. Should be fairly straightforward to fix though.

[Pete9008]

Not ideal! FFS that needs sorting! How long has this behaviour been known about!

[Bigpie]

Quite recently, last couple of weeks.

[Pete9008]

Right, I've taken a few deep breaths and calmed down a little. That's not how CAN should be done, every critical message must have a dedicated timeout, throttle and brake in particular.

If you need any help putting a fix together let me know, don't want to tread on toes but this needs sorting!

[Bigpie]

I'm not great with c/c++ but can muddle my way through.

Ideally I'd want unit test and integration tests too. Not a massive fan of just testing in production, hence trying to get the bench setup working.

[Pete9008]

If you need a hand let me know, even if it's help getting the bench setup working.

[johu]

Alright, lets start with the reasoning behind this alleged madness.
Originally the RX time was only set after a valid CAN mapped message was received. So not any random message floating around on the bus will trigger this. Since in most usecases the inverter receives messages only from a VCU this covers the most common error cases:
- electrical bus issues (like in bigpies case)
- The VCU node goes down
Of course if you were to set up the inverter to receive, say, current limits from a BMS directly and throttle commands from a VCU you could end up with the VCU node going down and the inverter not timing out. Also, since version 5.27.R SDO messages also update rxTime.

Not ideal, FFS, whatever you wanna call it - it should be improved.

I think the most foolproof way is to read and store the two pot values, then set them to some invalid value, e.g. -1. If they stick at -1 for too long you know they're not being updated anymore.
Advantage: no extra configuration or new data paths needed
Disadvantage: a plot of pot/pot2 will look a little ugly

Other easy alternative: per message timeout. Also doesn't need extra configuration.

I recall having my OI potMax parameter set to 3030. Which would equate to about 2.5V. Of course if the throttle was pressed hard enough to exceed this voltage - as soon as the voltage went above this, OI would shut down PWM and the motor would coast to a stop. When throttle was slowly released and voltage dropped below 2.5V, motor would instantly accelerate (to what OI thought was full throttle again).

Of course that is not great. Could also justify introducing a limp mode. Full shutdown on faults like this is probably more dangerous than the current behaviour in traffic.

Curious thing was that on random cases, it seemed like the throttle "stuck" and the motor would continue accelerating, instead of stopping after 2.5V. At the time I discounted it as stupid settings on my part --- ie --- potMax should have been 4095 since I was feeding the analog signal with 3.3V for max throttle. This testing was not done on the road (vehicle was up on blocks). But reading this thread got me wondering - could something possibly saturate the throttle input? Or the code managing the throttle (torque) demand calculation? And how scary would it be to have the vehicle acting on its own and not responding to your inputs! YIKES!

Not sure what exactly happened there. Values 200 digits above potmax are still accepted and mapped to 100%. Maybe that was it?

[rstevens81]

It scareed the poop out me this sort of error but does highlight that there always should be an analog override in our case brake input and ignition cutting hv (as a minimum)

[uhi22]

For the safety relevant CAN messages, there are better methods in the field than just timeouts. They cover also situations where the sender is just repeating the same message again and again, and also cases when an other sender is sending garbage on a wrong ID which matches the critical ID.
The concept is: The message contains a running counter (e.g. four bits), and an CRC over all payload bytes and the counter. The receiver ignores messages which does not fit into the expected counter range, and ignores messages with wrong CRC. The receiver knows the nominal cycle time of the message, and checks in the same cycle whether a new, correct message was received. It fills a queue of n elements with the information "ok" or "fail", and uses the message only, if at least k of n messages are fine. The cycle time and the parameters n and k are chosen to match the fault tolerance time. (This is just a very basic description. The details are more complicated.) I guess different manufacturers using different methods, but I also guess a running counter and an additional CRC should be the minimum common standard.

[Pete9008]

For the safety relevant CAN messages, there are better methods in the field than just timeouts. They cover also situations where the sender is just repeating the same message again and again, and also cases when an other sender is sending garbage on a wrong ID which matches the critical ID.
The concept is: The message contains a running counter (e.g. four bits), and an CRC over all payload bytes and the counter. The receiver ignores messages which does not fit into the expected counter range, and ignores messages with wrong CRC. The receiver knows the nominal cycle time of the message, and checks in the same cycle whether a new, correct message was received. It fills a queue of n elements with the information "ok" or "fail", and uses the message only, if at least k of n messages are fine. The cycle time and the parameters n and k are chosen to match the fault tolerance time. (This is just a very basic description. The details are more complicated.) I guess different manufacturers using different methods, but I also guess a running counter and an additional CRC should be the minimum common standard.

Agree, if you look at something like the Leaf protocol they do all this and for good reason.

[tom91]

My current logic on my products does the following:
1. check if the received values are plausible, even if the source is a analogue input.
2. timeouts on any connected devices that receive controls from this ECU, or are used to control anything in the ECU
3. Implausible states, like brakes overriding throttle or BMS requiring power reductions when throttle is applied
4. Counters or additional CRCs if implemented, last line of defence

My main concern would be when does one value become implausible how does it cascade. This is why a dual signal throttle is a hard requirement from my point of view plus an overriding throttle input. You could argue a dual input brake signal is also required.

[RetroZero]

And connecting the brake signal in parallel with a resistor to one of the accelerator pedal signals? It would then distort one signal, but create a fault each time you brake. Maybe a similar solution to a current sensor? But that messes with regen. Dual signal switch might be more plausable.

[johu]

Just found on the news: https://eu.usatoday.com/story/money/car ... 439645007/
At least it's not just the hobby level open source stuff

[EV_Builder]

Pistol coming:

System safety starts with hw design.... then comes implementation level....

An CANBus driven inverter should start with implementing hw layer errors. CANBus perhipial errors. And that for all used perph in the hw in the micro. Do we auto resend? Do we know that we are auto resending? etc.

The Round Robin should have Monitoring on the duration of the task and freq. of it. to implement that safe it should be timed by another timer. (Like internal watchdogs). This is what an RTOS brings to the table...

The hwwatchdog mentioned before in this thread will blow some inverters correct? Not good. IMHO its the biggest product issue to solve. Especially because the coils are controlled by the drive unit.
I recommend against it.

basic but important things:
- The usage of priorities should be though through; are they? What are the numbers and why; and on equal priority what gets priority? Can we detect priority choices to monitor it?

I suspect in both cases that because of something it went solo. And that feels like cruise but isn't. Non driven wheels should be part of cruise control. Its a big safeguard against starting the inverter in cruise mode. Frontwheel axle speed should be within 10% and from 10mph before cruise should control.

You wanted a trigger to log values to disk well each warning and error generated is a trigger. You can only trigger on what you know. So generate knowledge/data and you generate triggers.

Add rules for warnings. Choose a save path when detected and write a bunch of vars to the freeze frame flash page.
That page is readout and can be analysed.

The frame should be constructed and saved when the error is detected and when it's solved.

And please think a bit more self secure! Dare to add fatal (but secure errors). If 2 throttle inputs go whack and we follow the lowest ..it still can be one bit lower then error setpoint and that still can result in issues. So a broken throttle means 0 speed and a good throttle means it works. Or jump into limb mode but that means max 10mph.

- Forward is forward and reverse is reverse I keep saying it.
if we mix this up we do the simple things wrong; swap with a config;
- Disable the serial terminal during 'production';
- Disable the wifi after 10min during production (or better switch on via can, externally);
- a broke flash page crc means config needed. Sorry bud we are stranded... or double save parameters...

Brake input should be pulled to ground in hw. And should be +12Volt when no brakes are engaged. A bad connection is an error so monitor it's state each mS and uniform the program execution doing this instead of reading it in each 'if'. and have an implausible brake signal error. Hey now we can fail state the brake input!

- On any error disable regen.
- disable cruise and crawl;
- always limit PI controller ranges in % from start values;
(Anti windup usage at some point is an error?!?);
- limit current on any error or warning (and drop into neutral if engine isn't acting to it / needed);

- implement unit testing;
- simulators and stimulators are a great way to test behavior against expected behavior.

The best way to find errors is to make the detection crude and the results impactful.

I realize it became a pistol that wasn't my intention. Will put a warning on the top.

[EV_Builder]

Just found on the news: https://eu.usatoday.com/story/money/car ... 439645007/
At least it's not just the hobby level open source stuff

Well the brake solves here the problems..

"The vehicle may continue to accelerate until the brake is applied or may not slow down as expected if the gas pedal is released which could increase the risk of a crash."

[uhi22]

Found a public standard, which describes the end-to-end protection of CAN signals, much more detailled than I tried above.
https://www.autosar.org/fileadmin/stand ... ibrary.pdf
https://www.autosar.org/fileadmin/stand ... RS_E2E.pdf
But yes, this standard seems to be the collection of many different ideas, so it is flexible, large, difficult to read and difficult to find the point. In best case there is already an open source implementation of such an end-to-end protection, but I was not able to find one.

[muehlpower]

I think we have to be careful not to overdo it. CRC and counter make it more difficult to integrate the motor into a system. A timeout for CAN signals and the possibility to stop the PWM output with the RND switch would be enough for me. If everything goes wrong, the HV contactors are disconnected by the ignition switch, with the risk of destruction. That corresponds roughly to a combustion engine car from the 90s.

[johu]

Currently got friends over for visit, so not much capacity for long answers. Just want to say all input appreciated.
If more conditions are added it should be automatic, with no user interaction and it should be possible to disable them. Already now people can't get regen to work because of the required preconditions. When adding more conditions these have to be explained, perhaps over and over.
Maybe there could be a safety level parameter. If you go for max safety you need to be prepared to do more digging to satisfy all mechanisms.

[Pete9008]

I would suggest that as an absolute minimum:

• All safety critical parameters are excluded from being can mappable.
• New fixed messages (possibly with just the id being configurable) are added for each safety critical parameter that needs to be transferred, each using the approaches detailed in uhi's post above.
• The same messages are added to Zombie and other of the shelf VCUs so that the two will work together out of the box, without any configuration, in a safe/secure manner.
• Document the messages to allow the messages to be integrated into other VCUs.

In many ways this will make using an OI inverter board with standard VCUs a lot easier. It will raise the bar a little for using it with custom VCU's but tbh if someone is unable to add the messages in their own code then they probably shouldn't be writing safety critical code yet!

I would not provide any way to bypass or reduce the security.


Edit - revised to be less direct. I really don't have the energy to argue something that shouldn't even need to be discussed. Think I'm done here.

[EV_Builder]

I agree Pete for this moment it's the best quick solution.

Most important IMHO is respecting brake inputs.
And make a class which analyses input behaviour so its signal is uniform through the application and it's safe.

An error disables the drive and needs a reboot to clear.

[johu]

That sounds pretty straight forward and indeed doesn't impede usability.
I'd collect 4 values in a single can message with configurable ID and fixed mapping:

1. pot [0:11]
2. pot2 [12:23]
3. canio [24:29]
4. cruisespeed [32:47]

Then a running counter in [48:51] and some 8-bit checksum over the 1st 7 bytes in [56:63]

1st rough idea, haven't read above linked documents but it sort of reminds me of Nissans protocol.
All additional BMS current limits, regenpreset etc. remain freely mappable like now.
EDIT: might be worthwhile to mangle 7 bits of regenpreset in there as well.
EDIT2: might also be worthwhile to disallow the pot signals to saturate at 0 and 4095 but rather about 500 and 3500 so that the out-of-range check has something to detect.

[Aragorn]

Another simple addition would be to look at what the OEMs do when throttle and brake are requested simultaneously.

Almost every electronic throttle car I've driven has responded the same way: if brake is asserted while throttle pedal is >0, throttle is closed. This "panic" state is exited by either releasing the brake, or by fully releasing and reapplying the throttle pedal.

The VAG stuff I'm very familiar with also have two separate brake inputs fed directly to the engine control unit and from memory one is active high and one active low.

[muehlpower]

Most important IMHO is respecting brake inputs.

What do you mean by that? should that give priority to the brake over the gas pedal and make it impossible to accelerate and brake at the same time?

[EV_Builder]

Exactly; and do filtering and sanity checks on the brake input signal / hw. When I'm at it I will come up with more ideas and hw etc.