How can I improve this code?

The code below is what my teacher and I are trying to use for a GP-20U7 Chip and a Micro; however, it doesn’t show the information that we need to see in the serial monitor, such as the longitude, latitude, etc. Does anyone know how to improve the code below or a site with a code that may work?

/*
Example code for connecting a Parallax GPS module to the Arduino
Igor Gonzalez Martin. 05-04-2007
igor.gonzalez.martin@gmail.com
English translation by djmatic 19-05-2007
Listen for the $GPRMC string and extract the GPS location data from this.
Display the result in the Arduino's serial monitor.
*/
 #include <string.h>
#include <ctype.h>
int ledPin = 5;                  // LED test pin
int rxPin = 2;                    // RX PIN
 int txPin = 1;                    // TX TX
int byteGPS=-1;
char linea[300] = "";
char comandoGPR[7] = "$GPGGA";
int cont=0;
int bien=0;
int conta=0;
int indices[13];
void setup() {
   pinMode(ledPin, OUTPUT);       // Initialize LED pin
   pinMode(rxPin, INPUT);
   pinMode(txPin, OUTPUT);
   Serial.begin(9600);
   for (int i=0;i<300;i++){       // Initialize a buffer for received data
     linea[i]=' ';
   }  
 }
void loop() {
   digitalWrite(ledPin, HIGH);
   byteGPS=Serial.read();         // Read a byte of the serial port
   if (byteGPS == -1) {           // See if the port is empty yet
     delay(100);
   } else {
     // note: there is a potential buffer overflow here!
     linea[conta]=byteGPS;        // If there is serial port data, it is put in the buffer
     conta++;                     
     Serial.print(byteGPS);
     if (byteGPS==13){            // If the received byte is = to 13, end of transmission
       // note: the actual end of transmission is <CR><LF> (i.e. 0x13 0x10)
       digitalWrite(ledPin, LOW);
       cont=0;
       bien=0;
       // The following for loop starts at 1, because this code is clowny and the first byte is the <LF> (0x10) from the previous transmission.
       for (int i=1;i<7;i++){     // Verifies if the received command starts with $GPR
         if (linea[i]==comandoGPR[i-1]){
           bien++;
         }
       }
       if(bien==6){               // If yes, continue and process the data
         for (int i=0;i<300;i++){
           if (linea[i]==','){    // check for the position of the  "," separator
             // note: again, there is a potential buffer overflow here!
             indices[cont]=i;
             cont++;
           }
           if (linea[i]=='*'){    // ... and the "*"
             indices[12]=i;
             cont++;
           }
         }
         Serial.println("");      // ... and write to the serial port
         Serial.println("");
         Serial.println("---------------");
         for (int i=0;i<12;i++){
           switch(i){
             case 0 :Serial.print("Time in UTC (HhMmSs): ");break;
             case 1 :Serial.print("Status (A=OK,V=KO): ");break;
             case 2 :Serial.print("Latitude: ");break;
             case 3 :Serial.print("Direction (N/S): ");break;
             case 4 :Serial.print("Longitude: ");break;
             case 5 :Serial.print("Direction (E/W): ");break;
             case 6 :Serial.print("Velocity in knots: ");break;
             case 7 :Serial.print("Heading in degrees: ");break;
             case 8 :Serial.print("Date UTC (DdMmAa): ");break;
             case 9 :Serial.print("Magnetic degrees: ");break;
             case 10 :Serial.print("(E/W): ");break;
             case 11 :Serial.print("Mode: ");break;
             case 12 :Serial.print("Checksum: ");break;
           }
           for (int j=indices[i];j<(indices[i+1]-1);j++){
             Serial.print(linea[j+1]);
           }
           Serial.println("");
         }
         Serial.println("---------------");
       }
       conta=0;                    // Reset the buffer
       for (int i=0;i<300;i++){    // 
         linea[i]=' ';            
       }                
     }
   }
}

Moderator edit: CODE TAGS. Why is it so hard to read a few simple guidelines?

Use [ code ] tags. The forum software has misinterpreted some of your code as italic.

Please edit your post and use code tags

[code]your code here [/code]

will result in

your code here

limerek:
The code below is what my teacher and I are trying to use for a GP-20U7 Chip and a Micro; however, it doesn’t show the information that we need to see in the serial monitor, such as the longitude, latitude, etc. Does anyone know how to improve the code below or a site with a code that may work?

Your sketch does not even compile :slight_smile:

  for (int i = 0; i < 300; i++) { // Initialize a buffer for received data
    linea = ' ';
  }

You’re trying to assign a character to a character array; that is not the same as trying to assign a value to the element of a character array. Below the error message that I get (IDE 1.6.6).

C:\Users\sterretje\Documents\Arduino\Projects\Forums\topic_385580.0\topic_385580.0.ino: In function 'void setup()':
topic_385580.0:27: error: incompatible types in assignment of 'char' to 'char [300]'
     linea = ' ';
           ^

You need to indicate which element in ‘linea’ you want to modify using an index

  for (int i = 0; i < 300; i++) { // Initialize a buffer for received data
    linea[i] = ' ';
  }

Next error

C:\Users\sterretje\Documents\Arduino\Projects\Forums\topic_385580.0\topic_385580.0.ino: In function 'void loop()':
topic_385580.0:47: error: ISO C++ forbids comparison between pointer and integer [-fpermissive]
         if (linea == comandoGPR[i - 1]) {

                                      ^

You’re trying to compare a character array (actually a pointer to a character array) to a single character. Use an index to indicate which character of ‘linea’ you want to compare against which character in ‘comandGPR’

      // The following for loop starts at 1, because this code is clowny and the first byte is the <LF> (0x10) from the previous transmission.
      for (int i = 1; i < 7; i++) { // Verifies if the received command starts with $GPR
        if (linea[i] == comandoGPR[i - 1]) {
          bien++;
        }
      }

I think that is enough information to fix other issues as well.

You're trying to assign a character to a character array

No, that is not what OP is doing. The forum software saw the [ i ] as the start of italics, because the [ i ] was not between code tags.

OP: When you post about a problem you are having, "it doesn't work" is useless information. Describe what the software does, and how that differs from what you expect.

Add Serial.print() statements to see what the code is doing. Get the GPS off of the hardware serial port.

[thankyou un-named moderator]

// note: there is a potential buffer overflow here!

Well, to improve the code, I would definitely start here. And the other buffer overflow noted.

Then I would use the checksum provided by the GPS. Even on a short wire, serial communication can be easily corrupted. You don’t want a single inductive power spike to put your GPS position out by 2000km do you? Check the checksum and discard the sentence if it doesn’t pass.

Then you have the problem that Paul noted: you are trying to receive GPS on the same port as the serial monitor. Use SoftwareSerial or an Arduino variant that does have multiple hardware serial ports, such as the Teensy. The Micro might also going be a good choice for this, depending on exactly what you intend to do with this.

Then run over it again to clean up the code. You have #included two libraries but don’t appear to be using anything from those two libraries. The compiler works it all out, but the code is easier to read if you don’t include useless statements.