Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Going backwards? #18

Open
kds69 opened this issue Sep 5, 2023 · 14 comments
Open

Going backwards? #18

kds69 opened this issue Sep 5, 2023 · 14 comments

Comments

@kds69
Copy link
Contributor

kds69 commented Sep 5, 2023

Hi,
The latest version has now regressed partially: no hourly, no detail next 3 days...
API 2.0 was working already for me. Am I missing a point with this release?
Please let me know.

@siku2
Copy link
Owner

siku2 commented Sep 5, 2023

@kds69 you're absolutely right. There were a few unintended regressions with the release. I wanted to refactor the code a bit and then re-integrate the changes you made, but I completely forgot about the latter...
I also forgot to include the config entry migration.

Just to clarify one thing though: up until this release the integration was using v1 of the srf meteo api. This version is deprecated and will stop working sooner or later, so the main point of this release was actually to move to the new v2 api.

@siku2
Copy link
Owner

siku2 commented Sep 5, 2023

Btw, the no hourly might have something to do with the new home assistant weather entity. The forecast is no longer exposed as an attribute (that's deprecated) and instead you provide two methods for consumers to get the hourly and daily forecast separately.
The srf weather will actually provide the next 3 days in hourly resolution (and then the 3-hour resolution) for the hourly method.

If you use the default weather card and switch it to "hourly forecast" you should be able to see this.
I have to admit I haven't played around with this a lot yet, but this is how weather entities are supposed to work going forward.

Can you confirm that this aligns with what you're seeing, @kds69 ?

@kds69
Copy link
Contributor Author

kds69 commented Sep 6, 2023

@siku2 thanks for the explanation.
The integration gets no forecasts.
No idea why.
image

@siku2
Copy link
Owner

siku2 commented Sep 6, 2023

The integration gets no forecasts.
No idea why.

That's what I'm saying, it's no longer an attribute. You can't see it in the debug view. I'm surprised by this as well.
If you're interested in the technical details, you can read up on it here: https://developers.home-assistant.io/docs/core/entity/weather/#weather-forecasts

@siku2
Copy link
Owner

siku2 commented Sep 7, 2023

Just saw that the new HA version 2023.09 was released today. Be sure to try it out, @kds69. The changes to the forecast system are fully implemented now.

@kds69
Copy link
Contributor Author

kds69 commented Sep 9, 2023

you are absolutely right. There was indeed a timing issue in the various migrations (my HA got totally instable for 24h).
Now I am good with the entity from your integration. Well done!
I might need to add some new data that you skipped: snow and irradiance, if you are ok with it.
A branch would work for me as well, but it should be quite straightforward so it is a bit overkilling. Please let me know.

(I am playing around with irrandiance to create my own solar forecast... which btw is now a bit cumbersome with weather data being in cache =I'll have to create automation to call a service to populate a specific irradiance sensor. There is even a petition open to roll back this new way to manage weather entities. Sooner or later, solar forecast will move to cache as well I presume. Not my battle).

@siku2
Copy link
Owner

siku2 commented Sep 9, 2023

I might need to add some new data that you skipped: snow and irradiance, if you are ok with it.

yes, that's what I was referring to in my original reply. I didn't actually intend on removing those, but I completely forgot to add them back after the tedious process of refactoring the code :)

(I am playing around with irrandiance to create my own solar forecast... which btw is now a bit cumbersome with weather data being in cache =I'll have to create automation to call a service to populate a specific irradiance sensor

I know where you're coming from, but give it some time. Btw, I'm more than happy to add a separate sensor for this to the integration as well. I have my own irradiance sensor, so I never really required it from an api, but it never hurts to have multiple options :)

@siku2
Copy link
Owner

siku2 commented Sep 9, 2023

Alright, I went ahead and added the missing fields real quick. Before I release it, mind giving feedback if this is what you wanted, @kds69?

class ForecastSrfExtra(TypedDict, total=False):
symbol_code: int | None
symbol24_code: int | None
# for daily forecast
sunrise: str | None
sunset: str | None
sunshine_hours: int | None
# hourly forecast
temphigh: float | None
fresh_snow_cm: int | None
sunshine_minutes: int | None
irradiance: int | None
color: api.Color | None

These fields will be available in the forecast as well as in the attributes of the weather entity.

@kds69
Copy link
Contributor Author

kds69 commented Sep 11, 2023

yes!
I looked quickly into the code, saw the class extension as per above but I can't relay how extra data gets into entity while it doesn't seems to be coded (yet?) in _set_forecast_now.

@siku2
Copy link
Owner

siku2 commented Sep 11, 2023

yes! I looked quickly into the code, saw the class extension as per above but I can't relay how extra data gets into entity while it doesn't seems to be coded (yet?) in _set_forecast_now.

The trick is this part here:

for key in (
ForecastSrfExtra.__required_keys__ | ForecastSrfExtra.__optional_keys__
):
value = forecast_now.get(key)
if value is not None:
self._attr_extra_state_attributes[key] = value

all the keys that are part of the "srf extra" set are copied over to the extra state attributes.
But let me just create a new release, that way you can check it out directly.

@kds69
Copy link
Contributor Author

kds69 commented Sep 11, 2023

wow!
Very class..y ;)
Too advanced for me!

@kds69
Copy link
Contributor Author

kds69 commented Sep 16, 2023

Still a little issue: 3-hours forecasts are loaded as hourly.
Why nit but it is then quite annoying when templating sensors with get_forecast service. Can’t exclude them.
Not sure they can be tagged freely though, like 3_hours instead of hourly.
Or do what I did previously, skip hourly records after 48hrs as this is science fiction to rely on hourly anything, and let 3 hours one kick in from there.

Also realizing scan interval is too short: 15mn = api used 96x a day > 50x for freemium… may this be every 30mn, which is more than decent obviously

@siku2
Copy link
Owner

siku2 commented Sep 17, 2023

Not sure they can be tagged freely though, like 3_hours instead of hourly.

No, hass only supports hourly, twice-daily, and daily forecasts.
There is still a bug in there though, the forecasts shouldn't skip back in time when the 3 hour ones start appearing. Instead, 1 hour forecasts should be used until there aren't any more of them and then it should use the 3 hour ones from that point onward.

Additionally, I can add an additional key to the forecasts indicating whether it's a 1 or 3 hour one.

Also realizing scan interval is too short: 15mn = api used 96x a day > 50x for freemium… may this be every 30mn, which is more than decent obviously

That's not what's happening. The integration is updating its internal state every 15 minutes, yes, but the actual API requests are performed based on the rate limits reported by the API. The logic for this is here:

def request_next_api_call(self) -> datetime:
if self.consumers == 0:
_LOGGER.warn(
"untracked consumer is requesting an api call", stack_info=True
)
self.consumers = 1
now = datetime.now(tz=timezone.utc)
ratelimit = self.client.ratelimit
if not ratelimit:
return now + self.consumers * timedelta(hours=1)
calls_per_consumer = ratelimit.available / self.consumers
if calls_per_consumer <= 0:
return ratelimit.reset_time
duration_to_reset = ratelimit.reset_time - now
delay = duration_to_reset / math.floor(calls_per_consumer)
return now + max(delay, _MIN_DELAY)

@kds69
Copy link
Contributor Author

kds69 commented Oct 15, 2023

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants