Serial input Command line - is there a better way?

Hi everyone,
I have to control an arduino with a PC, so I wrote the attached sketch, I would like feedback if there is a different way to do this, or if there are any fundamental flaws in my logic.

All input welcome.

char inCommand [255];		//	Command line MAX length
String Commands [10];		//	Number of 'words' in the command
String temp = "";
int inChar;
int Terminator = 0;
int location = 0;
int Space;

void setup()
{

Serial.begin(115200);

}

void loop()
{
if (Serial.available()>0)
{
DoCommands();
}

}




/*
 *
 *	This gets the input from the Serial port and returns the complete command
 *	line. You can either use direct comparison to have commands, or you can
 *	strip the returned string and write an interpreter. 
 *
 */
String getCommandLine()
{
if(Serial.available() > 0)
{
	getInput();
}
temp="";
if (Terminator == 1)
{
	temp = inCommand;							// convert inCommand to a usable format
	temp.toUpperCase();							// and convert to uppercase
	memset(inCommand, 0, sizeof(inCommand));
	Terminator = 0;								// Reset Terminator
	location = 0;								// Reset String Location Pointer
}	
return(temp);
}


/*
 *
 *	This gets the command line from the serial port
 *
 *
 */
void getInput()
{

	while (Terminator == 0)
	{
		if(Serial.available()>0)
		{
		inChar = Serial.read();
			if (inChar == '\n')
			{
			Terminator = 1;
			break;
			}
		inCommand[location] = inChar;
		location++;
		}
	}
}


void Stripcommand(String Command)
{

	Space = Command.indexOf(" ");
	Commands[1] = Command.substring(0,Space);
	Command = Command.substring(Space+1,Command.length());
	for(int here = 2; here < 11; here++)
	{
		Space = Command.indexOf(" ");
		Commands[here] = Command.substring(0,Space);
		Command = Command.substring(Space+1,Command.length());
		if(Space<1)
		{
			Command[0] = here; // Pass back the command count
			break;
		}
	}
}

void DoCommands()
{
		

		String Command = getCommandLine();
		if (Command != "")
		{
			Stripcommand(Command);
			Command="";					//	Clear the command
		}

		if (Commands[1] == "LED")
		{
			if (Commands[3] == "ON")
			{
				digitalWrite(Commands[2].toInt(),HIGH); //	I don't test if the value is in range here!
			}
			if (Commands[3] == "OFF")
			{
				digitalWrite(Commands[2].toInt(),LOW);
			}

		}

		if (Commands[1] == "DIGITALWRITE")
		{
			if (Commands[3]== "HIGH")
			{
				digitalWrite(Commands[2].toInt(),HIGH);
			}
			if (Commands[3]== "LOW")
			{
				digitalWrite(Commands[2].toInt(),LOW);
			}

		}

		if (Commands[1] == "PINMODE")
		{
			if (Commands[3] == "OUTPUT")
			{
				pinMode(Commands[2].toInt(),OUTPUT);
			}
			
			if (Commands[3] == "INPUT")
			{
				pinMode(Commands[2].toInt(),INPUT);
			}

		}

		if (Commands[1] == "ANALOGREAD")
		{

			{
				Serial.print(analogRead(Commands[2].toInt()));
			}


		}

}

Better way, yes. Get ride of the stupid String class and use a proper C++ sting. See Trap23 ;)

The way you collect the command line is not good, as it will block in waitInput until your terminator is received. If the terminator is not received, it will never return. You should take whatever characters are available, then return, WITHOUT waiting, but don't process the command line until the terminator is received. That allows the rest of the program to continue doing other things while waiting.

You should get rid of the String class, and use strtok, or at least strcmp, to parse the commands. Also, use strupr to force all the commands to upper case before parsing, so the user can enter lower case or upper case, and both will work. Case-sensitive user input is very 1970s....

Regards, Ray L.

@Ray: You raise an interesting point about blocking until the termination character is read. If a program cannot do anything until the input is read, what's the best way to collect the input? If there's nothing to do until all the input is read, blocking is less of an issue. If there is something that might happen while waiting (e.g., an ISR perhaps), then blocking becomes an issue. Your thoughts?

Not Ray but…
Call readSerial1 from loop and if it returns true you have a full line. If the sender is uncontrolled then you may need to buffer the line or stop calling readSerial1 until the line is processed as it will start overwriting the current line as soon as it’s called again.

if (readSerial1()){                   // Call serial1 read routine (it returns true if linefeed character found)
}

-------------------------------------------

boolean readSerial1() {
  static byte index = 0;                // Buffer array index counter
  
  while (Serial1.available() > 0) {     // Does serial1 have characters available?
    char rc = Serial1.read();           // Read a character
    
    if (rc == '\n') {                   // Is it newline?
      charBuffer1[index] = 0;           // Yes, so terminate char array with 0
      index = 0;                        // Reset array index
      return true;                      // Return true (we have a full line)
    }
    if (rc >= '0') {                    // Is value between 32 and 127?
      charBuffer1[index++] = rc;        // Yes, so store it and increment index
      if (index > CHARBUFFERSIZE) {     // Have we filled the buffer?
        index--;                        // Yes, so decrement index
      }
    }
  }
  return false;                         // Return false (we don't have a full line)
}

@Riva: Given what I see, why not just use:

   char buffer[MAXCHARSEXPECTED};
   int charsRead;
   
   // perhaps some more code?

   if (Serial.available() > 0) {
      charsRead = Serial.readBytesUntil('\n', buffer, MAXCHARSEXPECTED - 1);
      buffer[charsRead] = '\0';
      // the packet is now a string...do whatever...
   }

Have a look at the examples in serial input basics - simple, reliable non-blocking ways to receive data - and no Strings (capital S).

...R

I would like feedback if there is a different way to do this, or if there are any fundamental flaws in my logic.

Probably best you describe just what you are trying to do and provide an example of the command string being sent from the pc.

econjack: @Riva: Given what I see, why not just use:

   char buffer[MAXCHARSEXPECTED};
   int charsRead;
   
   // perhaps some more code?

  if (Serial.available() > 0) {       charsRead = Serial.readBytesUntil('\n', buffer, MAXCHARSEXPECTED - 1);       buffer[charsRead] = '\0';       // the packet is now a string...do whatever...   }

I had not included the full code, I had just lifted most relevant section from the sketch I wrote it for. The reason your example is no good is it's blocking. The code I posted you call periodically and it returns true when a full line has arrived else it returns false.

Programme Serveur WEB
Configuration Serial commande Line (cmdLine)

Log Serial :

  • Ouver le Moniteur série ou IDE Arduino, PUTTY, KUTTY, COM - Termite.exe …
  • Commande disponible :

help

  • General commands :
    help aide commande cmd Serial (man)
    ram RAM Disponible

  • Configuration commands :
    print print configuration courrante
    defaults set default configuration values
    mac set local MAC address (default: DE:AD:BE:EF:FE:ED
    dhcp-on enable DHCP
    dhcp-off disable DHCP
    ip set local IP address Exemple : ip 192.168.0.66
    subnet set Masque de sous réseau
    gateway set Passerelle

  • EEPROM access commands:
    reload load EEPROM avec la configuration sauvegarde
    save save configuration en EEPROM
    reset Restart Arduino et load la configuration enregistre en EEPROM

Pour changer l’IP de l’Arduino :
Commande ip xxx.xxx.xxx.xxx puis save et enfin reset
pour redemarrer l’arduino dans la nouvelle configuration

Programme testé ok !!

By

Prise_IP_SRV_WEB_Ethernet_Login_ConfIP_Serial_cmd_BoutonWEB_v8.ino (17.4 KB)

ConfigIP_Serial_CmdLine.h (9.55 KB)

index_HTML_PROGMEM.h (2.28 KB)

a good command parser will recognise reserved words (or abbreviated words), and their position within the command line. If an invalid word is detected in any position, it should be flagged in the return code. Similarly, when a complete command is parsed, it must be a valid combination of keywords for the current context, if not tell the user what’s wrong.

You can do this with a list of valid keywords for each position - maybe in an array.... e.g SET, CLEAR, LOCK, UNLOCK — first word 0/1/2 MODE, LED, RELAY, ALL — second word 0/1/2 ON, OFF, DEFAULT, blank — third word 0/1/2

Develop a scoring algorithm* to make a best guess at misspelled words, or reject them. Normalise the case if necessary to permit case insensitive typing if that’s what you want.

Finally, you have a value e.g. 2.2.1, which suggests LOCK RELAY OFF was typed. Then you can use your choice of selector to run the functions you want.

  • While a little more complicated, scoring allows simple mistyping to get through as long as the command isn’t ambiguous... e.g. reLy cannot be confused with any other word in the second position, and almost matches RELAY without any ambiguity... so you can safely assume they neant to type reLay. Advanced devs will allow auto completion once a keyword has been identified.