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

[muehlpower]

I feared that. For me, braking and accelerating at the same time is a normal driving situation. It should be parameterizable, then everyone has the choice.

[celeron55]

Here's an additional way of thinking about this, and documenting this:

You can write down imagined or real world scenarios that hypotethically wouldn't be, or were proven to not be properly handled.

Then you, specifically for each, write down which mechanisms/failsafes (hardware, software, user interface) are capable of dealing with the situation and what preconditions are required for them to work.

Then you see how many and what types of failsafes there are for each scenario, and can review whether you're happy with what there is. If you're not happy, it will now be easier to imagine new ones that fill in the blanks. You can focus the effort.

Some examples of scenarios would be:

Potnom values do not correspond to pedal hardware for unknown reason
* The brake pedal will override throttle to 0, but only if it's wired up, configured, the wires are intact and the switch is working
* If the values are beyond the acceptable range, throttle could be forced to 0?
* If the values are coming through CAN, there could be carefully designed checksums, timeouts and whatnot to make this as unlikely as possible
* Shifting to neutral stops torque being produced

Cruise control does not turn off using the assigned main input
* Brake pedal like before
* The input getting unassigned should be made as unlikely as possible
* CAN stuff, again
* Shifting gears could/will turn it off

The inverter loses control of the motor in such a way that unintentional torque is created
* Shifting to neutral stops PWM? Of course only if you actually manage it, which depends on input assignment, CAN stuff, wiring and so on.

These are just examples, but looking at these it would seem focus should be put on configuration integrity checks, wiring and switch integrity checks and CAN integrity checks. Extending it further with more real world data and better imagined scenarios would give better and more accurate results.

Further down the line these can be used for the basis of writing tests and simulations.

One way to think about this is that all this is just obvious. But is it?

[EV_Builder]

I feared that. For me, braking and accelerating at the same time is a normal driving situation. It should be parameterizable, then everyone has the choice.

Can you explain that? Street use?

I think the product should follow and support the OEM logic.

Anyway since we didn't seem to detect a brake signal it's for me clear that there is where the work lies.

[muehlpower]

I use both pedals in drag racing to warm up the tires, for drifting and when parking on curbs to better control performance and save time switching from one pedal to the other. In addition, when driving on the street, I want to use the second analog input and the brake light signal to variably control the recuperation. Of course, not everyone has this requirement and current OEMs don't offer these options like the old ones, but I don't see a problem in giving the user the choice.

[johu]

Can I just stress that the brake pedal has been overriding throttle from day one.
I see some assumptions here like no safety measures have been put in place at all. That is super demotivating.

[EV_Builder]

I use both pedals in drag racing to warm up the tires, for drifting and when parking on curbs to better control performance and save time switching from one pedal to the other. In addition, when driving on the street, I want to use the second analog input and the brake light signal to variably control the recuperation. Of course, not everyone has this requirement and current OEMs don't offer these options like the old ones, but I don't see a problem in giving the user the choice.

Yeah so your use-case is like special.

@johu; No I didn't assume it was currently in or out but I see it as one of the sanity checks possible.

@muehlpower
I'm against being able to configure safety.
Better is build/make a non safe version your self.
Which should literally be a couple of //

[johu]

I should be able to start coding tomorrow.
Now with the CAN frame format settled, what is the best way to track the cycle counter? Say the cycle counter were 3 bits then an array with 8 ints could contain 8 timeout values. If one of them reaches 0 that is an error condition. This makes sure a stuck frame doesn't command any throttle. Remains the discussion of false positives. What if 7 out of 8 cycle counters are good and one went missing?
Suggestion
- one or two missing frame counter triggers a message but no action
- three missing counters trigger configurable throttle limit
- four missing counters (50%) trigger 0 throttle
Does that sufficiently protect against false positives? No throttle in traffic is a safety issue by itself so shouldn't be taken lightly.
I have also seen on the VW CAN bus that counters are repeated in different parts of the frame. Another sanity check that everything belong together, e.g. loword and hiword.

Checksum: for ease of implementation would go for one byte of the integrated crc32 hardware generator. Alternatives? Should protect against stray frames commanding random inputs.

Of course the above still protects against regular timeouts caused by dead VCU or broken CAN bus.

Closing sentence: while this is all very good we still haven't found out why cruise control failed to disengage as described in the entry post. This was a no-CAN car. Please keep reviewing the code and find conditions where torque would be generated despite the brake pedal being pressed.

[Bigpie]

All sounds like positive changes.

From messing with VW battery messages, VW only check that the counter is different to the last, it doesn't care if you just flip between 2 values. They do appear to have 2 counters running, but at least within the battery bus, the second appears of little use.

I think it would be worth coming up with a way of automating some tests, be it unit and/or functional as a longer term goal for the project.

[muehlpower]

I would set the values ​​to a safe value, e.g. accelerator pedal = 0, if the same counter reading occurs twice in a row or an incorrect CRC or a message does not come after 3 to 5 times the cycle time. e.g. 6ms-10ms at 2ms cycle time

I suggest 8 bits for the CRC. Here is a page for calculating CRC and program examples.
http://www.sunshine2k.de/coding/javascr ... rc_js.html

usually CRC8 is on byte 0 (0/8) and the counter the low nible of byte 1 (8/4) the CRC is then formed over bytes 1 to 7.

Code: Select all

public static byte Compute_CRC8_Simple_OneByte_ShiftReg(byte byteVal)
{
    const byte generator = 0x1D;
    byte crc = 0; /* init crc register with 0 */
    /* append 8 zero bits to the input byte */
    byte[] inputstream = new byte[] { byteVal, 0x00 };
    /* handle each bit of input stream by iterating over each bit of each input byte */
    foreach (byte b in inputstream)
    {
        for (int i = 7; i >= 0; i--)
        {
            /* check if MSB is set */
            if ((crc & 0x80) != 0)
            {   /* MSB set, shift it out of the register */
                crc = (byte)(crc << 1);
                /* shift in next bit of input stream:
                 * If it's 1, set LSB of crc to 1.
                 * If it's 0, set LSB of crc to 0. */
                crc = ((byte)(b & (1 << i)) != 0) ? (byte)(crc | 0x01) : (byte)(crc & 0xFE);
                /* Perform the 'division' by XORing the crc register with the generator polynomial */
                crc = (byte)(crc ^ generator);
            }
            else
            {   /* MSB not set, shift it out and shift in next bit of input stream. Same as above, just no division */
                crc = (byte)(crc << 1);
                crc = ((byte)(b & (1 << i)) != 0) ? (byte)(crc | 0x01) : (byte)(crc & 0xFE);
            }
        }
    }
    return crc;
}

[johu]

I added the specification to the wiki: https://openinverter.org/wiki/CAN_communication

Arranged in a way for data to start mostly at byte boundaries for easier use.

Went for 2 2-bit counters now

Thanks for the CRC8 code but I think I'll stick with the hardware crc generator as it can be used both by ZombieVerter and the Inverter
It is available as C-Code here: https://github.com/jsphuebner/esp32-web ... n.cpp#L191

[johu]

Everyone able to compile can inspect and test this now: https://github.com/jsphuebner/stm32-sin ... safer_can2

[uhi22]

For me this looks like a big step into the right direction. Of course, it is always room to make is even more safe, but I would consider this solution as sufficient to cover most of the potential faults. And it is simple enough to avoid running into a lot of special cases which have the risk of design faults. (I only inspected the differences, no tests planned from my side. It really would be great to have some automated test cases which would verify the behavior.)

[Bigpie]

Thanks for the effort @johu I'll get it tested asap

[muehlpower]

I have looked at the code. Where is the place with the CRC left out at "data" to generate the CRC. If the CRC is made over the whole message including CRC, I would have to know the CRC before calculating the CRC. Have I overlooked something?

[johu]

I have looked at the code. Where is the place with the CRC left out at "data" to generate the CRC. If the CRC is made over the whole message including CRC, I would have to know the CRC before calculating the CRC. Have I overlooked something?

Here: https://github.com/jsphuebner/stm32-sin ... ol.cpp#L83

Here's the python code I used for testing

Code: Select all

import can
from optparse import OptionParser
from time import sleep

def calcStmCrc(data, idx, len):
    cnt = 0
    crc = 0xffffffff

    while cnt < len:
        word = data[idx] | (data[idx+1] << 8) | (data[idx+2] << 16) | (data[idx+3] << 24)
        cnt = cnt + 4
        idx = idx + 4

        crc = crc ^ word

        for i in range(0, 32):
            if crc & 0x80000000:
                # Polynomial used in STM32
                crc = ((crc << 1) ^ 0x04C11DB7) & 0xffffffff
            else:
                crc = (crc << 1) & 0xffffffff
    return crc

PAGE_SIZE_BYTES = 1024


parser = OptionParser()
parser.add_option("-d", "--device", dest="device",
                  help="serial interface")

(options, args) = parser.parse_args()

if not options.device:   # if device is not given
    parser.error('Device not given')
    exit()

bus=can.interface.Bus(bustype='socketcan', channel=options.device, bitrate=500000)

pot = 500
pot2 = 250
incr = 10
canio = 8
ctr = 0
cruisespeed = 0
regenpreset = 100

while True:
    bytes = [ pot & 0xFF, (pot >> 8) | ((pot2 & 0xF) << 4), pot2 >> 4, canio | (ctr << 6), cruisespeed & 0xFF, (cruisespeed >> 8) | (ctr << 6), regenpreset, 0]
    crc = calcStmCrc(bytes, 0, 8) & 0xFF
    bytes[7] = crc
    msg = can.Message(arbitration_id=0x3F, is_extended_id=False, data=bytes)
    bus.send(msg)
    print(msg)
    sleep(0.1)
    
    if pot > 3500 or pot < 500:
        incr = -incr

    pot = pot + 2 * incr
    pot2 = pot2 + incr
        
    ctr = (ctr + 1) % 4
    #cruisespeed = (cruisespeed + 10) % 8000
    regenpreset = (regenpreset + 1) % 100

[MattsAwesomeStuff]

Of course, it is always room to make is even more safe

Not to go off into the weeds, but, this is true and will never stop being true. If we sent Johannes 50 engineers tasked with improving safety, they would never run out of things to improve. It's kinda like pulling a vacuum. At 1,000,000 units sold, and the magnitude of what happens when something goes wrong, is why OEM safety standards are so high, and so impossibly high compared to an open source solution that's mostly one man plus helpers. So, yeah, I agree, there's a level of reasonableness for safety. This is already a fringe case (has happened twice ever we know of), but, sure, with many eyeballs on it, a sharper safety-oriented overhaul seems like a good use of time. Catch that low-hanging what-if fruit. Again, like pulling a vacuum, there's not many gas particles left. It seems like as a result of this discussion, sniping out a few more possible fringe cases, the code is going to be 2 orders of magnitude more safety robust, maybe 3? That seems well into the "this community isn't large enough to ever discover an incident in several lifetimes" kind of territory.

[Romale]

today I conducted an interesting experiment with the 5.27 foc firmware. I submitted a cruise +12 to the entrance and kept it there until the motor started rotating. The behavior of the controller was very interesting! firstly, as soon as I pressed pot1, the motor started and kept certain revolutions by itself. pressing the brake pedal INCREASED the speed of the motor! releasing the gas pedal increased the rotation speed, and pressing the pedal on the contrary lowered the engine speed. even when I chose to enable the cruise on the can bus in the parameters, the behavior was still like in the video, provided there was a high din cruise input.

[johu]

Can you add the parameters to your post?
cruisespeed always is at -1

[Romale]

unfortunately I forgot to save them. I'll do it tomorrow

[johu]

Ok, try to provide enough data so I can reproduce it

[Romale]

ima+leaf+cruise.json

(3.01 KiB) Downloaded 65 times

I remind you, the purpose of this experience was to check if the cruise button initially stuck in the pressed state at the topikstarter?? that's why I closed my button and performed all the tests, including the controller with the cruise button pressed in advance.

[Romale]

as for the new changes for the purpose of cruise safety, my personal opinion is that they are redundant. as my experience has shown, it is enough that pressing the brake or turning on the neutral cancels the cruise even if it is pressed with a lock, and it is restarted only after the cruise button is turned off and pressed again.

[johu]

ima+leaf+cruise.json


I remind you, the purpose of this experience was to check if the cruise button initially stuck in the pressed state at the topikstarter?? that's why I closed my button and performed all the tests, including the controller with the cruise button pressed in advance.

I'll try to summarize what I see
- You have field weakening turned off
- syncofs=0 is about correct for Leaf motor, provided it is hooked up correctly
- Maximum speed is limited to 1500 rpm
- potmode is set to SingleRegen
- 100A offthrotregen current is programmed, attenuated by pot2 to about 60%, so 60A
- 300A brake regen current is programmed, again attenuated by pot2 to say 200A
- cruisemode is set to CAN
- idlemode is turned off

In the video we see full throttle which takes the motor to the limited 1500 rpm. din_cruise is high all the time but cruisespeed is -1 - probably this is not relevant. The throttle is taken away but instead of regenning the motor accelerates past the fmax limit (2300) and when pressing the brake pedal it accelerates more (2500 rpm).

It looks like it is not directly related to cruise control but to regen. Regen current accelerates instead of slowing down. Regen current is not limited by the fmax limiter because it is supposed to slow down. That is how it gets past it.
Does the same happen if din_cruise is low? Can you record potnom? Is syncofs really set up correctly?

[Romale]

no, if the cruise is turned off, then the behavior of the motor is adequate and the reaction to the gas pedal and the brake pedal is correct. however, this is not exactly a Nissan leaf motor. this is the EM57 motor from Nissan note e-power. I tried to drive yesterday and very often got overcurrent, I suspect that the syncofs are not very true. I will try to perform the configuration.

[jrbe]

I use both pedals in drag racing to warm up the tires, for drifting and when parking on curbs to better control performance and save time switching from one pedal to the other. In addition, when driving on the street, I want to use the second analog input and the brake light signal to variably control the recuperation. Of course, not everyone has this requirement and current OEMs don't offer these options like the old ones, but I don't see a problem in giving the user the choice.

..how to logic this out (my poor edit)..

Bosch ecu's will typically allow a few seconds of left foot braking allowing the full range of engine power request from the throttle pedal. But after those few seconds of the brake pushed it will quickly (but smoothly!) ramp throttle down to 0, assuming a stuck throttle. I think the ecu coding considers both steady on braking and repeated brake "stabbing" over that time as driver panic / a stuck throttle and uses either to ramp throttle to 0. After a few seconds of no brake input but with throttle input the throttle ramps back up over maybe 2 seconds to match the throttle input (if I remember correctly..)

The smooth throttle reduction (if it senses a stuck throttle) is a requirement to not lose control if cornering anywhere near the limit. I've been in a vehicle when a throttle cut happened (speed sensor dropped out) and the ok-ish driver almost lost it.

muehlpower could do a line lock for burnouts and the above delay should work for parking and stuff. Alternatively, using a normally closed, brake input defeat switch (similar to how one would use a line lock) should allow the driver the choice to defeat the safeties if / when they want to. This gives "the user chose to defeat the safeties," out.

I'm not sure what the code would do seeing no brake pedal switch input but seeing a decel pot request with some throttle. A dual circuit defeat switch could break the circuit for the regen pot as well if needed. But then regen out of range (when pressing the defeat switch) might also trigger a fault.

Some consideration should be given for a physically disabled person using hand controls (if it hasn't already.) The shift to electric / drive by wire should make conversions easier to do for them.

I personally prefer the idea of needing to press the brake to get the vehicle "started" & moving. I recently had an e-brake failure total a car. It's got me rethinking things I took for granted and safety things I assumed a person like me doesn't need.