search

home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
71.14k stars 29.82k forks source link

Utility Meter doesn't account for HA downtime #20931

Closed RezzZ closed 5 years ago

RezzZ commented 5 years ago
Home Assistant release with the issue: arch armv7l
dev false
docker true
hassio false
os_name Linux
python_version 3.6.6
timezone Europe/Amsterdam
version 0.87.0
virtualenv false

Last working Home Assistant release (if known):

Operating environment (Hass.io/Docker/Windows/etc.): Raspbian stretch, docker

Component/platform: Utility Meter

Description of problem: I just rebooted HA (with some time in between updating raspbian) and my power and gas meters didn't account for the downtime and just continued counting as if nothing happened. I still have my old sensors that records the utility sensors state at midnight and calculates daily usage based on the current state minus the midnight state. The new utility meter doesn't seem to do this yet.

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

Traceback (if applicable):

Additional information:

dgomes commented 5 years ago

The utility_meter sensors are constantly updated (actually whenever the source sensor is updated).

The sensors are a sum of the differences of the source (current source value - previous source value).

This means that if the source does not have a previous value, it will not be able to calculate the difference. Some sensors store the previous state (and value) some others don't...

My understanding is that your source sensor does not store it's previous value. Which source are you using ?

RezzZ commented 5 years ago

i have set it up exactly like in the instructions for DSMR so three separate sensors for high/low/gas per period type. I will test some more with downtime of HA. see if I can reproduce it.

daily_power_offpeak:
  source: sensor.power_consumption_low
  cycle: daily
daily_power_peak:
  source: sensor.power_consumption_normal
  cycle: daily
daily_gas:
  source: sensor.gas_consumption
  cycle: daily
monthly_power_offpeak:
  source: sensor.power_consumption_low
  cycle: monthly
monthly_power_peak:
  source: sensor.power_consumption_normal
  cycle: monthly
monthly_gas:
  source: sensor.gas_consumption
  cycle: monthly
yearly_power_offpeak:
  source: sensor.power_consumption_low
  cycle: yearly
yearly_power_peak:
  source: sensor.power_consumption_normal
  cycle: yearly
yearly_gas:
  source: sensor.gas_consumption
  cycle: yearly
dgomes commented 5 years ago

Just checked DSMR code https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/sensor/dsmr.py

It does not make use of RestoreState to get the previous value to HA shutdown. Someone should change the code of the DSMR to add that functionality, and that will fix this issue.

Unfortunately I do not have access to such device to test :( so I can't fix this issue.

bouwew commented 5 years ago

To continue our discussion in this topic: https://community.home-assistant.io/t/utility-meter-keep-values-after-restart/97712/26

As I see it, the algorithm you describe in the above comment is not the best choice, it can only work when the monitored sensor is always there. When it disappears for a while, for whatever reason, the utility-meter sensor-values will be off/wrong. I think the algorithm of the Utility-Meter sensor should be like this: it must save (to a non-volatile storage medium) the value of the monitored sensor at the end of each period (daily, weekly, monthly, yearly), that is the reference value that then can be used to determine the value of the utility-meter sensor (= present value of monitored sensor - saved value of monitored sensor at end of last period). Nothing else needs to be saved, only this reference value. This save-action must always be successful, so the server should not be offline/rebooting, etc during the end of the chosen monitor period(s). This is the responsibility of the user. When anything happens in-between, it does not matter because the utility-meter sensor can always calculate the correct value when the monitored sensor reappears, because the reference-value is still there.

Do you understand what I mean?

dgomes commented 5 years ago

You are making assumptions on the source sensor, you are assuming the source sensor doesn't get reset during the day and that it is available at the period shift.

You are lucky and have smart meter, but others are using this with DIY meters (that seldomly reset) others are using OCR sensors (that some times take bad readings), some meter IoT devices (power plugs/lightbulbs) that can have their counters reset, the list can go on. Point being that the utility_meter implementation is a best effort approach to cover all these cases.

Let me comment your statements:

Sources should be the ones responsible for assuring reliability of the readings.

bouwew commented 5 years ago

Thank you for your feedback, I always enjoy a technical discussion :)

I will just give you my general thought about this topic: if problems arise, they need to be solved at the source. Anything you do "'after" will only cause a mess. You should not try to solve the problems a source is having in the Utility-Meter source-code. If you implement a fix for one, you will break something for another.

About your comments:

I understand what you want to achieve but I don't like the result: inaccurate sensor values. The method I propose can cause the loss of one full period but the chance this happens is very small. You could find a way in the middle by keeping the logging ongoing and use the "best" reference value: either the value stored at the end of the period or either the first available value since the end of period. If no trouble happens at one "end of period", the value provided by the utility-meter sensor is always correct, when the monitored sensor provides data. If something happens during the period, that data will be lost but the utility-meter sensor will still show the correct value once the sensor returns. On the odd chance that something happens during the end of period, the utility-meter sensor can "recover" by using a first obtained value from the monitored sensor when the "something happens"is over.

If you implement the algorithm like this, you will fix the error reported in this topic, you will make me happy and you will end up with the best of both worlds and an improved sensor, in my opinion ;)

dgomes commented 5 years ago

Your first paragraph pretty much sums my position. The issue you are facing is caused by the DSMR sensor platform which does not store previous states. I would gladly fix that platform, but I don't have such smart meter to test the code.

If anyone is available for a long ping pong of testing code, please join me on discord (I use the same handle)

bouwew commented 5 years ago

I'm sorry, I don't understand what you are getting at. I think I miss some context: you write: "is caused by the DSMR sensor platform which does not store previous states". Why do you want the DSMR sensor platform to store (and provide I guess?) the previous states? And what do you mean by "previous states" exactly?

dgomes commented 5 years ago

Ok :) you brought the discussion to GitHub and I got all technical.

The utility_meter consumes the states provided by the source sensor https://github.com/home-assistant/home-assistant/blob/6b46ed850b48e4f9adfbfc396b59f1bebc2a538b/homeassistant/components/utility_meter/sensor.py#L91

What happens when a sensor starts is that the old_state is None, and therefore the utility_sensor misses that "beat". If the sensor implements RestoreState

https://github.com/home-assistant/home-assistant/blob/5580bec1d3df5e40a366a38ee33203232714bfbc/homeassistant/helpers/restore_state.py

then the old_state will be the one stored, and no information is lost.

bouwew commented 5 years ago

Oh, now it getting very technical :)

I have a electronics and system engineering/architecture background, so I understand most of the hardware and high-level software stuff (at functional, algorithm level) but this is getting maybe too technical for me.

Anyway, I think I understand what the restore_state does and why you want to use it, as your algo can be simple if the previous value is available.

But I wonder, will the DSMR and equivalent sensors ever implement this? I think not. Because these (software) sensors are just a means to get the kWh-values obtained from an external device that, among others, as this is the topic of interest of the Utility Meter, provides the result of a kWh-counter at regular intervals. And this counter normally never looses its value, never resets by itself, etc., and because of that these software sensors do not implement the restore_state. When one or some more values are "lost", it does not matter, when valid values are coming back in, they always have the correct value for the moment they become available. Because the device that the data is obtained from, keeps track of them.

So, back to the point I'm trying to make, and as us Dutch people are known for :) , I will be straightforward:

Now, I've written enough about this topic. I will not bother you any more with my rantings. Thank you and good luck with your future developments for Home Assistant :)

RezzZ commented 5 years ago

@bouwew I think your missing the point that the dsmr.py file is part of homeassistant therefor we are in control of changing it and implementing the restorestate function. As you already mentioned, we should tackle this issue at the source and the dsmr.py is our source in this case. Instead of changing this utility_meter, which should just record specific values, we should improve the dsmr python script which read and stores values from our DSMR (correct me if I'm wrong @dgomes .

In any case, I will contact you soon via discord @dgomes

bouwew commented 5 years ago

Ok, please help me understand how the Utility Meter works. I see in the code that a "diff" is determined. I guess this is the difference between two consecutively received values? What is done with these diffs? They are all added up from the start of the reset until the end of the period is reached? If correct, what then to do with the restore_state value?

bouwew commented 5 years ago

Also, I'm thinking, then it is fixed for the DSMR sensor. But what about the many other sensors out there that do not and probably will never implement this?

In my case, I have created a sensor that receives the value from a script (executing a curl-command and then some filtering the get the actual value). Will you "fix" that particular type of sensor then too?

dgomes commented 5 years ago

@bouwew its a question of separation of concerns. The utility meter is an abstract entity that should not be aware of the inconsistencies of the source sensor.

In your case, your script must do the restoration of state on load, if it is similar to your other script (using sensor.template initialized to 0) then you have a problem. I advise you to use an MQTT sensor with the retain flag, it's an easy trick that will assure state restore without any need for extra code. You just need to publish from your curl script into the right MQTT topic.

bouwew commented 5 years ago

I still don't understand how this all is supposed to work. Can you please explain this to me? See my question 3 posts up.

dgomes commented 5 years ago

I've done my best to explain in detail how it works in https://github.com/home-assistant/home-assistant/issues/20931#issuecomment-462897239

You describe correctly how it works, the issue with reset is that 1 diff will be lost (because the first value after reset will not have a previous value against which to calc a diff) and that is caused by the source sensor not providing the previous value (which should have been restored by the source)

bouwew commented 5 years ago

Ok, it's clear now, thank you. And I'm starting to see the big picture as well after digging a bit in the code of restore_state.py.

bouwew commented 5 years ago

Also, thank you for your suggestion to use the MQTT sensor. But, I just checked the HA documentation, and the source-code, it does not have the retain flag? Or is that in the works?

dgomes commented 5 years ago

the retain flag is to be set by your script.

It's an MQTT flag in the publish message (it's not HA related), by using MQTT your delegating to the MQTT broker the restore_state functionality. It will be the MQTT broker storing the state while HA reboots

bouwew commented 5 years ago

Ok, thanks for advising, I think I will have to do a lot of googling during the weekend...

tollerm commented 5 years ago

Today my Pi that collects the data from the DSMR sensor was offline during 4 hours and as result of this accident the utility-meter lost 5 kWh of the energy. Trying to find the reason for this phenomenon, I spent a few hours digging in my configuration file and python code of this component, and finally arrived to this conversation. If I understood this thread correctly, the utility-meter is doing the following: It differentiates the integral of a function using the saved state, than integrates again to get this integral. Clearly, with this algorithm it can work correctly only if the input function is continuous and differentiable that is not the case. It is entirely possible to miss short pulses of high energy consumption. That's why all energy consumption meters have analog integrators! In my opinion the algorithm used in the utility meter is completely wrong and need to be changed.

dgomes commented 5 years ago

@tollerm you are missing the point of what the utility_meter is and where it is running... I've extensively explained how it works, if I still can't make my self clear, I'm sorry.

You can always improve HA by submitting a pull request.

tollerm commented 5 years ago

I clearly miss the point because I do not expect that my power consumption is zero when my sensor is offline :) What I expect is that the utility-meter simply subtracts from the current value of a sensor the value of the same sensor at some specified time. And that specified time is changed every day for daily measurements and every month for monthly measurements. In current implementation the utility-meter is doing something completely different. As result it produces wrong values if there was not communication with the source sensor, while the information is not lost, because the source sensor integrates continuously the consumed property such as power, volume, mass etc. Thus the utility-meter is not useful for me.

tiaanv commented 5 years ago

@dgomes I am using the utility meter extensively, and had an issue with one of them not adding up values quite correctly, and came across this thread.

I fully understand the operation of the utility meter, and think as it stands it is perfectly functional, however I do tend to agree with some comments above. The biggest issue being that the way the meter works at the moment that any "glitch" in the source sensor (old_state vs new_state) will render useless data in the meter.

I took a look at the source for the utility meter sensor.py

diff = Decimal(new_state.state) - Decimal(old_state.state) could potentially look something like: diff = Decimal(new_state.state) - Decimal(self._last_source_state) where last_source_state is captured after the source is read

The sensor would have a much wider use-case if it did not depend on the old_state value of the source to calculate the diff. Perhaps a potential enhancement would be to maintain the "old_state" internally, and use that to calculate the diff. That way the dependency on the source to have old_state is removed.

It seems to me that this would have virtually no negative effect, and satisfy the requirements above.

I am happy to contribute to the project, from a development perspective, and will do so in time, but I am still noob in HASS terms, and would like to get to know a bit more...

I think this can fall in the "enhancement" category rather than "bug" @tollerm @bouwew

dgomes commented 5 years ago

@tiaanv

You proposal (which has already been pitched) just shifts the fix from the misbehaving HA component to the utility_meter. If a new component shows up, that it too depends on old state, you will need to replicate this "fix"... The right way is to fix this at the source (in the misbehaving components).

If we were discussing an external component, I would be fine with this proposal, but we are discussing an issue with HA components. We should fix and improve those, not have internal code that routes around problems in our internal code.

tiaanv commented 5 years ago

@dgomes

After spending a bit of time in the code, and looking at how it works, I have now come to the conclusion that the problem might actually be bigger. A component not having an old_state is remediable, as you mentioned by addressing the functionality of the sensor in it's code, and should definitely be addressed in that way.

The biggest problem with the way it is written now, is that it expects well behaved data ALL THE TIME. This simply cannot be guaranteed, because even if the code for a sensor (e.g. mqtt) lies within control of HA devs, the actual data that feeds it does NOT.

It is a BAD idea to calculate a diff on each sensor update, and add that to the total.

If we were to have a single bad/out of sequence value, greater that the current value, the utility meter will not only calculate and report an incorrect total, it will also stop working from that point onward. Whilst I can agree, that the sensor that feeds this BAD data is to blame, I firmly believe in developing robust code as far as possible, and I am of the opinion this is not, as has been proven.

The utility_meter component, as it is, can surely work for some cases, where the sensor input data is robust, but I, for one, can't use it, since I don't have control of the input sensor data, and it does sometimes misbehave. Pity.

I think a more robust method is to "snapshot" the opening balance form the source, (on init, or after reset), and simple set the state to the difference between that snapshot, and the current value. this would make it rather immune to a single misbehaving value. I have modified the existing meter code for my own use, and am testing it as a custom component now. If it proves to be more reliable, I will propose this as an alternative in the future.

Final note. I appreciate the level of commitment and effort that you and the rest of the contributors are doing. The mere fact that there is a utility_meter component is super awesome. Thank you for even taking the time to respond to these comments. So often the users (including myself) sit on the other side, and have this EXPECTATION. Mine is rather admiration!

dgomes commented 5 years ago

If the "source of the source" misbehaves my recommendation is that you add a filter sensor in-between the source and the utility_meter.

https://www.home-assistant.io/components/sensor.filter/

Continuous update of the utility meter is something useful for many users (myself included)

tiaanv commented 5 years ago

If the "source of the source" misbehaves my recommendation is that you add a filter sensor in-between the source and the utility_meter.

https://www.home-assistant.io/components/sensor.filter/

Continuous update of the utility meter is something useful for many users (myself included)

Thx.

tiaanv commented 5 years ago

FWIW here is what I had in mind. It still updates in realtime, just uses a different method to do the calculation.

utility_meter.zip

to use the alternative method, set optional config value mode to "alt"

  pv_prod_meter_alt:
    source: sensor.pv_prod
    cycle: daily
    mode: alt
    tariffs:
      - normal
dgomes commented 5 years ago

I've debated this solution in other threads. If the source sensor resets meanwhile you loose everything.

tiaanv commented 5 years ago

I agree.. that is why the default method is still there. You choose the method that best suits your needs. I really cannot understand why you simply REFUSE to see the counter argument.

dgomes commented 5 years ago

I've been trying to participate in all these threads to improve the component and attend the needs of the broadest audience possible. I've actually incorporated several additions into utility_meter from all these threads, from proposals made by other users.

What I'm refusing here it to monkey patch the utility_meter with less then optimal solutions, when better solutions can be achieved outside of utility_meter.

I see your problem, I don't see the solution being utility_meter.

tiaanv commented 5 years ago

@dgomes

No problem. Thx for the tutoring us monkeys.

Anyone who would like an less than optimal "utility_meter" I will refine and maintain the code here: https://github.com/tiaanv/hass-utility_meter_alternative

dgomes commented 5 years ago

https://en.m.wikipedia.org/wiki/Monkey_patch

tiaanv commented 5 years ago

https://en.m.wikipedia.org/wiki/Monkey_patch

Thx again. 100% appropriate. :grin:

dgomes commented 5 years ago

Just making it clear I was not offending anyone

tiaanv commented 5 years ago

Just making it clear I was not offending anyone

Took it lightly.

As mentioned you are 100% right. My "solution" is not for everyone., I'm not suggesting it should be mainstream, and I think it's fair to put it in the monkey_patch category.

RezzZ commented 5 years ago

@tiaanv You can always share your solution as a custom/modified component for others to use (if they require the special adjustments). Just share it on git 👍

tiaanv commented 5 years ago

@tiaanv You can always share your solution as a custom/modified component for others to use (if they require the special adjustments). Just share it on git 👍

I already have. scroll up ;)

RezzZ commented 5 years ago

Yes, I noticed that zip file but a dedicated git for just that component unzipped might be better for the community and future updates?

edit: I had to scroll even more up. found your git page:

Anyone who would like an less than optimal "utility_meter" I will refine and maintain the code here: https://github.com/tiaanv/hass-utility_meter_alternative

tiaanv commented 4 years ago

FWIW. Anyone still reading this thread.

A year later, with some additional wisdom and experience. I do not recommend using my alternative approach. Although it does achieve its intention, it has its own set of drawbacks. More importantly. @dgomes 's point of view is the RIGHT way to approach this. The "limitations" of the utility_meter is based on flawed data, not a flawed component. If we were to try and address every "bad data" scenario, we'd have bloated code in EVERY component.

There are GOOD ways of creating a buffer component that will "fix" the data before the utility meter does its job.