Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for rms_current_ph_b/c #324

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
+107 −33
Open

Add support for rms_current_ph_b/c #324

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e2e336
Add support for rms_current_ph_b/c
abmantis Dec 9, 2024
abfd574
Make max attribute name overrideable; Add tests
abmantis Dec 10, 2024
a924e3b
Use attribute for conditional create_platform_entity
abmantis Dec 10, 2024
30bfcbc
Fix cluster handler config test
abmantis Dec 10, 2024
7fab2e3
Merge branch 'dev' into current_3ph
abmantis Dec 23, 2024
a0296f5
Use ph B as base for C to reduce duplicated attrs
abmantis Dec 24, 2024
e3ea5ca
Merge branch 'dev' into current_3ph
abmantis Jan 4, 2025
4447b6f
Merge branch 'dev' of github.com:zigpy/zha into current_3ph
abmantis Jan 6, 2025
6976e49
Revert "Use ph B as base for C to reduce duplicated attrs"
abmantis Jan 6, 2025
7c3627f
Address review suggestions
abmantis Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions tests/test_cluster_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,17 @@ async def poll_control_device_mock(zha_gateway: Gateway) -> Device:
zigpy.zcl.clusters.homeautomation.ElectricalMeasurement.cluster_id,
1,
{
"ac_frequency",
"ac_frequency_max",
"active_power",
"active_power_max",
"apparent_power",
"rms_current",
"rms_current_ph_b",
"rms_current_ph_c",
"rms_current_max",
"rms_current_max_b",
"rms_current_max_c",
"rms_voltage",
"rms_voltage_max",
},
Expand Down
46 changes: 39 additions & 7 deletions tests/test_sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import asyncio
from collections.abc import Awaitable, Callable
from datetime import UTC, datetime
from functools import partial
import math
from typing import Any, Optional
from unittest.mock import AsyncMock, MagicMock
Expand Down Expand Up @@ -298,22 +299,27 @@ async def async_test_em_power_factor(


async def async_test_em_rms_current(
zha_gateway: Gateway, cluster: Cluster, entity: PlatformEntity
current_attrid: int,
current_max_attrid: int,
current_max_attr_name: str,
zha_gateway: Gateway,
cluster: Cluster,
entity: PlatformEntity,
) -> None:
"""Test electrical measurement RMS Current sensor."""

await send_attributes_report(zha_gateway, cluster, {0: 1, 0x0508: 1234})
await send_attributes_report(zha_gateway, cluster, {0: 1, current_attrid: 1234})
assert_state(entity, 1.2, "A")

await send_attributes_report(zha_gateway, cluster, {"ac_current_divisor": 10})
await send_attributes_report(zha_gateway, cluster, {0: 1, 0x0508: 236})
await send_attributes_report(zha_gateway, cluster, {0: 1, current_attrid: 236})
assert_state(entity, 23.6, "A")

await send_attributes_report(zha_gateway, cluster, {0: 1, 0x0508: 1236})
await send_attributes_report(zha_gateway, cluster, {0: 1, current_attrid: 1236})
assert_state(entity, 124, "A")

await send_attributes_report(zha_gateway, cluster, {0: 1, 0x050A: 88})
assert entity.state["rms_current_max"] == 8.8
await send_attributes_report(zha_gateway, cluster, {0: 1, current_max_attrid: 88})
assert entity.state[current_max_attr_name] == 8.8


async def async_test_em_rms_voltage(
Expand Down Expand Up @@ -515,10 +521,32 @@ async def async_test_change_source_timestamp(
(
homeautomation.ElectricalMeasurement.cluster_id,
sensor.ElectricalMeasurementRMSCurrent,
async_test_em_rms_current,
partial(async_test_em_rms_current, 0x0508, 0x050A, "rms_current_max"),
{"ac_current_divisor": 1000, "ac_current_multiplier": 1},
{"active_power", "apparent_power", "rms_voltage"},
),
(
homeautomation.ElectricalMeasurement.cluster_id,
sensor.ElectricalMeasurementRMSCurrentPhB,
partial(async_test_em_rms_current, 0x0908, 0x090A, "rms_current_max_ph_b"),
{
"ac_current_divisor": 1000,
"ac_current_multiplier": 1,
"rms_current_ph_b": 0,
},
{"active_power", "apparent_power", "rms_voltage"},
),
(
homeautomation.ElectricalMeasurement.cluster_id,
sensor.ElectricalMeasurementRMSCurrentPhC,
partial(async_test_em_rms_current, 0x0A08, 0x0A0A, "rms_current_max_ph_c"),
{
"ac_current_divisor": 1000,
"ac_current_multiplier": 1,
"rms_current_ph_c": 0,
},
{"active_power", "apparent_power", "rms_voltage"},
),
(
homeautomation.ElectricalMeasurement.cluster_id,
sensor.ElectricalMeasurementRMSVoltage,
Expand Down Expand Up @@ -1122,7 +1150,11 @@ async def test_elec_measurement_skip_unsupported_attribute(
"active_power_max",
"apparent_power",
"rms_current",
"rms_current_ph_b",
"rms_current_ph_c",
"rms_current_max",
"rms_current_max_ph_b",
"rms_current_max_ph_c",
"rms_voltage",
"rms_voltage_max",
"power_factor",
Expand Down
64 changes: 45 additions & 19 deletions zha/application/platforms/sensor/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ class Sensor(PlatformEntity):
_attr_native_unit_of_measurement: str | None = None
_attr_device_class: SensorDeviceClass | None = None
_attr_state_class: SensorStateClass | None = None
_skip_creation_if_none: bool = False
TheJulianJES marked this conversation as resolved.
Show resolved Hide resolved

@classmethod
def create_platform_entity(
Expand All @@ -183,6 +184,12 @@ def create_platform_entity(
)
return None

if (
cls._skip_creation_if_none
and cluster_handlers[0].cluster.get(cls._attribute_name) is None
):
return None

return cls(unique_id, cluster_handlers, endpoint, device, **kwargs)

def __init__(
Expand Down Expand Up @@ -636,17 +643,22 @@ def __init__(
super().__init__(unique_id, cluster_handlers, endpoint, device, **kwargs)
self._attr_extra_state_attribute_names: set[str] = {
"measurement_type",
f"{self._attribute_name}_max",
self._max_attribute_name,
}

@property
def _max_attribute_name(self) -> str:
"""Return the max attribute name."""
return f"{self._attribute_name}_max"

@property
def state(self) -> dict[str, Any]:
"""Return the state for this sensor."""
response = super().state
if self._cluster_handler.measurement_type is not None:
response["measurement_type"] = self._cluster_handler.measurement_type

max_attr_name = f"{self._attribute_name}_max"
max_attr_name = self._max_attribute_name
if not hasattr(self._cluster_handler.cluster.AttributeDefs, max_attr_name):
return response

Expand Down Expand Up @@ -705,6 +717,35 @@ class ElectricalMeasurementRMSCurrent(PolledElectricalMeasurement):
_div_mul_prefix = "ac_current"


@MULTI_MATCH(cluster_handler_names=CLUSTER_HANDLER_ELECTRICAL_MEASUREMENT)
class ElectricalMeasurementRMSCurrentPhB(ElectricalMeasurementRMSCurrent):
"""RMS current measurement."""

_attribute_name = "rms_current_ph_b"
_unique_id_suffix = "rms_current_ph_b"
_attr_translation_key: str = "rms_current_ph_b"
_skip_creation_if_none = True

@property
def _max_attribute_name(self) -> str:
"""Return the max attribute name."""
return "rms_current_max_ph_b"
TheJulianJES marked this conversation as resolved.
Show resolved Hide resolved


@MULTI_MATCH(cluster_handler_names=CLUSTER_HANDLER_ELECTRICAL_MEASUREMENT)
class ElectricalMeasurementRMSCurrentPhC(ElectricalMeasurementRMSCurrentPhB):
"""RMS current measurement."""

_attribute_name: str = "rms_current_ph_c"
_unique_id_suffix: str = "rms_current_ph_c"
_attr_translation_key: str = "rms_current_ph_c"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this one also need _skip_creation_if_none = True?

Just so I understand correctly:

  1. For existing devices with these attributes you will have to click the "re-configure" button (and probably reload ZHA) to get them to show up.
  2. If you don't do that, there won't be any disabled entities.
  3. For new joins, they just work properly?

Copy link
Contributor Author

@abmantis abmantis Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this one also need _skip_creation_if_none = True?

ElectricalMeasurementRMSCurrentPhC inherits from ElectricalMeasurementRMSCurrentPhB, so no need to repeat the attribute.

Just so I understand correctly:

1. For existing devices with these attributes you will have to click the "re-configure" button (and probably reload ZHA) to get them to show up.

2. If you don't do that, there won't be any disabled entities.

3. For new joins, they just work properly?

It works a bit differently: it adds the entities when the integration gets loaded if there is a value for the attribute already. So, this means that:

  1. If the user already had a 3 phase device, after this update the new entities would just show up (because the ph_b and ph_c attributes already have values on the database).
  2. For new joins the two new entities will not show until the user reboots HA or reloads ZHA. This is because when the device is added the attributes have no values.

We already used this approach for a few other entities like this .

Ideally, ZHA would add entities as soon as the attribute gets a value, but I don't think we support that ATM.
Also we could use exposes_feaures to add the entities for devices that expose a 3_phase feature, instead of waiting for a value on the attr.

I am planning to eventually work on the exposes_feaures feature, but IMO it is better to have support for the 3ph devices now, even if it is not perfect, than delaying it even more.

@property
def _max_attribute_name(self) -> str:
"""Return the max attribute name."""
return "rms_current_max_ph_c"


@MULTI_MATCH(cluster_handler_names=CLUSTER_HANDLER_ELECTRICAL_MEASUREMENT)
class ElectricalMeasurementRMSVoltage(PolledElectricalMeasurement):
"""RMS Voltage measurement."""
Expand Down Expand Up @@ -1116,29 +1157,14 @@ class SmartEnergySummationReceived(PolledSmartEnergySummation):
_unique_id_suffix = "summation_received"
_attr_translation_key: str = "summation_received"

@classmethod
def create_platform_entity(
cls: type[Self],
unique_id: str,
cluster_handlers: list[ClusterHandler],
endpoint: Endpoint,
device: Device,
**kwargs: Any,
) -> Self | None:
"""Entity Factory.
This attribute only started to be initialized in HA 2024.2.0,
""" This attribute only started to be initialized in HA 2024.2.0,
TheJulianJES marked this conversation as resolved.
Show resolved Hide resolved
so the entity would be created on the first HA start after the
upgrade for existing devices, as the initialization to see if
an attribute is unsupported happens later in the background.
To avoid creating unnecessary entities for existing devices,
wait until the attribute was properly initialized once for now.
"""
if cluster_handlers[0].cluster.get(cls._attribute_name) is None:
return None
return super().create_platform_entity(
unique_id, cluster_handlers, endpoint, device, **kwargs
)
_skip_creation_if_none = True


@MULTI_MATCH(cluster_handler_names=CLUSTER_HANDLER_PRESSURE)
Expand Down
16 changes: 16 additions & 0 deletions zha/zigbee/cluster_handlers/homeautomation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,26 @@ class MeasurementType(enum.IntFlag):
attr=ElectricalMeasurement.AttributeDefs.rms_current.name,
config=REPORT_CONFIG_OP,
),
AttrReportConfig(
attr=ElectricalMeasurement.AttributeDefs.rms_current_ph_b.name,
config=REPORT_CONFIG_OP,
),
AttrReportConfig(
attr=ElectricalMeasurement.AttributeDefs.rms_current_ph_c.name,
config=REPORT_CONFIG_OP,
),
AttrReportConfig(
attr=ElectricalMeasurement.AttributeDefs.rms_current_max.name,
config=REPORT_CONFIG_DEFAULT,
),
AttrReportConfig(
attr=ElectricalMeasurement.AttributeDefs.rms_current_max_ph_b.name,
config=REPORT_CONFIG_DEFAULT,
),
AttrReportConfig(
attr=ElectricalMeasurement.AttributeDefs.rms_current_max_ph_c.name,
config=REPORT_CONFIG_DEFAULT,
),
Comment on lines +80 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the comment here, this will also cause four additional attributes to be read at every startup per "smart plug", even if the device doesn't support the attributes.

I guess we can't really avoid this for now and maybe it doesn't really matter overall, but it might be worth looking into only polling attributes if they're not marked as unsupported on startup in the future? Since a couple of versions, the startup attribute polling can at least be disabled completely from the ZHA config UI.
Or expand "quirk id / exposes features" to also cover "rare" devices (even if they conform to ZCL spec) for phase b and c devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there 3 phase devices are not super common, maybe the exposes features quirk would be fine for this.
We could also make it so that for devices that do not have the exposes features flag we only setup attr reporting after we get at least one value (similar to what we have with the _skip_creation_if_none). But I am not sure if a device will send anything before attr reporting, so that may just not work at all.

AttrReportConfig(
attr=ElectricalMeasurement.AttributeDefs.rms_voltage.name,
config=REPORT_CONFIG_OP,
Expand Down
Loading