Louisvdw / dbus-serialbattery

Battery Monitor driver for serial battery in VenusOS GX systems
MIT License
492 stars 157 forks source link

Call frequency of CVL_ICONTROLLER #1041

Closed cflenker closed 1 month ago

cflenker commented 2 months ago

Describe the bug

In former versions of the driver, the IController algorithm was called every time the manage_charge_voltage_linear() was called. As far as I know this is something link one call per second. In the latest version of @mr-manuel fork https://github.com/mr-manuel/venus-os_dbus-serialbattery it is also calculated at every call of manage_charge_voltage_linear() but self.control_voltage is only set every LINEAR_RECALCULATION_EVERY (default setting 60seconds) by using the set_cvl_linear-Function. For the behaviour and the stability of an discrete IController the call-frequency is essential. So by reducing the call-frequency from 1s to 60s has made the IController 60 times slower and less stable. I do not see any sense to reduce the call-frequency of the IController to 60s because the calculation effort is done every second anyway. The inital calibration (CVL_ICONTROLLER_FACTOR=0.2) is also tuned to fit a call frequency of 1/second.

How to reproduce

Charge to CVL

Expected behavior

IController is calculated every second.

Driver version

1.3.20240417dev

Venus OS device type

Cerbo GX

Venus OS version

3.30

BMS type

JKBMS / Heltec BMS

Cell count

16

Battery count

1

Connection type

Serial USB adapter to RS485

Config file

not relevant

Relevant log output

not relevant

Any other information that may be helpful

To achieve the old behaviour of the IController I changed LINEAR_RECALCULATION_EVERY from 60s to 1s. But this causes much more cpu load.

mr-manuel commented 2 months ago

Could you test with the latest nightly?

cflenker commented 2 months ago

Hello @mr-manuel, Thank you for your fix! The code looks fine now. I just updated my own system to v1.3.20240428dev. It will be sunny enough to reach 100% tomorrow. So I will be able to give you a feedback tomorrow evening. If it works fine we can close this issue and https://github.com/Louisvdw/dbus-serialbattery/issues/1038. Thank's a lot!

cflenker commented 1 month ago

It works. But not as smooth as with the old version 1.0.20240102beta. image The CVL-controller does not lower CVL enough and so the cell voltages rise much more. I did not change anything in the config.ini I will try to find the difference.

cflenker commented 1 month ago

It's propably the rounding. An I-Controller increases/decreases only very little every timestep. The rounding seems to kill these increase/decrease if the cell voltage is only a little above MAX_CELL_VOLTAGE. I changed the rounding to 6 (instead of 2) digits. Let's see if it works better today.

cflenker commented 1 month ago

Now it works fine. The rounding has to be removed:

              if utils.CVL_ICONTROLLER_MODE:
                    if self.control_voltage:
                        control_voltage = round(
                            self.control_voltage
                            - (
                                (
                                    self.get_max_cell_voltage()
                                    - (
                                        utils.SOC_RESET_VOLTAGE
                                        if self.soc_reset_requested
                                        else utils.MAX_CELL_VOLTAGE
                                    )
                                    - utils.CELL_VOLTAGE_DIFF_KEEP_MAX_VOLTAGE_UNTIL
                                )
                                * utils.CVL_ICONTROLLER_FACTOR
                            ),
                            2,
                        )
                    else:
                        control_voltage = self.max_battery_voltage

image image

mr-manuel commented 1 month ago

That means a rounding to 6 digits is not enough?

cflenker commented 1 month ago

That would be enough. But does it make sense? The display in Cerbo UI is rounded to 2 digits anyway.

mr-manuel commented 1 month ago

Yes, often there are useless roundings after calculations like 62.1000000001.

cflenker commented 1 month ago

Ok. Then 6 digits is fine. I tested with 6 digits.

mr-manuel commented 1 month ago

Fixed with v1.3.20240510dev.