Trouble w/serial control of common cathode RGB LED

I am receiving a four number string from an Android app, 1-255 over serial from bluetooth in the form of (### ### ### ###),(R,G,B,A) Throwing out the alpha for now. I can read them on the serial monitor so I know they are being received. The wiring is very simple for the RGB LED, pins 9,10,11, to a 220 ohm resistor in series with the LED legs, and the cathode to ground. The LED works if power applied to resistor, so its not fried.
Not sure if I broke something, or where to go from here.
Any and all help appreciated.
Thanks in advance

//Thanks to Author: Trevor Shannon,et al For Arduino code, snippets tweaked by Mike 
//see http://trevorshp.com/creations/android_led.htm
 
//pin definitions.  must be PWM-capable pins!
const int redPin = 9;
const int greenPin = 10;
const int bluePin = 11;

//maximum duty cycle to be used on each led for color balancing.  
//if "white" (R=255, G=255, B=255) doesn't look white, reduce the red, green, or blue max value.
const int max_red = 255;
const int max_green = 90;
const int max_blue = 100;
byte colors[4] = {0, 0, 0, 0}; //array to store led brightness values
byte lineEnding = 0x29; //   ")"             //originally 0A; //10 in decimal, ASCII newline character

void setup() 
{ 
 Serial.begin(9600); //note this may need to be changed to match your module 
 Serial.println("OK then, you first, say something....."); 
 Serial.println("Go on, type something in the space above and hit Send,"); 
 Serial.println("or just hit the Enter key"); 
   
  pinMode(redPin, OUTPUT);
  pinMode(greenPin, OUTPUT);
  pinMode(bluePin, OUTPUT);
  
 
} 
 
void loop() 
//this is just to test the serial connection
{ 
 while(Serial.available()==0) 
 {} 
 delay(500); 
 Serial.println(""); 
 Serial.println("I heard you say:"); 
 while(Serial.available()>0) 
 { 
 Serial.write(Serial.read());  // note it is Serial.WRITE 
 } 
 Serial.println(""); 
 
//this is to take received RGB & set PWM outputs
//check that at least 3 bytes are available on the Serial port
  if (Serial.available() > 2){
     //store data up until lineEnding (0x29) in the bytesRead array
     int bytesRead = Serial.readBytesUntil(lineEnding, (char*)colors, 4);//changed 3 to 4
  }  
  //set the three PWM pins according to the data read from the Serial port
  //we also scale the values with map() so that the R, G, and B brightnesses are balanced.
  analogWrite(redPin, map(colors[0], 0, 255, 0, max_red));
  analogWrite(greenPin, map(colors[1], 0, 255, 0, max_green));
  analogWrite(bluePin, map(colors[2], 0, 255, 0, max_blue));


}
 while(Serial.available()==0) 

{}
delay(500);
Serial.println("");
Serial.println("I heard you say:");
while(Serial.available()>0)
{
Serial.write(Serial.read());  // note it is Serial.WRITE
}
Serial.println("");

You DO commentout this before you test, right? It will eat characters as you type them. Remember, loop() keeps on keeping on.

You say you are sending a string, in the form of (xxx xxx xxx xxx)

So it that "100 150 200 250" ?
"(100 150 200 250)" ?
"100 150 200 250)" ?
"100150200250)"
or is it five bytes, of values, 100, 150, 200, 250, 0x29?

Not sure if I broke something, or where to go from here.

So it was working at one time?

You DO commentout this before you test, right? It will eat characters as you type them. Remember, loop() keeps on keeping on.

No, I haven't. I was using that to watch the incoming data on the serial monitor. It eats it???? The string doesn't persist through the loop?

You say you are sending a string, in the form of (xxx xxx xxx xxx)

So it that "100 150 200 250" ?
"(100 150 200 250)" ?
"100 150 200 250)" ?
"100150200250)"
or is it five bytes, of values, 100, 150, 200, 250, 0x29?

(100 150 200 250)<--------This is exactly what shows in the serial monitor.

So it was working at one time?

Well, the serial write portion does, and I have seen Trevor's portion(the RGB stuff) working in a video.
But I have not seen it all working together, no.

Thanks for answering, this is my first project after the examples in the book that came with my kit.
I appreciate it.

The string doesn't persist through the loop?

What string? You are not constructing a string, so how can you expect it to persist?

It eats it?

No. It removes all the data from the incoming serial buffer and sends it to the outgoing serial buffer. You seem to think that the data will then still be in the incoming serial buffer. Why you think that is a mystery.

It eats it???? The string doesn't persist through the loop?

Yes, but as PaulS points out, once it is read by the Serial.read(), it is removed from the serial buffer (which is what I meant by eating it). The Arduino, compared to the serial data transfer, is so blindingly fast that there is never a chance for more than 2 characters to be in the serial buffer before the first if() finds a single character. available. So that character is sent to the Serial output, and Serial.avalable() becomes 0 again. So you need to get rid of that whole thing that tests the serial. You don't need it at all.

(100 150 200 250)

OK.. so let's take a look at what actually gets transferred to the Arduino's serial buffer. This is in hexadecimal, and the spaces between each byte are not in the buffer. They just separate the bytes for us to more easily see what's happening. But there are actual spaces transmitted between each group, as shown in the second line.

28 31 30 30 20 31 35 30 20 32 30 30 20 32 25 20 29    or in characters...
(  1  0  0  sp 1  5  0  sp 2  0  0  sp 2  5  0  )

Now you need to make a choice. You can gather up all the bytes one at a time and convert them one at a time into bytes you can use, or you can use readUntil() to grab the whole lot at once, then step through the buffer to deal with the bytes one at a time. But in either case, you need to do this...

  1. Get a byte. If it's a '(' you can discard it. if it's a number, convert it as in step 2

  2. Make the ASCII numbers into values. You can do this by subtracting hexadecimal 20 (or 48 decimal, or '0', all same thing)

  3. Put each converted value into a byte, one at a time. Hint: multiplying by 10 and adding works well. Or you can use the function atoi()

  4. if it's a space, you're done with one color variable, and you can discard the space and go back to step 1 and keep going, but using the next color variable.

  5. If it's a ')', you are done all three color variables, and you can set your LEDs with them.

Quote
It eats it???? The string doesn't persist through the loop?

Yes, but as PaulS points out, once it is read by the Serial.read(), it is removed from the serial buffer (which is what I meant by eating it). The Arduino, compared to the serial data transfer, is so blindingly fast that there is never a chance for more than 2 characters to be in the serial buffer before the first if() finds a single character. available. So that character is sent to the Serial output, and Serial.avalable() becomes 0 again. So you need to get rid of that whole thing that tests the serial. You don't need it at all.

OK, got it. I had no idea. I know I am getting the data now so I have no problem chunking that.

Quote
(100 150 200 250)

OK.. so let's take a look at what actually gets transferred to the Arduino's serial buffer. This is in hexadecimal, and the spaces between each byte are not in the buffer. They just separate the bytes for us to more easily see what's happening. But there are actual spaces transmitted between each group, as shown in the second line.

Code:
28 31 30 30 20 31 35 30 20 32 30 30 20 32 25 20 29 or in characters...
( 1 0 0 sp 1 5 0 sp 2 0 0 sp 2 5 0 )

  1. Get a byte. If it's a '(' you can discard it. if it's a number, convert it as in step 2

  2. Make the ASCII numbers into values. You can do this by subtracting hexadecimal 20 (or 48 decimal, or '0', all same thing)

OK, now I understand this part, I think. My translation is

char ch = Serial.read();
    if( isDigit(ch) ) 
    {
       value = (ch - '0');

Correct?

Now this part,

  1. Put each converted value into a byte, one at a time. Hint: multiplying by 10 and adding works well. Or you can use the function atoi()

is giving me fits. I understand the math part...255 = 2+(510)....25+(510)
but I don't I get the take the separate values and put into a byte part.
Would it be redvalue = (value * 10) + (ch - '0')
I think that's right.
So, together, I have

int bytesRead = Serial.readBytesUntil(lineEnding, (char*)colors, 4); //originally 3
char ch = Serial.read();
if( isDigit(ch) ) // is this an ascii digit between 0 and 9?
{
value = (ch - '0'); // ASCII value converted to numeric value
redvalue = (value * 10) + (ch - '0')
If(32?)
{??}

Am I on the right track?
Sorry, forgive me for my ignorance. I'm an electrician by trade and grasp the electronics and circuitry, but the programming aspect gives me fits sometimes.
I definitely appreciate the help.

Am I on the right track?
Sorry, forgive me for my ignorance. I'm an electrician by trade and grasp the electronics and circuitry, but the programming aspect gives me fits sometimes.

No need for apologies. You can't expect to be good at something until you learn and practice it.

You are somewhat on the right tack, but you have combined two different methods. One method is to read the entire string, then scan through character by character, parsing it. The other is to read one character at a time, parsing it as you go.

Serial.readBytesUntil() reads bytes until the line ending character arrives, or until the number of bytes specified arrive, or until it times out. Serial.read() reads one byte only. So your first two lines are sort of right, in that the first line reads up to the lineEnding character, but the second line reads nothing, because the first line emptied the serial buffer. So ch ends up with whatever it had before, and you then convert that to redvalue. Oops.

I've writtten up one way of doing this, using the readBytesUntil() function. You seem like you want to learn, so I hope you read through this and understand it.

Comments are in the code.

byte rgba[4];    // 4 color values
char lineEnding = '\n';
byte indx = 0;

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

void loop() {

  if ( Serial.available() ) {
    char colors[18] = {'\0'}; // 17 chars plus a NULL terminator init to all zeros
    int bytesRead = Serial.readBytesUntil(lineEnding, (char*)colors, 18); // read entire line
    Serial.println(colors);  // show colors[]
    indx = 0;  // we will use this to step through the array of color value ints ( rgba[] )
    int value = 0;  // temp int to hold one converted value
    for (int i = 1; i < bytesRead; i++) {   // use i as index into colors[] (start with 1 to skip '<')
      if ( isDigit(colors[i]) ) { // if it's a digit, convert it
        value = (value * 10) + (colors[i] - '0');
      }
      else {  // if it isn't a digit, we have a value
        rgba[indx] = value;  // so we put it into one rgba[] entry
        value = 0;           // reinitialize out temp int
        indx++;              // and get ready for the next color
      }
    }
    for (int i = 0; i < 4; i++) {
      Serial.print("x");
      Serial.println( rgba[i] ); // show the values of red, green, blue, alpha
    }
  }
}

A few notes:
This method lets you put in 1, 2, or three digit values, so "<9 125 34 255>" is OK.
It also allows you to put in something like "<125 54>" to change only the red and green values.
There is no error checking.

So, to set the colors, it would be
analogWrite(redPin, map(rgba[0], 0, 255, 0, max_red));
analogWrite(greenPin, map(rgba[1], 0, 255, 0, max_green));
analogWrite(bluePin, map(rgba[2], 0, 255, 0, max_blue));
going to disregard alpha(rgba[3]) for now.

I appreciate you taking the time to come up with the code, and to comment it out line by line.
I have read through it (many times,lol) and I can understand most of it if I go line by line.
This line:

for (int i = 1; i < bytesRead; i++) { // use i as index into colors[] (start with 1 to skip '<')

particularly the bolded part still confuses me.

mtlandry78:
I appreciate you taking the time to come up with the code, and to comment it out line by line.
I have read through it (many times,lol) and I can understand most of it if I go line by line.
This line:

for (int i = 1; i < bytesRead; i++) { // use i as index into colors[] (start with 1 to skip '<')

particularly the bolded part still confuses me.

That's a comment only. What it's saying is that when you do the Serial.readBytesUntil(lineEnding, (char*)colors, 18), you read everything from the '<' to the '>', and between them are the numbers. Arrays in C++ (or C) are zero-based, so colors[0] contains the first character. We don't want to start reading (and parsing ints) on a delimiter (any non numeric character), so we start with the first numeric character, using the for loop counter as an index.

Ah, I see. Stars counting at 0...and thats the open parentheses character , so skip it. Gotcha.
Thanks

Ah, I see. Stars counting at 0...and thats the open parentheses character , so skip it. Gotcha.

No. Array indexing starts at 0, and that is the < (not an open parenthesis).