ldotlopez / ha-ideenergy

ideenergy integration for home-assistant
GNU General Public License v3.0
74 stars 13 forks source link

Create Instant Consumption reading as a sensor #37

Closed luca-angemi closed 1 year ago

luca-angemi commented 1 year ago

Instead of using the current consumption as an attribute of the accumulated consumption sensor, this PR creates the Instant Consumption sensor.

ldotlopez commented 1 year ago

Thanks for your contribution! Before merging it, I have some thoughts about the sensor. I'm not sure about it, not against it, just not sure.

Here's the situation: The "instant measures" and "accumulated measure" are both being updated from the same API endpoint as they are directly read from the service point. DataCoordinator updates this data within the last 10 minutes of each hour to ensure accuracy of statistics and graphs.

We need to figure out a mechanism to request the value and update only the "Instant measure" in the DataCoordinator (since "accumulated measure" should only be updated at the end of each hour). This could possibly be tied with a "button" that makes the request to the DataCoordinator.

The current setup with "Instant measure" being tied with "accumulated measure" might not meet user expectations, as it only gets updated at the end of each hour and not by demand.

Any thoughts?

luca-angemi commented 1 year ago

Thanks for your response! The idea behind this PR was to address the idea in HA that an attribute that updates too often should be considered as a sensor.

About your point, I agree, being tied might not meet user expectations and a button could be a good idea.

Questions: if you are reading from the same service point

https://github.com/ldotlopez/ideenergy/blob/6c88136dc9c579fc1b627b38b234402f941ae43e/ideenergy/client.py#L342

the idea of the button would be to:

In any case (whether only the instant sensor or both sensors are updated) in my opinion the user should be aware that hitting the button too many times might end up with the account being blocked by i-DE.

ldotlopez commented 1 year ago

The idea behind this PR was to address the idea in HA that an attribute that updates too often should be considered as a sensor.

That's a good point. I will recheck this PR with this in mind, I want to test it on my 'production' setup.

the idea of the button would be to:

  • only update the instant sensor AND in any case update both sensors between minute 50 and 59 when you do the accumulated reading?
    • or never update the instant between 50-59 but only when the user hits the button?

The button should call the API to read the service point (the code you pointed out) which will (hopefully) return both instant and accumulated consumption but only update internal data for "instant consumption". Subsequent calls to the API at the 50-59 interval should only update the "accumulated consumption" internal data.

Maybe we are losing some info for each API call but I think this may be the most consistent behavior from the user viewpoint (comments are welcome)

In any case (whether only the instant sensor or both sensors are updated) in my opinion the user should be aware that hitting the button too many times might end up with the account being blocked by i-DE.

Agree, maybe the button should throttle itself and become unavailable for X minutes after begin pressed?

luca-angemi commented 1 year ago

Maybe we are losing some info for each API call but I think this may be the most consistent behavior from the user viewpoint (comments are welcome)

We'll have 24 calls/day for the accumulated plus the ad-hoc calls for instants. Do you know at what point to they get angry and start to block people?

Agree, maybe the button should throttle itself and become unavailable for X minutes after begin pressed?

Nice feature! I like the idea!

luca-angemi commented 1 year ago

BTW, here how it looks with the PR installed on my Production:

image

image

image

I changed all the names on my side and set the value to int instead of float in your code.

ldotlopez commented 1 year ago

We'll have 24 calls/day for the accumulated plus the ad-hoc calls for instants. Do you know at what point to they get angry and start to block people

Sometimes (>50%) direct reading fails, so the 24 hourly calls may be +48 o +72. This is unsolvable, that API method is not reliable. Currently, we retry that call three times in 10 minutes before giving up for another hour (check barrier.py#L178).

However, I have been running this integration for months without issues but some users with multiple contracts (multiple attempts in the 50-59 window) have reported blocks. I hypothesize that bans are time-based, more than two concurrent calls or more than 6 calls in 10 minutes result in a ban.

luca-angemi commented 1 year ago

However, I have been running this integration for months without issues

I've had same as you.

but some users with multiple contracts (multiple attempts in the 50-59 window) have reported blocks. I hypothesize that bans are time-based, more than two concurrent calls or more than 6 calls in 10 minutes result in a ban.

Sounds about right.

ldotlopez commented 1 year ago

I changed all the names on my side and set the value to int instead of float in your code.

Where did you change the type?

ldotlopez commented 1 year ago

PR looks OK, do you want to merge now or do we wait for the 'refresh button' feature?

luca-angemi commented 1 year ago

Here:

https://github.com/ldotlopez/ha-ideenergy/blob/d60b6bde18765d18a7e421151fa36ba2174112fa/custom_components/ideenergy/datacoordinator.py#L220

Altough I think it should be done here

https://github.com/ldotlopez/ideenergy/blob/6c88136dc9c579fc1b627b38b234402f941ae43e/ideenergy/client.py#L344

luca-angemi commented 1 year ago

PR looks OK, do you want to merge now or do we wait for the 'refresh button' feature?

Thanks, I'm ok with either decision!

ldotlopez commented 1 year ago

Hi @luca-angemi, can we retake this PR? Let's integrate the sensor and see how it's going.

I made some changes in main, can you review your changes? Thanks!

luca-angemi commented 1 year ago

Addressed the changes. Should be ok.

mmb25 commented 1 year ago

@luca-angemi @ldotlopez What is the status of this?

This functionality is something I am really interested in.

Are there any plans to merge it with master?