Problem reading using Serial.read()

I have tried this code, that is supposed to take user input (“on” or “off”) and turn an LED on or off, as the input requests. This code helped me only to turn on the LED, but the OFF “command” does not work. I’ve tried a lot of other methods, which didn’t work. Feel free to ask me anything and send me some of your suggestions, please!

// the setup function runs once when you press reset or power the board
int fade = 11;

void setup() { 
  pinMode(fade, OUTPUT);
  Serial.begin(9600);
  Serial.println("Input n for on. Input f for off");
}

char inChar;
char inData[4];
int index = 0;

void loop()
{
  while(Serial.available() > 0){ // Don't read unless there you know there is data 
      if(index < 3) // One less than the size of the array
      {
          inChar = Serial.read(); // Read a character
          inData[index] = inChar; // Store it
          index++; // Increment where to write next
          inData[index] = '\0'; // Null terminate the string
      }
  }

  if(strcmp(inData, "on") == 0){
    digitalWrite(fade, HIGH);
  }else if(strcmp(inData, "off") == 0){
    digitalWrite(fade, LOW);
  }//else Serial.println(inData);
  
}

After you've successfully recognized an "on" or "off" you need to reset your input buffer so it is ready to accept the next command. Make a little function to do this, so you can call it from each of the different commands. You will need to write all zeroes to the buffer and set index back to zero.

One thing to consider before writing that function is make the size of the buffer a constant that can be used all over the program instead of writing "3" or "4" in the code. That's called a "magic number" and it's always bad to use magic numbers.

It might be easier to switch to one-character commands. "o" for "on" and "f" for "off"?

If you reset the buffer only after a successful command, an unsuccessful command will sit in the buffer and block all future commands.

Since each command starts with ‘o’ you could use that to reset the buffer index, like this:

  if (Serial.available()) // Don't read unless there you know there is data
  {
    inChar = Serial.read(); // Read a character

    // If you see an 'o', figure it's the start of a command.
    if (inChar == 'o')
      index = 0;

    if (index < 3) // One less than the size of the array
    {
      inData[index] = inChar; // Store it
      index++; // Increment where to write next
      inData[index] = '\0'; // Null terminate the string
    }
  }

Have a look at the examples in Serial Input Basics - simple reliable ways to receive data. There is also a parse example to illustrate how to extract numbers from the received text.

It will be much easier for your Arduino code if you just send ‘N’ for ON and ‘F’ for OFF

…R

The OP has explicitly said that he wants to use this command: on to ON (ignite) the Led; he wants to use this command: off to OFF (extinguish) the Led; instead, we are suggesting him to use these commands: o/N for ON and f/F for OFF.

The OP is looking for a solution in this way:

if(received string is equal to "on")
{
   ON (ignite) Led
} 
else
{
   if(received string is equal to "off")
   {
      OFF (extinguish) Led
   }
}

The codes that the OP has presented should track the above control structures.

GolamMostafa:
instead, we are suggesting him to use these commands: o/N for ON and f/F for OFF.

I'm not sure who you mean by "we" but it does not include me. I was proposing something much simpler in Reply #3

...R

Robin2:
I'm not sure who you mean by "we" but it does not include me. I was proposing something much simpler in Reply #3

Here, 'we' does include all the readers/participants of the Arduino Forum including you, me, and all others.

GolamMostafa:
Here, 'we' does include all the readers/participants of the Arduino Forum including you, me, and all others.

Like I said in Reply #5, please exclude me from YOUR suggestion that the OP sends "o/N for ON and f/F for OFF. "

...R

Robin2:
Like I said in Reply #5, please exclude me from YOUR suggestion that the OP sends "o/N for ON and f/F for OFF.

Post#1 has suggested OP to send ‘o’ to ON the Led and ‘f’ to OFF the Led; Post#3 due to @Robin2 has suggested OP to send ‘N’ to ON the Led and ‘F’ to OFF the Led. Having combined these two suggestions, I have said that we (not subjective) are suggesting the OP to send ‘o/N (o or N)’ to ON the Led and ‘f/F (f or F)’ to OFF the Led; whereas, the OP has exclusively wanted to ON the Led by the “on” command and OFF the Led by the “OFF” command. Let us see how we can help him (the OP) to meet his objectives using “on” and “off” commands.

I confess I did not read the Original Post sufficiently closely, but I am not the only person guilty of that.

I now note that it says very clearly in the code

Serial.println("Input n for on. Input f for off");

so that my suggestion in Reply #3 was unnecessary.

...R

@OP

There has been a great pleasure in this thread by virtue of your interesting post. Here is a sketch (Edit: the program has some serious limitations/flaws) that I have tested using UNO + Built-in Led where the Led becomes ON only in response to on command from the Serial Monitor; the Led goes OFF only in response to off command from the Serial Monitor. The sketch may help you in the modification of your own sketch that you have posted.

char inChar;
//char inData[4];
int index = 0;
int fade = 11;  //tested using DPin-13 built-in Led of UNO
bool flag1 = false;

void setup()
{
  pinMode(fade, OUTPUT);
  digitalWrite(fade, LOW);
  Serial.begin(9600);
  Serial.println("Input 'on' for ON and Input 'off' for OFF");
}

void loop()
{
  byte n = Serial.available();
  if (n != 0)
  {
    if (flag1 == false)
    {
      inChar = Serial.read();
      Serial.print(inChar);
      if (inChar == 'o')
      {
        index++;  //1
        flag1 = true;
      }
    }
    else
    {
      inChar = Serial.read();
      //Serial.println();

      if (inChar == 'n')
      {
        Serial.print(inChar);
        index++; //2
        if (index == 2)
        {
          digitalWrite(fade, HIGH); //Led is only ON against on command
          index = 0;
          flag1 = false;
          Serial.println();
        }
      }
      else
      {
        if (inChar == 'f')
        {
          Serial.print(inChar);
          index++;    //2
          if (index == 3)
          {
            digitalWrite(fade, LOW); //Led OFF against only off command
            index = 0;
            flag1 = false;
            Serial.println();
          }
        }
      }
    }
  }
}

Edit: Edited in view of Post#11.

GolamMostafa:
@OP

There has been a great pleasure in this thread by virtue of your interesting post. Here is a sketch that I have tested using UNO + Built-in Led where the Led becomes ON only in response to on command from the Serial Monitor; the Led goes OFF only in response to off command from the Serial Monitor. The sketch may help you in the modification of your own sketch that you have posted.

char inChar;

//char inData[4];
int index = 0;
int fade = 11;  //tested using DPin-13 built-in Led of UNO
bool flag1 = false;

void setup()
{
 pinMode(fade, OUTPUT);
 digitalWrite(fade, LOW);
 Serial.begin(9600);
 Serial.println("Input 'on' for ON and Input 'off' for OFF");
}

void loop()
{
 byte n = Serial.available();
 if (n != 0)
 {
   if (flag1 == false)
   {
     inChar = Serial.read();
     Serial.print(inChar);
     if (inChar == 'o')
     {
       index++;  //1
       flag1 = true;
     }
   }
   else
   {
     inChar = Serial.read();
     //Serial.println();

if (inChar == 'n')
     {
       Serial.print(inChar);
       index++; //2
       if (index == 2)
       {
         digitalWrite(fade, HIGH); //Led is only ON against on command
         index = 0;
         flag1 = false;
         Serial.println();
       }
     }
     else
     {
       if (inChar == 'f')
       {
         Serial.print(inChar);
         index++;    //2
         if (index == 3)
         {
           digitalWrite(fade, LOW); //Led OFF against only off command
           index = 0;
           flag1 = false;
           Serial.println();
         }
       }
     }
   }
 }
}

If someone sends 'ofn' your code gets stuck and will never turn the LED on or off again. The 'o' will set 'flag1' to true. Any character after that other than 'n' or 'f' will be ignored. The 'f' will increment the index to 2. The 'n' will increment the index to 3. Any 'n' or 'f' after that will do nothing but increment the index.

johnwasser:
If someone sends ‘ofn’ your code gets stuck and will never turn the LED on or off again. The ‘o’ will set ‘flag1’ to true. Any character after that other than ‘n’ or ‘f’ will be ignored. The ‘f’ will increment the index to 2. The ‘n’ will increment the index to 3. Any ‘n’ or ‘f’ after that will do nothing but increment the index.

Now, I have got the job of ‘tuning the program’. (I was aware about the ‘limitations’ of the program that you have mentioned; but, thinking that the tuning would require some more time, let the OP have a sketch that (at least) works as per his directives based on which he may correct his own program. If I would have waited to post a ‘program with out flaw’, I would certainly miss the opportunity to witness the ‘analytical faculty’ that @johnwasser possesses.)

johnwasser:
If someone sends 'ofn' your code gets stuck and will never turn the LED on or off again. The 'o' will set 'flag1' to true. Any character after that other than 'n' or 'f' will be ignored. The 'f' will increment the index to 2. The 'n' will increment the index to 3. Any 'n' or 'f' after that will do nothing but increment the index.

It looks like (tested) that the above limitations are cleaned from the program by introducing flag2 which assumes LH-state once Led becomes ON. Now, the flag2 defends the entry of n into the string "off".

// the setup function runs once when you press reset or power the board
char inChar;
//char inData[4];
int index = 0;
int fade = 13;  //tested using DPin-13 built-in Led of UNO
bool flag1 = false;
bool flag2 = false;

void setup()
{
  pinMode(fade, OUTPUT);
  digitalWrite(fade, LOW);
  Serial.begin(9600);
  Serial.println("Input 'on' for ON abd Input 'off' for OFF");
}

void loop()
{
  byte n = Serial.available();
  if (n != 0)
  {
    if (flag1 == false)
    {
      inChar = Serial.read();
      // Serial.print(inChar);
      if (inChar == 'o')
      {
        Serial.print(inChar);
        index++;  //1
        flag1 = true;
      }
    }
    else
    {
      inChar = Serial.read();
      //Serial.println();

      if (inChar == 'n')
      {
        if (flag2 == false)
        {
          Serial.print(inChar);
          index++; //2
          if (index == 2)
          {
            digitalWrite(fade, HIGH); //Led is only ON against on command
            index = 0;
            flag1 = false;
            flag2 = true;
            Serial.println();
          }
        }
      }
      else
      {
        if (inChar == 'f')
        {
          Serial.print(inChar);
          index++;    //2
          if (index == 3)
          {
            digitalWrite(fade, LOW); //Led OFF against only off command
            index = 0;
            flag1 = false;
            flag2 = false;
            Serial.println();
          }
        }
      }
    }
  }
}

Why is nobody showing a simple, robust means of parsing arbitrary string tokens, using simple line endings to de-limit entries and reset the buffer? Most of the methods I’ve seen here impress me as really poor approaches to a VERY simple and VERY common problem. Is this not all covered to death, in MUCH better ways, in the sticky post on dealing with Serial I/O?

Regards,
Ray L.

RayLivingston:
Why is nobody showing a simple, robust means of parsing arbitrary string tokens, using simple line endings to de-limit entries and reset the buffer? Most of the methods I've seen here impress me as really poor approaches to a VERY simple and VERY common problem. Is this not all covered to death, in MUCH better ways, in the sticky post on dealing with Serial I/O?

It is very hard to think (for people like us who started programming with the hand-coding of the 8085 instructions) about parsing of tokens other than characters as the user may enter any combinations of any length of string using the Serial Monitor from which only two specific tokens/strings ("on" and "off") are to be honored. It would be very nice and highly appreciable to see the postings of sophisticated versions of the current program.

RayLivingston:
Why is nobody showing a simple, robust means of parsing arbitrary string tokens,

I'm not sure what you have in mind.

Doesn't the parse example in Serial Input Basics cover the point?

...R

Robin2:
I'm not sure what you have in mind.

Doesn't the parse example in Serial Input Basics cover the point?

...R

That's my point - I think that shows MUCH better ways of doing it than some of the contortions I've seen in this thread. The right way is to capture delimited data (line ends being the logical delimiter, given the intent), to trigger "parsing", then use strcmp or something similar to determine what, if anything to do, then flush the buffer and start over.
Regards,
Ray L.

Here’s a pretty robust and extensible serial command receive example that can be easily adapted for the OP’s purposes.

It contains elements encompassing most of what folks are bickering about plus a couple off cool feature :slight_smile:

const int pinLED = LED_BUILTIN;

#define BUFF_SIZE       8   //buffer size
#define MSG_TIMEOUT     50  //mS timeout from last character received

//create a buffer to hold the received message
byte RxBuffer[BUFF_SIZE];
//used to timeout a message that is cut off or too slow
unsigned long
    lastCharacterTime;

//this creates a type that is a pointer to a command function, used in the structure below
typedef void (*CmdFunc)();
//command structure
typedef struct
{
    char    *szCommand;         //command string e.g. "ON", "OFF"; use caps for alpha characters 
    CmdFunc CmdFunction;        //pointer to the function that will execute if this command matches

}structCmdStruct;

//for this example, there are two commands; on and off
#define NUM_CMDS        2
//prototypes of command functions
//need these prototypes before the following CmdTable variable or the compiler
//will complain
void CommandON( void );
void CommandOFF( void );

structCmdStruct CmdTable[NUM_CMDS] = 
{
    {   
        .szCommand = "ON",              //capitalized command for turning the LED on
        .CmdFunction = &CommandON       //if "ON" is received, run CommandON
    },
    {
        .szCommand = "OFF",             //off
        .CmdFunction = &CommandOFF      //off command function
    }
};

void setup( void )
{
    //usual start-up drivel; 9600 baud and an LED pin to show functions work
    Serial.begin(9600);
    pinMode( pinLED, OUTPUT );
    digitalWrite( pinLED, LOW );
    
}//setup

void loop( void )
{
    //can be called as part of a loop doing lots of other things
    ReceiveMessage();
    
}//loop

void ReceiveMessage( void )
{
    byte
        ch;
    static byte
        idxMsg = 0;

    //if we've received at least one character, check for a timeout
    if( idxMsg > 0 )
    {
        //if MSG_TIMEOUT mS has passed since the last character, reset the
        //buffer index
        //(i.e. ignore whatever message this might have been...)
        if( (millis() - lastCharacterTime) > MSG_TIMEOUT )
        {
            idxMsg = 0;
            
        }//if
        
    }//if

    //if there's a character waiting...
    if( Serial.available() > 0 )
    {
        //update the last char timer
        lastCharacterTime = millis();
        //read the character
        ch = Serial.read();
        //if it's the newline (example delimter for typical serial monitor messages)
        //this is the end of the message...
        if( ch == '\n' )
        {
            //insert a NULL to "null-terminate" the message
            RxBuffer[idxMsg] = '\0';
            //and go process it
            ProcessMsg();
            //when we come back, reset the buffer index ready for the next msg
            idxMsg = 0;
            
        }//if
        else
        {
            //still receiving the msg
            //convert alphabetical characters to uppercase (don't have to do this but be sure
            //to watch case when typing in the serial monitor!) and store in the buffer
            RxBuffer[idxMsg] = toupper(ch);
            //bump the buffer pointer but not beyond the limits of the buffer
            idxMsg++;
            if( idxMsg == BUFF_SIZE )
                idxMsg--;
        }//else
        
    }//if
    
}//ReceiveMessage

void ProcessMsg( void )
{
    //got a message...walk through the command table to try to find a match
    for( int i=0; i<NUM_CMDS; i++ )
    {
        //if the received message matches the NULL-terminated command string, we
        //have a match...
        if( strcmp( RxBuffer, CmdTable[i].szCommand ) == 0 )
        {
            //...so execute that command's function
            CmdTable[i].CmdFunction();
            //and leave
            return;
                
        }//if
        
    }//for

    //if we make it here we didn't find a match in the command table
    //so just send an "Unrecognized command" response and what the offending message was
    Serial.print( "Unrecognized command: " ); Serial.println( (char *)RxBuffer );
    
}//ProcessMsg

// Command Functions
//these functions execute the commands. They are pointed to in the command table
void CommandON( void )
{
    //turn on the LED and send a message
    Serial.println( "LED on" );
    digitalWrite( pinLED, HIGH );
        
}//CommandON

void CommandOFF( void )
{
    //turn off the LED and send a message
    Serial.println( "LED off" );
    digitalWrite( pinLED, LOW );
    
}//CommandOFF

Blackfin:
Here's a pretty robust and extensible serial command receive example that can be easily adapted for the OP's purposes.

The program of Post#18 is compiled and uploaded; but, it does not respond to "on" and "off" commands to trigger the built-in Led (L) of UNO; hence, it does not serve the need of the OP.

Let us begin with something that works, and then add ornamental features to it. That's why AE has said that a wise man always starts with something simple and then gradually moves to the complex one.