mkaiser / Sungrow-SHx-Inverter-Modbus-Home-Assistant

Sungrow SH Integration for Home Assistant for SH3K6, SH4K6, SH5K-20, SH5K-V13, SH3K6-30, SH4K6-30, SH5K-30, SH3.RS, SH3.6RS, SH4.0RS, SH5.0RS, SH6.0RS, SH5.0RT, SH6.0RT, SH8.0RT, SH10RT, SH5.0RT-20, SH6.0RT-20, SH8.0RT-20, SH10RT-20, SH5.0RT-V112, SH6.0RT-V112, SH8.0RT-V112, SH10RT-V112, SH5.0RT-V122, SH6.0RT-V122, SH8.0RT-V122, SH10RT-V122, SH4.6R
345 stars 92 forks source link

Current code issues, future plans #38

Open mkaiser opened 1 year ago

mkaiser commented 1 year ago

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 #43

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

  7. Document, how to write own automations, as asked here: https://github.com/mkaiser/Sungrow-SHx-Inverter-Modbus-Home-Assistant/issues/50#issuecomment-1407123886

  8. Maybe split current sungrow_modbus.yaml into multiple files (e.g. Base, Battery, History), #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.

andi-blafasl commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Idea from Louis712 to get rid of the secrets

https://github.com/mkaiser/Sungrow-SHx-Inverter-Modbus-Home-Assistant/pull/47#issuecomment-1404740741

mkaiser commented 1 year ago

https://github.com/mkaiser/Sungrow-SHx-Inverter-Modbus-Home-Assistant/pull/47#issuecomment-1404740741

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

reesaroo74 commented 10 months ago

@mkaiser FYI: https://github.com/mkaiser/Sungrow-SHx-Inverter-Modbus-Home-Assistant/commit/e8ec5473c2d6d199de4634e4e7e9c5d51cab4ff6

mkaiser commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 5 months ago

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