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

Weather module (yr) stops after a while #3296

Open
Jessendelft opened this issue Dec 20, 2023 · 12 comments
Open

Weather module (yr) stops after a while #3296

Jessendelft opened this issue Dec 20, 2023 · 12 comments

Comments

@Jessendelft
Copy link

Running the latest MM v2.25.00 with the YR weather fix from #3227
I have 2 weather modules, 1 which shows the current weather and 1 that shows the forecast with the following config:

// Weather now { module: "weather", position: "top_right", config: { weatherProvider: "yr", type: "current", initialLoadDelay: 1000, updateInterval: 60000, appendLocationNameToHeader: false, onlyTemp: false, showWindDirection: false, showSun: false, lat: xx.xxxx, lon: xx.xxxx, altitude: 111 } }, // Daily forecast { module: "weather", position: "top_right", config: { weatherProvider: "yr", type: "daily", initialLoadDelay: 3000, updateInterval: 120630, appendLocationNameToHeader: false, maxNumberOfDays: 7, fade: true, fadepoint: 0.25, lat: xx.xxxx, lon: xx.xxxx, altitude: 111 }, },

After the module starts it stalls after a while, unsure how long after but within the same day.
I looked at the dev_tools, and this is the error log:

image

For some reason it gets an error response from YR, which causes the module to stop updating.

@khassel
Copy link
Collaborator

khassel commented Dec 20, 2023

For some reason it gets an error response from YR, which causes the module to stop updating.

as you already suggests "error response from YR" this sounds like problems on their side or they may restrict access if there are to many requests. You can try with an increased update interval.

@rejas is it possible to improve the code so that the weather module starts working again after such errors?

@sdetweil
Copy link
Collaborator

other weather providers and calendar module have the same problem. limited error recovery

@rejas
Copy link
Collaborator

rejas commented Dec 21, 2023

@rejas is it possible to improve the code so that the weather module starts working again after such errors?

probably :-) but since I am still recovering from surgery I dont think I can cook something up until the next release.

@Jessendelft
Copy link
Author

as you already suggests "error response from YR" this sounds like problems on their side or they may restrict access if there are to many requests. You can try with an increased update interval.

Yes, I agree it's from their side. Their API says they reply with a 429 error message if they feel like you're sending too often. I already tried limiting my api calls by lowering the forecast api calls (updateInterval: 120630), but it might still be too much.
Still, it would be nice if the module is a bit more robust and doesn't crash immediately when it receives a reply that it doesn't expect.

@sdetweil
Copy link
Collaborator

@Jessendelft your delay is only 120 seconds

the delay is milliseconds

5 or 10 minutes might be better
unlikely the forecast will change significantly

10 * 60 * 1000

@Jessendelft
Copy link
Author

Well I'll be damned... That was supposed to be 10 and ~20 minutes. Forgot a 0, and that should hopefully fix my issue of too many requests. I'll let you know.

@khassel
Copy link
Collaborator

khassel commented Dec 22, 2023

other weather providers and calendar module have the same problem. limited error recovery

no, tested this, all other providers and calendar will recover with next update interval if there were fetch problems.

The yr provider is special, it caches data in local storage in the browser. I deactivated my internet connection and even after browser restart (cached) yr data is displayed.

Will not look deeper into this, its a nightmare to debug.

@Jessendelft
Copy link
Author

5 or 10 minutes might be better unlikely the forecast will change significantly

Just to confirm, the error still pops up even with my intended delays of 10 and ~20 minutes for my two weather modules. So it's still an issue, and I'd appreciate a fix that recovers from an empty response.

@sdetweil
Copy link
Collaborator

10 min cycle gives 144 API calls per day, assuming no suspend/resume

yr is particularly sensitive

Screenshot_20231223_080809_Chrome

Screenshot_20231223_081013_Chrome

@MagMar94
Copy link
Contributor

I added a Pull Request that fixes this (3706). The module should now be able to recover from errors like HTTP 429.

The main reason for the errors is HTTP 429 Too Many Requests. From the terms of service:

Traffic

  • Do not ask too often, and don't repeat requests until the time indicated in the Expires response header.
  • Cache data locally and and use the If-Modified-Since request header to avoid repeatedly downloading the same data.
  • Don't generate unnecessary traffic, e.g. by repeated requests for data which never change (MetAlerts CAP files, images with timestamps and so on).
  • Don't schedule many requests at the same time, e.g. every hour on the dot or when the forecast model runs are finished. > - Add a random number of minutes to the time of the requests as our data are continously updated. Spread your traffic evenly out over time so it makes a flat curve, not a sawtooth.
  • When using requests with latitude/longitude, truncate all coordinates to max 4 decimals. There is no need to ask for weather forecasts with nanometer precision! For new products, requests with 5+ decimals will return a 403 Forbidden.
  • Avoid continuous updating of mobile devices. Applications on mobile devices must not retrieve new data as long as the application is not in use. If you need push notification (e.g. for MetAlerts), don't make more than one poll every 10 mins.

Clients which don't follow the terms are liable to being throttled. This means requests will start getting a 429 HTTP status code instead of any content. Always check response Status headers and limit traffic immediately if this should happen.

I don't know how we can limit the traffic. We have implemented caching and use the Expires and If-Modified-Since-headers. Even if users set a low updateInterval the actual API will only be called when the data is expired.

Maybe we should treat the MagicMirror as a "mobile device" and make sure we don't make more than one poll every 10 minutes? This can happen if the Expires-header expires before 10 minutes have passed.

We can add something like this in yr.js start-method:

if (this.config.updateInterval < 600000) {
	Log.error("The Yr weather provider requires an update interval of at least 10 minutes (600 000 ms). Please increase the update interval in your configuration.");
        throw new Error("Update interval too short");
}

What do you think? Should I add it to the PR?

@khassel
Copy link
Collaborator

khassel commented Jan 26, 2025

What do you think? Should I add it to the PR?

other suggestion: Set updateInterval to 600000 if the user provides a smaller value and log a warning. But you can decide ...

@MagMar94
Copy link
Contributor

Nice suggestion 😊 I agree changing the value instead of crashing is a much better user experience. I tried setting the updateInterval in the start-method with this.config.updateInterval = 600000 and this.setConfig({ ...config, updateInterval: 600000 }), but the setInterval still used the original value. Decided to throw an error, can discuss further in the PR.

khassel pushed a commit that referenced this issue Jan 27, 2025
Fixes #3296 

The problem was that the fetch-methods threw errors when something went
wrong instead of calling `updateAvailable()`. `updateAvailable()` must
be called in order to schedule the next update.

I added some filtering for the hourly forecast that removes hours in the
past. If the API call fails we use the cached data, but we should only
display hours in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants