Trouble with a function loop

I have been trying to get a Parallax GPS receiver to work with my Uno for a while now; I have posted in the Parallax forums but couldn't log in here for a while for some reason. I had a program that would read in the GPS data just fine as well as a program that read an optical sensor (catching bugs in a trap and counting when the beam breaks to keep track of how many bugs were caught). This is the closest I have it to working as intended; the Comboloop function works as a void loop and this does work... technically. What I have found is that whenever I try to make the function run in a loop (four times minimum, more iterations if a high count is obtained) what happens is that it runs one time and stops if the "if" statement inside the function is not met, and only seems to run up to 7 iterations if it is met. I had it print the "cycles" variable so that Icould see what was going on; if it only runs once it spits out 444 (initialized at 4). If I prop something into the sensor to make the count high, I get something like "Counts: 20 || 456 || Counts: 20 || 789 || Counts: 17 || 101112..." and so on. Sometimes when the program ends it keeps sending a stream of numbers ("1718192021..."). To make it even wierder, despite my limit being set at 10 counts per cycle it seems to operate using 15 as the limit. Can anyone see anything I'm doing wrong? Any help would be appreciated.

#include <string.h>
#include <ctype.h>
#include <SD.h>
#include <Wire.h>

int cycles;                      // # cycles before checking count (4)
#define limit 10                   // # bugs that cause another loop
int holdtime = 250;               // delay in ms for sensor
int iterations = 20;             // # pulses per sensor loop

String values = "";               // what is returned from the function
int ledPin = 13;                  // LED test pin
int rxPin = 0;                    // RX PIN 
int txPin = 1;                    // TX TX
const int chipSelect = 4;
int sensorPin4 = A3;
int x,o,k;         
int byteGPS=-1;
char linea[300] = "";
char comandoGPR[7] = "$GPGGA";
int cont=0;
int bien=0;
int conta=0;
int indices[13];
int sensorValue4 = 0;
float voltage4;
String voltageString4 = "default";

void setup()
{ 
    pinMode(rxPin, INPUT);
  pinMode(txPin, OUTPUT);
    pinMode(ledPin, OUTPUT);
      pinMode(10,OUTPUT);
    for (int i=0;i<300;i++){       // Initialize a buffer for received data
    linea[i]=' ';
                            }  
  if(!SD.begin(chipSelect)){
    Serial.println("Card Failed");
  return;
                            }
  else {
  Serial.println("card initialized");
        }
 // GPS.begin(4800);
  Serial.begin(4800);
}
//****************************************************************
String GPS(){
   String placeholder;
       placeholder = "";    
   for(int q=0;q<130;q++){

  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 {
    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
      digitalWrite(ledPin, LOW); 
      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
        for (int i=0;i<300;i++){
          if (linea[i]==','){    // check for the position of the  "," separator
            indices[cont]=i;
            cont++;
                             }
          if (linea[i]=='*'){    // ... and the "*"
            indices[12]=i;
            cont++;
                              }
                                }     
        for (int i=0;i<14;i++){        
         for (int j=indices[i];j<(indices[i+1]-1);j++){           
            placeholder += linea[j+1];
                                                       }
      placeholder += ",  ";      
                               }   
                  }
      conta=0;                    // Reset the buffer
      for (int i=0;i<300;i++){    //  
        linea[i]=' ';             
      }                 
                    }
        }
                          }
   byteGPS = -1;
  return placeholder;
}
//****************************************************************
 int Sensorloop(){ 
 int  o=0;
   for(x=0; x< iterations; x++){
  // read the value from the sensor: 
  sensorValue4 = analogRead(sensorPin4);
 // convert to voltage   
  voltage4 = sensorValue4*(5.0/1023.0);
  delay(holdtime);           
  if(voltage4 > 1.0) {
     o+=1;
     }
   }
   return o;
}
//***************************************************************
void Comboloop()
{
  values = GPS();  
  if(values != "")          {
      File dataFile = SD.open("GPSBUGS.txt", FILE_WRITE);
// if the file is available, write to it:
  if (dataFile) {
k = Sensorloop();
    Serial.println(values);
    Serial.print("Counts:  ");
    Serial.println(k);
    dataFile.println(values);
    dataFile.print("Counts:  ");
    dataFile.println(k);

    dataFile.close();
  }  
  // if the file isn't open, pop up an error:
  else {
    Serial.println("Error opening GPSBUGS.txt");
  }  
                              }
}
//****************************************************************
void loop()
{    
    k=0; 
    values="";  
    cycles=4;
    for(int w=0;w<cycles;w++){
      Comboloop();
      while(values==""){
        Comboloop();
                        }
      Serial.print(cycles);
      if(k>=limit){
        cycles++;
        k=0;
                  }
                              }
}

Have

you noticed
that
when
text wanders all
over the
page it
is really hard
to
read?

Try using the auto format tool and repost your code.

Also do yourself (and us) a favor by putting each { and } on separate lines before you run the auto format command.

The first thing that jumps out at me is that the code in GPS() is an unpleasant mixture of the String class, and c-strings (null-terminated char arrays). I suggest you use c-strings, but please choose one way or the other and stick with it.

The second thing that jumps out is that you seem to be performing a fixed number of reads of the GPS serial port and then delay if there was nothing available to read. That's not a robust way to handle serial port input and I have no confidence that what you've implemented will return the correct answer consistently. As soon as it doesn't, everything else will stop working since you seem to take 'no input available' as a loop termination condition.

The whole structure of the code is rather puzzling and I can't guess what it is trying to do.

The GPS loop is a (barely) modified version of the tutorial from the Playground; when I tried saving each piece into variables (small char arrays) it caused the loop to bog down or something, making NMEA sentences start in the middle of other sentences. The code is meant to read GPS data (GPGGA sentence specifically), read an optical sensor (counting how many times the beam is broken), and storing those things together on a txt file on the SD card. As stated, it does work as the void loop but I want to tell it how many times to run when it cycles. As requested, here's the code autoformatted:

#include <string.h>
#include <ctype.h>
#include <SD.h>
#include <Wire.h>

int cycles;                      // # cycles before checking count (4)
#define limit 10                   // # bugs that cause another loop
int holdtime = 250;               // delay in ms for sensor
int iterations = 20;             // # pulses per sensor loop

String values = "";               // what is returned from the function
int ledPin = 13;                  // LED test pin
int rxPin = 0;                    // RX PIN 
int txPin = 1;                    // TX TX
const int chipSelect = 4;
int sensorPin4 = A3;
int x,o,k;         
int byteGPS=-1;
char linea[300] = "";
char comandoGPR[7] = "$GPGGA";
int cont=0;
int bien=0;
int conta=0;
int indices[13];
int sensorValue4 = 0;
float voltage4;
String voltageString4 = "default";

void setup()
{ 
  pinMode(rxPin, INPUT);
  pinMode(txPin, OUTPUT);
  pinMode(ledPin, OUTPUT);
  pinMode(10,OUTPUT);
  for (int i=0;i<300;i++)
  {       // Initialize a buffer for received data
    linea[i]=' ';
  }  
  if(!SD.begin(chipSelect))
  {
    Serial.println("Card Failed");
    return;
  }
  else 
  {
    Serial.println("card initialized");
  }
  // GPS.begin(4800);
  Serial.begin(4800);
}
//****************************************************************
String GPS(){
  String placeholder;
  placeholder = "";    
  for(int q=0;q<130;q++)
  {

    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 
    {
      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
        digitalWrite(ledPin, LOW); 
        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
          for (int i=0;i<300;i++)
          {
            if (linea[i]==',')
            {    // check for the position of the  "," separator
              indices[cont]=i;
              cont++;
            }
            if (linea[i]=='*')
            {    // ... and the "*"
              indices[12]=i;
              cont++;
            }
          }     
          for (int i=0;i<14;i++)
          {        
            for (int j=indices[i];j<(indices[i+1]-1);j++)
            {           
              placeholder += linea[j+1];
            }
            placeholder += ",  ";      
          }   
        }
        conta=0;                    // Reset the buffer
        for (int i=0;i<300;i++)
        {      
          linea[i]=' ';             
        }                 
      }
    }
  }
  byteGPS = -1;
  return placeholder;
}
//****************************************************************
int Sensorloop()
{ 
  int  o=0;
  for(x=0; x< iterations; x++)
  {
    // read the value from the sensor: 
    sensorValue4 = analogRead(sensorPin4);
    // convert to voltage   
    voltage4 = sensorValue4*(5.0/1023.0);
    delay(holdtime);           
    if(voltage4 > 1.0) 
    {
      o+=1;
    }
  }
  return o;
}
//***************************************************************
void Comboloop()
{
  values = GPS();  
  if(values != "")         
  {
    File dataFile = SD.open("GPSBUGS.txt", FILE_WRITE);
    // if the file is available, write to it:
    if (dataFile)
    {
      k = Sensorloop();
      Serial.println(values);
      Serial.print("Counts:  ");
      Serial.println(k);
      dataFile.println(values);
      dataFile.print("Counts:  ");
      dataFile.println(k);

      dataFile.close();
    }  
    // if the file isn't open, pop up an error:
    else 
    {
      Serial.println("Error opening GPSBUGS.txt");
    }  
  }
}
//****************************************************************
void loop()
{    
  k=0; 
  values="";  
  cycles=4;
  for(int w=0;w<cycles;w++)
  {
    Comboloop();
    while(values=="")
    {
      Comboloop();
    }
    Serial.print(cycles);
    if(k>=limit)
    {
      cycles++;
      k=0;
    }
  }
}

In setup() Serial.begin() goes before any print statements!.

As for the rest of the code the best advice is bin t and start over thinking things through before writing code.

Mark

Okay, Serial.begin is now before the print statement (checking for a memory chip on the shield, but okay). Maybe I wasn't clear about what I wanted... I have done some "real" programming and have worked with other pseudo-programming systems (MATLAB, LabView, etc.) so I have a decent grasp of the basics, but I have no experience before this project with Arduino microprocessors or this particular GPS module. The advice of "start over and do it right" isn't particularly helpful because what I am asking for here is help figuring out what I'm doing wrong. The problem I had before putting the String variable into the mix was that opening the SD card or using the other sensor at all was making the GPS reading program mess up, and there were similar problems if I tried saving each piece of GPS data into variables. (I do understand how the parsing system works on the tutorial and, as stated, was using it as a basis. Arduino Playground - GPS) I did switch out the RMC string for the GGA string, but little else was changed from that tutorial. The other point is that this program is working in that it reads the string of data (which I can parse out later as long as I leave the commas in) and ties it to the "counts" from the optical sensor. What I don't get is why it doesn't cooperate with a for loop; again, it still runs but only once unless the if statement is met (which doesn't make sense to me because that is inside the loop). If I do need to scrap it and start over, that recommendation should probably come with at least a guess about how to go about redoing it. Otherwise the comment isn't particularly helpful. I do have other approaches that I am trying but (seeing as this one at least works a bit and nobody seems to be able to make "Smart Mode" work with this processor) this has been the most successful approach so far.

Assuming I have no idea what you're trying to achieve (because I haven't) can you describe what the sketch is intended to do in terms of the inputs and outputs it receives and sends? In particular, the relationship between the inputs and outputs, which is presumably your loops are trying to implement.

ETA: I can't really disagree with the advice to start again because to me, without any preconception of what the code is intended to do, it looks completely chaotic. I suggest that the first thing needed in order to start again is a clear understanding of what the sketch is intended to achieve. If you want us to advise you how to design the sketch then you need us to have that understanding.

For what it's worth, it is working now; I put some print statements into the code to figure out where it was breaking down, and apparently making it print to the monitor whenever the GPS code didn't return data before looping back around gave it time to think or something.
The combined program is designed to count the number of times the beam is broken on an optical sensor (hooked to a bug trap) and to tie that information with the GPS data (time and location being the relevant bits) to gather data on specific pests in soybean fields. I had a working program to print the GPS data to the monitor (from the Playground tutorial) and a working program to use the optical sensor, open the SD card, and write to it. The trouble was getting the SD card and the GPS receiver to play together nicely. This thing has been through a lot of iterations and while it looks chaotic, breaking the programs into pieces and calling them separately was the only way I could get them to function at the same time at all. If it makes anyone feel better my working "rough draft" code always looks a little crazy, I tend to go in afterwards and clean it up. The process: I initialize the variables I'm using for my "run this many times" loop. I then run my combination program (see below) at least four times, more if I'm getting a lot of action on my bug counter. The loop ends once it's run its course (the physical setup will be moved around between cycles). The "comboloop" reads the GPS data and puts the GGA sentence into a string to be parsed later. If there is data (meaning that we got a clean GGA string) then it opens the SD card and runs the optical sensor program, then puts the number of bugs counted and the GPS data into a txt file together. Then the SD card is closed, and rinse/wash/repeat.

I took another quick look at your original code and found this:

for(int q=0;q<130;q++){

    digitalWrite(ledPin, HIGH);
    byteGPS=Serial.read();

Unless you took other precautions, most of the times you read "byteGPS", it's going to be -1.
Just waiting 1/10th of a second could cause your input buffer to overflow.

Can you run the auto-format tool over your current code and repost?

I apologize for the delay; for some reason when I am logged in the forum doesn't always want to load ("Page cannot be displayed" error for the forum but not for the playground, reference, etc). I just wanted to say a couple of things. First, the last code posted is auto-formatted and I apologize if it is still difficult to navigate. I wrote the process out above to make it a little easier to figure out. Secondly, I appreciate the last input by AWOL, that seems like a likely cause for my problems and I will mess around with that first if I run into trouble when I expand my project. For now, I have what I need working and just want to thank people for their help here.