Welcome, Guest
Username: Password: Secret Key Remember me

TOPIC: Big GPS Clock

Re: Big GPS Clock 9 years 1 day ago #2399

  • jmessina
  • jmessina's Avatar
  • Offline
  • Senior Boarder
  • Posts: 44
  • Thanks received: 189
nmea.bas has some serious buffer handling issues. If you ever start dropping chars from the gps (which you will if the nmea.FBuffer array ever fills up), there's a very good chance that the nmea.getitem() and nmea.getfield() routines will start marching all over your memory. There's next to no error handling in the entire module, and with the exception of a check to see if FBuffer has gotten too many characters, there's no checks on the length of strings that it's copying, or whether the data makes any sense or not.

IF you never drop a character from the gps, never get a uart error, never get a mal-formed nmea string, and manage to process every single character that comes in, your good to go. If you don't manage to get the characters out of the buffer before FOverflow ever gets set, then you're probably hosed. You're lucky if all that happened was it corrupted your AdjTime variables.

As a simple example, trace through the logic and see what happens if you get a string of 81 "$" characters followed by a "whitespace" character like a CR. (hint: take a look at the declaration for TNMEA.Line()). While this is a very unlikely scenario, if you start dropping characters or get unexpected data there are a number of these "gotcha" situations.

The comment in the source code for "1.1 - Added overflow flag to prevent overwriting of the ring buffer" is true... it won't over-write the ring buffer. But the rest of the comment about the "interrupt will re-sync with any NMEA sentence" is wishful thinking. There are a number of loops that will run until a null character is found without regard to how much data it's processing. About the only thing that stops the module from blowing up all the time is the checksum validation and the NMEAItem.Valid flag.

IMHO, the module needs to be trashed.

Re: Big GPS Clock 9 years 1 day ago #2400

  • MMcLaren
  • MMcLaren's Avatar
  • Offline
  • Fresh Boarder
  • Posts: 13
  • Thanks received: 30
Jerry,

That's pretty harsh but you obviously studied the module much more then I did.

I was going to suggest Graham not use that module and just qualify each $GPRMC sentence manually. At 4800 baud he has a whopping 2-msecs between incoming characters.

I did it something like this on a 10F200 project (bit banged serial and LCD). When the parser is off the routine throws away characters until the next '$' character is received.

Regards, Mike
;
;  const rom char hdr[] = { "GPRMC" };    //
;  const rom char seg[] = { 0b00111111,   // "0"   -|-|F|E|D|C|B|A
;                           0b00000110,   // "1"   -|-|-|-|-|C|B|-
;                           0b01011011,   // "2"   -|G|-|E|D|-|B|A
;                           0b01001111,   // "3"   -|G|-|-|D|C|B|A
;                           0b01100110,   // "4"   -|G|F|-|-|C|B|-
;                           0b01101101,   // "5"   -|G|F|-|D|C|-|A
;                           0b01111101,   // "6"   -|G|F|E|D|C|-|A
;                           0b00000111,   // "7"   -|-|-|-|-|C|B|A
;                           0b01111111,   // "8"   -|G|F|E|D|C|B|A
;                           0b01101111 }; // "9"   -|G|F|-|D|C|B|A
;
;  unsigned char field = 3;               // parser off initially
;  unsigned char ndx = 0;                 // field index
;  unsigned char time[10];                // time string
;
;  #define parser (field < 3)             // parser on flag
;
;  void main()
;  { init();                              // init hardware
;    while(1)                             // loop
;    { data = Get232();                   // get gps character
;      if(data == '$')                    // if new 'sentence'
;      { field = 0; ndx = 0;              // reset field and index
;      }                                  //
;      else                               // not new sentence
;      { if(parser)                       // if parser 'on'
;        { if(data == ',')                // if new field
;          { field++; ndx = 0;            // bump field, reset index
;          }                              //
;          else                           // not new field
;          { if(field == 0)               // if "header" field
;              if(data!=hdr[ndx])         // if not "GPRMC"
;                field = 3;               // turn parser off
;            if(field == 1)               // if "time" field
;              time[ndx] = data;          // add char to time string
;            if(field == 2)               // if "valid" field
;              if(data == "V")            // if valid data
;                display();               // update LED display
;            ndx++;                       // bump field index
;          }                              //
;        }                                //
;      }                                  //
;    }                                    //
;  }                                      //
;

Re: Big GPS Clock 9 years 1 day ago #2401

  • jmessina
  • jmessina's Avatar
  • Offline
  • Senior Boarder
  • Posts: 44
  • Thanks received: 189
Yeh, Mike, I guess that was a bit harsh.

While the example I gave was very contrived, the problem is that the module has a number of places where it will blow up if things go awry. Defensive Programming 101 teaches "what can go wrong will go wrong", especially when you're dealing with input from the outside world, and this is a good example. Graham wanted to do something seemingly simple, and the module even has a comment that leads you to believe "Hey, no problem. I already took care of that for you".

How much trouble is it to do some sanity checks on things like the length of strings BEFORE you copy them, or make sure that you're not putting 200 characters into an 80 character buffer, or make sure that you didn't get 100 checksum characters, etc. On second thought, it IS a lot of trouble, but that's what it's all about when writing a good program. NMEA.bas doesn't fall into that category.

It's a good thing the GPS clock isn't launching a missile...

Jerry
Attachments:

Re: Big GPS Clock 9 years 1 day ago #2402

  • MMcLaren
  • MMcLaren's Avatar
  • Offline
  • Fresh Boarder
  • Posts: 13
  • Thanks received: 30
Graham,

I forgot to say that those displays look extremely nice. Are you just using two of them?

Regards, Mike

Re: Big GPS Clock 9 years 1 day ago #2405

I finished the clock at work today during a "break" - though I forgot to bring it home.. which means it's still running the original code (for now!)

Thanks for your guidance Jerry and Mike, I really appreciate it. I had a feeling that my variables were being "stomped-on" by the NMEA.bas library, and your explanation makes it clear Jerry.
the module even has a comment that leads you to believe "Hey, no problem. I already took care of that for you".
It seems I have run into a number of SF quirks the last few weeks - I had good confidence in the module given it was written by David Barker himself. There's likely a few revisions which haven't been published since (either by himself, or others) - hopefully they are shared someday.

I've got some code ready to test which manually decodes the NMEA sentences (as suggested by Mike) and update the daisy chained displays (which are controlled independently atm). I took some snaps at work today of the completed enclosure]new.digitaldiy.io/images/GPS_Clock.png[/img]

Re: Big GPS Clock 9 years 1 day ago #2406

  • MMcLaren
  • MMcLaren's Avatar
  • Offline
  • Fresh Boarder
  • Posts: 13
  • Thanks received: 30
Wow! Very nice!
Time to create page: 0.268 seconds