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");
          } 
        }
    }
}
  • 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. – Majenko Nov 23 '17 at 13:19
  • @Majenko Why one character? I write 034000017 to serial. – Lowder Nov 23 '17 at 13:25
  • No, you write "0" to the serial. Then you write "3" to the serial. Etc. – Majenko Nov 23 '17 at 13:27
  • This may be of use to you: hackingmajenkoblog.wordpress.com/2016/02/01/… – Majenko Nov 23 '17 at 13:32
  • 1
    What's the point in spitting the string and then combing it back again into the same string? – gre_gor Nov 23 '17 at 14:29
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.

  • 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. – Majenko Nov 23 '17 at 15:32
  • @Majenko. You are right; I edited it. – user31481 Nov 23 '17 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? – Chris Stratton Nov 23 '17 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. – user31481 Nov 23 '17 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. – Chris Stratton Nov 24 '17 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

  • This is much closer to being a correct answer. – Chris Stratton Nov 24 '17 at 0:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.