vbms32_micro.lisp charge-ok fix

[Seminko]

Since vc_charge_start can be set higher than vc_charge_end in VESC tool (created an ISSUE for that), we need to always check for that scenario regardless of charging status + charging should only start if c-max is lower than vc_charge_start.

EDIT: I have some more fixes that might be useful for others but I wouldn't put them inside this official script. Would you accept it if I created a new package, eg "vbms32_micro_mortificator_custom"? Or if you have a different naming convention in mind, let me know.

[Seminko]

@vedderb Hi, would appreciate any comment on this. If you think those are not good changes, I'd like to know why if possible. Thanks

[vedderb]

I don't quite understand what this fixes. How would not having it lead to overcharging? Also, to prevent overcharging (and other dangerous conditions) you need to configure things properly anyway. If any of vc_charge_start or vc_charge_end are set to an unsafe voltage the configuration is wrong and dangerous.

[Seminko]

@vedderb

Also, to prevent overcharging (and other dangerous conditions) you need to configure things properly anyway. If any of vc_charge_start or vc_charge_end are set to an unsafe voltage the configuration is wrong and dangerous.

I guess it's about general approach. I understand giving people control and in lieu of that the ability to mess things up. But there are scenarios where I think some guardrails should be in place still.

re: 2728505
I can't think of a scenario where setting vc_charge_start higher than vc_charge_end would make sense and be done deliberately. If that assumption is true, why not check for that and save some poor soul who misclicked?
It would be best to implement it directly in the app so that users CANNOT set vc_charge_start higher than vc_charge_end.

Currently:

(setq charge-ok (and
    (< c-max (if is-charging
        (bms-get-param 'vc_charge_end)
        (bms-get-param 'vc_charge_start)
    ))

Let's say someone messes up and sets vc_charge_start higher than vc_charge_end (end is set correctly to let's say 4.175V). If we're currently not charging this will check if c-max is lower than vc_charge_start which is true, and starts charging despite the cells being over vc_charge_end which will lead to overcharging.

main-ctrl function
(setq charge-ok (and
    (< c-max (if is-charging
        (bms-get-param 'vc_charge_end)
        (bms-get-param 'vc_charge_start)
    ))
    ; Since vc_charge_start can be set higher than vc_charge_end in VESC tool, we need to always check for that
    (< c-max (bms-get-param 'vc_charge_end))

This checks for that and stops that user oversight.

---

re: 575387c
In main-ctrl if c-max is higher than vc_charge_start is checked for in main-ctrl function, but not in start-fun - so I implemented it there. There might me be a reason for the current setup which I can't think of.

---

Also as I said in the main post:
I have some more fixes (eg that stop the Funwheel on-off charging cycle) that might be useful for others but I wouldn't put them inside this official script. Would you accept if I created a new package, eg "vbms32_micro_mortificator_custom"? Or if you have a different naming convention in mind, let me know.

[vedderb]

If you set vc_charge_start to an unsafe voltage it can lead to overcharging, but I think setting any of the charge limits to something unsafe is a major mistake in the configuration. Both charge limits should be kept at a safe voltage no matter what.

It might make sense to run some sanity checks on the configuration and block charging completely if they don't pass. Could even add a status error message for that. But I don't like this change as it is as it does not really solve the problem and makes the code more confusing to reason about.

[Seminko]

If you set vc_charge_start to an unsafe voltage it can lead to overcharging, but I think setting any of the charge limits to something unsafe is a major mistake in the configuration. Both charge limits should be kept at a safe voltage no matter what.

Formatting your hard drive when you don't want can also lead to bad outcomes. That's not an argument. That's why a confirmation is always needed before potentially messing things up in a big way.

 

It might make sense to run some sanity checks on the configuration and block charging completely if they don't pass. Could even add a status error message for that

VESC Tool is getting into more and more hands, more people are gonna blunder and it will cause stuff. So it would be great.

 

But I don't like this change as it is as it does not really solve the problem and makes the code more confusing to reason about.

I does solve it though and is far from confusing. Not the whole problem and not globally, granted. So if your point is to tackle the whole topic holistically rather than fixing specific stuff in random packages - I agree, it would be a much better approach.

 

Anyways, thanks for your work - without it I wouldn't ride a onewheel!