Adapting Serial.readBytes to use with String

So, I've been writing a function that shall receive a String working as a data package in the following format for example "2:n;12:n;11:n;", ";" splits individual command couples, where the first element (a led to be lit up) is divided from the second element (a time variable, 0 for 50ms;1 for 200;anything for no time) by a ':'.
This is what the function looks right now:

void processpkg(String pkg){
	char input [PKG_SIZE + 1] ;
	byte size = Serial.readBytes(input, PKG_SIZE);
	//pkg.getBytes(input,PKG_SIZE);
	input[size]=0;
    Serial.println(input);
	char* command = strtok(input,";");
	while (command != 0){
		char* separator = strchr(command, ':');
		if (separator != 0){
			*separator = 0;
			int LED = atoi (command);
			++separator;
			int TME = atoi (separator);
			digitalWrite(led[LED], HIGH);
			switch (TME) {
			    case 0: 
			      delay(tempo[0]);
			      break;
			    case 1:
			      delay(tempo[1]);
			      break;
			    default:
			      break;
			}
		}
		command = strtok(0, ";");
	}
}

As you can clearly see by the use of byte size = Serial.readBytes(input, PKG_SIZE); it was built to receive data from the Serial monitor, not from a string in a function model. I need to adapt it to work in that model, I tried using String.getBytes as it sounded like a logic alternative, but that didn't work. I'm not so sure if that idea failed because it's completely wrong or because I implemented it badly. How can I do this ?

PS.:If you want the whole test code:

#define PKG_SIZE 30
int tempo [] = {50,200,0,0};
int led [] = {2,12,11,4,13,10,7,8,9};
int col[]  = {	6,5,3};
void setup() {
	//
	Serial.begin(9600);
    // CONFIGURAÇÃO DOS PINOS  "SAIDA" led[0]
    for (int l = 0; l<=8;l++)
    {
    	pinMode(led[l],OUTPUT);
    }
    for (int i = 0; i<=2;i++)
    {
    	pinMode(col[i],OUTPUT);
    } 
    tempo[2] = 0;
    tempo[3] = 0;
    Serial.println("Setup Done, code starting...  ");
}

void loop() {
	digitalWrite(col[0], HIGH);
	digitalWrite(col[1], HIGH);
	digitalWrite(col[2], HIGH);
  processpkg("2:n;12:n;11:n;4:n;13:n;10:n;7:n;8:n;9:n");

}
void processpkg(String pkg){
	char input [PKG_SIZE + 1] ;
	byte size = Serial.readBytes(input, PKG_SIZE);
	//pkg.getBytes(input,PKG_SIZE);
	input[size]=0;
    Serial.println(input);
	char* command = strtok(input,";");
	while (command != 0){
		char* separator = strchr(command, ':');
		if (separator != 0){
			*separator = 0;
			int LED = atoi (command);
			++separator;
			int TME = atoi (separator);
			digitalWrite(led[LED], HIGH);
			switch (TME) {
			    case 0: 
			      delay(tempo[0]);
			      break;
			    case 1:
			      delay(tempo[1]);
			      break;
			    default:
			      break;
			}
		}
		command = strtok(0, ";");
	}
}

If you are trying to get the characters from a String instance, the toCharArray() method does that.

Do you need to use Strings at all when you are eventually going to use the toCharArray() method to convert a String to a string ? Could you not use C style strings in the first place ?

UKHeliBob, that soluttion worked great, the problem was a lot simpler than I thought, just a misuse of String x string. Thanks a lot for the help.

Full fixed code:

#define PKG_SIZE 30
int tempo [] = {50,200,0,0};
int led [] = {2,12,11,4,13,10,7,8,9};
int col[]  = {	6,5,3};
void setup() {
	Serial.begin(9600);
    for (int l = 0; l<=8;l++)
    {
    	pinMode(led[l],OUTPUT);
    }
    for (int i = 0; i<=2;i++)
    {
    	pinMode(col[i],OUTPUT);
    } 
    tempo[2] = 0;
    tempo[3] = 0;
    Serial.println("Setup Done, code starting...  ");
}

void loop() {
	digitalWrite(col[0], HIGH);
	digitalWrite(col[1], HIGH);
	digitalWrite(col[2], HIGH);
  processpkg("0:n;1:n;2:n;3:n;4:n;5:n;6:n;7:n;8:n");

}
void processpkg(char pkg[]){
    Serial.println(String(pkg));
	char* command = strtok(pkg,";");
	while (command != 0){
		char* separator = strchr(command, ':');
		if (separator != 0){
			*separator = 0;
			int LED = atoi (command);
			++separator;
			int TME = atoi (separator);
			digitalWrite(led[LED], HIGH);
			switch (TME) {
			    case 0: 
			      delay(tempo[0]);
			      break;
			    case 1:
			      delay(tempo[1]);
			      break;
			    default:
			      break;
			}
		}
		command = strtok(0, ";");
	}
}
    Serial.println(String(pkg));

WTF? The Print class knows how to print strings. There is no f**king need to wrap the string in a String so that the println() method has to unwrap the stupid thing.

PaulS, if I did such a stupid thing it must have been because the compiler complained when there was no String(), I don't remember but I wouldn't add something there for no reason. Either way, I'm a bit rusty so I'm sorry for such a stupid mistake.
Now, interesting thing, I went ahead and added the comments inside the processpkg function, some conditional statements and a small change in package parameter, now it looks like this

"C0:n;H0:1;L0:1;F0:n;"

Where C stands for turning on a col[] and F turning it off, and H turning on a led[] and L turning it off, and the following number is which index of led[] or col[] contains the port of the led/column to be turned on/off.
However, it doesn't work, and I'm not sure why. The part before the ':' is stored in command[], so I processed the letters with command[0] and the index identifiers with command[1], but that yields strange results, like command[0] being printed as nothing, and I dont understand why. Does anyone have a clue on why is command[1] working well, but command[0] not? (PS. Also even though Serial.println shows it as nothing, it still triggers the if statement, weird right?)

Full code:

#define PKG_SIZE 30
int tempo [] = {50,200,0,0};
int led [] = {2,12,11,4,13,10,7,8,9};
int col[]  = {	6,5,3};
void setup() {
	Serial.begin(9600);
    for (int l = 0; l<=8;l++)
    {
    	pinMode(led[l],OUTPUT);
    }
    for (int i = 0; i<=2;i++)
    {
    	pinMode(col[i],OUTPUT);
    } 
    tempo[2] = 0;
    tempo[3] = 0;
    Serial.println("Setup Done, code starting...  ");
}

void loop() {
	//digitalWrite(col[0], HIGH);
	//digitalWrite(col[1], HIGH);
	//digitalWrite(col[2], HIGH);
    //processpkg("C0:n;C1:n;C2:n;H0:n;H1:n;H2:n;H3:n;H4:n;H5:n;H6:n;H7:n;H8:n;N:1;L0:n;L1:n;");
    processpkg("C0:n;");//C1:n;C2:n;H0:n;");

}
void processpkg(char pkg[]){
    //Serial.println(pkg);
	char* command = strtok(pkg,";");
	while (command != 0){
		char* separator = strchr(command, ':');
		if (separator != 0){
			*separator = 0;
			//Serial.println(command[1]);
			++separator;
			//char buff []= command;
                        char IND = command[0];
			int CMN = command[1]-'0';
			Serial.println(IND+" // "+String(CMN));
			int TME = atoi (separator);
			if      (IND = 'H'){digitalWrite(led[CMN], HIGH);Serial.println("LED HIGH = "+String(CMN));}
			else if (IND = 'L'){digitalWrite(led[CMN], LOW) ;Serial.println("LED LOW = " +String(CMN));}
			else if (IND = 'C'){digitalWrite(col[CMN], HIGH);Serial.println("COL HIGH = "+String(CMN));}
			else if (IND = 'F'){digitalWrite(col[CMN], LOW) ;Serial.println("COL LOW = " +String(CMN));}
			else if (IND = 'N'){}
			else {Serial.println("It looks like there's something wrong on your command string");}
			//while(1){}
			switch (TME) {
			    case 0: 
			      delay(tempo[0]);
			      break;
			    case 1:
			      delay(tempo[1]);
			      break;
			    default:
			      break;
			}
		}
		command = strtok(0, ";");
	}
}

Given:

    processpkg("C0:n;");//C1:n;C2:n;H0:n;");
	char* command = strtok(pkg,";");

Will result in command = "C0:n".

		char* separator = strchr(command, ':');

Will result in separator pointing to the :.

			*separator = 0;

Will replace the colon with a NULL.

			++separator;

Will result in separator = "n".

                        char IND = command[0];
			int CMN = command[1]-'0';

Will result in IND = 'C' and CMN = 0.

			int TME = atoi (separator);

Is absolute nonsense, since separator ="n".

Serial.println(IND+" // "+String(CMN));
			int TME = atoi (separator);
			if      (IND = 'H'){digitalWrite(led[CMN], HIGH);Serial.println("LED HIGH = "+String(CMN));}
			else if (IND = 'L'){digitalWrite(led[CMN], LOW) ;Serial.println("LED LOW = " +String(CMN));}
			else if (IND = 'C'){digitalWrite(col[CMN], HIGH);Serial.println("COL HIGH = "+String(CMN));}
			else if (IND = 'F'){digitalWrite(col[CMN], LOW) ;Serial.println("COL LOW = " +String(CMN));}

Aside from violating convention by having multiple statements on one line, andpissingeveryoneoffbynotatleastusingsomespaces, pisses away more resources on the USELESS String class. Get over it. Banish the damned String class from your repertoire.

Ok, firstly the multiple statements in one line are simply there for debugging purposes, once its working those Serial.println() will be removed, I figured since it was just a testing feature I'd keep it in the same line, make it easier to remove it later on. About the String(CMN), yeah that was pretty stupid.
Now, the

int TME = atoi (separator);

sounds like absolute nonsense, but this is how it should work, most times the separator will be 0 or 1, n is just there so that it doesn't activate anything on the switch() statement, and thus no delay happens. If i change the 'n' to 9 for example, so that it still doesn't trigger the switch, the code also doesn't work. And exactly as you said, it should result in IND = 'C' and CMN =0, perfect, but what happens if i run the code and open the Serial Monitor this shows up:

Setup Done, code starting...
Æ0
LED HIGH =
/0
LED HIGH =

Why does this happen is still a mistery to me.

Current code:

#define PKG_SIZE 30
int tempo [] = {50,200,0,0};
int led [] = {2,12,11,4,13,10,7,8,9};
int col[]  = {	6,5,3};
void setup() {
	Serial.begin(9600);
    for (int l = 0; l<=8;l++)
    {
    	pinMode(led[l],OUTPUT);
    }
    for (int i = 0; i<=2;i++)
    {
    	pinMode(col[i],OUTPUT);
    } 
    tempo[2] = 0;
    tempo[3] = 0;
    Serial.println("Setup Done, code starting...  ");
}

void loop() {
	//digitalWrite(col[0], HIGH);
	//digitalWrite(col[1], HIGH);
	//digitalWrite(col[2], HIGH);
    //processpkg("C0:n;C1:n;C2:n;H0:n;H1:n;H2:n;H3:n;H4:n;H5:n;H6:n;H7:n;H8:n;N:1;L0:n;L1:n;");
    processpkg("C0:9;H0:9");//C1:n;C2:n;H0:n;");

}
void processpkg(char pkg[]){
    //Serial.println(pkg);
	char* command = strtok(pkg,";");
	while (command != 0){
		char* separator = strchr(command, ':');
		if (separator != 0){
			*separator = 0;
			//Serial.println(command[1]);
			++separator;
			//char buff []= command;
            char IND = command[0];
			int CMN = command[1]-'0';
			Serial.println(IND +" // "+String(CMN));
			int TME = atoi (separator);
			if      (IND = 'H'){digitalWrite(led[CMN], HIGH);      Serial.println("LED HIGH = "+CMN);} //Serial.println for debugging, will be removed.
			else if (IND = 'L'){digitalWrite(led[CMN], LOW) ;      Serial.println("LED LOW = " +CMN);}
			else if (IND = 'C'){digitalWrite(col[CMN], HIGH);      Serial.println("COL HIGH = "+CMN);}
			else if (IND = 'F'){digitalWrite(col[CMN], LOW) ;      Serial.println("COL LOW = " +CMN);}
			else if (IND = 'N'){}
			else {Serial.println("It looks like there's something wrong on your command string");}
			//while(1){}
			switch (TME) {
			    case 0: 
			      delay(tempo[0]);
			      break;
			    case 1:
			      delay(tempo[1]);
			      break;
			    default:
			      break;
			}
		}
		command = strtok(0, ";");
	}
}

Why does this happen is still a mistery to me.

Add some debug statements.

	char* command = strtok(pkg,";");

So, what IS is command pointing to?

		char* separator = strchr(command, ':');

So, what IS separator pointing to?

			*separator = 0;

What did that do to command? To pkg?

Put every { on a new line. Indent your code properly. Get rid of the commented out crap.

Ok, I got rid of all the commented garbage and changed the indentation on my code. Now to the debug statements. If i just run a

Serial.println(pkg);

on the processpkg() function, I get this:

Setup Done, code starting...
C0:9;H0:9;

which is absolutely correct.

Then if I print

strtok(pkg,";")

I get this:

C0:9

which is also correct.

Now if inside the while (command != 0) statement I attempt to print

strchr(command, ':')

it returns:

:9

which is also correct.

Printing separator after

*separator = 0;
++separator;

returns:

9

which is, again, correct.

At this same point in code, if i attempt to print command[0] and command[1] I get:

C

and

0

respectively, which is correct.

Now, if at the end of if (separator != 0) I attempt to print separator,command[0],IND,command[1] and CMN respectively separated by '/' as in :

Serial.println(separator + '/' + command[0] + '/' + IND + '/' + command[1] + '/' + CMN);
while(1){};

I get

Setup Done, code starting...
LED HIGH =
®µv¾`”]Ê�ª9ƒ%&öÑ­¸ü
]ù7MLÍ

Again, very weird, because the values are all correct untill the if statements and then they turn into weird things, and the led's that should be turning on aren't even turning on, even though they work fine when testled() is run. I ran the debug tests inserting the Serial.println in the code and a while(1){} after to stop ot there and get a clear response.I omitted the setup done message on all responses but the last.

Full code:

int tempo [] = {50, 200, 0, 0};
int led [] = {2, 12, 11, 4, 13, 10, 7, 8, 9};
int col[]  = { 6, 5, 3};
void setup()
{
  Serial.begin(9600);
  for (int l = 0; l <= 8; l++)
  {
    pinMode(led[l], OUTPUT);
  }
  for (int i = 0; i <= 2; i++)
  {
    pinMode(col[i], OUTPUT);
  }
  tempo[2] = 0;
  tempo[3] = 0;
  Serial.println("Testing LEDs, you should see one flashing on each column");
  testled();
  Serial.println("Setup Done, code starting...  ");
}

void loop()
{
  processpkg("C0:9;H0:9;");

}
void testled()
{
  digitalWrite(col[0], HIGH);
  digitalWrite(col[1], HIGH);
  digitalWrite(col[2], HIGH);
  digitalWrite(led[0], HIGH);
  delay(300);
  digitalWrite(col[0], LOW);
  digitalWrite(col[1], LOW);
  digitalWrite(col[2], LOW);
  digitalWrite(led[0], LOW);
}
void processpkg(char pkg[])
{
  char* command = strtok(pkg, ";");
  while (command != 0)
  {
    char* separator = strchr(command, ':');
    if (separator != 0)
    {
      *separator = 0;
      ++separator;
      char IND = command[0];
      int CMN = command[1] - '0';
      int TME = atoi (separator);
      if      (IND = 'H')
      {
        digitalWrite(led[CMN], HIGH);
        Serial.println("LED HIGH = " + CMN);
      } //Serial.println for debugging, will be removed.
      else if (IND = 'L')
      {
        digitalWrite(led[CMN], LOW) ;
        Serial.println("LED LOW = " + CMN);
      }
      else if (IND = 'C')
      {
        digitalWrite(col[CMN], HIGH);
        Serial.println("COL HIGH = " + CMN);
      }
      else if (IND = 'F')
      {
        digitalWrite(col[CMN], LOW) ;
        Serial.println("COL LOW = " + CMN);
      }
      else if (IND = 'N') {}
      else
      {
        Serial.println("It looks like there's something wrong on your command string");
      }
      switch (TME)
      {
        case 0:
          delay(tempo[0]);
          break;
        case 1:
          delay(tempo[1]);
          break;
        default:
          break;
      }
    }
    command = strtok(0, ";");
  }
}
Serial.println(separator + '/' + command[0] + '/' + IND + '/' + command[1] + '/' + CMN);

So, you add a pointer and a 3 characters and an int and 3 more characters, and print the sum, and you get weird garbage. I can't begin to imagine why.

Pound this into your head: The + operator DOES NOT DO CONCATENATION. It does ADDITION.

But if I use the String.concat method I'd have to use String again and you just told me to forget about it, i could only thing of using '+'. How should i concatenate them without using String.concat?

But if I use the String.concat method I'd have to use String again and you just told me to forget about it

Yes, I did, and I meant it.

i could only thing of using '+'.

Perhaps you should considering the # operaor, instead. It doesn't concatenate strings, either.

How should i concatenate them

Well, the first thing that you need to ask yourself is what possible advantage is there in concatenating the strings, characters, and ints. So you can use one Serial.print() statement instead of 9 is NOT a good enough reason.

If you absolutely MUST, you need to look into sprintf().

Ok, I simply wrote the Serial.println separately, thanks. So, running this:

     Serial.println(separator);
      Serial.println(command[0]);
      Serial.println(IND);
      Serial.println(command[1]);
      Serial.println(CMN);

Returns this:

Testing LEDs, you should see one flashing on each column
Setup Done, code starting...
LED HIGH =
9
C
H
0
0

If we review it we see that the separator value is correct, command[0] is also correct, but then IND is taking its value from next sequency's command[0], even though CMN is being assigned the correct value from command[1], 0(I double checked by changing the second sequency's command[1] to 1and it worked flawless). Also, adding a final Serial.println(pkg); returns this :

C0

. So the source of the problem was identified, now we just need to undestand why is IND grabbing the wrong value. Any ideas?

now we just need to undestand why is IND grabbing the wrong value. Any ideas?

Maybe one.

      if      (IND = 'H')

= != ==

Oh my god, I can't believe I made that mistake hahahaha. It's woking perfectly now PaulS, I can't thank you enough for the patience through a series of stupid mistakes. Thanks a lot.

Arengorn:
Oh my god, I can't believe I made that mistake hahahaha. It's woking perfectly now PaulS, I can't thank you enough for the patience through a series of stupid mistakes. Thanks a lot.

You will make mistakes again.

It is part of the learning process.

Learn to live with it.

Learn to learn from it.

It's woking perfectly now

I'm happy to help when the results are good and/or you learn something.

One weird thing I just noticed, the code doesn't loop. It runs a first time but that's it, even if i call processpkg() twice it wil only run once. So i added this debug line at the start of the function, I had a feeling the problem was there:

Serial.println("Processing package: ");
Serial.print(pkg);

Now, if I run the code with only this

processpkg("C2:9;H0:9;H1:9;H2:9;H3:9;H4:9;H5:9;H6:9;H7:9;H8:0;L0:9;L1:9;L2:9;L3:9;L4:9;L5:9;L6:9;L7:9;L8:9;F2:0;");

on the void loop(), It returns this:

Testing LEDs, you should see one flashing on each column

Setup Done, code starting...

Processing package:

C2:9;H0:9;H1:9;H2:9;H3:9;H4:9;H5:9;H6:9;H7:9;H8:0;L0:9;L1:9;L2:9;L3:9;L4:9;L5:9;L6:9;L7:9;L8:9;F2:0;Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

C2Processing package:

and it repeats "C2Processing package:" forever without ever repeating the led sequency. The only reason I can think for this to happen is if char pkg[] is being overwritten at the end of the function, and not rewritten when the function is called again, but that makes no sense to me. I shouldn't have used a package where it was unnoticiable wheter it was looping or not while debugging. Any clues on this one?

So i

modified the code without posting it...

The only reason I can think for this to happen is if char pkg[] is being overwritten at the end of the function

You ARE modifying pkg in the function - inserting a NULL in it. strtok() does NOT copy data from the string being parsed. It returns pointers to parts of the string. When you make a change through a pointer returned by strtok() you are modifying the string that is being parsed.