[Bug]: Double battery device since 1.4.0 alpha 1

hvorragend commented 7 months ago

Describe the issue

Since the first alpha (and also in alpha2) I have my battery device twice.

All devices:

Device: Batteriespeicher

New battery device since alpha version: The entities in the sensor area were absolutely identical. I have currently deactivated them. But you can see that there are no select entities in the configuration area.

Bescribe your Huawei Solar Setup

Inverter Type: SUN2000-8KTL-M1 Inverter Firmware version: V100R001C00SPC159 SDongle present: yes Power meter present: three phase Battery: LUNA 2000 2x 5 kWh Battery Firmware version: V100R002C00SPC127

How do you connect to the inverter?

Via the SDongle, wired connection

Upload your Diagnostics File

config_entry-huawei_solar-421c4f96daf35cfb9816aefa6bc40b59.json

Upload your relevant debug logs

There are no relevant log lines.

Please confirm the following:

[X] I'm running the latest release of Home Assistant.
[X] I'm running the latest release of this integration.
[X] I did not find an existing issue describing this problem.
[X] I did upload the diagnostics-file that I could retrieve from the 'Devices & Services Page'
[X] I did upload the relevant debug logs (via 'Enable Debug Logging'-feature or by manually configuring HA logging)

wlcrs commented 7 months ago

I agree that many of those entities are a duplicate of those that were already included in the integration before 1.4.0 . This is why they were absent in earlier versions, and why I have disabled them by default. However, they can be of interest for people who have 2 batteries and want to monitor them separately. This new battery device now also makes it possible to monitor battery temperature, which is an often requested feature (#631, #541, #74)

Thanks to recent improvements of Home Assistant, there is no performance penalty for having entities which are disabled.

hvorragend commented 7 months ago

But almost all new entities in the new battery device are redundant and already exist in the first battery device.

Or can you even take it to the extreme?

One device for the battery control unit
One device per battery pack (2 in my case)

wlcrs commented 7 months ago

Many of them are indeed redundant in the installations containing only one battery. However, they are potentially of interest for people owning two batteries.

This is the endless battle of trying to balance the feature set of this integration with the perceived complexity. And I have the impression that there will always be complaints 🙃

As already said: I have disabled all of these duplicate entities by default. As long as they are disabled there is no performance impact.

I do note that I will need to clearly document why those "duplicate" sensors exist, as most installations only have one battery installed.

hvorragend commented 7 months ago

Thanks for the explanation. I just wanted to help.

But please let me ask you one more thing:

What do you mean by "one battery"?

I have a Luna control unit with 2x5kWh battery packs.

Is that "one battery" for you then?

The FusionSolar app also differentiates:

ESU No.1

ControlUnit with the various daily and total values
Battery pack 1 with e.g. temperature
Battery pack 2 with e.g. temperature

wlcrs commented 7 months ago

A SUN2000 inverter can be connected to 2 LUNA2000 batteries, each having up to 3 battery packs.

Ivano62 commented 7 months ago

HI, I understood the purpose of the change, but honestly I would think it would be clearer if the "Batteries" device had associated only the common entities, if there were any, to all the batteries installed, whether one or two, while the "Battery 1" and , where present, "Battery 2" had associated the specific entities of these batteries. In any case, the device "Battery 1" has associated entities "sensor.battery_1_none", "sensor.battery_1_none_2", "sensor.battery_1_energy" and "sensor.battery_1_energy_2" which I would ask you to kindly explain. Thank you.

wlcrs commented 7 months ago

@Ivano62 what language is set in your HA? This issue seems to be caused by missing translations.

Ivano62 commented 7 months ago

English.

Thanks

--- Messaggio originale --- Da: Thijs W. @. Data: 8 aprile 2024 07:36:07 A: wlcrs/huawei_solar @. Cc: Ivano62 @., Mention @. Oggetto: Re: [wlcrs/huawei_solar] [Bug]: Double battery device since 1.4.0 alpha 1 (Issue #673)

@Ivano62https://github.com/Ivano62 what language is set in your HA? This issue seems to be caused by missing translations.

— Reply to this email directly, view it on GitHubhttps://github.com/wlcrs/huawei_solar/issues/673#issuecomment-2041900210, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKIRD3OCPRCFBBMVQMJZAZLY4IUEBAVCNFSM6AAAAABFZGNIZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRHEYDAMRRGA. You are receiving this because you were mentioned.Message ID: @.***>

Roving-Ronin commented 7 months ago

Hi Thijs,

Could you please confirm how the 'Batteries' sensor actually calculates the output for its sensors? For example are they: a) the average reading for all batteries and sensors like: Bus Current, Bus Voltage, Charge/Discharge Power, SoC %. b) the sum total of all battery for sensors like: Day Charge, Day Discharge, Total Charge, Total Discharge.

Meaning we can't dispense with needing to create a whole bunch of template sensors to combine battery sensors, like:

  # Provides the combined avaeraged State of Capacity both batteries, as a %.
  - sensor:
    - name: "Batteries - State of Capacity"
      unique_id: batteries_state_of_capacity
      unit_of_measurement: "%"
      device_class: "battery"
      state_class: measurement
      state: >-
        {% set battery_1_soc = states('sensor.battery_state_of_capacity') |float(default=0) %}
        {% set battery_2_soc = states('sensor.battery_state_of_capacity_2') |float(default=0) %}

        {% if states('sensor.battery_state_of_capacity_2') == 'unknown' %}
          {{ battery_1_soc }}
        {% else %}
          {% set count = 0 %}
          {% set sum = 0 %}

          {% if states('sensor.battery_state_of_capacity') != 'unknown' %}
            {% set sum = sum + battery_1_soc %}
            {% set count = count + 1 %}
          {% endif %}

          {% if states('sensor.battery_state_of_capacity_2') != 'unknown' %}
            {% set sum = sum + battery_2_soc %}
            {% set count = count + 1 %}
          {% endif %}

          {% if count > 0 %}
            {{ (sum / count) }}
          {% else %}
            N/A
          {% endif %}
        {% endif %}
      availability: >-
        {{ states('sensor.battery_state_of_capacity')   | is_number
        or states('sensor.battery_state_of_capacity_2') | is_number }}

Or a combined battery_temperature that shows the higher temperature of any connected battery, like:

  - sensor:
    - name: "Batteries - Temperature"
      unique_id: batteries_temperature
      unit_of_measurement: "°C"
      device_class: "temperature"      
      state: >-
        {% set battery_temp_1 = states('sensor.battery_battery_1_temperature') | float(default=0) %}
        {% set battery_temp_2 = states('sensor.battery_battery_2_temperature') | float(default=0) %}
        {{ max(battery_temp_1, battery_temp_2) | round(1) }}
      availability: >-
        {{ states('sensor.battery_battery_1_temperature') != 'unavailable'
        or states('sensor.battery_battery_2_temperature') != 'unavailable' }}

Then we'd just need an 'Inverters' sensor also to be added, for those running multiple inverters and needing a single sensor for each of the inverterS readings ;-)

BTW: The friendly name is "Batteries", but the sensor names are prefixed by 'battery' not 'batteries' ?

"A SUN2000 inverter can be connected to 2 LUNA2000 batteries, each having up to 3 battery packs." Just wait for the LUNA S1 7/14/21kWh, that supports up to 4 in parallel per inverter, so that will also mean up to 12 battery packs in total. ;-)

wlcrs commented 7 months ago

All numbers are retrieved from the inverter and passed to HA without any additional computation. What they precisely mean is something that you have to ask Huawei. I don't know anything more than the modbus PDF's tell me.

wrt sensor ids: those are automatically generated by HA the first time an integration is added. If the naming evolves over time, this ID does not change with it. HA even keeps track of them if you remove and re-install the integration. Users that install v1.4.0 on a HA that has not seen any older versions of this integration, the sensor id will be sensor.batteries_... .

Hadn't seen the LUNA S1 announcement yet. Luckily I did code everything quite nicely so that scaling up the number of battery sensors can be done with just a few lines of code. I will need somebody to provide me with the updated modbus registers PDF when that becomes available.

github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

Roving-Ronin commented 6 months ago

GitHub - Comment to keep this open. Need to update with new modbus registers when made public.

Ivano62 commented 6 months ago

.

wlcrs commented 5 months ago

v1.4.0 is out.

wlcrs / huawei_solar