search

DarwinsBuddy / WienerNetzeSmartmeter

A home-assistant integration supporting WienerNetze Smartmeters as sensors
144 stars 15 forks source link

refactor(sensors): Merge live + statistics sensor into WNSMSensor #277

Open DarwinsBuddy opened 1 week ago

DarwinsBuddy commented 1 week ago

Hi @tschoerk and @reox

I invested some time to refactor the sensors into one: WNSMSensor It's basically the live sensor, but it's calling additionally an importer, which bears the logic from StatisticsSensor. I used following integration (included in the core integrations) as a blueprint: Elvia

The statistics sensor I sunset and marked deprecated it. After releasing this I'd properly document this in the docs and the release notes and would suggest deleting the statistics sensor i.g. A later release (some time later) would then remove the StatisticsSensor from the code base eventually.

The historical data is exposed to home assistant differently. Instead of sensor.atXXXXXXXXXXXXXXX_statistics it will now be offered under wnsm:atXXXXXXXXXXXXXXX which is valid according to following line https://github.com/home-assistant/core/blob/6f947d2612374cb5870729ddec864e2fd59200ef/homeassistant/components/recorder/statistics.py#L326

Following things are to be considered:

I'd really appreciate your input on this. Be careful, you might have to do a data wipe if you want to switch between this branch and main

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.22%. Comparing base (119ba21) to head (c038a71).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #277 +/- ## ======================================= Coverage 86.22% 86.22% ======================================= Files 5 5 Lines 305 305 ======================================= Hits 263 263 Misses 42 42 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

tschoerk commented 1 week ago

Wow very nice work! :) Let me get on to testing this for a couple days, with all the different data quirks we know can happen and get back to you!

reox commented 1 week ago

Nice! Unfortunately, I'm very short on time right now and probably cannot do a full review in the next weeks :( Just one question: Does it now handle states + statistics in a sane way, i.e., is this now supported by HA? Back then, when we decided to have two sensors, we had the problem that having a state would result in broken statistics.

DarwinsBuddy commented 1 week ago

@reox yes, I guess so. Since we do not necessarily need an explicit sensor to save something with recorder. We just have to use the domain of our integration (wnsm) and update the stats there on update of our sensor.

This means that we have 2 histories produced.

So only one sensor, but two histories, hence no need for a separate sensor that shows no status whatsoever and would conflict with the dates imported (since they wrote into the same recorder stream)

DarwinsBuddy commented 3 days ago

@reox @tschoerk Any update on your findings? I'd like to release a new version containing a fix for #279 and considering adding all of this to it (iff there is no second thoughts from your side)

reox commented 2 days ago

@DarwinsBuddy sorry, still hadn't had the time to look at the code properly :( But I guess it's fine? Not having the two entities sounds like a good idea!

tschoerk commented 2 days ago

Sorry for getting back so late. The API seemed to have a lot of problems in the last week, since I got a ton of error messages regarding server problems (Could not obtain API, No response, etc.).

What I did notice though is that the update of data takes a lot longer than the old method. I get the home assistant warning Update of sensor is taking over 10 seconds every hour for each sensor. I looked through the code and noticed two things which probably could be improved upon:

  1. For both updates the whole login process is done twice. So it logins for the meter readings and afterwards another login for the statistics update, although they happen basically in the same process.
  2. Both meter readings and statistics are checked now every hour, even if the newest data from last day was already received. For meter readings there was never a method implemented to prevent that but the old statistics update always waited 24 hours before starting with the calls (though this led to the import bug with the big spikes).

Both could save a ton of API calls which would speed up the process and lead to fewer errors overall (maybe some server errors I got were due to rate limiting, but I can't test that).

Regarding the NACHTSTROM fix, good idea with the error message just showing the label, so we can fix it faster in the future.

reox commented 2 days ago

For both updates the whole login process is done twice. So it logins for the meter readings and afterwards another login for the statistics update, although they happen basically in the same process.

I remember that the login gives a token and a refresh token. I believe it should be possible to store both and call the refresh every now and then, to not run the full login process again. I think last time we discussed that, the problem was that there was no good way to store such information in home assistant?

tschoerk commented 2 days ago

For both updates the whole login process is done twice. So it logins for the meter readings and afterwards another login for the statistics update, although they happen basically in the same process.

I remember that the login gives a token and a refresh token. I believe it should be possible to store both and call the refresh every now and then, to not run the full login process again. I think last time we discussed that, the problem was that there was no good way to store such information in home assistant?

Yeah I read that discussion and I think they are only valid for 30 minutes and would need a seperate process to refresh, which is not possible in HA (at least at the time of your discussion).

In this case I would just use the tokens which were already provided from the login in the meter reading update and pass the AsyncSmartmeter object to the importer. I will comment on the lines, so you know what I mean.

DarwinsBuddy commented 2 days ago

Thank you so much for your feedback, That's very insightful. I missed these points as I refactored. As soon as I have time I'll try to implement the mentioned improvements.