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

many improvements #393

Merged
merged 11 commits into from
Jan 11, 2023
Merged

many improvements #393

merged 11 commits into from
Jan 11, 2023

Conversation

ppuetsch
Copy link
Contributor

@ppuetsch ppuetsch commented Jan 5, 2023

This PR:

  • Makes things that should be configured configurable in an .ini file
  • allows the user to use a config.ini for user changes, while default_config.ini keeps the defaults that are maintained by the project
  • makes usages of variables from utils more explicit (got rid of one wildcard import from utils)
  • add gitattributes so that sh and py files will end up with linux line endings
  • enhances codeQL analysis to be able to include velib_python
  • adds tons of docstrings
  • adds tons of typehints
  • improves the get_midpoint method
  • publishes the config values to dbus / mqtt if configured

I tested this locally with a JKBMS/16c and it works nicely. I had no chance to test against other BMSes.

If you like this PR, i suggest to change the documentation (especially https://github.com/Louisvdw/dbus-serialbattery/wiki/How-to-install#how-to-edit-utilspy as utils.py shouldn't be edited anymore by a user - instead a config.ini file should be created as copy from default_config.ini where only the values to be changed remain)

@ppuetsch
Copy link
Contributor Author

Hi @Louisvdw ,

please find in this PR quite a lot refactorings (see the PR description for details)
Let me know if you like to have something changed or would like to discuss something.

Best regards,
Philip

@Louisvdw
Copy link
Owner

All the things you move to the ini is going to move to the GUI settings that I am busy changing. I like the default ini as we can use that to create different default value sets. I'll have to change the GUI settings to read from that. The values in that branch is then stored in the dbus, cause any changes can then trigger a update to the GUI.

The rest of the items look fine.

@ppuetsch
Copy link
Contributor Author

Hi @Louisvdw ,

thank you for the time reviewing this change.
Unfortunately, I don't yet understand what you would like to have changed.

Am I right with my understanding that I should:

  • remove publishing of the variables in utils.py to dbus ?

Should I also:

  • remove the reading of settings from the ini to the variables in utils.py
  • but keep the default_config.ini?

@ppuetsch
Copy link
Contributor Author

I now removed default_config.ini, reading that values to utils.py and publishing the data to dbus.
However, the code is still present in https://github.com/ppuetsch/dbus-serialbattery/tree/improvements_with_config

As I understood, you are fine with the remaining change.

Let me know If I can support with this default_ini approach somehow.

Best regards, Philip

Philip Puetsch added 2 commits January 11, 2023 08:12
@Louisvdw
Copy link
Owner

Sorry. I was not very clear.
I like your ini and how it will work.
I was only mentioning the GUI settings so that you know where we are going and if you have any ideas how we can best integrate both.

@Louisvdw Louisvdw merged commit 133f310 into Louisvdw:master Jan 11, 2023
@ppuetsch
Copy link
Contributor Author

How is the gui-settings to work? Do you have an Idea where to persist the changes, so that they remain set if the driver is reinstalled?

@Louisvdw
Copy link
Owner

@ppuetsch
I'm using Victron's localsettings to store the values. Localsettings is saved to the /data/ partition (same as the driver). It's also available inside the dbus.
Parts of it are already in the master branch but commented out. Lines 55-80 in dbushelper.py which sets up the default and create the store path.
Also see https://github.com/victronenergy/localsettings

The GUI Settings branch is to create a few new qml display files that bind to these values that the user can then change. So no more files to edit or change.

@Dr-Gigavolt
Copy link

Hello @Louisvdw and @ppuetsch,

I follow your development of charge/discharge parameters calculation. Before I tried the Version 0.14.3 (I think it was 0.14.0) the charge voltage limitation according the most full cell as well as "Floating" didn't work and SOC from JK BMS is useless anyway (bad current measurement). Therefore I integrated own SOC and charge/discharge parameter calculation into my AggregateBatteries. Now I see your progress and think about discontinuation and removal of charge/discharge parameters calculation from my tool to keep it simple and not to do your work second time. Other option is to finish it as another approach and perhaps support you - I believe it can be done simpler as momentarily in your program. I tried today the newest commit with default_config.ini, but it didn't start, I'll try to understand it later, for this time I continue with the last release. Your calculation is not fully clear for me, please answer/comment:

  • what is the reason of FLOAT_CELL_VOLTAGE, MAX_VOLTAGE_TIME_SEC and SOC_LEVEL_TO_RESET_VOLTAGE_LIMIT if Lithium cells don't float (have almost no self-discharge)? I wrote you, Louis, some weeks ago that it didn't work and you answered that I don't need it. So why do you keep it?
  • if the BMS's SOC is useless, just set CCCM_SOC_ENABLE and DCCM_SOC_ENABLE to False?
  • the relative fraction of charge/discharge current (default_config.ini) is much better than absolute value of current for each kind of reduction, please continue in this way
  • lists like CELL_VOLTAGES_WHILE_CHARGING etc. are of fixed length or you iterate through them?
  • how the PENALTY_AT_CELL_VOLTAGE works and why so complicated? In my tool (last solution, not published yet) I simply add all cell overvoltages together, subtract this sum from last measured battery voltage and set the result as max. charge voltage. Additionally I reduce the charge current to zero.
  • if you intend to move all settings into GUI, please store everything to a file, otherwise it gets lost on each restart or reinstall like /CustomName now.
  • please think about more clear comments in utils.py / default_config.ini and perhaps the code itself

Last but not least - thanks a lot for your work, for me it is not just a software to use, but also a great opportunity to learn new things.

Best Regards,
Anton

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.

3 participants