0

In my code I have that string:String code = rs1 + rs2 + rs3; //rs1=034, rs2=000, rs3=017 It means that code = "034000017". But if I run if (code.equals("034000017")) Serial.println("OK"); There is nothing in serial monitor. Here is full code:

String code, rs1, rs2, rs3, readString;
void setup() {
    Serial.begin(9600);
    Serial.println("Serial ready!");
}

void loop() {
    if (Serial.available()) {
        while (Serial.available()) {
          delay(10);  
          if (Serial.available() >0) {
            char c = Serial.read();
            readString += c;
            rs1 = readString.substring(0, 3); //034
            rs2 = readString.substring(3, 6); //000
            rs3 = readString.substring(6, 9); //017
            code = rs1 + rs2 + rs3;
            Serial.println(code);
            if (code.equals("034000017")) Serial.println("OK");
          } 
        }
    }
}
Improve this question
7
  • 1
    Why do you read just one character and add it to the string then split the string up as if it had at least 9 characters in it? You need to read the entire string first before splitting it. Commented Nov 23, 2017 at 13:19
  • @Majenko Why one character? I write 034000017 to serial. Commented Nov 23, 2017 at 13:25
  • No, you write "0" to the serial. Then you write "3" to the serial. Etc. Commented Nov 23, 2017 at 13:27
  • This may be of use to you: hackingmajenkoblog.wordpress.com/2016/02/01/… Commented Nov 23, 2017 at 13:32
  • 1
    What's the point in spitting the string and then combing it back again into the same string? Commented Nov 23, 2017 at 14:29

2 Answers 2

Reset to default
2

Lots of errors in your code. Mine is basically the same that @frarugi87's answer.

I only have to add: when reading serial, wait for the end of line ('\r' and/or '\n') to know when you have all your data, so you don't depend on length (which can be corrupted).

Edit (on Majeko's comment)

String code, rs1, rs2, rs3, readString;
void setup()
{
  Serial.begin(9600);
  Serial.println("Serial ready!");
}

void loop()
{
  static bool inData = true;

  while (Serial.available()) {
    char c = (char) Serial.read();

    if (c != '\r' && c != '\n') {
      readString += c;
      inData = true;
    }
    else {
      if (inData) {
        rs1 = readString.substring(0, 3); //034
        rs2 = readString.substring(3, 6); //000
        rs3 = readString.substring(6, 9); //017
        code = rs1 + rs2 + rs3;
        Serial.println(code);

        if (code.equals("034000017")) {
          Serial.println("OK");
        }
        readString = "";
        inData = false;
      }
    }
  }
}

This code works for any configuration of line ending ('\r', '\n' or '\r\n').

It doesn't test for length (or validity or timeout); it's left as an exercise to the reader :-)

I hope it shows how code grows when making it robust in all scenarios.

Improve this answer
6
  • The only failing with your code is if the string is terminated with \r\n - in that case, the \r will trigger a parsing, and then straight after the \n will trigger another with an empty string. You should decide on just one to use, not either/or. Discard the one you're not using. A switch/case can be good for that. Also checking the length of the string can be useful to make sure you got all the data. Commented Nov 23, 2017 at 15:32
  • @Majenko. You are right; I edited it. Commented Nov 23, 2017 at 17:03
  • ...but why are you retaining the absurd splitting of the String into parts only to expensively concatenate them right back together in the same order? Commented Nov 23, 2017 at 21:09
  • @ChrisStratton. Because that is not what the OP asked. I took his code and made it better. He will learn by comparing his version with mine. If I were to post the way I program, he will understand nothing and my answer will be useless to him. If you are so brilliant, go and post a better (in your opinion) answer. Commented Nov 23, 2017 at 21:26
  • The point you are making in your text isn't an important one - it's not actually necessary to wait for the end of the line, you can keep checking and failing. It is wasteful to do that, but it's also wasteful to keep in the pointless things you kept in. The only thing you're proposing that actually has a chance of helping with the actual problem is clearing the accumulated string, but you call no attention to that fact. Commented Nov 24, 2017 at 0:31
1

In your code there is a trivial error and a conceptual error.

The trivial error is in line 16, you wrote

rs2 = readString.substring(6, 9); //017

but it should have been

rs3 = readString.substring(6, 9); //017

(3 instead of 2)

The conceptual one is that you do not have to check the string every time; if you know that the string must be 9 chars long, check for it:

void loop() {
    while (Serial.available()) {
        readString += (char)Serial.read();
        if (readString.length() >= 9) {
            rs1 = readString.substring(0, 3); //034
            rs2 = readString.substring(3, 6); //000
            rs3 = readString.substring(6, 9); //017
            code = rs1 + rs2 + rs3;
            Serial.println(code);
            if (code.equals("034000017")) Serial.println("OK");
            readString = "";
        }
    }
}

Note that in this code I cleaned up a bit your code, and MOST IMPORTANT I reset the readstring to nothing after the check (otherwise only the first string is accepted)

I left a lot of things as they were (for instance in my opinion using a string in this case is pointless - better use a byte array; or splitting the string in three for joining it again later), since I think this is part of a larger program

Improve this answer
1
  • This is much closer to being a correct answer. Commented Nov 24, 2017 at 0:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.