String comparison yields bad results

I read from my serial and print to make sure entries are correct. When "red" "green" or "blue" (typed without the quotes) none of the comparisons work. The printed line shows the correct color. Hardware all checks with the last else flashing the colors correctly.

I tried if (myColor == "red") and if (myColor.equals("red") and none of the colors work.

I substituted String x="red" and then if (x.equals("red")) and it works as expected. I know that the problem is with the Serial reading to myColor. I just can't figure out how to make it work.

Code:

int redPin=8;
int greenPin=9;
int bluePin=10;

String myColor;
String msg="What color do you want? ";

void setup() {
    // put your setup code here, to run once:
    Serial.begin(9600);
    pinMode(redPin, OUTPUT);
    pinMode(bluePin, OUTPUT);
    pinMode(greenPin, OUTPUT);
}

void loop() {
    Serial.println(msg);

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

    myColor=Serial.readString();
    Serial.println(myColor); //check to see that it was entered correctly

    if (myColor == "red"){
        digitalWrite(redPin, LOW);
        digitalWrite(bluePin, HIGH);
        digitalWrite(greenPin, LOW);
    }
    else if (myColor == "green"){
        digitalWrite(redPin, LOW);
        digitalWrite(bluePin, LOW);
        digitalWrite(greenPin, HIGH);
    }
    else if (myColor == "blue"){
        digitalWrite(redPin, LOW);
        digitalWrite(bluePin, HIGH);
        digitalWrite(greenPin, LOW);
    }
    else {
        for (int x = 0; x < 3; x++){
            digitalWrite(redPin, HIGH);
            delay (500);
            digitalWrite(redPin, LOW);
            delay (500);
            digitalWrite(bluePin, HIGH);
            delay (500);
            digitalWrite(bluePin, LOW);
            delay (500);
            digitalWrite(greenPin, HIGH);
            delay (500);
            digitalWrite(greenPin, LOW);
            delay(500);
        }
    }
}

Solution

Change your checking code to be something like:

String x = String("[") + myColor + String("]");
Serial.println(x);
Serial.println(myColor.length());

There's a good chance that your string may have other stuff in it, such as a newline character or space. If it does, you'll see output like:

[red ]
[ red]
[red
]

rather than the single-line [red]. The second println is to output the length for verification as well, to ensure that red matches 3, blue matches 4, and so on.

And just two final things:

I'd tend to turn off current outputs first before turning on the relevant one. It may not be necessary but it comes from being burnt by systems that don't like having multiple outputs on at the same time. That means, for blue, you would do something like the following, where the other two are first driven low, then blue is driven high:
```
digitalWrite(redPin,   LOW);
digitalWrite(greenPin, LOW);
digitalWrite(bluePin,  HIGH);
```
Your code block for red is currently the same as blue, you probably should have this instead:
```
digitalWrite(bluePin,  LOW);
digitalWrite(greenPin, LOW);
digitalWrite(redPin,   HIGH);
```

On that first bullet point above, you could simplify your code with a little refactoring, such as introducing a function to do the selection of which pin is on:

void offOffOn(int offA, int offB, int on) {
    digitalWrite(offA, LOW);
    digitalWrite(offB, LOW);
    digitalWrite(on,   HIGH);
}

Then your loop() section becomes shorter (with a "compressed" else clause as well):

if (myColor == "red") {
    offOffOn(greenPin, bluePin, redPin);
} else if (myColor == "green") {
    offOffOn(redPin, bluePin, greenPin);
} else if (myColor == "blue") {
    offOffOn(redPin, greenPin, bluePin);
} else {
    int allPins[] = { redPin, bluePin, greenPin };
    size_t pinCount = sizeof(allPins) / sizeof(*allPins);
    for (int count = 0; count < pinCount * 3; ++count) {
        digitalWrite(allPins[idx % pinCount], HIGH); delay (500);
        digitalWrite(allPins[idx % pinCount], LOW);  delay (500);
    }
}