How to call a function?

Using Arduino Programming Questions
1

Hello!

I want to call my_function in a loop. How to do it?

  int led = 13; 
  
  void setup()
  {
     pinMode(led, OUTPUT); 
     Serial.begin(9600, SERIAL_8N1);
     Serial.flush();  
  }
  
  void my_function()
  {
    char buf[4];
    int charsRead;
    charsRead = Serial.readBytesUntil('\n', buf, 4);
    buf[charsRead] = '\0'; 
    return my_function();
  }
 
  void loop()
  {
  
     if (strcmp(my_function(), "on") == 0) 
     {
         digitalWrite(led, HIGH); 
         Serial.println("on");     
     }
     
     else if (strcmp(my_function(), "off") == 0)
     {
         digitalWrite(led, LOW);
         Serial.println("off");       
     }
  }

I have a problem.

sketch_dec01a.ino: In function ‘void loop()’:
sketch_dec01a.ino:22:36: error: invalid use of void expression
sketch_dec01a.ino:28:42: error: invalid use of void expression
2 
return my_function();

You try to return a the function. Two thing why this does not work. First of all, you define your function as void, which is saying, this function will not return anything. Second of all, why return the function? I think you're interested in what's in the buffer...

3

jedneraz:
Hello!

I want to call my_function in a loop. How to do it?

  int led = 13;

void setup()
 {
    pinMode(led, OUTPUT);
    Serial.begin(9600, SERIAL_8N1);
    Serial.flush();  
 }
 
 void my_function()
 {
   char buf[4];
   int charsRead;
   charsRead = Serial.readBytesUntil('\n', buf, 4);
   buf[charsRead] = '\0';
   return my_function();
 }

void loop()
 {
 
    if (strcmp(my_function(), "on") == 0)
    {
        digitalWrite(led, HIGH);
        Serial.println("on");    
    }
   
    else if (strcmp(my_function(), "off") == 0)
    {
        digitalWrite(led, LOW);
        Serial.println("off");      
    }
 }




I have a problem.

sketch_dec01a.ino: In function ‘void loop()’:
sketch_dec01a.ino:22:36: error: invalid use of void expression
sketch_dec01a.ino:28:42: error: invalid use of void expression

void my_function() {}; // returns nothing, that's what void means

strcmp( char *, char * ) is not strcmp( void, char * ) which gives you those 2 errors.

You need

  char * my_function(  )
  {
    char buf[4];
    int charsRead;
    charsRead = Serial.readBytesUntil('\n', buf, 4);
    buf[charsRead] = '\0'; 
    return buf;
  }

And those strcmp() will have to match exactly to work. I changed them to strstr() since I have serial monitor add a newline to every line it sends. I could have gone with matching "on\n" and "off\n" thopugh.

Compiles and works:

  byte led = 13; // don't use an int where a byte would do on an 8 bit machine.
  
  void setup()
  {
     pinMode(led, OUTPUT); 
     Serial.begin( 115200 ); // why use slower?
     Serial.flush();  
  }
  
  char * my_function(  )
  {
    char buf[4];
    byte charsRead;
    charsRead = Serial.readBytesUntil('\n', buf, 4);
    buf[charsRead] = '\0'; 
    return buf;
  }
 
  void loop()
  {
  
     if (strstr(my_function(), "on") > 0 )  // you can change this back
     {
         digitalWrite(led, HIGH); 
         Serial.println("on");     
     }
     
     else if (strstr(my_function(), "off") > 0)  // you can change this back
     {
         digitalWrite(led, LOW);
         Serial.println("off");       
     }
  }
4 
     Serial.begin(9600, SERIAL_8N1);
     Serial.flush();

Open the serial port. Then, block until all outgoing serial data has been sent. Why?

Here's a clue. You have not sent any data, so it is stupid to wait for an empty buffer to be empty.

Do not call functions that you do not know what they do.

5

Actually, the statement:

return my_function();

is syntactically correct by itself, provided it's not a void function. However, since you defined it with a void function type specifier, the compiler knows it cannot return a void data type, hence the hiccup.

6

econjack:
Actually, the statement:

return my_function();

is syntactically correct by itself, provided it's not a void function. However, since you defined it with a void function type specifier, the compiler knows it cannot return a void data type, hence the hiccup.

The recursive nature of the call, if the return type was not void, would lead to stack exhaustion very quickly.

7

GoForSmoke:
You need

  char * my_function(  )

{
   char buf[4];
   int charsRead;
   charsRead = Serial.readBytesUntil('\n', buf, 4);
   buf[charsRead] = '\0';
   return buf;
 }

This is invalid because you return a pointer to an automatic variable.

econjack:
Actually, the statement:

return my_function();

is syntactically correct by itself, provided it's not a void function. However, since you defined it with a void function type specifier, the compiler knows it cannot return a void data type, hence the hiccup.

It's valid even with it returning void. Notice that the compiler messages said nothing about that line. Here's a quick demonstration:

// return-void.c
void foo() { return foo(); }

c@franz:~$ gcc -Wall -O2 -c return-void.c
c@franz:~$

Cool! No errors.

PaulS:
The recursive nature of the call, if the return type was not void, would lead to stack exhaustion very quickly.

The compiler is smart enough (at -O2 or higher optimization) to turn a tail call into a "jump" instruction rather than a "call" instruction, which means it doesn't use additional stack space for each recursive call.

8 
  byte led = 13; // don't use an int where a byte would do on an 8 bit machine.

Why?

Thank you all for your help.

9

septillion:

return my_function();

You try to return a the function. Two thing why this does not work. First of all, you define your function as void, which is saying, this function will not return anything. Second of all, why return the function? I think you're interested in what's in the buffer...

Much worse than that.... return my_function() attempts to CALL my_function() and return it's return value. If my_function were not declared as void(), this would result in infinite recursion, causing an almost instantaneous crash due to the stack consuming all of RAM.

Regards,
Ray L.

10

jedneraz:

  byte led = 13; // don't use an int where a byte would do on an 8 bit machine.

Why?

Thank you all for your help.

Because an int takes two bytes but byte only takes 1. 13 fits into a byte so using int just wastes a whole byte of RAM. On the RAM limited Arduino it is best to save memory space wherever you can.

11

christop:
This is invalid because you return a pointer to an automatic variable.

How strange since it compiles and runs just fine.

I was sure that it wouldn't work, on return SP moves and you lose stack vars so WTH I tried it and it works. This is in Arduino IDE 1.6.1. I enter "on" and led13 turns on, I enter "off" it turns off.

Try it.

12

How strange since it compiles and runs just fine.

I was sure that it wouldn't work, on return SP moves and you lose stack vars so WTH I tried it and it works. This is in Arduino IDE 1.6.1. I enter "on" and led13 turns on, I enter "off" it turns off.

Try it.

Try calling a couple of other functions in between. I'm positive you won't be so happy.

13

byte led = 13; // don't use an int where a byte would do on an 8 bit machine.

Also, use constant as you really won't be changing the value of led and don't need it in RAM.

const byte led = 13;

14

jedneraz:

  byte led = 13; // don't use an int where a byte would do on an 8 bit machine.

Why?

Thank you all for your help.

Arduino Uno has 2048 bytes for everything. Automatically typing int gets to be a RAM wasting habit.

A byte can be 0 to 255 which covers pin numbers and most loop indexes. A char can be -128 to 127.

It really begins to add up when you use arrays, easy to do then when you use int by default.

15

GoForSmoke:

This is invalid because you return a pointer to an automatic variable.

How strange since it compiles and runs just fine.

Bwahahaha! Oh, it compiles just fine. No worries there. No worries at all. But you will start to get weird, inexplicable things happening. That's because buf[] is a stack variable, and if anyone else uses the stack in between when you get that value back and when you use it, it's bad mojo.

Most of the time, what you are doing there will promptly cause things to cease working. The bad cases are when it works perfectly fine … except when an interrupt happens at juuuust the wrong moment.

16

PaulMurrayCbr:
How strange since it compiles and runs just fine.

Bwahahaha! Oh, it compiles just fine. No worries there. No worries at all. But you will start to get weird, inexplicable things happening. That's because buf[] is a stack variable, and if anyone else uses the stack in between when you get that value back and when you use it, it's bad mojo.

Most of the time, what you are doing there will promptly cause things to cease working. The bad cases are when it works perfectly fine … except when an interrupt happens at juuuust the wrong moment.

So you ran the sketch.....

17

LarryD:
Also, use constant as you really won't be changing the value of led and don't need it in RAM.

const byte led = 13;

Or don't declare anything at all and use the predefined constant LED_BUILTIN.

18

GoForSmoke:
So you ran the sketch.....

Accessing an object that no longer exists results in undefined behavior, which means the program can do anything, including doing what you expect it to do.

Even if it seem to work right now, it's not guaranteed to work tomorrow or with different compiler flags or if you change something else in your code.

Besides all that, the compiler should give you a warning (assuming you have verbose compilation enabled) such as "function returns address of local variable [-Wreturn-local-addr]" which tells you that you're doing something wrong.

19

I know. You're right. Didn't doing that used to generate an error?

I expected a problem right away since the clock interrupt happens so much but no.
Yup, if your data is where the next stack write goes, it's gonna change!

Really for the OP, don't code it like that what I did above. It is 'almost right' which is baaad.
If I had made it a static array, it would be safe on the heap instead of the stack.

This code shows where the real problem is, in loop(), fixed below.

byte led = 13; // don't use an int where a byte would do on an 8 bit machine.

void setup()
{
  pinMode( led, OUTPUT );
  Serial.begin( 115200 ); // why use slower?
  Serial.flush();
}

char * my_function( char *buf )
{
  byte charsRead;
  for ( byte i = 0; i < 4; i++ ) buf[ i ] = 0;
  charsRead = Serial.readBytesUntil( '\n', buf, 4 );
  buf[ charsRead ] = 0;
  return buf;
}

void loop()
{
  static char buff[ 4 ];

  my_function( buff ); // read once, not twice to get data entered once
  
  if ( strcmp( buff, "on" ) == 0 )
  {
    digitalWrite( led, HIGH );
    Serial.println( "on" );
  }
  else if ( strcmp( buff, "off" ) == 0 )
  {
    digitalWrite( led, LOW );
    Serial.println( "off" );
  }
}