pulquero / BatteryAggregator

MIT License
35 stars 7 forks source link

CVL is not aggregated when using CVL_ICONTROLLER_MODE = True #45

Closed Albarge closed 7 months ago

Albarge commented 7 months ago

Hi, using v3.0.50 and serialbattery v.1.3.20240417dev I could see Icontroller lower CVL for one battery to 27.2V, but BatteryAggregator stayed at 28V for all three batteries, I'm not sure if this is a serialbattery issue or batteryaggregator.

This stops the icontroller CVL from working to limit Voltage when a cell goes over max Voltage.

See here: https://github.com/Louisvdw/dbus-serialbattery/pull/882#issuecomment-2064658498

pulquero commented 7 months ago

See https://github.com/pulquero/BatteryAggregator/issues/44.

Ideally, what I need to know is "if the batteries have these dbus path values X then the aggregator should do Y". Currently, if any battery is reporting as balancing then the max voltage is used to ensure it can be balanced (at the risk of overvoltage on the other batteries, what else can I do?), otherwise the safer min voltage is used. So the biggest issue is really working out want the best behaviour should be, and if that behaviour will be appropriate for everyone or whether it needs to be configurable.

Albarge commented 7 months ago

I don't know the answer to make it work for everyone or easiest way for you to implement, but I would suggest:

For example, If: Info:MaxChargeVoltage is 27.2V on any battery then aggregator should follow this for all batteries.

Yes it will (temporarily) lower CVL of batteries still balancing, but if a cell(s) are overvoltage it will protect them from overcharge.

If in sbattery config: CVCM_ENABLE = True + LINEAR_LIMITATION_ENABLE = True then the "CVL reset based on cell voltage diff (linear mode)" can be used to control when to change CVL to Float.

I think it is a key part of sbattery that it can control CVL if any cells go over voltage, this is a higher risk to accommodate before next making sure all batteries are balanced.

pulquero commented 7 months ago

I've pushed a new version that supports a CVL "mode" flag. In the JSON config, add the property "cvlMode": "min_when_balancing" See if this achieves what you require. You'll need to restart the BatteryAggregator after modifying the config.

Albarge commented 7 months ago

That's great thanks! I've installed and will see how it works tommorow in the sun!

I didn't put brackets on this property, but does It need brackets like this in the json?

{ "cvlMode": "min_when_balancing" }

Albarge commented 7 months ago

Just looked and saw an old bug? from an earlier version came back with v3.0.51 I only have 3 batteries, but in system and dbus batteryaggregator thinks I have 4?

Screenshot_20240419-231953

pulquero commented 7 months ago

That's great thanks! I've installed and will see how it works tommorow in the sun!

I didn't put brackets on this property, but does It need brackets like this in the json?

{ "cvlMode": "min_when_balancing" }

Yes, if not already present.

pulquero commented 7 months ago

Just looked and saw an old bug? from an earlier version came back with v3.0.51 I only have 3 batteries, but in system and dbus batteryaggregator thinks I have 4?

Screenshot_20240419-231953

If you check /System/Batteries you can see the names of the 4 batteries to see what it thinks the fourth one is.

Albarge commented 7 months ago

I think it is adding the SmartShunt again (ttyS7), is one of these the system/batteries location you mean on dbus-spy?

system

batteries

I thought it was excluded with the "primaryServices" property but it wasn't so I added "excluded.." but aggregator is still seeing 4 batteries?

excluded

Albarge commented 7 months ago

So it looks like it doesn't like my json:

log

This is the latest config it ignored also;

json

Did I format incorrectly, or could this be related to your solution for this?

https://github.com/pulquero/BatteryAggregator/issues/33#issue-2206198341

pulquero commented 7 months ago

Your json is incorrect, it should be of the form { "KEY1": "value", "Key2": "value", "Key3": "value" }

Albarge commented 7 months ago

Like this?

{ 
 "primaryServices": {"com.victronenergy.battery.ttyS7": ["/Soc"]

 "cvlMode": "min_when_balancing" 
}
pulquero commented 7 months ago

{ "primaryServices": {"com.victronenergy.battery.ttyS7": ["/Soc"] },

"cvlMode": "min_when_balancing" }

Albarge commented 7 months ago

That worked! Only 3 batteries showing and no JSON warning.

I really appreciate your work on this driver and the support, Thankyou. : )

Albarge commented 7 months ago

The update works. Thanks.

Using the P-CONTROLLER in Serial Battery:

I've been watching the CVL gently kick in every so often to Limit the Max Voltage for all batteries which keeps the runner cells in one battery from going too high, until the cell voltage differentials reduce closer to the target Voltage. This also makes the final stage of absorption charging smoother. So it seems to really help with unbalanced, or runner cells jumping ahead. :)