Current weather / Wind speed in km/h issue

[Killator]

Hi MM-Team !

It's a windy day today for me and I noticed a bug by looking at my beautiful magic mirror ! (docker / up-to-date)

Without any parameter (config.js) the wind speed displayed is: 7 (Beaufort)
With the parameter "useBeaufort: false" the wind speed displayed is: 16 (m/s because I'm in "metric" units).
And it's just the rounded value from the OpenWeatherMap API: 15.8

But with the parameter "usebeaufort: false" and "useKmh: true" the displayed value is: 207 (km/h) (HURRICANE ?! ^^)

It should be ~57 (56,8 Km/h)... (m/s * 60 * 60) / 1000.

I have tried to challenge the logical tests in current.njk file...
And to find a bug in the weatherobject.js file (line 93)...

... but I'm not JS "fluent", I don't see anything wrong :-(

Could you please trying to solve this issue with me ? Thanks

[rejas]

Mhhh...

It seems to me that the data from openweathermap is converted twice to km/h. Once when displayed (the njk template calls the kmhWindSpeed function) and already before when creating the weatherobject (in function generateWeatherObjectFromCurrentWeather(currentWeatherData)

Maybe the later one can be removed, no other provider does convert it like that?

[Killator]

That it's !

I have manually changed the line 93 in the weatherobject.js file to remove the second conversion and now, I have the correct value displayed in Km/h !
Before:
return this.windUnits === "imperial" ? this.windSpeed * 1.609344 : (this.windSpeed * 60 * 60) / 1000;
And now:
return this.windUnits === "imperial" ? this.windSpeed * 1.609344 : this.windSpeed;

Thx @rejas !

"no other provider does convert it like that?" > I'm not sure to understand... You want to make some tests with others providers to check any regression with this modification ?

[rejas]

Good to hear that this fix worked for you, although I think that wont make it into the official code :-)

My ramblings were directed into the vague directions of other developers to get their feedback. After a nights sleep I have the following thoughts / addendums to mine above:

What value for the windspeed should the wetherobject hold? Should it be the raw one which is reported by the provider or should it be already converted into what the user wants via his config?

The current code (in openweathermap) doesnt seem to know what it wants, it converts it into kmh when it gets the data and also when it displays it. Same with ukmetoffice and some other providers. Newer providers, which came after the useKmh change dont have that extra conversions (smhi envcanada)

I would make the conversion only when its needed, so when displaying the data on the screen. And not inside the weatherobject.

Also, looking at the module, it doesnt seem to send the data out to other modules (except the MM2s own compliments module) so it doesnt affect anything elese.

comments, @khassel @MichMich ?

Might prepare a PR already for that :-)

[khassel]

@rejas sounds good but didn't have time to dig into it and since you're already in there...

[rejas]

Created a PR fixing openweathermap, alas looking at other weather providers should maybe done by the people who submitted them?

So I see following providers doing a windspeed conversion when loading the data:

• envcanada (@Crazylegstoo)
• ukmetoffice (@maloakes)
• ukmetofficedatahub (@XBCreepinJesus)
• weathergov (@vincep5)

Maybe the 4 mentioned devs can take a look if the windspeed display is correct when metric AND useKmh is selected?

[Crazylegstoo]

No problem! I will take a look at envCanada in the next day or two and make sure windspeed is handled correctly. I will let you all know whatever happens. Kevin

…

On Mar 22, 2022, at 4:39 PM, Veeck ***@***.***> wrote:  Created a PR fixing openweathermap, alas looking at other weather providers should maybe done by the people who submitted them? So I see following providers doing a windspeed conversion when loading the data: envcanada ***@***.***) ukmetoffice ***@***.***) ukmetofficedatahub ***@***.***) weathergov ***@***.***) Maybe the 4 mentioned devs can take a look if the windspeed display is correct when metric AND useKmh is selected? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[XBCreepinJesus]

Re. ukmetofficedatahub - I'm happy to have a look, but I'm not sure if I'll get a chance for a few days, so I'm also happy to let someone else sort it if they're looking for something to do!

[maloakes]

Thanks for letting me know. I’m rather busy at the moment and don’t have a MM setup. I’ll try to take a look (ukmetoffice) when I can. Although this is probably superseded by the UKMO DataHub product (ukmetofficedatahub interface).

On Tue, 22 Mar 2022 at 20:39, Veeck ***@***.***> wrote: Created a PR fixing openweathermap, alas looking at other weather providers should maybe done by the people who submitted them? So I see following providers doing a windspeed conversion when loading the data: - envcanada ***@***.*** <https://github.com/Crazylegstoo>) - ukmetoffice ***@***.*** <https://github.com/maloakes>) - ukmetofficedatahub ***@***.*** <https://github.com/XBCreepinJesus>) - weathergov ***@***.*** <https://github.com/vincep5>) Maybe the 4 mentioned devs can take a look if the windspeed display is correct when metric AND useKmh is selected? — Reply to this email directly, view it on GitHub <#2812 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI74YDJVIZK55NO2OPY6FBTVBIVYDANCNFSM5OS5GPOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Kind regards Malcolm Oakes

[Crazylegstoo]

I've looked at envcanada and yes there is an issue when metric AND useKmh are selected. Here what is happening:

1. envcanada is by default getting windspeed in kmh from the backend system (Environment Canada)

2. envcanada is checking windUnits. If metric, then weatherobject is simply given the windspeed value provided by the backend, But if windUnits is set to imperial, envcanada does the conversion on its own and gives that result to weatherobject

3. Note envcanada ignores useKmh

4. current.njk is seeing useKmh is true and causing the weatherobject kmhWindSpeed function to be called. The function seeing that units are metric, and so is calculating kmh under the assumption that the windSpeed value is reflecting m/s. With envcanada, this not good since windSpeed is already in kmh. Example... envcanada windSpeed of 19kmh ends up displaying as 68kmh.

FWIW, I notice that setting useBeaufort has the same issue in assuming metric windSpeed value is reflecting m/s and applying conversions accordingly.

I would say that having metric AND useKmh set is an unusual combination for envcanada provider and I guess the doc could simply recommend not doing this.

I think the bigger issues are are probably (1) we have various conversion logic sprinkled across 'base' weather code AND provider code and (2) provider backends do not all provide data values in the same units (hence the need for providers to sometimes do the conversions). All of that can lead to some inconsistencies. I'm not sure there is a 100% clean solution (have to think about this). I think one idea could be to for provides to give weatherobject more information about the current windSpeed value - e.g. is it mph, is it kmh, is it ms, is it Beaufort... and then its conversion functions could be made smarter about how to convert. Alternatively, envcanada could forget about kmh and provide weatherobject a metric value in m/s and let the weather module work with that in a consistent way. This requires more thought...

[Killator]

Hi !

I have take the time to test the fix include in the release 2.19.0 : everything seems good ! 👍

OpenWeather API: 3,1 m/s ==> MM Display: 11 Km/h

Thanks a lot ! I close this issue ;-)

[rejas]

Too bad the issue got closed, maybe we need to open a new one for discussing it @MichMich @Crazylegstoo

I think the bigger issues are are probably (1) we have various conversion logic sprinkled across 'base' weather code AND provider code and (2) provider backends do not all provide data values in the same units (hence the need for providers to sometimes do the conversions). All of that can lead to some inconsistencies. I'm not sure there is a 100% clean solution (have to think about this). I think one idea could be to for provides to give weatherobject more information about the current windSpeed value - e.g. is it mph, is it kmh, is it ms, is it Beaufort... and then its conversion functions could be made smarter about how to convert. Alternatively, envcanada could forget about kmh and provide weatherobject a metric value in m/s and let the weather module work with that in a consistent way. This requires more thought...

1. yes, having conversion logic all around the place and duplicated is bad. There should be one place to do converting and such

2. Some provider backends indeed dont provide stuff with the same units but that isnt a big problem since this isnt rocket science. We only need to do some general conversion like from mph to kmh or such.

We can fix 1) and 2) if we demand the weather providers to internally all use the same units:

• So calling the API of WeatherProviderXYZ might only deliver mph, the fetched data is immediatly conerted to kmh.
• If the API provides kmh, nothing needs to be done.

Now when the UI wants to display data, it can safely assume that the value is in a given format.
Also, when sending a notification we always send the same unit around, so third party modules also benfit from this.

The only (small) downside I see so far, is that we have to manually check that new weather providers do follow this too. Minimum documenation update is needed for this.

What do you think about this?

[Crazylegstoo]

We can fix 1) and 2) if we demand the weather providers to internally all use the same units:

* So calling the API of WeatherProviderXYZ might only deliver mph, the fetched data is immediatly conerted to kmh.

* If the API provides kmh, nothing needs to be done.

Now when the UI wants to display data, it can safely assume that the value is in a given format. Also, when sending a notification we always send the same unit around, so third party modules also benfit from this.

The only (small) downside I see so far, is that we have to manually check that new weather providers do follow this too. Minimum documenation update is needed for this.

What do you think about this?

I think I agree with the approach. So we set a standard that says all Providers (backends) must populate weatherobject using - for example - Metric units as the standard (temp = Celsius, wind = m/s, etc). Each Provider then worries about any conversions they need to do to satisfy this weatherobject standard. From there, we let the various .njk templates and base weather code perform any conversions required to satisfy the User's config preferences for UI purposes. Like you say, it's a pain in the butt to update all the Provider code to satisfy the approach, but it keeps things clean and understandable going forward.