Core: ZHA: Innr smart plug SP 120 reports wrong values for active power

[robertlandes]

Home Assistant release with the issue:

0.103.0

Last working Home Assistant release (if known):
n/a

Operating environment (Hass.io/Docker/Windows/etc.):

• Hass.io (32bit)
• Raspberry PI 4 (4 GB version)
• Dresden Elektonik Conbee II Zigbee adapter (USB with latest available firmware)

Integration:

https://www.home-assistant.io/integrations/zha/

Description of problem:
Since recently I was using the Conbee adapter throught deCONZ containter add-on and everthing was working as expected. I decided to migrate to native ZHA integration (zigpy-deconz) to take advantage of the capability using light switch buttons (remotes) directly within automations, than using the somewhat awkward usage of deconz_events.

I have several Innr SP 120 smart plugs which measure current power, total power, current voltage and current. However since moving them to native ZHA integration the values for i.e. power are divided by 10 so 34 Watts become 3.4 Watts in HA.

Looking at ElectricalMeasurement (Endpoint id: 1, Id: 0x0b04, Type: in) and there at active_power (id: 0x050b)the value is correct. I checked on ac_power_divisor (id: 0x0605) and ac_power_multiplier (id: 0x0604) which both return "None".

Trying to set above mentioned attributes doesn't work or at least not change anything.

This might be related to #15958

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

Using ZHA through integration. (Not sure what part would be required here)
This is from _core.config_entries_

            {
                "connection_class": "local_push",
                "data": {
                    "radio_type": "deconz",
                    "usb_path": "/dev/ttyACM0"
                },
                "domain": "zha",
                "entry_id": "ad57da9229804ee688ade46e301a18cd",
                "options": {},
                "source": "user",
                "system_options": {
                    "disable_new_entities": false
                },
                "title": "/dev/ttyACM0",
                "version": 1
            }

Traceback (if applicable):

Additional information:

[probot-home-assistant[bot]]

Hey there @dmulcahey, @adminiuga, mind taking a look at this issue as its been labeled with a integration (zha) you are listed as a codeowner for? Thanks!

[presslab-us]

I believe this is the errant code in question.
https://github.com/home-assistant/home-assistant/blob/d6f317c0a988a6fdc492fbcec55c59b479533ecc/homeassistant/components/zha/sensor.py#L87-L91

[Adminiuga]

Yeah, cause those were the __multiplier__ and __divisor__ for the devices the code was tested when it was initially introduced.
We need to take into account the multiplier and divisor for that cluster. I can see two approaches:

1. modify the channel , so it sends the "cooked" value for the attribute update
2. Have a specific "sensor" class for active power channel which initializes with correct multiplier and divisor?

To me, the 1st approach is "meh" and I'm learning toward the 2nd one. I'm actually facing the same challenges with zha thermostats. I just have no ideas how to make it work with different vendors using a single ZHA entity class.

[dmulcahey]

We could mirror this: https://github.com/home-assistant/home-assistant/blob/d6f317c0a988a6fdc492fbcec55c59b479533ecc/homeassistant/components/zha/core/channels/smartenergy.py#L77

[dmulcahey]

To me, the 1st approach is "meh" and I'm learning toward the 2nd one.

Can you elaborate on the reasoning? In this case what do we gain? The way metering is implemented handles this exact case cleanly. What benefits do we get from a separate sensor class?

[robertlandes]

The problem I see here is that, what to "assume" as the default value for multiplier/divisor (and possibly a lot of other required values) taking into account, that there seems to be a good amount of devices reporting back "none".

I had a look at deCONZ and it seems that they implemented "workarounds" on a device basis similar to the zigpy quirks and I guess we might end up needing the same. That's the general problem with Zigbee devices or better the vast amount of manufactures that just don't stick to the standards.

EDIT:
ElectricalMeasurement Cluster
https://github.com/dresden-elektronik/deconz-rest-plugin/blob/dafa799f9a09080db9e167f4ae37b3fb8f157f95/bindings.cpp#L1195

SP 120 specific
https://github.com/dresden-elektronik/deconz-rest-plugin/blob/dafa799f9a09080db9e167f4ae37b3fb8f157f95/bindings.cpp#L1231

[presslab-us]

While I do agree that Zigbee has testing and compliance issues I don't think that's the case here.

The multiplier and divisor attributes look to be optional in the spec. So None is perfectly valid, and should be handled something like how I did it here.

https://github.com/home-assistant/home-assistant/blob/d6f317c0a988a6fdc492fbcec55c59b479533ecc/homeassistant/components/zha/core/channels/smartenergy.py#L139-L142

[presslab-us]

The way metering is implemented handles this exact case cleanly. What benefits do we get from a separate sensor class?

This part is a bit "not clean" perhaps.
https://github.com/home-assistant/home-assistant/blob/d6f317c0a988a6fdc492fbcec55c59b479533ecc/homeassistant/components/zha/sensor.py#L224-L231

But maybe to fix that all channels (or the base class) should have a formatter_function.

[Adminiuga]

We could mirror this:
home-assistant/homeassistant/components/zha/core/channels/smartenergy.py

I was looking at this issue and was like: there was a fix for this. I clearly remember seeing a PR which was taking into consideration the multiplier and divisor. But that was for metering sensor.

To me, the 1st approach is "meh" and I'm learning toward the 2nd one.

Can you elaborate on the reasoning? In this case what do we gain? The way metering is implemented handles this exact case cleanly. What benefits do we get from a separate sensor class?

Just for consistency. e.g. we're "cooking" the values for temperature/humidity in ZHA entity itself -- channels just sends the raw data, but for metering sensor we're processing the raw value in channel itself and send to zha entity the normalized value. I understand why we did it and it gets the job done. With a "registry", I think we could be more flexible and cleaner. I'll try to mock something to demonstrate.
Something, which allows us to get away from formatter registry, unit_of_measurement registry to

@entity_registry.register("temperature_channel")
class TempSensor(ZHASensor):
    unit_of_measurement = "*C"
    def formatter(value):
        return round(value / 100, 1)

@entity_registry.register("humidity_channel")
class HumiditySensor(ZHASensor):
    unit_of_measurement = "%"
    def formatter(value):
        return round(value / 100, 1)

just an example. actual registration could be much more flexible, ie picking not only channel names, but model/manufacturer, channel_id, etc

[Adminiuga]

@dmulcahey here's PoC https://github.com/Adminiuga/home-assistant/commits/ac/zha-entity-registry

Just need to fix the device tracker battery formatter.
I also think that with a slight modification this could be adapted for "device discovery" Just need to write tests to capture the results of current discovery process to ensure I don't break anything.

[Adminiuga]

Electrical measurement cluster may require to register a few sensors. On my thermostat i'm getting a reading for rms_voltage, rms_current, active_power so we should create a sensor for each supported attribute on that cluster. I see no clean way to implement it currently. A work around to implement it as device state attributes, but then user have to create template sensors, meh.

[Adminiuga]

@robertlandes do you get any readings for power_multiplier attribute id 0x0402 and power_divisor attribute id 0x0403?

[robertlandes]

@Adminiuga I wanted to post values I am getting days ago, but just found the time to have a look:

Both power_multiplier attribute id 0x0402 and power_divisor attribute id 0x0403 return None

I actually also noticed that ZHA is not reporting total consumption in kW/h at all, so I had a look which attributes are actually returning any values:

ElectricalMeasurement (Endpoint id: 1, Id: 0x0b04, Type: in)

• rms_current attribute id 0x0508
  In my test case returned _5843_ and that reflects _5.843 A_ or _5843 mA_
• rms_voltage attribute id 0x0505
  In my test case returned _225_ and that reflects _225 V_
• measurement_type attribute id: 0x0000
  In my test case returned _0_
• active_power attribute id: 0x050b
  In my test case returned _1310_ and that reflects _1310 W_

Metering (Endpoint id: 1, Id: 0x0702, Type: in)

• current_summ_delivered attribute id: 0x0000
  In my test case returned _1862_ and I suppose that reflects _1862 Wh_, but not sure
• metering_device_type attribute id: 0x0306
  In my test case returned _0_
• status attribute id: 0x0200
  In my test case returned _0_
• summa_formatting attribute id: 0x0303
  In my test case returned _42_
• unit_of_measure attribute id: 0x0300
  In my test case returned _0_

All other attributes within above mentioned two clusters returned None
Hope this helps and if you need any additional info, please let me know.

[Adminiuga]

Yeah, currently we only support/expose the 'active_power' attribute atm. I'm thinking about some refactoring around device discovery so we could have multiple entities per zha channel.

[Adminiuga]

So #30130 should take care of the active_power and for other attributes I need to try a few ideas around ZHA device initialization.

[robertlandes]

@Adminiuga I am going to test from dev branch and report back

[Adminiuga]

yep. Reopen this if you run into any issues. Now it defaults to 1/1 for multiplier and divisor if it cannot read those from the device.
Maybe this should be marked as a breaking change.

[robertlandes]

@Adminiuga I can confirm it works as expected. Active power shows now with correct values. Total consumption still not working, but that is something for another issue/day.

Thanks!

[juhanjuku]

What is 'smartenergy_metering' which is exposed for SP120, but showing always 'unknown'?

[presslab-us]

Looks like that is cluster 0x0702:
https://github.com/zigpy/zigpy/blob/dev/zigpy/zcl/clusters/smartenergy.py#L21-L23
The ZCL spec describes that as "Commands and attributes for reporting metering data"
For it to show unknown you must be querying an attribute? What attribute are you looking at?

[juhanjuku]

Actually I'm looking for consumption, kWh. After adding device three items showed up and I hoped smartenergy_metering is kWh.

[jopvanraaij]

I was also hoping to get some value instead of 'unknown' for smartenergy_metering. Is there some manual on how to debug this? I assume it is just a link to the correct attribute (and maybe some calculation, like in the PR which resolved the measurement above)?

The electrical_measurement is working fine.