Confusing issue with my code

Hi, I’ve I’ve attached a program I wrote for an Etherten for controlling the temperature of a fridge. It uses the ethernet device to ask a HTTP server what temperature it should try and keep the fridge at, as well as reporting the current temperature to the HTTP server (for graphing and analysis).

However I have a problem with this code, it will randomly decide to start heating, even though that should only be happening when it is below a certain temperature. I can’t work out any pattern to when it turns on the heating, and it has never had both on at the same time, indicating it is something wrong with the logic around the if statement and not some other random thing deciding to turn that pin on.

I’m still quite new to Arduino coding, is anyone able to take a look at this and figure out where the problem is?

etherten.ino (3.56 KB)

The String class is almost always a problem. I would get rid of it and use standard C null-terminated strings and then see if it still turns on the heating when it shouldn't.

Pete

Time related variables should be unsigned long, not long.

Time variables and constants should never be added, only subtracted.

60000 is not a int. Append UL to get it handled correctly.

  if (Ethernet.begin(mac) == 9) {
    Serial.println(" FAIL!");
  } else {
    Serial.println(" OK.");
  }

A rather strange concept of FAIL. Anything but 0 is a fail.

void loop()
{
  everyMinute();
  always();
}

Useless names!

el_supremo:
The String class is almost always a problem. I would get rid of it and use standard C null-terminated strings and then see if it still turns on the heating when it shouldn’t.

I’ll give this a go tonight.

PaulS:
Time related variables should be unsigned long, not long.

Makes sense, never going to be a negative number might as well get a larger number out of it.

PaulS:
Time variables and constants should never be added, only subtracted.

Not sure I understand what you mean here. Are you saying where I have:

if (lastMinute == 0 || lastMinute+60000 < millis()) {

I should change that so that it’s not adding?

PaulS:
60000 is not a int. Append UL to get it handled correctly.

  if (Ethernet.begin(mac) == 9) {

Serial.println(" FAIL!");
  } else {
    Serial.println(" OK.");
  }



A rather strange concept of FAIL. Anything but 0 is a fail.

This was copy-pasted, I didn’t understand the responses from Ethernet.begin and left it as is.

PaulS:

void loop()

{
  everyMinute();
  always();
}



Useless names!

Thanks for the review, I’ll make these changes and give it another go tonight.

mijofa:
Not sure I understand what you mean here. Are you saying where I have:

if (lastMinute == 0 || lastMinute+60000 < millis()) {

I should change that so that it’s not adding?

I’d suggest something like this:

if((millis() - lastMinute >= 60000) || (lastMinute == 0))
{

You could get a similar effect by initialising lastMinute to -60000:

const unsigned long interval = 60000;

unsigned long lastMinute = -interval;

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

void loop()
{
  if(millis() - lastMinute >= interval)
  {
    Serial.println("Minute");
    lastMinute += interval;
  }
}

The signed integer literal '60000' is outside the range of an 16 bit Arduino signed integer. Try using an unsigned long integer literal with '60000UL'.

lloyddean:
The signed integer literal ‘60000’ is outside the range of an 16 bit Arduino signed integer. Try using an unsigned long integer literal with ‘60000UL’.

Not needed - plain 60000 works correctly.

Not needed - plain 60000 works correctly.

Perhaps in that context it isn't.

It's still a good habit to get into, recognizing the limits of the range of values, and using the correct value (literal) all the time.

Understood - wouldn't want your incorrect code causing your (supposedly) working code to fail would we!

Alright so I’ve applied all these changes, and somehow introduced a new bug where after a few hours the code seems to get lock up at some point, it still responds to ping, but doesn’t update the LCD display, make the HTTP request, or switch the cooling/heating relays. As you can imagine this is quite slow to test and fix because it takes a few hours for each test, this also means that all the logging data I could’ve collected in this time, didn’t get collected. Although I have not seen the previous problem occur since, I’m not sure if I would have in that short time

I’ve attached the current version of the code, I’ll add some more Serial debugging to it tonight and leave a computer plugged into it and see what I can work out from that.

Edit: Now I’ve attached the current version of the code.

etherten.ino (3.77 KB)

You might want to look at http://playground.arduino.cc/Code/AvailableMemory, and see how much (or little) memory you have.

You might want to look into using the F() macro.