next topic »

[on:]

Hey all,

I have been playing around with some serial communication and have the following issue: Every time i execute a function to send a command to a slave and then read the answer from it, the Serial.available() function causes another global variable to be overwritten. I know this sounds weird... let me explain a little more.
I have an LCD with 3 buttons, which control a menu displayed on the LCD. I keep track where I am in the menu with a variable called 'menu' (int). In a timed loop (using interrupts) I check the buttons and then modify the variable 'menu' appropriately. There is a switch/case statement that prints the menu text corresponding to the number in 'menu'.

The following code is responsible for reading the answer into array (which is later converted).

Code:

void SerialRead(int port){
  Start:
  if (Serial.available() < 0){
    goto Start;
  }
  int i=0;
  while (Serial.available() > 0){
    array[i] = Serial.read();
    i++;
  }
}

I have narrowed the problem down to this segment. Meaning, if I don't execute this function (but everything else) there is no problem at all. As soon as this function is called, my 'menu' variable gets somehow overwritten and the menu is not displayed anymore.
I put a default case in my Switch/Case statement (as explained above) to print 'menu' on the LCD. As expected, this works and it shows a really high number (3395) which indicated that we are in menu index 3395...

I have spent a couple hours already troubleshooting but without luck. However, I was able to narrow the problem down to a single line of code:

Code:

 while (Serial.available() > 0){

As soon as I replace the expression with something else (like i<2) the problem disappears.

I have done some research online and found a post in this forum about an Serial.available() issue. They claimed that a delay needed to be inserted after a Serial.read and before Serial.available() is executed again.

Has anyone encountered a similar issue? Any suggestions what it might be/what might solve it? Is there a bug in the serial library that could cause it?
I am aware that interrupts could cause such issues, but since I can narrow it down to a single line of code I find the repeatability a little odd.

[Reply #1 on:]

How big is array declared to be? What happens if you add some checking so that you don't write more characters into array than it can hold?

You don't need the goto call.

Code:

while(Serial.available() < 0)
{
   // do nothing
}

[Reply #2 on:]

Array is declared as array[10]. When I tested the device I'm querying I received 5 bytes or less. Also, the response is given by the manufacturer as a 1 byte identifier and a maximum of 4 following data bytes. I figured that an array of 10 bytes would be sufficient enough.

Guess what? I increased the size of array and that fixed it. I somehow expected that my 'menu' variable was overwritten somehow but didn't think that far... Anyway, it turns out that the 'menu' variable is declared globally right after array[10]. Moving the latter to the bottom of the declaration list will also fix it.

I can see why you're saying that the goto call is not needed. However, the device I'm talking to needs a couple mili-seconds to make the measurement and send the response back. If I eliminate the goto statement my code will start reading and immediately exiting the while loop because there is no data in the buffer yet. I would have to call the method again, which I don't want to do.
Is my reasoning wrong?

Other question: If I determined that the answer is only 5 bytes long (maximum) and declared the array for the incoming data 10 bytes long (as above) is it wrong to assume that this array never gets filled up as long as I reset the pointer to the array back to zero every time I read new data?

Thanks for your help

[Reply #3 on:]

Code:

Start:
  if (Serial.available() < 0){
    goto Start;
  }

and

Code:

while(Serial.available() < 0)
{
   // do nothing
}

accomplish the same objective - preventing further code from executing if there is nothing to read. The second method avoids the dreaded goto statement, and is clear what is happening. You can't inadvertently add code between the Start label and the if test that gets executed over and over again.

Changing the size of the array, without adding error checking, is a bandaid fix. It works, but is not robust.

Moving where array is declared will NOT fix the problem. Something else will get overwritten, instead. Maybe that won't impact you now, but a change to the program 5 years from now, with a different compiler, with different optimization strategy, and you will get bitten in the rear end. Hard.

is it wrong to assume that this array never gets filled up as long as I reset the pointer to the array back to zero every time I read new data?

It's a valid assumption. But, when you are reading data from a source where you do not control the length, whether that is a file, a serial port, or user input, it is always a good idea to verify that there is room in the array for the new byte.

Code:

while(Serial.available() > 0)
{
   int in = Serial.read();
   if(i < (sizeof(array)/sizeof(array[0])) // Make sure there is room
   {
      array[i] = in;
      i++;
   }
}