Fix Daikin integration power sensors #51905
Conversation
|
Hi @fredrike ! |
|
Looks good to me! But it is a breaking change as sensor ids change? |
|
@mlemainque should we release a maintainment version of pydaikin without your changes that potentially get merged in HA before this breaking change? |
|
Well I noticed the sensor ids have changed but the only consequence is that the history of data is lost and the widget configuration requires an update. Is it that bad? |
It usually requires a larger release (2021.7?) with breaking changes.. But July is coming so it might not be so bad. |
|
Do you know if it is possible to detect if there is already a sensor in HA with the previous type |
|
No, I don't think that is how it is intended to work.. |
|
I agree that this should be in the next major release then. Correct me if I'm wrong: if we merge into |
It is related to sensors updated hourly with the consumption of the last hour. Thus in this special case, energy and power is exactly the same value as |
|
This makes no sense... Power is always an instantaneous value... are you saying that the HVAC system maintains a constant power value for a full hour ? (that's not how most HVAC devices work) If it's really energy used in the last hour, then the integration should sum all the values and keep track of that sum (quite useful I must say) |
|
Perhaps it reports the average power over the last hour.. |
|
Each daikin AC reports the energy consumed each hour by cool/heat modes on a hourly basis. So there is always one hour lag. In my opinion the sum of the energy is not useful at all for the user. The user wants (at least I want) to know how much energy has been consumed within a given interval of time, not since its birth. The best way to give this information to the user without choosing one interval of time (it could be hourly, daily, weekly, monthly, etc.) is to give an instant power in Note that the |
|
But what does Daikin device provide ? kW or kWh ? power and energy are completely different things... and you cannot take a random kW value and multiply it by 1h to get Energy (expressed in kWh) to do this conversion you must integrate the values (it's not an average) I'm fine with any solution as long as it does not deviate from what the physical device actually provides. |
|
Well the daikin API provides numbers without specifying the unit. The content looks like The daikin official app has a completely different way of displaying these data as HA does: in the app they show the history of the data as an histogram, so in this case it makes sense to use |
|
So the values are Energy 100 kWh per unit It is basically a "utility_meter" which resets every-hour, and IMHO it would make sense to provide an histogram similar to what daikin provides. |
|
Below is an example of what we have today with the version prior to 2021.6. It is exactly the same thing as what is displayed in the daikin official app except it is shifted of one hour. To avoid this shift we should be able to set the value of the sensor one hour in the past, which I don't think is possible with HA. The new version is fixing some glitches such as the one which happened at midnight on the screenshot, and changes the unit from
|
|
I think you should not change from kWh to kW because it simply is not a power measurement. I do think we should debate in https://github.com/home-assistant/architecture for the need to write past values as your use case clear requires that. |
|
This PR now only fixes the wrong attribute name causing all the issues (it has been renamed from "energy" to "power" within pydaikin) and adds the new compressor frequency sensor |
Discussion opened here: home-assistant/architecture#580 |
| if self._device_attribute == ATTR_COOL_ENERGY: | ||
| return round(self._api.device.last_hour_cool_energy_consumption, 3) | ||
| return round(self._api.device.last_hour_cool_power_consumption, 2) |
There was a problem hiding this comment.
this should be reverted back to energy
There was a problem hiding this comment.
Please also update the description
There was a problem hiding this comment.
This is the pydaikin API
There was a problem hiding this comment.
I could push a new pydaikin versioniif you like..
There was a problem hiding this comment.
@mlemainque are you preparing code for the pydaikin lib or should I?
There was a problem hiding this comment.
Is it really necessary @dgomes? :)
There was a problem hiding this comment.
@mlemainque is this enough?
diff --git a/pydaikin/daikin_base.py b/pydaikin/daikin_base.py
index 404721c..cb868f1 100644
--- a/pydaikin/daikin_base.py
+++ b/pydaikin/daikin_base.py
@@ -208,9 +208,9 @@ class Appliance(DaikinPowerMixin): # pylint: disable=too-many-public-methods
)
data.append(('cool_today', self.energy_consumption(ATTR_COOL, TIME_TODAY)))
data.append(('heat_today', self.energy_consumption(ATTR_HEAT, TIME_TODAY)))
- data.append(('total_power', self.current_total_power_consumption))
- data.append(('cool_power', self.last_hour_cool_power_consumption))
- data.append(('heat_power', self.last_hour_heat_power_consumption))
+ data.append(('total_power', self.current_total_energy_consumption))
+ data.append(('cool_power', self.last_hour_cool_energy_consumption))
+ data.append(('heat_power', self.last_hour_heat_energy_consumption))
if file.tell() == 0:
file.write(','.join(k for k, _ in data))
file.write('\n')
@@ -238,9 +238,9 @@ class Appliance(DaikinPowerMixin): # pylint: disable=too-many-public-methods
data.append(
f'heat_today={self.energy_consumption(ATTR_HEAT, TIME_TODAY):.01f}kWh'
)
- data.append(f'total_power={self.current_total_power_consumption:.02f}kW')
- data.append(f'cool_power={self.last_hour_cool_power_consumption:.01f}kW')
- data.append(f'heat_power={self.last_hour_heat_power_consumption:.01f}kW')
+ data.append(f'total_power={self.current_total_energy_consumption:.02f}kW')
+ data.append(f'cool_power={self.last_hour_cool_energy_consumption:.01f}kW')
+ data.append(f'heat_power={self.last_hour_heat_energy_consumption:.01f}kW')
print(' '.join(data))
def represent(self, key):
@@ -349,28 +349,28 @@ class Appliance(DaikinPowerMixin): # pylint: disable=too-many-public-methods
return self._parse_number('shum')
@property
- def current_total_power_consumption(self):
+ def current_total_energy_consumption(self):
"""Return the current total power consumption in kW."""
# We tolerate a 50% delay in consumption measure
- return self.current_power_consumption(
+ return self.current_energy_consumption(
mode=ATTR_TOTAL, exp_diff_time_margin_factor=0.5
)
@property
- def last_hour_cool_power_consumption(self):
+ def last_hour_cool_energy_consumption(self):
"""Return the last hour cool power consumption of a given mode in kW."""
# We tolerate a 5 minutes delay in consumption measure
- return self.current_power_consumption(
+ return self.current_energy_consumption(
mode=ATTR_COOL,
exp_diff_time_value=timedelta(minutes=60),
exp_diff_time_margin_factor=timedelta(minutes=5),
)
@property
- def last_hour_heat_power_consumption(self):
+ def last_hour_heat_energy_consumption(self):
"""Return the last hour heat power consumption of a given mode in kW."""
# We tolerate a 5 minutes margin in consumption measure
- return self.current_power_consumption(
+ return self.current_energy_consumption(
mode=ATTR_HEAT,
exp_diff_time_value=timedelta(minutes=60),
exp_diff_time_margin_factor=timedelta(minutes=5),
diff --git a/pydaikin/power.py b/pydaikin/power.py
index 08ce689..522543c 100644
--- a/pydaikin/power.py
+++ b/pydaikin/power.py
@@ -149,7 +149,7 @@ class DaikinPowerMixin:
_LOGGER.error('Impossible energy consumption measure of %s', mode)
return None
- def current_power_consumption( # pylint: disable=too-many-branches
+ def current_energy_consumption( # pylint: disable=too-many-branches
self,
mode=ATTR_TOTAL,
exp_diff_time_value=None,There was a problem hiding this comment.
The current_power should remain unchanged. https://bitbucket.org/mustang51/pydaikin/pull-requests/45/rename-power-to-energy-consumption
|
@mlemainque new version of pydaikin is pushed now. https://pypi.org/project/pydaikin/2.4.4/ Here is the changelog: https://bitbucket.org/mustang51/pydaikin/branches/compare/v2.4.4%0Dv2.4.3 |
mlemainque
force-pushed
the
feat/new-pydaikin-sensors
branch
2 times, most recently
from
692f7a7
to
271ee01
Compare
|
Since the integration is broken I tagged it for .7 release, but expect an extra pair of eyes reviewing this. |
|
I've tested this new version, everything seems to go as expected. Below are examples of what is measured.
|
mlemainque
changed the title
|
not sure about the middle graph how come that is Watts (Power) but you update based on WattsHour (Energy) ? |
Simply with The power is updated by pydaikin everytime 100 Wh are consumed so the delta energy is constant but not delta time. That way the power is more accurate and is updated faster than it used to be with older versions of pydaikin which were instead measuring the delta energy over a constant delta time (half an hour). |
|
It is by no way more accurate... lets say you have the 100Wh update spaced 1h apart, that does not mean that there was a steady consumption of 100W during an hour.... you could have 200W during 30min and 0W the next 30min.... and it could be even worst! a small peak of 1000W and then nothing (usual behaviour in these devices) My point: don't extrapolate... use directly what the device exposes or what can be actually measured |
You're right this is one limitation, this is an estimation of the power based on the energy consumption. I get your point, you would like to have access to the raw data without any transformation to be as close to what returns the AC as possible. My opinion is that the end-user is more interested in the instant power (would it be an estimation) than the energy consumption as it is easier to interpret. Anyway I don't think this is the place to discuss about that. This PR is about fixing the current behavior of the integration and we're currently discussing about something that is in HA for almost a year. Should not we open a thread in pydaikin repository instead? |
|
I think other than the above discussion, from the HA perspective, this PR looks good. Is it OK to go? |
|
I approved :) |
Proposed change
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: