vedderb / bldc

The VESC motor control firmware
2.14k stars 1.34k forks source link

Safety feature for balance app: Disable BMS driven current limiting #531

Closed surfdado closed 1 year ago

surfdado commented 1 year ago

For balance applications we need the ability to disable any BMS/CAN driven current limiting, especially the SoC limiting. There have been nosedives/crashes reported recently by users getting SOC driven current limiting.

It should be up to the user whether they want to leverage the BMS CAN data for informational purposes only, or whether they want actions to be taken. It takes a certain level of trust in your BMS / configuration before allowing the controller to take such drastic measures

Thermal driven actions can be effectively disabled by raising the temps, but SOC driven limiting cannot be disabled today afaik.

I intend to develop a fix for this myself eventually, as soon as I have time to work on it, but I wanted to log this issue asap - users are now leaving the CAN bus disconnected, which is obviously not what we want to happen

vedderb commented 1 year ago

This is an attempt at a fix:

image

Commit: https://github.com/vedderb/bldc/commit/765ed4d3d45ec3843fcd500c6e4e5a4c5e300356

Can you give it a try?

surfdado commented 1 year ago

Holy cow that was quick :) I have to admit that I don't yet have a working VESC BMS (coming soon hopefully) so i cannot test this right now, but I did review the code. This looks exactly like what we'd want, very nice!

One thing I'd like to request though - could we make this opt-in instead of opt-out? I.e. make the default 0 and not 3? So the default would still allow users to see all the BMS data, but not have any BMS driven current limiting enabled.

Using a bitfield is great idea for this. However, in the vesc_tool commit why include those 6 "Unused" fields? Those can be added as needed once used, e.g. current limiting due to cell undervoltage, or cell imbalance. Either way new fields will be needed in order to configure each additional "limit mode" anyway.

Cheers, Dado

vedderb commented 1 year ago

Every bitfield is always 8 bits, but I just updated the editor to hide unused bits: https://github.com/vedderb/vesc_tool/commit/e90755a4784a812c926dc82f819c8fad7e5e812d

I think these should be on by default because on everything except a balance vehicle I would prefer to slow down before I overdischarge the battery and destroy the cells. Even on balance I think you should have a way to prevent overdischarging the battery. One way around this would be to change the setting in the wizard if balance is chosen as the application.

surfdado commented 1 year ago

I think these should be on by default because on everything except a balance vehicle I would prefer to slow down before I overdischarge the battery and destroy the cells. Even on balance I think you should have a way to prevent overdischarging the battery. One way around this would be to change the setting in the wizard if balance is chosen as the application.

Fair point, I guess users should make sure their SOC calculation is working correctly when they set it up, it's just too bad that it can have such catastrophic consequences if that gets screwed up initially. But you're right about the wizard - when Balance Vehicle is selected in the Wizard we could make the appropriate changes to the defaults (btw, balance vehicles by default already prevent over-discharge via low voltage tiltback alerting the user / making it hard to keep riding)