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

Current code issues, future plans #38

Open
mkaiser opened this issue Dec 20, 2022 · 18 comments
Open

Current code issues, future plans #38

mkaiser opened this issue Dec 20, 2022 · 18 comments
Labels
help wanted Extra attention is needed

Comments

@mkaiser
Copy link
Owner

mkaiser commented Dec 20, 2022

Future work items and disussion about code quality

Discussions, suggestions and help are much appreciated. I have not very much free time at the moment to check address all this by myself. So I will just write down what is in my mind right now :)

  1. use "unique_id" for every sensor. With a unique_id, every sensor can be edited via GUI (this feature was not yet available, at first release of this integration)
    When this is completed it is possible to MANUALLY ( :/ ) assign each value a room. In the Dashboard the sungrow inverter values would be grouped in that room and the values would not be randomly spread among the "generic sensors"

  2. Find a way to hide some values like the internally used "device type", from which the user-readable device strings are derived in a template sensor.
    They have a unique_id now, so at least they can be hidden by the user via GUI.

  3. Search for alternatives to set the user-specific parameters (IP, slave address, port) outside the .yaml configuration files and outside the secrets.yaml (currently it is there).

  4. Find a way to read parameters for maximum battery charge (inverter requests that value from battery at run-time). Then use this value to set range of the maximum battery charge current.

  5. Provide a second yaml file for slave inverters SH10RT Master & Slave #43

  6. Collect more dashboard examples from users (see cdaa5e8)

  7. Document, how to write own automations, as asked here: Automation for switching EMS Mode and battery forced charge // maybe tibber as trigger #50 (comment)

  8. Maybe split current sungrow_modbus.yaml into multiple files (e.g. Base, Battery, History), Deactivate/ hide battery features #65

Most of that would look like an ABI-change and users cannot simply upgrade by replacing the .yaml files. Should be something for a testing / v2 branch then.

@mkaiser mkaiser added the help wanted Extra attention is needed label Dec 20, 2022
@andi-blafasl
Copy link
Contributor

Hey, just thinking loud a bit. Maybe we can go a more "official" road with the integration?
I think following the documentation from HA would be a bit complicated and time consuming.
But HACS seems to be a more lightweight approach.

@elektrinis
Copy link

elektrinis commented Jan 6, 2023

I am very happy with rate of progress of development, however would really love it if this was installable via HACS.
While I managed to install it, it is really hard to maintain, and all the entities are not logically grouped in any way, so I have to keep an extra tab with all of entities to pick from.

Sadly I am unable to help with this :( Hopefully someone will jump in.

@mkaiser
Copy link
Owner Author

mkaiser commented Jan 18, 2023

hi,

I just decided not to write an HACS extension, because there is this project https://github.com/bohdan-s doing exactly this.

So this project will focus on the simple (understandable), easy to fix YAML-based integration.

@mkaiser
Copy link
Owner Author

mkaiser commented Jan 26, 2023

Idea from Louis712 to get rid of the secrets

#47 (comment)

@mkaiser
Copy link
Owner Author

mkaiser commented Jan 26, 2023

#47 (comment)

could be realized by using another "configuration tab" in the GUI like in #48

@mkaiser mkaiser changed the title Current code issues Current code issues, future plans Feb 6, 2023
@reesaroo74
Copy link
Contributor

@mkaiser FYI: e8ec547

@mkaiser
Copy link
Owner Author

mkaiser commented Nov 12, 2023

@mkaiser FYI: e8ec547

good catch!
Do you want me to just integrate it or do you want to create a Pullrequest (and do not miss all the fame! ;) )

@reesaroo74
Copy link
Contributor

Happy for you to merge.

While you're in there:

Where did you get the inverter state binaries from ... here's a new one.

image

I tweaked your code to punch out the state:
image

I suspect it is from some incorrect clamping of the three phase. This is for a SH10RS on a three phase system. If only I could have taken a photo of the installers faces when they had to wire that one up. They looked like me when I first hit yaml a couple of days ago. I'll try and get them back today to fix that up. It is stuffing up my EMS.

@mkaiser
Copy link
Owner Author

mkaiser commented Nov 12, 2023

okay, will merge it :)

Found this in the SG* modbus description. In my last version of SH* there isn't such value

image

"The inverter runs according to the scheduling instructions received from the monitoring background"

I don't know that is meant by "monitoring background"... Do you have any clue?

@reesaroo74
Copy link
Contributor

I'm moving across to Amber energy here in Australia.
It is possible that they are hooking up and controlling it via their SmartShift ?? I'll ping the installers and Sungrow today.

@rleidl
Copy link

rleidl commented Feb 7, 2024

Battery health during winter.

I have some thoughts about my battery during winter. Where I live, the battery won't have hardly any charge for two or three months. (mid of november to mid of february approx). If I let the minimum SOC at say the normal 5% the battery will be charging from nominal 5% to 10% and back.

In my head I have the idea that for optimum battery health the SOC should be at 80% when not in use. Am I correct? So I would like to be able to set Minimum SoC to 80 %. ATM the max SoC is 50% with the slider.

Could one change this so I can set Minimun SoC to 80% - maybe I am wrong about this battery health issue, maybe someone could clarify this. Thanks! R.

@elektrinis
Copy link

In my head I have the idea that for optimum battery health the SOC should be at 80% when not in use. Am I correct? So I would like to be able to set Minimum SoC to 80 %. ATM the max SoC is 50% with the slider.

Optimum SOC for LFP is around 30%. So just keep it within 10-70% and you'll be fine.

@dylan09
Copy link
Contributor

dylan09 commented Feb 7, 2024

But be aware that Sungrow recommends max SoC 100% because of balancing. If you did not charge until 100% there will be no balancing. And your cells my drift.

@rleidl
Copy link

rleidl commented Feb 7, 2024

Thank You.

@dylan09 - this means I should charge the Battery to 100% on a regular basis (in summer) or just now and then?

R

@danielclau
Copy link

I would set min at 40% and for the whole of winter and let it let it go up and down 10% from there.
Not balancing cells for 3 months will only make the remaning estimation a bit less accurate but as long as you aren't planning to take it down to single digits, it will be fine. There will be no permanent damage/degradation to the battery.
When spring breaks in Feb, charge it up fully and let the cells balance then. If you haven't balanced them for 3 months, it sit at 99% for more than an hour but once balanced, the estimation will be accurate again.

@dylan09
Copy link
Contributor

dylan09 commented Feb 7, 2024

My minimum SoC in winter is between 50% and 60% because of the backup function. And every few weeks I charge once to 100% (when the electricity price is very low).
I also think it's OK for a few weeks if the battery is not charged to 100%. With my BYD, it then takes a few days of charging up to 100% until all the cells are balanced again.
As @danielclau has written, it will not harm the cells if they are not charged to 100% all the time. The battery then needs some time until the BMS estimate is correct again.

@mastameista
Copy link
Contributor

Sungrow Germany PM Team recommends: Summer 20%, Winter 50% SOC-Reserve
https://www.photovoltaikforum.com/thread/174702-erfahrungen-sungrow-sbr/?postID=2862068#post2862068

Reason for using SOC-Reserve Parameter: Can be set with normal account (no installer account required)

@Gnarfoz
Copy link
Contributor

Gnarfoz commented May 6, 2024

A note on bullet point 2:
That would probably require extending the Modbus integration itself to expose the entity_registry_enabled_default and entity_registry_visible_default entity registry properties on the sensors it creates.
See https://developers.home-assistant.io/docs/core/entity/#registry-properties and https://github.com/home-assistant/core/blob/2e945aed54b1dd11fdf5212805383b515a09c59f/homeassistant/components/modbus/base_platform.py#L96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

9 participants