zigpy / zha

Zigbee Home Automation
Apache License 2.0
30 stars 26 forks source link

Remove all rounding of attribute values and entity state #102

Open prahal opened 4 months ago

prahal commented 4 months ago

I had a local patch in my homeassistant docker build to switch ElectricalMeasurement to 3 decimals https://github.com/zigpy/zha/blob/dev/zha/application/platforms/sensor/__init__.py#L555

I added _decimals = 3

 homeassistant/components/zha/sensor.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/homeassistant/components/zha/sensor.py b/homeassistant/components/zha/sensor.py
index 9e98060667..e5664b9c76 100644
--- a/homeassistant/components/zha/sensor.py
+++ b/homeassistant/components/zha/sensor.py
@@ -461,6 +461,7 @@ class Battery(Sensor):
 class ElectricalMeasurement(PollableSensor):
     """Active power measurement."""

+    _decimals = 3
     _use_custom_polling: bool = False
     _attribute_name = "active_power"
     _attr_device_class: SensorDeviceClass = SensorDeviceClass.POWER

This let me get the RMS current from my Nous A1Z (zigbee 3) power plug at the milliamp level (even though it shows power below 3W as 0W).

Could at least the amps be given with 3 decimals in ZHA? ie ElectricalMeasurementRMSCurrent? https://github.com/zigpy/zha/blob/dev/zha/application/platforms/sensor/__init__.py#L635C7-L635C38

Do you know how to add a modified zha library to a local homeassistant docker build?

puddly commented 4 months ago

I think you can choose the number of decimal places to display within Home Assistant's frontend, no?

prahal commented 4 months ago

@puddy no you misunderstood the issue. I can choose the number of "decimals to display" but "the stored by homeassistant decimals" below the "_decimals =1" set in zha are all "0" without this zha change ... I am all for Zha setting 3 decimals to store and letting homeassistant "decimals to display" to the current one decimal and let the user decide if he wants to display more. Though if we could display 3 decimals for current in amps in homeassistant that would be good to, but not a priority. The issue is that currently only one decimal is stored so changing the "decimals to display" is useless.

Should I send a PR with _decimals 3 for the ElectricalMeasurement superclass or only the current derivatives? (is there any other than rms_current?) And how could I test my PR before sending it?

dmulcahey commented 4 months ago

@puddly @TheJulianJES Should we remove all rounding in the lib completely? We can push this concern to the presentation tier... which in this case is HA. Then we won't have requests like this trickle in on a cluster by cluster or attribute by attribute case. thoughts?

puddly commented 4 months ago

Absolutely, rounding doesn't belong in the lib.