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

[owmweather] refreshing weather data fails with "Not OWM data" #3114

Closed
aglidden opened this issue Nov 25, 2023 · 8 comments · Fixed by espruino/Espruino#2436
Closed

[owmweather] refreshing weather data fails with "Not OWM data" #3114

aglidden opened this issue Nov 25, 2023 · 8 comments · Fixed by espruino/Espruino#2436

Comments

@aglidden
Copy link

Affected hardware version

Bangle 2

Your firmware version

2v19.75

The bug

Hello. My troubleshooting of owmweather led me to #3110.

The app returns "Not OWM data" when refreshing the weather data. Is this related to it calling require("Storage").readJSON(, require("Storage").writeJSON(, and JSON.parse( in lib.js and the new relaxed JSON parser?
I installed off of the loader https://banglejs.com/apps/ and I tried the dev version on https://espruino.github.io/BangleApps/ as well (I'm not 100% sure I did that correctly, but it seemed to install correctly from there).

It works fine on firmware 2v19, returning "Success". One should be able to reproduce this by installing Weather and OWM Weather on firmware 2v19.75, installing an openweathermap API key via the app manager, then going to Settings -> Apps -> OWM Weather -> Force refresh

Installed apps

No response

@nxdefiant
Copy link
Contributor

#3110 is not yet on the official app loader and only affects the interface.html, not the app itself. So I guess its not this change.

@aglidden
Copy link
Author

I was thinking, perhaps erroneously, that it might be a change in the 2v19.75 firmware that #3110 was addressing and might need to be applied to lib.js as well? I don't know. I thought it looked json parsing related though, for what it's worth.

@bobrippling
Copy link
Collaborator

Uncertain for the moment - waiting for my owmweather key to activate. Do you have the JSON you get back from the request to take a look at in the meanwhile?

@aglidden
Copy link
Author

{"coord":{"lon":-105.2,"lat":39.89},"weather":[{"id":801,"main":"Clouds","description":"few clouds","icon":"02n"}],"base":"stations","main":{"temp":269.05,"feels_like":265.83,"temp_min":265.29,"temp_max":271.62,"pressure":1021,"humidity":68},"visibility":10000,"wind":{"speed":2.06,"deg":330},"clouds":{"all":20},"dt":1701045759,"sys":{"type":2,"id":2004578,"country":"US","sunrise":1701007078,"sunset":1701041913},"timezone":-25200,"id":7150586,"name":"Countryside","cod":200}

@bobrippling
Copy link
Collaborator

bobrippling commented Nov 27, 2023

Yeah it's an issue with JSON.parse in 2v19.75:

>s = '{"coord":{"lon":-105.2,"lat":39.89},"weather":[{"id":801,"main":"Clouds","description":"few clouds","icon":"02n"}],"base":"stations","main":{"temp":269.05,"feels_like":265.83,"temp_min":265.29,"temp_max":271.62,"pressure":1021,"humidity":68},"visibility":10000,"wind":{"speed":2.06,"deg":330},"clouds":{"all":20},"dt":1701045759,"sys":{"type":2,"id":2004578,"country":"US","sunrise":1701007078,"sunset":1701041913},"timezone":-25200,"id":7150586,"name":"Countryside","cod":200}'
=...
>JSON.parse(s)
={ }

Strings and arrays are ok, looks like an issue with objects:

>JSON.parse('{"c":1}')
={  }
>JSON.parse('["4",5,"six"]')
=[
  "4",
  5,
  "six"
 ]
>JSON.parse('["4",5,"six",{"x":5}]')
Uncaught SyntaxError: Got ':' expected ','
 at line 1 col 18
["4",5,"six",{"x":5}]

I believe it's from the early return in this espruino commit and I've raised a fix.

@aglidden
Copy link
Author

Hm, now with 2v19.78 it's saying �[JUncaught undefined in the debug logs and displaying SyntaxError: Got '{' expected ':

syntax

gfwilliams added a commit to espruino/Espruino that referenced this issue Nov 27, 2023
matching STR in the if statement caused us to lose the key name
@gfwilliams
Copy link
Member

Ooops, sorry - should have tested that. I just pushed a change that I think should fix it now

@aglidden
Copy link
Author

That did it, thank you so much!

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

Successfully merging a pull request may close this issue.

4 participants