macOS Why does order matter here?

[ArtOfWarfare]

I'm writing some Arduino code here (it's basically just CPP). The issue is that when I call dgps.updategga() matters - if I call it before dips.Status(), that method fails, but if I call it afterwards everything works fine.

The thing is, I want to remove the public facing updategga function and instead just have a single (public) update function which runs both update and updategga one after another. I didn't write the existing update or updategga function, so I'm not sure why the order they're run would matter. I'd love to hear what you guys think.

void loop() {
    dgps.update();    // Calling this updates the GPS data.  The data in dGPS variables stays the same unless
                                  // this function is called.  When this function is called, the data is updated.
  [b]//dgps.updategga(); // I can't place this here instead of later or things break. Why?[/b]
  if (!dgps.Status()) {
      Serial.println("GPS couldn't find enough valid satelites!");
  } else if (dgps.Checks() != dgps.Checked()) {
      ...
      [b]dgps.updategga(); // It works fine if I have it here instead.[/b]
      ...
  }
}

The implementations of the above dgps methods are:

bool dGPS::Status(){
    return (strcmp(cStatus, "A") == 0);
}

bool dGPS::update(){
 while(true){
  int byteGPS=-1;
  byteGPS=gpsSerial.read();      //read a byte from the serial port
  if (byteGPS == -1) {           // See if the port is empty yet
    delay(5); 
  } 
  else {
    linea[conta]=byteGPS;        // If there is serial port data, it is put in the buffer
    conta++;                      
    if (byteGPS==13){            // If the received byte is = to 13, end of transmission 
      cont=0;
      bien=0;
      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
        Serial.print("");        // For some reason, this needs to be here or you won't populate variables.  If anyone solves this problem, let us know.
        Serial.print("");
        Serial.print("-");
        Serial.println(linea);   //Print out the GPRMC string (just for reference)

          //------------------------------------------          
          // Get Time (UTC)
          char *data = "";
          memset(data, 0, sizeof(data));
          data = dGPS::subStr(linea, ",", 2);
          fTime = atol(data);
          delay(5);
        
          //------------------------------------------
          // Get Status
          memset(data, 0, sizeof(data));
          cStatus = dGPS::subStr(linea, ",", 3);
          delay(5);
          
          //------------------------------------------
          // Get Latitude (degrees)
          char *hemi;
          memset(data, 0, sizeof(data));
          char *cLat = "";
          cLat = dGPS::subStr(linea, ",", 4);
          fLat = strtod(cLat, NULL);
          fLat = conv_coords(fLat);
          hemi = subStr(linea, ",", 5);    // Sets the hemisphere.
          if(*hemi == 'S') {fLat = fLat*-1;}        // Setting the hemisphere.
          //Serial.println(hemi);
          delay(5);
          
          //------------------------------------------          
          // Get Longitude (degrees)
          memset(data, 0, sizeof(data));
          char *cLon = "";
          cLon = dGPS::subStr(linea, ",", 6);
          fLon = strtod(cLon, NULL);
          fLon = conv_coords(fLon);
          hemi = subStr(linea, ",", 7);    // Sets the hemisphere.
          if(*hemi == 'W') {fLon = fLon*-1;}        // Setting the hemisphere.
          //Serial.println(*hemi);
          delay(5);          
          
          //------------------------------------------
          // Get Velocity (knots)
          memset(data, 0, sizeof(data));
          char *cVel = "";
          cVel = dGPS::subStr(linea, ",", 8);
          fVel = strtod(cVel, NULL);
          delay(5);
          
          //------------------------------------------
          // Get Heading (degrees)
          memset(data, 0, sizeof(data));
          char *cHead = "";
          cHead = dGPS::subStr(linea, ",", 9);
          fHead = strtod(cHead, NULL);
          delay(5);          

          //------------------------------------------
          // Get Date (UTC)
          memset(data, 0, sizeof(data));
          char *cDate = "";
          cDate = dGPS::subStr(linea, ",", 10);
          lDate = strtod(cDate, NULL);
          delay(5);
          
          //------------------------------------------
          // Get Mode Indicator 
          memset(data, 0, sizeof(data));
          char *cstr= subStr(linea,",",11);       // Takes the whole substring at the end of the GPRMC string (eg:A*0C)
          //Serial.println(cstr);
          mode=subStr(cstr, "*", 1);              // picks up mode indicator A from A*0C
          delay(5);          
          
          //------------------------------------------
          // Get Checksum value from packet and compute checksum from packet. 
          // Printed only if flag (*fla) is Y or y
          memset(data, 0, sizeof(data));
          checkSum = subStr(cstr, "*", 2);       // picks up checksum 0C from A*0C
          delay(5);
          computedSum=Csum(linea);               // Compute checksum from the incoming  GPRMC string
          
          conta=0;                               // Reset the buffer
          memset(linea, 0, sizeof(linea));
	  break;
          
      }
      conta=0;                    // Reset the buffer
      memset(linea, 0, sizeof(linea));  
    }
  }
 } // END WHILE STATEMENT
 return true;  
}

// Function to update Number of Satellites used, HDOP, Altitude etc.
bool dGPS::updategga(){ 
  comandoGPG[0] = '$';
  comandoGPG[1] = 'G';
  comandoGPG[2] = 'P';
  comandoGPG[3] = 'G';
  comandoGPG[4] = 'G';
  comandoGPG[5] = 'A';
  while(true){
  int byteGPS=-1;
  byteGPS=gpsSerial.read();      //read a byte from the serial port
  if (byteGPS == -1) {           // See if the port is empty yet
    delay(5); 
  } 
  else {
    linea[conta]=byteGPS;        // If there is serial port data, it is put in the buffer
    conta++;                      
    if (byteGPS==13){            // If the received byte is = to 13, end of transmission 
      cont=0;
      bien=0;
      for (int i=1;i<7;i++){     // Verifies if the received command starts with $GPGGA
        if (linea[i]==comandoGPG[i-1]){
          bien++;
        }
      }
      if(bien==6){               // If yes, continue and process the data        
        Serial.print("");        // For some reason, this needs to be here or you won't populate variables.  If anyone solves this problem, let us know.
        Serial.print("");
        Serial.print("-");
        Serial.println(linea);   //Print out the GPGGA string (just for reference)
          
          //------------------------------------------          
          // Get Number of satellites in use
          char *data = "";
          memset(data, 0, sizeof(data));
          data = dGPS::subStr(linea, ",", 8);
          fSat = atoi(data);
          delay(5);
        
          //------------------------------------------
          // Get HDOP
          memset(data, 0, sizeof(data));
          data = dGPS::subStr(linea, ",", 9);
          hDop=atof(data);
          delay(5);
          
          //------------------------------------------
          // Get Altitude above sea-level (meters)
          memset(data, 0, sizeof(data));
          data = dGPS::subStr(linea, ",", 10);
          fAlt=atof(data);          
          delay(5);
          
          conta=0;                    // Reset the buffer
          memset(linea, 0, sizeof(linea));
	  break;
          
      }
      conta=0;                    // Reset the buffer
      memset(linea, 0, sizeof(linea));  
    }
  }
 } // END WHILE STATEMENT
 return true;  
}

I can share more of the code or answer any questions you have. I haven't modified any of the code from what I'm actually trying to run (other than I took out some lines and replaced them with ... in the loop() function... and obviously there's a lot of dGPS functions that I left out.)

 

[ghellquist]

Hard to say. You forgot to include the code of dgps.Checks() and dgps.Checked().
They might have som effect. Especially on the variable conta which should be zero at the entry, but is not set in updategga.

I also find this code quite interesting:
char *data = "";
memset(data, 0, sizeof(data));

char *data ="" allocates a one character string having only the character nil (end of string)

sizeof(data) will return the size of char * which normally would be two bytes on the Arduino.

So what you do is to allocate a one byte string, and fill it with two bytes. Later you fill it with even more bytes.

 

[Sander]

I agree with ghellquist. You declare the "data" variable to assign the result from the dGPS::subStr() function to it. There's no need to explicitly clear it in the mean time, and actually also not necessary to assign an empty string to it.

Getting rid of this would already remove some of the aspects of your code which I like to call the "and suddenly, nothing happened" idiom. That's code which makes the reader scratch their head, thinking "what's going on?", then looking up the author, seeing it's not farmerdoug and continuing to think "Hmm... ArtOfWarfare probably didn't put that stuff in there for nothing", then pondering it for a long time and reaching the conclusion "maybe it's there for naught anyway?"

There's more of this: Sometimes you clear this data variable (the wrong way, as ghellquist pointed out) only to not use it at all afterwards (see the code for Get Latitude (degrees)). After this piece of code, "data" is cleared again, only to not be used again

Clean this up and let us know if it helps - if anything, it'll help us focus on the real issue...