Need some help with some embedded C

[Crazyffrey]

Hi, I am trying to receive data on a PIC coming from a LCD screen connected with rs232. The data is only right sometimes. These are the functions I am having trouble with. Thanks

unsigned int16 ReceiveResponse()
{
    unsigned int16 i=0;
    memset(lcd_rx_msg,0,sizeof(lcd_rx_msg));   
    while(i<65535)
    {
      lcd_rx_msg[i] = fgetc(LCDSTREAM);  
      if(lcd_rx_msg[i] == 0X0D)
         break;
      i++;
    }
    return i;
}



unsigned int8 LCDGetVar(unsigned int8 var)  

{
   fprintf(LCDSTREAM,"get i%u\r",var);
   ReceiveResponse();
   if(lcd_rx_msg[0] != '!')
   {
      if(lcd_rx_msg[0] != '>')
      return lcd_rx_msg[0] - 0x30;      
   }
}

[SirPrattlepod]

Some observations

a) In RecieveResponse(), lines 5-10 you're not making sure that lcd_rx_msg[] doesn't overflow
b) In LCDGetVar the function doesn't always return a valid value; i.e. if lcd_rx_msg[0] != '!' and lcd_rx_msg[] == 0 control drops out of the conditional statement to the end of the function where there's no return...

[grumpy]

You might want to specify how you determined the data is "only right sometime". For example, samples of values expected versus values received would be helpful in recognising a problem. You haven't provided such information, so the most anyone can do is guess what your problem is. You also haven't given any context describing how these two functions are called.

That said, I see a few problems that tickle my bump of trouble.

You haven't specified what lcd_rx_msg is an array of (or a pointer to) nor how many elements it has. For arbitrary data received from the stream, your code would rely on lcd_rx_msg being an array of int (or a signed type with larger range), with at least 65535 elements, to avoid values being silently changed in conversion.

If lcd_rc_msg is an array of signed integral values (any type) your code is in trouble, since fgetc() can potentially return negative values (for example, EOF is outside the range of values that a char can represent, and is negative for some implementations).

If lcd_rx_msg is a pointer, that pointer needs to be initialised so it point to the first element of an array of appropriate size and type, and that array needs to continue to exist. Either that means it contains a pointer returned by a function like malloc(), or it needs to point at an array that is still in scope.

The behaviour of your code will be (ahem) entertaining if fgetc() ever returns EOF since - once EOF is returned for a stream, it is always returned. Since EOF rarely (in fact, never in my experience) is equal to 0x0D, this will cause your code to thrash - calling fgetc() each time - until i has a value 65535.

Since LCDGetVar() writing to LCDSTREAM, and then calling ReceiveResponse() which is reading from LCDSTREAM, the stream needs to be opened for both reading and writing. Even if it is opened in such a way (e.g. in update mode) it is generally necessary for the stream to be flushed (fflush()) or repositioned (fseek(), fsetpos(), rewind()) between either a writing operation followed by a reading operation or a reading operation which did not reach the end-of-file followed by a writing operation. Your code does not do this correctly.

Your LCDGetVar() also doesn't check the return value from ReceiveResponse() (which may or may not be an issue) and also drops through (no return statement) if lcd_rx_msg[0] is equal to '!' or to '>'. That causes the caller to have undefined behaviour if it uses the return value from LCDGetVar().

From a stylistic perspective, your code is using magic values (0x0D and 0x30). Even if you get your functions right, the meaning of those values is less than clear (yes, I know what they map to in the ASCII and related character sets - problem is, C compilers are not required to support an ASCII character set, and not all character sets have that mapping). That will be a headache if the functions ever need to be changed by someone (including yourself) who has forgotten the significance of those particular values.