Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved Battery Charging Control through Optional Use of System SOC #741

Closed
wants to merge 36 commits into from

Conversation

ogurevich
Copy link
Contributor

Problem:
In the current implementation, the SOC of the battery is used to decide when to set the Change Voltage Level (CVL) from Float Mode back to Maximum. However, some Battery Management Systems (BMS) do not calculate the SOC value reliably or correctly. This issue is further exacerbated when multiple batteries are operated in parallel.

Solution:
This change allows the driver to utilize the SOC value from the system, which could be from a SmartShunt or another reliable source, if available. This provides a more reliable source for the SOC value and improves decision-making for setting the CVL. This change is optional and not enabled by default, but can be enabled if needed to access a more reliable SOC value.

Impacts and Testing:
Since this change is optional, it should not have any negative impacts on existing systems that do not utilize it. Testing should be performed on systems both with and without a reliable system SOC source to ensure the change functions as expected and does not have any unwanted side effects.
Screenshot 2023-07-08 at 21 24 42
Screenshot 2023-07-08 at 21 24 25

ogurevich and others added 30 commits June 8, 2023 21:12
This commit introduces a new constant, PENALTY_BUFFER, to make the code more readable and maintainable. PENALTY_BUFFER is added to the penaltySum calculation when a cell's voltage exceeds MAX_CELL_VOLTAGE, thus creating a buffer or safety margin. This results in a stricter overcharge prevention mechanism as it reduces the control voltage further when a cell's voltage exceeds MAX_CELL_VOLTAGE. The value of PENALTY_BUFFER is currently set to 0.010.
This commit introduces several changes to the battery.py script to improve the handling of battery voltage calculations. Specifically:

1. Constants  and  are introduced to replace repeated calculations of these values.
2. These constants are then used to replace the previous direct calculations in various conditions and assignments.
3. Conditions for resetting  are updated to include a check against .

These changes aim to make the code more efficient by reducing repeated calculations, and also make the code easier to understand by using clearly named constants.
This commit refactors the battery.py script to use the existing class variables  and  instead of creating new local variables. This change simplifies the code and ensures that the same voltage values are used consistently across all methods in the  class.
…ogdev

put inits into prepare_voltage_management()
… for example by SmartShunt can prove to be more reliable than SOC by BMS
@ogurevich
Copy link
Contributor Author

i will test it a while, especially changes in dbushelper.py

@ogurevich ogurevich closed this Jul 9, 2023
@mr-manuel mr-manuel reopened this Jul 9, 2023
@mr-manuel mr-manuel marked this pull request as draft July 9, 2023 08:13
@mr-manuel
Copy link
Collaborator

I reopened it and converted it to a draft.

@ogurevich
Copy link
Contributor Author

for the implementation I need the SOC value of SmartShunt. For this I use VeDbusItemImport.

The system behaves mysteriously when I import the item from the service com.victronenergy.system. Everything works fine if the item is imported directly from the SmartShunt service (com.victronenergy.battery.ttyS5).

Is it possibly because com.victronenergy.system is huge? Do you have any idea what could be wrong with it?

does not work ( no bugs or exceptions, but second battery does not get any values .....)

  self._dbusDCBatterySocItem = VeDbusItemImport(
                 get_bus(),
                 "com.victronenergy.system",
                 "/Dc/Battery/Soc"
             )

it works fine, if we obtain the values directly from SmartShunt

           self._dbusDCBatterySocItem = VeDbusItemImport(
               get_bus(),
               "com.victronenergy.battery.ttyS5",
               "/Soc"
           )

@mr-manuel
Copy link
Collaborator

Could it be that you are generating a loop? It would be better, if you check on which address you find the SmartShunt and get the value directly from there. Victron already has something like that in the dbus-systemcalc-py that you probably can copy.

@ogurevich
Copy link
Contributor Author

thanks for the tip, check it out. 👍

@ogurevich
Copy link
Contributor Author

that's a rather philosophical question for the author and maintainers to answer.
What would be preferable here? Have the user fill in the exact ServiceName (and hope they know what they're doing), or have the System driver look at Settings/SystemSetup/BatteryService and consider their logic plus (No battery and Automatic).

My understanding is that the driver must remain as simple and lean as possible, using as few resources as possible.

For each battery pack a separate python process is started, if each process starts to monitor the DBUS too intensively and react to it, then I believe that this is not the primary task of the driver. What if later the user starts to change the settings, should the driver follow the changes?

What do you think of the option of having the service name entered in config.ini first?

@mr-manuel
Copy link
Collaborator

I'm still not convinced that it's the task of the driver to get the SOC of the shunt. If, then the battery aggregator should do it or you can do it by yourself by using the mqtt battery and aggregate the data yourself. As you said, if there are multiple batteries it makes still less sense to me to use a centralized SoC that apply to all batteries, since not the same current is drawn from each individual battery.

@ogurevich
Copy link
Contributor Author

i agree that is not a task of the driver to get the SOC of the shunt. But the task of driver is among other to leave float mode to the right moment.

I don't want to rely on the information from my JKBMS, all the more so that they still have the drift and calculate absolutely different SOC values.

In addition/alternativ , I have to intervene from time to time and adjust the config parameters and restart the driver. This is less user friendly. That could be seen as a solution. Although I don't insist, that's just a suggestion. :)

@mr-manuel
Copy link
Collaborator

With the new feature that I developed you can reset your SoC automatically every x days. Then the battery loads to OVP every e.g. 15 days to set the SoC to 100%. I‘m testing it since two weeks, but there are still improvements needed.

@ogurevich
Copy link
Contributor Author

That it very good news, your intended solution is in all respects better. This PR (741) is a workaround anyway, i think we don't need it. Let's close it ?

@mr-manuel mr-manuel closed this Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants