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

Wrong uniqe_id in HA for all sensors #91

Closed DaZeroXB closed 1 year ago

DaZeroXB commented 1 year ago

According to the script, all sensors have uniqe ids with prefix sg. However, HA seems to ignore these ids and just name them according to the friendly name. By doing so, the sensors loose the context of being related to the sungrow inverter which is quite unhandy I would say. I can confirm that also a colleague of mine experiences the same behaviour.

@mkaiser Any thoughts about that? wrong_uniqe_id

mkaiser commented 1 year ago

hi,

yes, I can confirm this behaviour.

My guess it that this is an issue in the modbus code, but I could not investigated it, yet. There are some references to unique_id, but I am not very familiar with Python...

HA modbus code

The HA modbus doc states image

but putting this at the beginning of the modbus config block does not help, either.

There are some other things bothering me, too. For example that I cannot set a room/ location for the inverter. Therefore, several sungrow sensors appear as input / binary/ etc. sensors in the complete dashboard overview. With more sensors it gets very crowded.

It would be great, if someone could investigate this more in detail. I guess an official HA modbus issue would be good (but I am lacking time at the moment)

~Martin

DaZeroXB commented 1 year ago

Hi Martin, thanks for your explanation and confirmation. How about to just change all friendly names by adding a "SG" prefix? BR, Martin

dylan09 commented 1 year ago

For me its normal Home Assistant behaviour since i started with HA in 2019. unique_id is an internal value. In case of yaml configuration only used to reference between yaml and core.entity_registry' entity_idis created by modifying thename. For human readable names there isfriendly_name` in customize.

A lot of sensors defined in yaml in my setup looks like this definition:

    - name: sg_daily_pv_gen_battery_discharge
      unique_id: 049dac4d-7785-4b25-81f6-2428f4b721f7
      slave: !secret sungrow_modbus_slave

unique_id is a random UUID.

The Unifi integration for example uses the MAC address of devices for unique_id.

If you like to have the sg_ in front of entity_id you have to add it to the name. There is also a document describing the rules how the entity_id was build from name.

DaZeroXB commented 1 year ago

Thanks @dylan09 you are right, I was not aware of the difference of uniqe_id and entity_id. @mkaiser, according to this knowledge, from my POV the script should change it's uniqe_ids to random guids in order to not confuse any longer and also consider putting SG in front of the name.

dylan09 commented 1 year ago

Hi @DaZeroXB. I also need a while to be aware of the meaning for entity_id, unique_id and friendly_name. And that friendly_name mostly is only changeable through customize.yaml But after working a while with UUIDs for unique_id I have switched to use meaningful ids. It's better readable. And it doesn't matter if you use something like 049dac4d-7785-4b25-81f6-2428f4b721f7 or sg_daily_pv_gen_battery_discharge as unique_id. It only has to be unique and should never be reused for sensors with other purposes. For the future I will use readable unique ids for my sensors.

mkaiser commented 1 year ago

uff, this took me a long time reading and trying to finally understand this (again)

This helped mea bit: entity_id

@dylan09 / all: do you know a better way to reference sensors within the yaml-file?

At the moment it is like

  sensors:
      - name: Sungrow device type code
        unique_id: sg_dev_code

reference by this: {% if ((states('sensor.sungrow_device_type_code') | int(default=0)) == 0x0D06) %}

Although spaces are just replaced with underscores, it is not very "copy/paste/replace all" -friendly.

Back to the original issue: I agree with dylan09. Changing to "uid numbers" has no benefiits. So lets not change this in the code. Maybe I add some explanation at he beginning of the YAML.

DaZeroXB commented 1 year ago

@mkaiser: For the example you've given above would it be ok from my pov, since 'Sungrow' is part of the name. However, all other sensors do not have 'Sungror' or 'SG' in it. I've created a modbus integration for my heat pump and stick to the convention, that each sensor name has a "GD HP" prefix. (Glen Dimplex Heat Pump). I would really suggest to put at least "SG" in front of the sensor names. Since I have already multiple devices that are reporting a Power value so I need some sort of distinction by device. Furthermore do I follow a convention when it comes to sensor names at all.

Here are some examples of my HP and my dad's PV

  sensors:
       - name: Solax PV Power <br />
         unique_id: RANDOM-GUID for all sensors
       - name: Solax PV Energy Total
       - name: Solax PV Energy Day
       - name: Solax PV MPPT1 Power
       - name: Solax PV MPPT2 Power
       - name: Solax Grid Power   (note: signed)
       - name: Solax Grid Import Power
       - name: Solax Grid Export Power
       - name: Solax Grid Import Energy Day
       - name: Solax Grid Export Energy Total
       - name: Solax Battery Power (note: signed)
       - name: Solax Battery Percentage
       - ...
       - name: GD HP Electric Power
       - name: GD HP Electric Energy Day
       - ...
       - name: GD HP Thermal Power
       - name: GD HP Thermal Power Day
       - ...
       - name: GD HP Inverter Frequency
       - name: GD HP Flow Temperature
       - ...

I'll get my new Sungrow PV this Wednesday and I'm super excited to have it integrated in HA 😃

dylan09 commented 1 year ago

do you know a better way to reference sensors within the yaml-file?

One possibility would be to set name to the same value as unique_id. For example

sensors:
      - name: sg_dev_code
        unique_id: sg_dev_code

Disadvantage: In the UI the sensors do not appear with plain text names. However, these can be changed quite easily in the UI. Just like the icon can be changed there. Alternatively, a customize.yaml with the friendly names could be provided here in git.

I named the sungrow sensors with name/entity_id according to the pattern above. All entityids start with 'sg'. And then comes battery for battery values, total for total values, daily for daily values, ... Simplifies the search / input of sensors immensely.

@DaZeroXB verwendet ja auch bereits ein ähnliches System.

mkaiser commented 1 year ago

I guess that is a good way to get things more structured and maybe add some more language support (I guess several users like to have in in german, too)

@dylan09 Can you share your customize.yaml ?

I hope that I can create a "version 2" of this integration in the next weeks, also including more secrets for battery defines and prepare the names for easy 2nd inverter support by just find & replace certain strings.

It will be version 2, because it will have breaking changes (not only copy & paste the modbus_sungrow.yaml)

dylan09 commented 1 year ago

Will try when there is a little spare time.

Has someone tried to add a customize section to a package?

mkaiser commented 1 year ago

started a bit with "project version 2"

open issue with chaning unique_ids here:

https://community.home-assistant.io/t/best-practise-for-renaming-unique-ids-of-yaml-based-sensors/580013/2

@dylan09: did you make some progress regarding the customizing?

I like to have the names consolidated like

yaml-inverter#1:
name: SG1 Sungrow device type code

yaml-inverter#2:
name: SG2 Sungrow device type code

but for users with only one inverter the "SG1" name prefix should be ommittable in the GUI