Arduino 'crashed?' after 12ish hours

Question

I am all new to the Arduino world. Wrote the following code - using the DHT22 sensor. After 12 hours the heater for my greenhouse wasn't on when it was below the desired temp.

After turning off power and turning back on everything is back to working just fine. I know this code is very messy - but does it have any memory leaks? Or something else that could be causing it to stop working?

http://pastebin.com/CcdUN3jb

Edit - I found an open counter that increased by 1 every 2 seconds. I cant imagine that was it, but I changed it here: http://pastebin.com/nuRjHJkR

Answer 1

The one danger evident in your code is the sprintf(). The world would be a better place if nobody ever used this again. Replace this

void loop() {
  ...
  char buf[128];
  sprintf(buf, "Integer-only reading: Temperature %hi.%01hi C, Humidity %i.%01i %% RH",
               myDHT22.getTemperatureCInt()/10, abs(myDHT22.getTemperatureCInt()%10),
               myDHT22.getHumidityInt()/10, myDHT22.getHumidityInt()%10);
  //Serial.println(buf);

with

void loop() {
 ...
 const size_t sizeBuf = 128;
 char szBuf[128+1];

 snprintf(szBuf, sizeBuf, "Integer......", ... );
 szBuf[sizeBuf] = '\0';

The "n" versions exist for all the string functions, and all provide for the specification of the destination buffer size so that you can be sure that they will never overrun the buffer. Note that the functions do not ensure zero termination, so the extra line to guarantee.

So with the snprintf, you can be safe that if 1 error in a million data error slips through that would cause some unexpected string conversion, your code keeps running.

Along this line, there is not much benefit to allocating the char buf[] inside the loop. Since all a program does is execute loop() endlessly, you are not saving memory resources by making this a local variable on the stack. There is only a microscopic window where the stack memory is no used. But by being on the stack, if you do overflow buf, an overflow can erase the return address which would surely make a program crash. It is all hazard with no return.

Think like this

const size_t sizeBuf = 128;
char szBuf[128+1];

void loop() {
 ...

 snprintf(szBuf, sizeBuf, "Integer......", ... );
 szBuf[sizeBuf] = '\0';

In this method, the memory is allocated statically, and you know at compile time how much memory your program requires.