GNOME Bugzilla – Bug 160475
Visibility data misparsed when low
Last modified: 2005-01-04 13:12:53 UTC
An observation from METAR returned the following string: KBOS 130254Z 34009KT 1 3/4SM R04R/5500VP6000FT -SN BR BKN007 OVC021 00/M01 A3041 RMK AO2 TWR VIS 4 CIG 005V011 SLP298 P0002 60007 T00001011 57003 $ The visibility field, "1 3/4 SM", could not be parsed; the current conditions window listed the value as "unknown".
Created attachment 34497 [details] [review] Proposed fix The visibility field may include fractions when it is less than 3 statute miles, not just 1, and may use a numerator other than 1 (e.g.: 3/4). This patch is based on the description of the vilibilty field at http://www.met.tamu.edu/class/METAR/metar-pg7-vis.html
Created attachment 34500 [details] [review] Includes fix for bug #151896
Woo! This is great work. Committed!
Frank, I have noticed that somehow this patch causes the pressure to be marked as Unknown, although I can't see why. Not using this patch causes the pressure to be shown normally.
I think I've got it narrowed down and should have another update ready tomorrow.
Created attachment 34678 [details] [review] Fixes pressure display The original implementation wouldn't work when visibility was expressed as units plus fraction since the parsing code assumed that the space was the delimiter between expressions. This reworks the parsing so that spaces can be part of the expression. It also doesn't reuse regular expressions. I'm thinking the pressure string may have also matched one of the earlier expressions as well and stored the value into the wrong field.
Created attachment 34681 [details] [review] Another Naturally, the first update I received after submitting the patch included multiple fields for cloud cover. This fixes that, using the first string (vs. the last).
Ok, that seems to work. Thanks Frank.
Created attachment 34724 [details] [review] Additional patch: on top of previous pages The conditions field can also be set with multiple values and appears before cloud cover information. This fix doesn't lose everything after present weather.
Created attachment 34725 [details] [review] Additional patch: on top of 34681 I just noticed that the original report in this bug would have not been processed correctly due to the runway info. This should work.
Comment on attachment 34725 [details] [review] Additional patch: on top of 34681 Ok, this is committed too. Thanks for all your work on the parsing code.
Hmm, something seems hideously wrong with the parsing code now, all I'm getting is invalid either with, or without this patch.
Can you try including the patch to bug #160572 ? There's a one-line difference in there that prevents retrying the first three patterns. If that doesn't work, could you include the reading getting parsed so I can test with that?
Created attachment 34743 [details] [review] Condition parsing moved out to separate files The file weather.c is getting a bit long. I'm breaking the response parsing out into separate files to ease maintenance; the files to be added will follow momentarily.
Created attachment 34744 [details] New "weather-*.c" files for parsing responses. I'm running into the bug here too.. the problem is the field in the middle for runway conditions was throwing off the state machine. I've changed the regex call so that it will look further into the string but ensure that if it's not at the start, the previous character is ' '. This also includes the fix for bug #160572 (windchill error)
Ok, this is a significant cleanup, so I'm going to commit it. Although we appear to be back to the original bug which is it doesn't seem to be parsing visibility data.
Created attachment 34766 [details] [review] CAVOK sets both visibility and sky fields Parse functions also return the new state. This avoids the inner loop in the parser that needed exceptions and makes handling this case easier.
I'll test this in the morning after I've slept and my mind has reawoken. Also Frank, I want to point you towards our work for moving gweather to using a weather.gnome.org service in the future. Since you have put an amazing amount of work into fixing up the parsing code, you should be interested in where I hope we can go with this. See http://live.gnome.org/WeatherApplet
Created attachment 34795 [details] [review] One line fix to above.. Minor fix to above: parsing loop needed to actually use the returned value (I should've waited for my mind to reawake before submitting.. *blush*) Thanks for the invite -- I'll look into it some more later today.
Created attachment 34816 [details] [review] Force ignoring RMK data Text in the remark field tricked the parser into advancing further than it should have. This forces processing to end at RMK by writing a '\0' into the position.
Ok, I'm testing this now. It has fixed an issue I've had for half a day where the applet was brokenly reporting 'smoke'. Visibility is back. All numbers seem consistant with values reported by the BOM.
Hmm, seems that temperature parsing has broken.
Here is my METAR: 201000Z 25014KT CAVOK 24/17 Q1012 FM1200 25010KT 9000 FU BKN010 I see after the visibility field (CAVOK) we go straight to what appears to be the temperature group.
Ok, I have added in workarounds to reorder the the chain parser. I'm going to test that for a while before I do anything with it. It's hacky, and works by checking if your location starts with 'YP'. Not the best solution. Perhaps we should completely rewrite the parser to avoid chaining where possible.
Yikes! The transition from visibility to clouds on "CAVOK" was based on the description of the int'l standard from http://www.met.tamu.edu/class/METAR/metar-pg3.html, but the description there suggests that your sky conditions (BKN010) should have been between CAVOK and the temperature field. Was it Grace Hopper who said "the nice thing about standards is that there are so many of them"? I'll try checking google for some other descriptions to see if I can find one that's more accurate.
Frank, yes also take note of the two visbility fields (one is CAVOK, the other is 9000). The current observation is: YPPH 201600Z 26005KT CAVOK 19/17 Q1012 NOSIG which the parser can deal with as it is currently. Searching on randomly scattered locations, it seems non of them really follow the spec in the specific order listed. Hence it seems that chaining the parser to parse in a particular order is not a good idea. On the other hand, we should be able to parse each fragment using a regex of what characters have to appear. ie. if it ends with a Z it's the time field, if it ends with KT it's the wind field. CAVOK or a number on it's own is visbility. dd/dd is the temperature dew point. Numbers starting with Q are pressures etc. The only thing this might create a problem for is American locations, but if required we'll just use a different parser for those. Basically at the moment, it looks like we have a serious regression in the METAR code, and I've only just realised why. I am loathe to release 2.9.3 without either getting this fixed or reverting it. What do you think?
Agreed -- I'll work on it this evening. (I was looking for an excuse to put off starting my Christmas shopping anyway :-)
Created attachment 35071 [details] [review] undo state parsing mechanism The quote is by Andrew Tannenbaum.
Created attachment 35074 [details] [review] patch in testing This is the patch I'm testing against CVS HEAD. It seems to be holding up so far.
Just to help you guys track, if I understand the current state of the code, this should be marked 'High', right?
*** Bug 160572 has been marked as a duplicate of this bug. ***
I think it's closed -- it's in CVS HEAD at any rate.