search

pail23 / stiebel_eltron_isg_component

Stiebel Eltron ISG integration for Home Assistant
MIT License
43 stars 20 forks source link

Bugfix for energy sensor reset (#202) and decreasing values (#153) #222

Closed CSchlipp closed 3 months ago

CSchlipp commented 3 months ago

Coming from the discussion in #202, there was some resistance against using SensorStateClass.TOTAL_INCREASING for the energy meters. An alternative solution is using SensorStateClass.TOTAL in combination with last_reset, as described in https://developers.home-assistant.io/docs/core/entity/sensor/#entities-representing-a-total-amount.

last_reset is set whenever a meter reads 0, telling HA that the meter value hasn't just decreased, but started a new cycle. Any other value will set last_reset to None, making HA ignore this field. As only sensors with SensorStateClass.TOTAL are allowed to have the last_reset property, I had to split the Sensor Class.

This change restores the previous sensors and removes the misbehaving combined (daily+total) sensors, making it backwards compatible to 2024.2.1 again. It will thus allow to keep the history of those sensors of previous versions.

Decreasing values as reported in #153 will be correctly considered in the sensor readings.

This change has been tested locally for 2 days now.

pail23 commented 3 months ago

Many thanks for the contribution. In which regards are the combined sensors misbehaving? They work fine for me and I would like to keep them since they represent the total consumed energy very well.

CSchlipp commented 3 months ago

See https://github.com/pail23/stiebel_eltron_isg_component/issues/202#issuecomment-1991089840 and the screenshots within: The sensor subtracts the daily value end of the day when it is reset to 0 and some time later increases again when the total value is updated by the ISG itself. While I would favor a total sensor that increases over the day instead of once every night as well, that would require a better alignment during those periods to avoid the down&ups that occur with the current implementation.

I'd be fine with updating this branch to not touch the new sensors, if they provide value 🤷🏼‍♂️

pail23 commented 3 months ago

I have implemented some logic to make sure the energy values are only increasing (or resting to 0). There should not be down&ups anymore. Please have a look at the main branch and test it.

CSchlipp commented 3 months ago

This assumes that reduced values as reported in #153 are incorrect and will supress them. I spent a thought on that as well, but found it to be cleaner if the values reported by ISG are taken as the single source of truth without any additional filter added afterwards by the integration.

However, I've rebased & updated the PR to not touch the new sensors at all and only fix the old ones to keep them supported in a backwards compatible way. I would appreciate getting this merged in before any other tests on the new totals to avoid rebasing 😉