Serial port input causes Arduino to crash

Using Arduino Programming Questions
1

Hello,

I have the code below for a home security system I've been working on. I have a raspberry pi I'm using as display for this system, and I'm expecting messages with checksum & timestamp in return from raspberry pi to ensure the messages are being received. However time to time, this causes arduino to completely freeze.

I used minicom (serial client) on raspberry pi to replicate the issue and test it. If I type a few random characters like "asdf", most of the time I can get Arduino to crash. I get the following on serial monitor (the lines with pi msg received) when this happens:

If I don't attempt reading serial input, nothing happens. I understand many shun using strings and recommend char arrays for text, but on an Arduino Mega, I would expect it should handle a few characters. I would appreciate any insight anyone may have for the reason of these crashes, and any changes I could try to fix it.

byte alarm_status;
bool serial_comm_error=false;
bool verify_pi=false;
bool incSerialDataChk=false;
char receivedChars[64];
byte reserved_byte=00;
String prev_sensor_numbers="F";
String timestamp="0";
String pi_msg_send="";
unsigned long iteration_time;
unsigned long loop_time;
bool expecting_ack=true;
byte pi_timer=0;

void setup(){
    Serial2.begin(9600);
    Serial.begin(9600);
    for (int i=22; i <= 54; i++){
    pinMode(i, INPUT_PULLUP);
    pinMode(11,OUTPUT);
    }
    //Read alarm state from sdcard instead of setting to 0 here
    alarm_status=0;
}

void loop(){
if (millis()-iteration_time>=1000){
    pi_timer++;
    iteration_time=millis();
    Serial.println("loop begin");
    incSerialDataChk=false;
    //Alarm sound
    digitalWrite(11,LOW);
    //Read Connected Sensors
    String sensor_numbers=GetReedSensorData();
    sensor_numbers=sensor_numbers+","+GetOtherSensorData();
    if (alarm_status!=0){CheckAlarmStatus(sensor_numbers);}
    if (prev_sensor_numbers!=sensor_numbers||pi_timer>15){
        prev_sensor_numbers=sensor_numbers;
        pi_timer=0;
        timestamp=String(millis());
        pi_msg_send=sensor_numbers+",0"+alarm_status+",0"+reserved_byte;
        String checksum=CalcChecksum(pi_msg_send);
        pi_msg_send="upd,"+timestamp+","+pi_msg_send+","+checksum;
        expecting_ack=true;
    }
    if (expecting_ack==true){
        Serial2.println("^"+pi_msg_send+"$");
        Serial.println("^"+pi_msg_send+"$");
    } else {
        Serial2.println("kpl");
    }
    String pi_msg_recvd=GetPiOutput();
    if (pi_msg_recvd.substring(0,3)=="ack" && pi_msg_recvd.substring(4)==timestamp){
        expecting_ack=false;
        pi_timer=0;
        }
    if (pi_msg_recvd.substring(0,3)=="rta" && expecting_ack==false){
        //ok to arm
    }
        

    //Serial.println("loop end");
    }
}

String CalcChecksum(String number_string){
    Serial.println(number_string);
    uint16_t sum1 = 0;
    uint16_t sum2 = 0;
    String leading_zero="0";
    String chksum="";
    char cbuf[3];
    for (int i=0;i<=21;i+=3){
        String temp_str=number_string.substring(i,i+2);
        //Serial.println("substr " + temp_str);
        temp_str.toCharArray(cbuf,3);
        int data=strtoul(cbuf,NULL,16);
        sum1 = sum1+data;
        sum2 = sum2+sum1;
    }
    sum1 = sum1%255;
    sum2 = sum2%255;
    Serial.println(sum1);
    Serial.println(sum2);
    if (String(sum1,HEX).length()==1){chksum=chksum+leading_zero;}
    chksum=chksum+String(sum1,HEX)+",";
    if (String(sum2).length()==1){chksum=chksum+leading_zero;}
    chksum=chksum+String(sum2,HEX);
    Serial.println(chksum);
    return chksum;
}

String GetPiOutput(){
    String received_serial="";
    if (Serial2.available()){
        received_serial=ReadSerialInput();
        Serial.println("new pi msg received: " + received_serial);
        return received_serial;
    }
}

String GetReedSensorData(){
    String output="";
    byte bytearray[4]={0,0,0,0};
    int count=0;
    String leading_zero="0";
    for (int i=22; i <= 53; i++){
        int sensorread = digitalRead(i);
        byte biti=(i-22)%8;
        if(i>22&&biti==0){count++;}
        if (sensorread == HIGH){
          Serial.println(String(count)+" " + String(biti));
          Serial.println(i);
            bitWrite(bytearray[count],7-biti,1);
            } else {
            bitWrite(bytearray[count],7-biti,0);
        }
    }
    for (int i=0;i<=3;i++){
        if (String(bytearray[i],HEX).length()==1){
            output+=leading_zero;
        }
        output+=String(bytearray[i],HEX);
        if (i<3){output+=",";}
    }
    Serial.println(output);
    return output;
}

String GetOtherSensorData(){
  String output="00,00";
  return output;
}

String ReadSerialInput() {
    static boolean recvInProgress = false;
    static byte ndx = 0;
    char startMarker = '^';
    char endMarker = '

;
char rc;
int numChars=64;
//char receivedChars[64];
boolean newData = false;
while (Serial2.available() > 0 && newData == false) {
rc = Serial2.read();
//Check for alphanumeric and punctuation
//if (!(isAlphaNumeric(rc)||isPunct(rc)||isSpace(rc))){return "serial error";}
if (recvInProgress == true) {
if (rc != endMarker) {
receivedChars[ndx] = rc;
ndx++;
if (ndx >= numChars) {
ndx = numChars - 1;
}
}
else {
receivedChars[ndx] = '\0'; // terminate the string
recvInProgress = false;
ndx = 0;
newData = true;
}
}
else if (rc == startMarker) {
recvInProgress = true;
}
}
if (newData == true) {
newData == false;
incSerialDataChk=true;
return receivedChars;
}
}

2

For the love of all that is holy, please learn to correctly use whitespace. Not trying to be mean, but the code is all but unreadable rn.

Yes, avoid "S"trings like the rona on any embedded MCU, no matter the model.

For theory behind robust serial communication, check out the tutorials Serial Input Basics and Serial Input Advanced.

3

Whenever the Arduino gets serial request it goes to reset. I think that you should have seen this, when you open the Serial monitor in the IDE. The minicom software you are using is a unstable software which sends continuous requests to the Arduino, thus the Arduino hangs up. Try using other software instead of the one you are using. It you keep using that serial port software, then your Arduino might be damaged. Please don't use that miniport software.

4

Thank you both for answering.

Power_Broker, I realize there are some indentation errors on that code, I wasn't aware of any PEP8 type coding guidelines for C. I'm not sure what part you're having trouble reading. I will try to convert it to char arrays, and also go over the links you've sent, thank you.

ArnavPawarAA, I use minicom only for troubleshooting, I get similar errors, although rarer, when I sent a serial response with python serial library. Unless I hit a key, minicom does not send anything through serial continuously, and arduino keeps running. If you have any recommendations for linux, I will try them though, thanks.

5

after looking at ReadSerialInput() i'm confused by a few things

the code only returns something, receivedChars if newData is true, otherwise it doesn't return anything. why isn't this an error? (maybe up the error checking -- Files->Preferences compiler warnings). Should it return NUL?

the 2nd thing is receivedChars is a char[], yet is returned from ReadSerialInput() as "String".

have you tested your overflow (64 < ndx) processing? On overflow, why not just terminate with a NUL and return the string instead of decrementing ndx : "ndx = numchars -1;"? If there's an overflow is there any guarantee of getting a "endMarker"?

is GetPiOutput() really needed? why not call ReadSerialInput() and test if ! NUL

       String pi_msg_recvd = GetPiOutput();
        if (pi_msg_recvd.substring(0,3) == "ack"

looks like the body of loop() is only executed once every second. at 9600 bps, a character can be transmitted every 1 msec, or 1000 / second. If not checked often enough, certainly more often once every second, the receive UART buffer can overflow.

i can understand you want to process some things less once every second, but you need to check the UART each loop() iteration

6

Thanks for answering gcjr, there was some unnecessary code as you mentioned, I'm in the process of cleaning it up. Raspberry Pi on the other side responds only if there's a message from Arduino, and even then send only an "ack,timestamp" message which is less than 20 bytes.

newData variable over there is to make sure the function receives the whole message and the string is null terminated before it returns it.

This is a hobby project, which I intend to finish at some point. I don't need to have every function I want in it immediately, but what's in there should work reliably. Thank you for all your help.

7

kmklv:
newData variable over there is to make sure the function receives the whole message and the string is null terminated before it returns it.

If newData equals false, you don't return anything. So the caller will use a pointer to a random location and use that. This is the warning that you get if you increase the warning level to ALL in file -> preferences

C:\Users\sterretje\AppData\Local\Temp\arduino_modified_sketch_235246\sketch_may06a.ino: In function 'String ReadSerialInput()':

C:\Users\sterretje\AppData\Local\Temp\arduino_modified_sketch_235246\sketch_may06a.ino:184:1: warning: control reaches end of non-void function [-Wreturn-type]

You should e.g. return NULL if newData equals false. And you should check for NULL before using it (see below).

You have a few more warnings like above.

Further receivedChars is a character array; your ReadSerialInput() function however is declared as returning a String (capital S); I'm surprised that the compiler does not complain about it.

And the compiler complains about

C:\Users\sterretje\AppData\Local\Temp\arduino_modified_sketch_901954\sketch_may06a.ino:180:13: warning: statement has no effect [-Wunused-value]

     newData == false;

Lastly, you have hacked Robin's Serial Input Basics code; not a good idea in my opinion, follow his examples.

  if (Serial2.available()) {

received_serial = ReadSerialInput();
    Serial.println("new pi msg received: " + received_serial);
    return received_serial;
  }

Robin's code also works properly if you unconditionally call ReadSerialInput(). The above should change to

 received_serial = ReadSerialInput();
  if (received_serial != NULL)
  {
    Serial.println("new pi msg received: " + received_serial);
    return received_serial;
  }

Under the condition that you return NULL from ReadSerialInput(). I'm not sure of the exact implications of mixing String and character arrays; I strongly suggest that you use character arrays.

8

Thank you sterretje, I have gotten rid of almost all strings, and I'm working on the rest, thank you for your assistance.