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
328 stars 86 forks source link

added monthly and yearly history sensors and real battery SoC #51

Closed Louisbertelsmann closed 1 year ago

Louisbertelsmann commented 1 year ago

I added monthly PV generation & export energy, yearly PV generation & export energy and a template that outputs the value for the current month/year. I also edited the dashboard to have a history tab.

mkaiser commented 1 year ago

hi,

that are a lot more energy sensors - thank you for your contribution.

It will take me a while to review it and merge it, please do not think I would not be appreciating this! :)

Louisbertelsmann commented 1 year ago

I was kind of in a hurry when creating the dashboard. I'll look at the new one and change mine a bit. The config for modbus will stay the same.

Louisbertelsmann commented 1 year ago

I'm done with the dashboard now. You can merge now, if you want to.

Louisbertelsmann commented 1 year ago

I don't really understand the conflicts. Some sensors I added were wrong. Well, only the Monthly and Yearly sensors are wrong. Those aren't in my new config yaml. So, that's fine. Is it possible for you to change my code for your new push to the config? I don't want to mess it up.

mkaiser commented 1 year ago

Started to resolve conflicts. Did not test yet, need sleep first :)

mkaiser commented 1 year ago

WARNING:

made a quick compare with the current main branch.... Needs some more love. I guess it will generate several errors at the current state

Louisbertelsmann commented 1 year ago

I just noticed that there I have two entries for each months export energy. While you‘re at it, could you remove one of them? I think that I was not focused and forgot I already had it.

Louisbertelsmann commented 1 year ago

Ideally the „Total export from …“ because I used the other one for all the templates and the dashboard.

mkaiser commented 1 year ago

Ideally the „Total export from …“ because I used the other one for all the templates and the dashboard.

The difference between these two sensors is, that the first one also takes exported energy from the battery into account. The "* from PV" measures the energy directly coming from PV

"Total exported energy" addr 13045 and "Total exported energy from PV" addr 13005

Louisbertelsmann commented 1 year ago

No, I meant the monthly Export Sensors. I added them twice.

mkaiser commented 1 year ago

Doing the merging via github webinterface went horribly wrong...

I restarted using the current main version, comparing it to your changes and inserted them in ascending modbus register order between reg 5725 "Backup phase C power" and reg 12999 "System state"

sensors removed:

Sensors added:

monthly PV generation Jan: address: 6226 # reg 6227 dec: address: 6237 # reg 6238

yearly pv generation 2019 address: 6257 # reg 6258 2029 address: 6277 # reg 6278

monthly export jan: address: 6595 # reg 6596 dec: address: 6606 # reg 6607

yearly export 2019: address: 6615 # reg 6616 2028: address: 6633 # reg 6634

renamed some of your sensors:

Adapted template sensors: Yearly PV generation (current) early PV generation (current) Monthly export (current) Yearly export (current)

simplified them using some string operations (took me while to learn, that you concatenate strings with '~', but searching for this was worth it!). Now the error-prone if / else can be omitted. e.g.:


      - name: "Yearly PV generation (current)"
        unique_id: sg_yearly_pv_generation_current
        unit_of_measurement: kWh
        device_class: energy
        state_class: total_increasing
        state: "{{ states('sensor.' ~ 'yearly_pv_generation_' ~ now().year ) }}"

      - name: "Monthly export (current)"
        unique_id: sg_monthly_export_current
        unit_of_measurement: kWh
        device_class: energy
        state_class: total_increasing
        state: "{{ states('sensor.' ~ 'monthly_export_' ~ '%0.2d' % now().month ~ '_' ~ now().timestamp() | timestamp_custom('%B') | lower ) }}"

Open issue:

"Yearly PV generation (2022)" shows me a value of around 18 000 kWh, but actually the generation was less, around 12 600 kWh.

Can you double check?

Louisbertelsmann commented 1 year ago

First of all, thank you. Right now I don't have the time to fix all the issues. I think you fixed everything. At first I didn't understand the monthly and yearly sensors, so I just added one of them and thought it would be the current year or something. I knew that it was possible to use string operations, but didn't know how to. That's a great way. I will have a look at it, but as far as I remember it worked fine for me.

Louisbertelsmann commented 1 year ago

It works fine for me. Are you done merging it or did you run into some problems?

Louisbertelsmann commented 1 year ago

What? I didn't close it. Well, I don't think it matters since you've already got the code.

Louisbertelsmann commented 1 year ago

Should I add the sensors to the current version of the repository and create a pull request, so you can merge it easily and there aren‘t any conflicts? @mkaiser

mkaiser commented 1 year ago

hi, sorry, I am currently very busy :/

please reopen the issue, so I future me won't overlook it :)

My current problem is, that for me wrong values are shown and I don't know why. It may be related to a inverter factory reset I made, but I am not sure.

This and the additional "load" of additional sensors ~600 Lines of code makes me hesitate to integrate it at the moment.

In the long run, I like to have the modbus_sungrow.yaml file modularized, so anyone can just select between things like

which would help a lot, since the codebase is getting larger (~2k LoC at the moment)

I tried a few "!include things", but they did not work. So any help / hints are apprechiated

Please find attached the separated history sensors. Just copy the content of the sensors and template sections into the sections of your modbus_sungrow.yaml history_sensors.yaml.txt

Louisbertelsmann commented 1 year ago

I also think that there‘s a way to remove the automations. It works right now, but I think it could work better. I think I found a way to set the register directly without using an automation. The creator used it to control a light, but it could be adapted to write anything for it. That would also remove the automation clutter, which would make everything a bit more clear when setting other automations. I‘ll try to configure something like that and if it works i‘ll add it to the pull request, along with the sensors. This is the link to the thread on the forum: https://community.home-assistant.io/t/light-template-and-modbus-registers/179595/6 Also, I know if else thingies aren‘t great, but wouldn‘t it be possible to add and remove stuff like battery control, battery sensors and history using that? For example, if a variable (sungrow_battery_sensors, for example) is 0 the battery sensors are disabled and if it‘s 1 it‘s enabled. That wouldn‘t make the code more clear, but it would remove the problem to remove every line of code one by one. In my opinion that makes it even easier than using !include to include separate yaml files and stuff like that.

Louisbertelsmann commented 1 year ago

Added history sensors again and added real battery SoC requested by @dylan09

Louisbertelsmann commented 1 year ago

Also, it doesn't have conflicts.

Louisbertelsmann commented 1 year ago

grafik

mkaiser commented 1 year ago

sorry, I am a bit "distracted" from working on the sungrow stuff at the moment :)

Please keep the two features separated in two pull requests - it is hard for me to track things at the moment and I could not focus on the ongoing discussion with the battery SoC templates, yet

Githubs integrated diff-viewer is not very good when moving text sections. Will check this with a good text editor, soon.

one more thing: I tried to put the modbus sensors in line with increasing modbus addresses. Can you put the history sensors accordingly?

Louisbertelsmann commented 1 year ago

Will do. What do you want first, the battery SoC templates or the history sensors? I can only create one pull request at at time.

mkaiser commented 1 year ago

My next thing on the Todo is to split up the huge modbus_sungrow.yaml.

If someone does not want the >30 history sensors it should be somehow un-includable.

If I merge the pr and someone updates at this point, all the sensors will pop up in home assistant and would need to be removed by hand.

Give me some more time on this to properly check for side effects using the "packages approach"

https://www.home-assistant.io/docs/configuration/splitting_configuration/

And here https://www.home-assistant.io/docs/configuration/packages/

So a separate PR with the battery soc would be better at this time.

If you fork this repo into your account, you can create multiple PRs.

~Martin

Louisbertelsmann commented 1 year ago

That shouldn‘t affect anything, since homeassistant is using the packages approach in their own config to include modbus.

mkaiser commented 1 year ago

That shouldn‘t affect anything, since homeassistant is using the packages approach in their own config to include modbus.

hmm sorry, but I cannot parse this - what do you mean?

image

separating the yaml-file apparently does not work, if two files are referencing the same modbus device /port.

I guess to really separate base, battery and history stuff, the 'yaml-way' is not enough and we need to got he 'python-way'

Louisbertelsmann commented 1 year ago

What if you just define the host and then just add packages to that? Like the different Sensors?

mkaiser commented 1 year ago

tried many things. Will open an Home assistant forum with that, soon.

I am pretty confident, that I will finally get this PR tested and merged this weekend :)

mkaiser commented 1 year ago

hey,

merging was hell.... I ended up committing the history stuff directly to the main branch and afterwards I edited your commit to reflect the changes with the SoC-related changes. I really hope I did not screw things up.

There are currently some issues with the template sensors... Committed them anyway, will fix them soon

yoda98mnt commented 8 months ago

Hello, now that the new year has begun, all history values to zero except for January. Why is that? Wouldn't it be better if only the current month was reset? sensor.monthly_pv_generation_01_january sensor.monthly_export_01_january

Louisbertelsmann commented 8 months ago

I don't think so. It might be confusing. Also, tell that to Sungrow. These sensors just read the predefined monthly sensors in the inverter.