Program sometimes freezing arduino

Hi,
I have been working on two small programs for a few days now. One of them runs on a Arduino Uno with a W5100 Ethernet Shield and is working as intended. The other one runs on a Arduino Nano (ATMega 328) and controls a 21x16 flip-disc display (Wikipedia) via a library (came with the display). The display controlling also works as intended. Now to the problem: the arduino with the ethernet shield is running a http server and sends a part of the request data to the arduino controlling the flip-disc display via I2C. Both arduinos share a common ground and there are 2K pull-up resistors between the I2C cables and 5V (internal resistors are disabled), the cables also have a makeshift shield (also connected to ground). In many cases sending data will work perfectly, but sometimes the receiving arduino (flip-disc controller) will crash while receiving and/or parsing the received data (I have got no idea where that actually is/which line of code is causing the crash, crash means the arduino will do nothing before you reset it, even watchdog stops working!). Maybe I am just not reading everything correctly, but I have not been able to find the mistake (5+ hours of debugging), so I wanted to ask you to look over the code and tell me if you have found a problem.

See more recent post at the bottom for updated code!

--- Code for arduino with ethernet shield ---

#include <SPI.h>
#include <Ethernet.h>
#include <Wire.h>

#define RESET 2

byte mac[] = {
  0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED
};

EthernetServer server(80);

void setup() {
  pinMode(RESET, OUTPUT);
  digitalWrite(RESET, HIGH);
  
  Serial.begin(9600);
  Wire.begin();
  
  Ethernet.begin(mac);
  server.begin();
  
  Serial.print("Flipdot Server is at ");
  Serial.println(Ethernet.localIP());
}


void loop() {
  EthernetClient client = server.available();
  if (client) {
    Serial.println();
    char arg[350] = "";
    boolean currentLineIsBlank = true;
    byte commandPos = 0;
    while (client.connected()) {
      if (client.available()) {
        char c = client.read();
        if(c == '/' && commandPos == 0) {
          commandPos = 1;
        }
        else if(c == ' ' && commandPos == 1) {
          commandPos = 2;
        }
        else if(commandPos == 1) {
          char str[2] = {c, '\0'};
          strcat(arg, str);
        }
        
        if (c == '\n' && currentLineIsBlank) {
          client.println("HTTP/1.1 200 OK");
          client.println("Content-Type: text/html");
          client.println("Access-Control-Allow-Origin: *");
          client.println("Connection: close");
          client.println();
          client.println("<!DOCTYPE HTML>");
          client.println("<html>");
          client.println("Request answered");
          client.println("</html>");
          break;
        }
        if (c == '\n') {
          currentLineIsBlank = true;
        } else if (c != '\r') {
          currentLineIsBlank = false;
        }
      }
    }
    delay(1);
    client.stop();

    if(arg[0] == 'x') {
      digitalWrite(RESET, LOW);
      delay(10);
      digitalWrite(RESET, HIGH);
      return;
    }

    Serial.println("Sending received data to flipdot...");
    
    Wire.beginTransmission(105);
    
    Wire.write(arg[0]);
    char len[11] = "";
    for(byte i = 2; i < 12; i++) {
      len[i - 2] = arg[i];
    }
    len[10] = '\0';
    long lenL = atol(len);
    {
      byte multiplier = 0;
      byte buf = 0b00000000;
      for(byte i = 0; i < 32; i++) {
        if(i % 8 == 0 && i != 0) {
          multiplier++;
          Wire.write(buf);
          buf = 0b00000000;
        }
        if(((lenL >> i) & 1) == 1) {
          buf |= 1 << (i - multiplier * 8);
        }
      }
      Wire.write(buf);
      Wire.write('\0');
    }
    Serial.println("Sending header data...");
    
    byte state = Wire.endTransmission();
    
    Wire.beginTransmission(105);

    if(arg[0] == 'i') {
      byte multiplier = 0;
      byte buf = 0b00000000;
      for(int i = 0; i < strlen(arg) - 12; i++) {
        if(i % 8 == 0 && i != 0) {
          multiplier++;
          Wire.write(buf);
          buf = 0b00000000;
          if(multiplier % 32 == 0 && multiplier != 0) {
            Wire.endTransmission();
            Wire.beginTransmission(105);
          }
          if(multiplier == 42) {
            break;
          }
        }
        if(arg[i + 13] == '1') {
          buf |= 1 << (i - multiplier * 8);
        }
      }
      Wire.write('\0');
      Serial.println("Sending 42 bytes of image data...");
    }
    else if(arg[0] == 'c') {
      char unix[11] = "";
      for(byte i = 13; i < 23; i++) {
        unix[i - 13] = arg[i];
      }
      unix[10] = '\0';
      long unixL = atol(unix);
      byte multiplier = 0;
      byte buf = 0b00000000;
      for(byte i = 0; i < 32; i++) {
        if(i % 8 == 0 && i != 0) {
          multiplier++;
          Wire.write(buf);
          buf = 0b00000000;
        }
        if(((unixL >> i) & 1) == 1) {
          buf |= 1 << (i - multiplier * 8);
        }
      }
      Wire.write(buf);
      Wire.write('\0');
    }
    else if(arg[0] == 'r') {
      Wire.write('\0');
    }

    if(Wire.endTransmission() == 0 && state == 0) {
      Serial.println("All data successfully sent!");
    }
    else {
      Serial.println("An error occurred while sending the data!");
    }
  }
}

More (didn´t fit)

-- Code for arduino controlling the flip-disc display ---

#include <Wire.h>
#include <wwFlipdot02.h>
#include <DS3231.h>

DS3231 clock;
RTCDateTime dt;

boolean numbers[10][5][3] = {
  {{1, 1, 1}, {1, 0, 1}, {1, 0, 1}, {1, 0, 1}, {1, 1, 1}}, //0
  {{0, 0, 1}, {0, 0, 1}, {0, 0, 1}, {0, 0, 1}, {0, 0, 1}}, //1
  {{1, 1, 1}, {0, 0, 1}, {1, 1, 1}, {1, 0, 0}, {1, 1, 1}}, //2
  {{1, 1, 1}, {0, 0, 1}, {1, 1, 1}, {0, 0, 1}, {1, 1, 1}}, //3
  {{1, 0, 1}, {1, 0, 1}, {1, 1, 1}, {0, 0, 1}, {0, 0, 1}}, //4
  {{1, 1, 1}, {1, 0, 0}, {1, 1, 1}, {0, 0, 1}, {1, 1, 1}}, //5
  {{1, 1, 1}, {1, 0, 0}, {1, 1, 1}, {1, 0, 1}, {1, 1, 1}}, //6
  {{1, 1, 1}, {0, 0, 1}, {0, 0, 1}, {0, 0, 1}, {0, 0, 1}}, //7
  {{1, 1, 1}, {1, 0, 1}, {1, 1, 1}, {1, 0, 1}, {1, 1, 1}}, //8
  {{1, 1, 1}, {1, 0, 1}, {1, 1, 1}, {0, 0, 1}, {1, 1, 1}}, //9
  };

boolean colon[5][3] = {
  {0, 0, 0},
  {0, 1, 0},
  {0, 0, 0},
  {0, 1, 0},
  {0, 0, 0}
  };

boolean dot[5][3] = {
  {0, 0, 0},
  {0, 0, 0},
  {0, 0, 0},
  {0, 0, 0},
  {0, 1, 0}
  };

byte positions[][2] = {
  {2, 2},
  {6, 2},
  {9, 2},
  {12, 2},
  {16, 2},
  {2, 9},
  {6, 9},
  {9, 9},
  {12, 9},
  {16, 9}
  };

byte positions1[][2] = {
  {7, 9},
  {11, 9}
  };

byte oldDay;

boolean configChange = false;

volatile char command = '\0';
volatile char text[65] = "";
volatile byte process = 0;
volatile long len = 0;
volatile long unix = 0;
volatile short count = -1;

void setup() {
  Serial.begin(9600);
  
  clock.begin();

  dt = clock.getDateTime();

  oldDay = dt.day;

  clock.armAlarm1(false);
  clock.armAlarm2(false);
  clock.clearAlarm1();
  clock.clearAlarm2();
  clock.setAlarm2(0, 0, 0, DS3231_EVERY_MINUTE, true);

  dotSetup(8, 21, 1, 16, 0);

  dotPowerOn();
  delay(50);
  resetAll(0);
  delay(100);

  Wire.begin(105);
  Wire.onReceive(receiveEvent);
  
  Serial.println("ready");
  
  configChange = true;
}

void loop() {
  if(clock.isAlarm1() && process == 3) {
    clock.armAlarm1(false);
    clock.clearAlarm1();
    process = 0;
    command = '\0';
    len = 0;
    unix = 0;
    for(byte i = 0; i < 65; i++) {
      text[i] = '\0';
    }
    count = -1;
    Serial.println("Resetting flipdot (alarm triggered)...");
    resetAll(0);
    delay(50);
    configChange = true;
  }
  else if(process == 3 || process == 1) {
    return;
  }
  else if(process == 2) {    // this processes the received data (I think that this is the part that´s crashing the arduino, but I could be wrong
    process = 3;
    Serial.println("Processing data...");
    if(command == 'i') {
      Serial.println("Displaying image...");
      for(byte i = 0; i < 42; i++) {
        byte x = floor((float) i / (float) 2) + 1;
        byte y = 9;
        if(i % 2 == 0) {
          y = 1;
        }
        for(byte j = 0; j < 8; j++) {
          if(((text[i] >> j) & 1) == 1) {
            setDot(x, y + j);
          }
          else {
            resetDot(x, y + j);
          }
          delay(1);
        }
      }
    }
    else if(command == 'c') {
      unix += len;
      Serial.print("Setting time to ");
      Serial.print(unix);
      Serial.println(".");
      clock.setDateTime(unix);
    }
    else if(command == 'r') {
      Serial.println("Reset flipdot!");
    }
    delay(20);
    dt = clock.getDateTime();
    byte minu = dt.minute;
    byte sec = dt.second + len;
    byte hou = dt.hour;
    if(command == 'r' || command == 'c') {
      sec = dt.second + 2;
    }
    while(sec > 59) {
      minu++;
      sec -= 60;
    }
    while(minu > 59) {
      hou++;
      minu -= 60;
    }
    oldDay = dt.day;
    clock.setAlarm1(dt.day, hou, minu, sec, DS3231_MATCH_DT_H_M_S);
    return;
  }
  
  if(clock.isAlarm2() || configChange) {
    dt = clock.getDateTime();
    char hour[3];
    String(dt.hour).toCharArray(hour, 3);
    char minute[3];
    String(dt.minute).toCharArray(minute, 3);
    char month[3];
    String(dt.month).toCharArray(month, 3);
    char day[3];
    String(dt.day).toCharArray(day, 3);

    if(dt.day != oldDay) {
      setAll(20);
      delay(100);
      resetAll(0);
      delay(100);
      oldDay = dt.day;
    }
    
    if(strlen(hour) == 1) {
      char num = hour[0];
      hour[0] = '0';
      hour[1] = num;
    }
    
    if(strlen(minute) == 1) {
      char num = minute[0];
      minute[0] = '0';
      minute[1] = num;
    }
    
    if(strlen(month) == 1) {
      char num = month[0];
      month[0] = '0';
      month[1] = num;
    }
    
    if(strlen(day) == 1) {
      char num = day[0];
      day[0] = '0';
      day[1] = num;
    }

    boolean (*data[])[3] = {
      numbers[((byte) hour[0]) - 48],
      numbers[((byte) hour[1]) - 48],
      colon,
      numbers[((byte) minute[0]) - 48],
      numbers[((byte) minute[1]) - 48],
      numbers[((byte) day[0]) - 48],
      numbers[((byte) day[1]) - 48],
      dot,
      numbers[((byte) month[0]) - 48],
      numbers[((byte) month[1]) - 48]
      };
      
    
    for(byte i = 0; i < 10; i++) {
      for(byte j = 0; j < 5; j++) {
        for(byte k = 0; k < 3; k++) {
          if(data[i][j][k]) {
            setDot(positions[i][0] + k + 1, positions[i][1] + j + 1);
          }
          else {
            resetDot(positions[i][0] + k + 1, positions[i][1] + j + 1);
          }
          delay(5);
        }
      }
    }
    configChange = false;
  }
}

// code for receiving i2c data (seems to work fine)
void receiveEvent(int howMany) {
  if(process == 2 || process == 3) {
    return;
  }
  
  char arg[4] = "";
  
  while(Wire.available() > 0) {
    char c = Wire.read();
    count++;
    if(process == 0) {
      if(count == 0) {
        command = c;
      }
      else if(count > 0 && count < 5) {
        arg[count - 1] = c;
      }
      else if(count > 4) {
        len = 0b00000000000000000000000000000000;
        byte multiplier = 0;
        for(byte i = 0; i < 32; i++) {
          if(i % 8 == 0 && i != 0) {
            multiplier++;
          }
          if(((arg[multiplier] >> (i - multiplier * 8)) & 1) == 1) {
            bitSet(len, i);
          }
        }
        process = 1;
        count = -1;
      }
    }
    else if(process == 1) {
      if(command == 'i') {
        if(count == 42) {
          count = -1;
          process = 2;
        }
        else {
          if(count < 42 && count > -1) {
            text[count] = c;
          }
        }
      }
      else if(command == 'c') {
        if(c == '\0') {
          unix = 0b00000000000000000000000000000000;
          byte multiplier = 0;
          for(byte i = 0; i < 32; i++) {
            if(i % 8 == 0 && i != 0) {
              multiplier++;
            }
            if(((arg[multiplier] >> (i - multiplier * 8)) & 1) == 1) {
              bitSet(unix, i);
            }
          }
          count = -1;
          process = 2;
        }
        else {
          if(count < 4 && count > -1) {
            arg[count] = c;
          }
        }
      }
      else if(command == 'r') {
        process = 2;
      }
    }
  }
}

This ↑ one displays a clock when not requested to show other data (ex.: image). Data is transmitted in bytes/chars which contain bits of data (If an image is transmitted, 1 would mean "enable dot", while 0 would mean "disable dot". If I pack the bits into bytes instead of transmitting bytes and only using one bit for 1/0, I can save lots of time and I2C transmissions).


Wiring

It is not a good idea to use the String (capital S) class on an Arduino as it can cause memory corruption in the small memory on an Arduino. Just use cstrings - char arrays terminated with 0.

The use of the String class is often the cause of unpredictable failures.

...R

Your code is quite opaque and not a lot of fun to read. Little structure, no functions with nice names, almost no comments. :frowning:

  else if (process == 3 || process == 1) {
    return;
  }

I would be interested to see what happens when you toggle a LED on and off with a short delay within this block, ie. when process == 1
Why have you used numbers for the process variable instead of a nice friendly enum or named constants?

    byte state = Wire.endTransmission();

    Wire.beginTransmission(105);
...
          if (multiplier % 32 == 0 && multiplier != 0) {
            Wire.endTransmission(); // need to check the return code here
            Wire.beginTransmission(105);
          }
...
    if (Wire.endTransmission() == 0 && state == 0) {
      Serial.println("All data successfully sent!");
    }

You should verify the return code of every endTransmission()

crash means the arduino will do nothing before you reset it, even watchdog stops working!

I don't see where you set up the hardware watchdog?

And, as advised by Robin2, String is a bad idea. You don't use it much, so it won't hurt much to get rid of it completely.

Hi,
How long is your I2C link?

Thanks.. Tom... :slight_smile:

Robin2:
It is not a good idea to use the String (capital S) class on an Arduino as it can cause memory corruption in the small memory on an Arduino. Just use cstrings - char arrays terminated with 0.

Changed it, see new code.

arduarn:
Your code is quite opaque and not a lot of fun to read. Little structure, no functions with nice names, almost no comments. :frowning:

  else if (process == 3 || process == 1) {

return;
  }




I would be interested to see what happens when you toggle a LED on and off with a short delay within this block, ie. when *process == 1*
Why have you used numbers for the *process* variable instead of a nice friendly enum or named constants?

Changed to enums and commented code, see new code.
Interestingly enough, doing something if process has a specific value (I changed a dot on the display if process was 0) makes the code work every? single time (I tried about 50 times).

arduarn:

    byte state = Wire.endTransmission();

Wire.beginTransmission(105);
...
          if (multiplier % 32 == 0 && multiplier != 0) {
            Wire.endTransmission(); // need to check the return code here
            Wire.beginTransmission(105);
          }
...
    if (Wire.endTransmission() == 0 && state == 0) {
      Serial.println("All data successfully sent!");
    }



You should verify the return code of every *endTransmission()*

Verified all transmissions, see new code. The ethernet arduino is still reporting perfect transmissions even if the other one crashed.

arduarn:
I don't see where you set up the hardware watchdog?

I removed it because it did absolutely nothing.

TomGeorge:
Hi,
How long is your I2C link?

Thanks.. Tom... :slight_smile:

About 20cm. I have tried shielding it, but that changed nothing (grounded tinfoil).

New code:

cyber__creeper:
Changed to enums and commented code, see new code.

Even those small changes make a huge difference. Whilst folks can see what what your code does by careful inspection, without the comments and appropriate naming they cannot understand what you intended to do.

cyber__creeper:
Interestingly enough, doing something if process has a specific value (I changed a dot on the display if process was 0) makes the code work every? single time (I tried about 50 times).

Please clarify what you mean here: Are you saying that when the device has "crashed" you are now able to flip dots? What does "the code works" mean? Where are you doing the flip?

The reason I asked about state == 1 is because it creates an infinite loop in loop(). You are then dependent on the (rather complicated) logic in receiveEvent() to break out of it. I get the feeling that if anything goes wrong in receiveEvent() and the message data gets muddled, you could get stuck in the infinite loop. But it is only a guess.

cyber__creeper:
[the watchdog] I removed it because it did absolutely nothing.

Probably not worth discussing since it has been removed, but the thought arose that perhaps the device was not crashing, but rather not responding in the way you expected.
We have to differentiate between the two modes (actually 3, but third is a combination) of operation, whether the watchdog is configured just to generate an interrupt (which could be ignored or otherwise go wrong), or generate a reset (which I would expect to be very reliable).
In any case, better to fix the real error rather than trying to conceal it with a reset.

If I were you, I would add more serial debug to the code to try and verify that it is crashing as you think, and to try and narrow down the location. It would also be worth printing the values of key variables, like process, and maybe even message content. Start simple and keep adding until you know more.

I may take another look if i get a little more time later.

arduarn:
Please clarify what you mean here: Are you saying that when the device has "crashed" you are now able to flip dots? What does "the code works" mean? Where are you doing the flip?

The reason I asked about state == 1 is because it creates an infinite loop in loop(). You are then dependent on the (rather complicated) logic in receiveEvent() to break out of it. I get the feeling that if anything goes wrong in receiveEvent() and the message data gets muddled, you could get stuck in the infinite loop. But it is only a guess.

I added this code to the start of the main loop:

if(process == STATE_WAITING) {
  setDot(1, 1);
}
else {
  resetDot(1, 1);
}

Just wanted to see what value process has when the program crashes (see further down for explanation). But with this code, the program stopped crashing. Why this is, I cannot explain, but a delay might also do the trick.

Now to crashing. When I say "crash", tht means that the device stops. This can be checked easily by adding a debug output at the start of the main loop.

Serial.println(millis());

(could output anything)
When the program crashes, the output in the serial monitor will stop -> a) maybe stuck somewhere in interrupt or b) the device actually stops processing the program

Try the attached (obviously untested) version to see if there is any change in behaviour.

fliptest_xxx.ino (9.84 KB)

Thanks for your code, it is working great!

cyber__creeper:
Thanks for your code, it is working great!

Before you crack out the Champagne, that was just some test code to prove a theory of mine. Although the test isn't foolproof, for the purposes of this post I'm going to assume that my theory has been proven.

So what is going wrong?
I think it is a combination of how you are using the Wire library and what I would basically say are flaws in the Wire library.

Your loop() can be simplified to:

void loop() {
  clock.isAlarmX()
}

In other words, basically a tight loop checking the alarm status. Each call to isAlarmX() results in a Wire.write() and, more importantly, a Wire.requestFrom().
While this is going on, you can also receive asynchronous messages from your other device. These result in an interrupt and your void receiveEvent() being called.
The tight loop causes a higher probability of collisions.

There are multiple layers to the Wire implementation. The lower in twi.c seems to be OK. The upper in Wire.cpp, which implements the external API, is where I see an issue. The receive buffer (rxBuffer) that is used in the upper layer is shared between both the asynchronous message receiver and the blocking requestFrom(). Neither of the buffer management variables rxBufferIndex nor rxBufferLength are correctly labelled as volatile. Even with the variables correctly labelled, I still think there is an instruction interleaving that could cause problems.

Basically what can happen is that the requestFrom() call (in the DS3231 library) receives a message, an interrupt occurs, the receiveEvent() scoffs the message, including the the requestFrom() reply, and the DS3231 library then hangs in a while(!Wire.available()) {}; waiting on a message that never arrives.

This is one failure mode that I believe can happen. I'm not saying that there couldn't be others.

You should be aware too that if requestFrom() is processing a message and an asynchronous message arrives, assuming that the fault as described doesn't happen, the usual behaviour is to discard the asynchronous message. This has implications for your receive protocol as implemented in your receiveEvent().
You should also be aware that your receiveEvent() can cause your sketch to get stuck in the STATE_READ_HEADER state if command is an invalid value, perhaps due to a missing message. There is no default case for an unknown command and no timeout for giving up after not receiving a full message.

So all that being said, my test code is not really a fix, but your could use it in conjunction with a correctly implemented reset watchdog. The real fix would need to take place in Wire.cpp.