Strange behaviour of "return value;"

Hello dear Arduino community,

To make it clear: I’ve already solved the problem with my code. But I don’t know why the solution is actually working,… :~
I really don’t know what’s wrong with the first code… Maybe someone can teach me?

Sorry for posting “dirty” code fragments, but I wasn’t able to reproduce the problem nicely and the whole project is a bit to big for posting here. I’ve commented the changes in the working code (just two lines) to make them more visible.

This is the buggy code. The function is always returning “false”!

byte USBcom_class::GetCommand() {
  byte n = 0;  // originally "byte n;" (modified due to the information in the next posting)
  while( Serial.available() && n < SERIALUSB_BUFFERSIZE ) {
    char inChar = Serial.read();
    if( inChar != CHR_SOH ){
      Buffer[ n++ ] = inChar;
      if( inChar == CHR_NUL ){
        Serial.print( Buffer );
        Serial.send_now();
        return --n;
      }
    }
    else{
      n = 0;  // originally "n == 0;" (modified due to the information in the next posting)
    }
  }
  return false;
}

This is the 100% corretly working code. Just one additional and one changed line. But what’s the difference to the first one?!

byte USBcom_class::GetCommand() {
  byte n = 0;  // originally "byte n;" (modified due to the information in the next posting)

  byte falseVar = 0; // <--- ########## Added this line! ##########

  while( Serial.available() && n < SERIALUSB_BUFFERSIZE ) {
    char inChar = Serial.read();
    if( inChar != CHR_SOH ){
      Buffer[ n++ ] = inChar;
      if( inChar == CHR_NUL ){
        Serial.print( Buffer );
        Serial.send_now();
        return --n;
      }
    }
    else{
      n = 0;  // originally "n == 0;" (modified due to the information in the next posting)
    }
  }

  return falseVar; // <--- ########## instead of "return false;" ##########

}
      n == 0;

Compare the value of 0 to n, and then discard the result of the comparison. Why bother?

  byte n;
  while( Serial.available() && n < SERIALUSB_BUFFERSIZE ) {
    char inChar = Serial.read();
    if( inChar != CHR_SOH ){
      Buffer[ n++ ] = inChar;

Rubbish code. n is NOT initialized. Only a doofus uses a local variable without initializing it.

And your solution is even worse you return the value of a var that has never been initialized or assigned a value any where.

The results you are getting are just random.

Mark

Thanks for the very informative answers! :slight_smile:

PaulS:
Compare the value of 0 to n, and then discard the result of the comparison. Why bother?

Yuk! This should be a assignment, not a comparision!

PaulS:
Rubbish code. n is NOT initialized. Only a doofus uses a local variable without initializing it.

I thought variables are getting initialized with Zero, if not otherwise defined. This is really a fundamentally important information! Thank you!

I'll udate the starting post to remove the bugs. As you might have noticed C/C++ is not the language I'm used to...

But the problem persists. Also the debugged codes result in the "stange behaviour".

I thought variables are getting initialized with Zero, if not otherwise defined.

Only if with global or static scope, and not initialised to some other value.

We would greatly prefer it if you posted your amended code in a new post, not amend your original one. Now that you corrected your first post, the comments under it (by other posters) don’t make any sense and are confusing.

As to your problem, it seems likely to me that you are falling through and executing the "return false" because serial communication is slow.

You may also find this helpful:

Thanks AWOL! There are really helpful people around here! :slight_smile:

But (referring to the topic) I’m still confused what’s the problem with the second code in my start posting… I realized, that it works when either using both “return” with a constant value or using both with a variable. But when using one variable and one constant (no matter in what order) I’ll alway get zero returned.

I’ve tried to reproduce with simpler code, but the following works exactly as expected (I get “abc” every two seconds):

void setup(){
  Serial.begin( 9600 );
}

void loop(){
  Serial.print( a() );
  Serial.print( b() );
  Serial.println( c() );
  delay(2000);
}

char a(){
  return 'a';
  return 'x';
}

char b(){
  char n = 'y';
  return 'b';
  return n;
}

char c(){
  char n = 'c';
  return n;
  return 'z';
}

[quote author=Nick Gammon link=topic=262541.msg1852200#msg1852200 date=1408748231]We would greatly prefer it if you posted your amended code in a new post, not amend your original one. Now that you corrected your first post, the comments under it (by other posters) don’t make any sense and are confusing.[/quote]I’ll keep that in mind for the future! (Different board, different rules…) I’ll add a comment, that it has been edited.

Do you KNOW what return does? If ENDS the function. Having two return statements makes no sense. The second one is never executed.

[quote author=Nick Gammon link=topic=262541.msg1852205#msg1852205 date=1408748483]As to your problem, it seems likely to me that you are falling through and executing the "return false" because serial communication is slow.

You may also find this helpful:

http://www.gammon.com.au/tips[/quote]
Looks very promising! I'll post a commentary after studying and testing.

PaulS:
Do you KNOW what return does? If ENDS the function. Having two return statements makes no sense. The second one is never executed.

Yes, I know what return does. I know (e.g.) about stack pushing and the difference between c and std calls and so on, but i don't know how the parser exactly works. This was merely a "parser test". A test if it gets confused when using constant and variable values with return. The code itself isn't making any sense. Guess I should have pointed that out more clearly...

To my knowledge in general: I'm a quite experienced programmer (although not ingenious and not professional), but I'm a newbie in C/C++ and µCs.

PaulS:
Do you KNOW what return does? If ENDS the function. Having two return statements makes no sense. The second one is never executed.

There are lots of reasons to have multiple return statements in a function. It might be an error return, or two "forks" of processing . It is one of the easy ways to avoid gotos. 8^)

Now multiple entry points - which almost made it into C with the original "entry" reserved keyword, I am not so sure about.

you could try using an array with pointers...

you can easily make the array a global variable, if you need.

void setup()
{
  
}


void loop()

{
  char data[3];

  getChars(&data[0]);
  
  Serial.print(data[0]);
  Serial.print(data[1]);
  Serial.println(data[2]);
  delay(3000);
}

void getChars(char *pdata)
{
  pdata[0] = 'a';
  pdata[1] = 'b';
  pdata[2] = 'c';
}

Brduino:
Looks very promising! I’ll post a commentary after studying and testing.

Imagine for a moment that there is one character available on the serial port (which is very likely). Your code will go through the “while” loop once, and then fall through and return false.

If you want to get a whole line (and block whilst doing it) here is an example of doing that:

void getline (char * buf, size_t bufsize)
{
byte i;
  // discard any old junk
  while (Serial.available ())
    Serial.read ();
  for (i = 0; i < bufsize - 1; )
    {
    if (Serial.available ())
      {
      int c = Serial.read ();
      if (c == '\n')  // newline terminates
        break;
      if (!isspace (c))  // ignore spaces, carriage-return etc.
        buf [i++] = c;
      } // end if available
    }  // end of for
  buf [i] = 0;  // terminator
  Serial.println (buf);  // echo what they typed
  }     // end of getline

That particular code ignores spaces which may or may not suit you.

It loops until a newline which is the “line terminator” in this case. You pass down as an argument where the data is to go (the buffer) and its size.

You may or may not want to discard any existing input (the “discard any old junk” part).

but i don't know how the parser exactly works. This was merely a "parser test".

Parser? What are you talking about?

There are lots of reasons to have multiple return statements in a function. It might be an error return, or two "forks" of processing . It is one of the easy ways to avoid gotos. 8^)

Can you cite a single example where two unconditional return statements in a row are ever useful?

OK! Everything's clear now! I've changed "byte n;" to "byte n = 0;" in both codes (the ones in my start posting) here in the forum, but only changed it in one source code (the already working one) here on my PC. facepalm

Long story short: It really was the uninitialized variable which was causing the trouble!
As so often, the bug was sitting in front of the computer. :wink:

Thanks to all for the nice and quick help!!

PaulS:
Parser? What are you talking about?

The C parser. Sorry... one letter was missing to make it understandable... It could have been that using a return with a constant and a return with a variable leads to complining wrong somehow... or so... or not... maybe... :wink:
Like said before, I'm not very familiar with C/C++.

@ Nick Gammon: Your "trap door collection" is really nice! Although most seem obvious when reading through I already managed to fall into two of them (uninitalized local variable and mixing up "==" and "=").

That code in reply number 12 is not very good. If the for loop is running faster than the serial, it will probably only get one character.

Not at all. I use that code in my hex code uploader which sits waiting for a file name. That’s been working for ages now.

for (i = 0; i < bufsize - 1; )
    {
    if (Serial.available ())
      {
      int c = Serial.read ();
      if (c == '\n')  // newline terminates
        break;
      if (!isspace (c))  // ignore spaces, carriage-return etc.
        buf [i++] = c;
      } // end if available
    }  // end of for

The main loop is waiting for the buffer to fill (until bufsize is reached). If there is no data it just loops. The “break” is when a newline is received. So it exits on a newline, or buffer full.

Just to emphasise, there is no increment on this line:

for (i = 0; i < bufsize - 1; )

So that loop potentially runs for hours.

So that loop potentially runs for hours.

I'd use a while statement, instead of a for loop. One expects a for loop to iterate a fixed number of times, and to increment automatically. One expects a while loop to keep iterating until the end condition is met.

While a for loop can be made to work like a while loop, and a while statement can be made to work like a for loop, the point of coding is to use the best statement possible. That way, later on, one does not have to study the code to pick of the for loop nature of the while statement, or the while nature of the for loop.