Closed jackyaz closed 2 years ago
ColinRobbins
commented
2 years ago You are seeing the warning in the logs, because the approach of sleeping in the update code works against the HA design principles. The following would seem a simpler solution to the same problem. https://github.com/HandyHat/ha-hildebrandglow-dcc/pull/135
jackyaz
commented
2 years ago Isn't the polling based on when HA starts the sensor, so if it starts at 12, the poll would be at 12 and 42, rather than around 0 and 30 when the data from Glow updates? I suppose it's not an issue, it just means you could have up to 29 minutes delay in the data (29 and 59 being the worst case)
ColinRobbins
commented
2 years ago I am not sure I have seen an thing that says the update must be at 00 and 30 minutes past the hour? Rather called only every 30 minutes.
As far as I know the new data is only polled when glow.py calls the catchup URL.
If it has to at 00 and 30, the I’d suggest we need to modify the code to integrate with HA automation in some way. Adding a sleep in the code works against the HA design principles. Hence the log warning.
jackyaz
commented
2 years ago https://forum.glowmarkt.com/index.php?p=/discussion/comment/544/#Comment_544
You need it to send once every 30 minutes, but soon after the actual :00 or :30
ColinRobbins
commented
2 years ago If that’s the case, I suggest we need to look at an alternate HA friendly fix. Rather than sleep, maybe return an error of some kind. Then modifying the sensor to reuse old data.
On Tue, 8 Feb 2022 at 19:11, Jack @.***> wrote:
https://forum.glowmarkt.com/index.php?p=/discussion/comment/544/#Comment_544
You need it to send once every 30 minutes, but soon after the actual :00 or :30
— Reply to this email directly, view it on GitHub https://github.com/HandyHat/ha-hildebrandglow-dcc/pull/136#issuecomment-1032965771, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGXZ6KN4G4HKTQC2C5C4TTU2FTENANCNFSM5N3JAUWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you commented.Message ID: @.***>
-- Colin Robbins (from mobile)
jackyaz
commented
2 years ago Fine by me - I'm new to HA (<1 month since I started using it) and wanted to try and help out fix the addon/integration to keep Glow happy and not turn off the free DCC service :)
ColinRobbins
commented
2 years ago Sorry if my comment seems negative - don’t be discouraged, there is a big learning curve on getting started with programming HA, we were all new to it once.
A halfway interim fix, might be to set the retry interval to 10 or 15 minutes. That way it’ll only do a few “wasted” retries, but also reduce the time delay past the hour.
On Tue, 8 Feb 2022 at 20:02, Jack @.***> wrote:
Fine by me - I'm new to HA (<1 month since I started using it) and wanted to try and help out fix the addon/integration to keep Glow happy and not turn off the free DCC service :)
— Reply to this email directly, view it on GitHub https://github.com/HandyHat/ha-hildebrandglow-dcc/pull/136#issuecomment-1033009231, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGXZ6ONTR4UA6AZWPTLAIDU2FZDZANCNFSM5N3JAUWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you commented.Message ID: @.***>
-- Colin Robbins (from mobile)
ColinRobbins
commented
2 years ago Thinking about this more, we’ll have to be careful not to hardwire a specific time. If we hardwire a time, every HA instance using the integration will trigger at the same time - with a potential impact of causing a distributed denial of service attack them. So we have to make sure there remains some element of randomness to it, within a given time window.
jackyaz
commented
2 years ago Indeed - which is what my PR does - in the 2 minute window around 0 and 30 it adds a random sleep between 1 minute and 3 minutes for each call - which hopefully will stagger the calls. Though that's with an ugly sleep approach!
jackyaz
commented
2 years ago Sorry if my comment seems negative - don’t be discouraged, there is a big learning curve on getting started with programming HA, we were all new to it once. A halfway interim fix, might be to set the retry interval to 10 or 15 minutes. That way it’ll only do a few “wasted” retries, but also reduce the time delay past the hour. On Tue, 8 Feb 2022 at 20:02, Jack @.> wrote: Fine by me - I'm new to HA (<1 month since I started using it) and wanted to try and help out fix the addon/integration to keep Glow happy and not turn off the free DCC service :) — Reply to this email directly, view it on GitHub <#136 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGXZ6ONTR4UA6AZWPTLAIDU2FZDZANCNFSM5N3JAUWA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you commented.Message ID: @.> -- Colin Robbins (from mobile)
Not at all - my approach isn't ideal but I couldn't easily see a way to have a sensor update run on a schedule, only an interval. I'd need a better understanding of HA and Sensors overall
ColinRobbins
commented
2 years ago HI, I am testing an alterative fix to this PR and https://github.com/HandyHat/ha-hildebrandglow-dcc/pull/135 (@si458) If it works during the day, I'll make a PR later merging in https://github.com/HandyHat/ha-hildebrandglow-dcc/pull/80 as well.
I've moved your time test from glow.py to sensor.py, and left the SCAN_INTERVAL at 2 minutes. I have modifed the call to the _glow_update function as follows:
async def _glow_update(self, func: Callable) -> None:
"""Get updated data from Glow."""
if self.initialised is True:
minutes = datetime.now().minute
if not ((0 <= minutes <= 2) or (30 <= minutes <= 32)):
# only need to update one per every 30 minutes
# anything else Glow will ignore
return
self.initialised = True
try:
self._state = await self.hass.async_add_executor_job(
func, self.resource["resourceId"]
)This way HA will still make a call to the code evey 2 minutes (backing off https://github.com/HandyHat/ha-hildebrandglow-dcc/pull/135) If it is outside the time window, the update function will not be called, HA will effectively randomise when in the 2 minute window the function gets called, based on when HA started and any execution delays.
The self.initialised part is there, so that when HA first starts, it puts a call in no matter where we are in the time window.
It may seem an overhead in calling the fuction every 2 minutes, but the overhead is tiny (and I'd argue it does if now - only worse as it makes the call out to Glow). Its like adding a condition in an automation.
jackyaz
commented
2 years ago Makes sense to me!
jackyaz
commented
2 years ago I wonder if we should increase the scan interval to say 3 or 4 minutes and widen the window so that API calls can be further spread out?
ColinRobbins
commented
2 years ago I did think about making it 5 minutes, but given the current code is 2 minutes kept it as 2. Not sure - @si458 do you have a view on this?
si458
commented
2 years ago @ColinRobbins sorry been driving to work haha
Personally my PR to change the interval to 30mins has been perfect!
No issues so fair and it's a simple easy fix no messing around with timers etc
HandyHat
commented
2 years ago Closing this as a similar solution is in #138. Thanks @jackyaz!
This is to address Glow's concerns referenced in https://github.com/HandyHat/ha-hildebrandglow-dcc/issues/126
There's probably a better way to do this, but it seems to work. I'm new to HA development so finding my feet a little.
This does cause HA to complain that the sensor update is taking over 10s, but doesn't seem to be a problem