BJReplay / ha-solcast-solar

Solcast Integration for Home Assistant
Apache License 2.0
160 stars 31 forks source link

Error 429 connecting to solcast on HA restart, zapped all solcast sensors #12

Closed gcoan closed 2 months ago

gcoan commented 3 months ago

Describe the bug I restarted Home Assistant and when the Solcast integration started up it got an error 429 I believe from retrieving the rooftop sites info: IMG_1030

This caused the rooftop sensor in HA to change to Unknown: IMG_1032

And ALL the solcast PV forecast sensors to change to zero: IMG_1031

Fortunately I spotted this pretty quickly and was able to resolve it by reloading the Solcast integration and all the sensors populated correctly

To Reproduce Restart HA and if Solcast is busy its likely that the same will occur

Expected behavior On HA start, if an error 429 occurs, use the previous solcast sensor values. Do not set any sensors to Unknown or zero.

Desktop (please complete the following information): HA 2024.5.5 HAOS 12.3 Solcast v4.0.25

autoSteve commented 3 months ago

ERROR = No cache, or another API error

Technically 'no cache' is not an error in my mind, @swests. A user either instructed the integration to delete it, or it's a fresh install. Either way, a successful poll then needs to occur for forecast data to be usable, and that is not being in an error state, just limbo until something happens.

BJReplay commented 3 months ago

What we're getting into here is the rapid spiral into home manualisation - that is that home automation that ought to be saving time is costing time as we spend more and more time working on edge cases that deliver no additional value, other than the asymptotic ideal of perfection. I'm after engineering completion here - NEIGE.

That doesn't mean that there's a relatively simple solution able to be cobbled together here, but more sensors and more logic to determine what each combination means is more complexity and more bug reports that aren't bugs.

Last Solcast Forecast Retrieved Time (or similar) tells you when you last actually retrieved something. Last Solcast Forecast Attempted Time (or similar) tells you when you last attempted to retrieve something.

gcoan commented 3 months ago

Like I said, it fails loudly with an error, but retries at debug log level. Folks would have no idea that it had actually thoughtfully retried before the failure occurred unless debug logging is on. I could log the retries about Solcast being busy as warnings if you both feel that that's better.

Last Poll Time confuses me. We have one that's already called called API Last Polled, and can't rename it (it should be called something like Forecast Last Received), so another name for Last Poll Time I think. API Last Attempted Poll???

The logic would then go something like, if last poll not successful and current time = last attempted poll time + some minutes then trigger the poll.

Thinking about it, the retries probably shouldn't be totally silent to normal users who don't have debug turned on. Should at least log a warning that it's retrying. Don't need all the lines that are in the debug but at least one message so 'normal users' can see in the log that solcast has had a 429 and it retried.

last_poll_time is I agree confusing and whilst we should rename the entity, it would be an undesirable breaking change. api_last_attempted_poll sounds fine, as long as we make clear the purpose in the docs.

@swests was trying to reduce proliferation of sensors wherever possible by adding extra attributes to an existing sensor where there's a common 'concept' such as 'polling information'. Just seems a cleaner way of doing it

gcoan commented 3 months ago

Last Solcast Forecast Retrieved Time (or similar) tells you when you last actually retrieved something. Last Solcast Forecast Attempted Time (or similar) tells you when you last attempted to retrieve something.

that'd work. assuming as attributes of existing sensor rather than yet more new sensors.

I think there's value in exposing the information so if others want to automate with it they can do, rather than writing lots of complex edge cases as you say within the integration or hiding it as if we know better

autoSteve commented 3 months ago

Logging improved, BTW.

autoSteve commented 3 months ago

And it might be a silly idea from the point of view of: Add another sensor? Then add another six language translations for it, where Google may or may not get it right... My Polish and Slovak are a bit rusty, and Germans have five words for some things 😂

gcoan commented 2 months ago

I think we're all fixed on this one now, thanks @autoSteve