Go Down

Topic: Device keeps resetting randomly (Read 628 times) previous topic - next topic

scouris

I've seen a lot of home automation projects out there that use the Arduino and one that I particularily liked uses MQTT to get/send messages over Ethernet as a publishing system. I've come up with the following code for my project - at this point it controls a 16-relay board.
The device registers with an MQTT server (running on a rPi) and listens for commands. These I send to the MQTT server which pushs them out to the Arduino. I send numbers 1-16 to essentially switch the state of the relay (on if off and vice-versa). I can also send '999' to get a status line which shows which relays are on and which are off. If I send a number larger than 16 but less than 999 then it doesn't do anything, returns 'Invalid input'.
When it works, it works fine. Sometimes it loses commands that are sent to it (and I can confirm it receives by the mountain of serial debugging code I've put in, as you can see). And at other times (especially if I send commands to it quickly) it will just reset all on it's own.
I have tested this with no additional boards on the Arduino (except for ethernet) and across two different Arduino's and the problem still exists, leading me to believe it's my code.
I'm open to any suggestions or criticisms - please let me know what I'm doing wrong!
Pastebin link to code here http://pastebin.com/JX7Hgg5L in case it's too large for below.

Code: [Select]
#include <SPI.h>
#include <Ethernet.h>
#include <PubSubClient.h>

#define datapin 2
#define clockpin 3
#define latchpin 4
int current = 0; // 00000000
const int relayValues[]={
  1,2,4,8,16,32,64,128,256,512,1024,2048,4096,8192,16384,32768};
int relayCheck[] = {
  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
int changeVal = 0;
String OnOrOff;
String DataResult;

byte mac[] = { 
  0xDE, 0xC0, 0xDE, 0xC0, 0xFF, 0xEE };
byte mqttserver[] = {
  192, 168, 2, 7 };
byte ip[] = {
  192, 168, 2, 101 };

EthernetClient ethClient;
PubSubClient clientr(mqttserver, 1883, callback);

/////////////////// calculateRelayVal ///////////////////
int calculateRelayVal(int newVal) {

  Serial.print("Starting calculateRelayVal with val of ");
  Serial.println(newVal);
  Serial.print("Current value is ");
  Serial.println(current);

  // if newVal==0 then return value as 11111111

  // newVal has relay we need to toggle
  // referenced in relayCheck as newVal-1


  int intRelayToWorkWith = relayValues[(newVal-1)];

  if(relayCheck[(newVal-1)]==0) {
    // relay is currently off, turn on
    changeVal = current | intRelayToWorkWith;
    relayCheck[(newVal-1)]=1;
  }
  else {
    // relay is currently on, turn off
    changeVal = current ^ intRelayToWorkWith;
    relayCheck[(newVal-1)]=0;
  }

  Serial.print("Finished checking values, changeVal currently ");
  Serial.println(changeVal);

  current = changeVal;
  return current;
  Serial.println("completing calculateRelayVal");
  Serial.println("");
}

/////////////////// doWork ///////////////////
void doWork(int val) {

  Serial.println("");
  Serial.print("doWork starting with val of ");
  Serial.println(val);

  // as relays need a '0' to turn on and a '1' to turn off, need to NOT this value
  unsigned int realVal = ~val;

  Serial.print("Doing NOT conversion gives result of ");
  Serial.println(realVal);

  Serial.println("Splitting into two parts for registers ...");
  uint8_t rVlow = realVal & 0xff;
  uint8_t rVhigh = (realVal >> 8);
  Serial.print("Low: ");
  Serial.println(rVlow);
  Serial.print("High: ");
  Serial.println(rVhigh);

  digitalWrite(latchpin,LOW);
  shiftOut(datapin,clockpin,MSBFIRST,rVhigh);
  shiftOut(datapin,clockpin,MSBFIRST,rVlow);
  digitalWrite(latchpin,HIGH);

  Serial.println("sent value, closing doWork");
  Serial.println("");
}

/////////////////// callback ///////////////////
static void callback(char* topic, byte* payload, unsigned int length) {
  char PublishData[100];
  Serial.print("callback with payload ");
  Serial.write(payload,(length/2)); // for some reason length is twice as long as it needs to be

  int readVar = (unsigned int)atoi((char *) payload);
  if(readVar==999) {

    Serial.println("Status check requested.");

    String strGetStatus="Status check: ";
    for(int i=0;i<16;i++){
      strGetStatus+=relayCheck[i];
    }
    strGetStatus.toCharArray(PublishData,100);
    clientr.publish("RelayControlReport", PublishData);
    clientr.publish("WebStatusCheck", PublishData);

    Serial.print("Status check sent as: ");
    Serial.println(strGetStatus);
  }
  else if(readVar>16) {
     Serial.print("Invalid input detected: ");
     Serial.write(payload,(length/2));
     clientr.publish("RelayControlReport","Invalid input");
  }
  else {
    Serial.println(readVar);
    String debugcalReV = "Calling calculateRelayVal with the payload of ";
    String debugcalReVGO = debugcalReV + readVar;
    Serial.print(debugcalReVGO);
    Serial.println(" ... ");
    int registerValue=0;
    if(readVar != 0) {
      registerValue = calculateRelayVal(readVar); 
    }
    else { // turns all relays off if received value is zero
      current=0;
      for(int i=0;i< 16;i++){
        relayCheck[i]=0;
      }
    }

    String debugregVal = "registerValue is set to ";
    String debugregValGO = debugregVal + registerValue;
    Serial.println(debugregValGO);
    Serial.println("doing work now for above value ... ");

    doWork(registerValue);

    String debugdoWork = "doWork complete for ";
    String debugdoWorkGO = debugdoWork + registerValue;
    Serial.println(debugdoWorkGO);

    if(relayCheck[(readVar-1)]==1) {
      OnOrOff = " turned on.";
    }
    else {
      OnOrOff = " turned off.";
    }
    if(readVar!=0){
      String DataMash = "Publishing data - relay ";
      DataResult = DataMash + readVar + OnOrOff;
      DataResult.toCharArray(PublishData,100);
    }
    else {
      DataResult = "All relays turned OFF";
      DataResult.toCharArray(PublishData,100);
    }
    clientr.publish("RelayControlReport", PublishData);
    Serial.println(DataResult);
  }

}

/////////////////// setup ///////////////////
void setup()
{
  pinMode(datapin,OUTPUT);
  pinMode(clockpin,OUTPUT);
  pinMode(latchpin,OUTPUT);
  digitalWrite(latchpin,LOW);
  shiftOut(datapin,clockpin,MSBFIRST,-1);
  shiftOut(datapin,clockpin,MSBFIRST,-1);
  digitalWrite(latchpin,HIGH);
  delay(500);
  Serial.begin(9600);
  Serial.println("Scouris - MQTT Control, booting up.");
  Serial.println("Initialising Ethernet ...");
  Ethernet.begin(mac,ip);

  // below is for when running with DHCP
  Serial.print("Ethernet is up with IP of ");
  for (byte thisByte = 0; thisByte < 4; thisByte++) {
    // print the value of each byte of the IP address:
    Serial.print(Ethernet.localIP()[thisByte], DEC);
    Serial.print(".");
  }

  if(clientr.connect("arduinoClient")) {
    clientr.publish("checkInTopic","Relay Controller Online");
    clientr.publish("RelayControlReport","Relay Controller Online");
    clientr.subscribe("RelayControl");
  }

  delay(500);

  Serial.println("Resetting relays to OFF ...");
  doWork(0);
  delay(1000);
  Serial.println("Setting all to on for test ...");
  doWork(-1);
  delay(1000);
  doWork(0);
  Serial.println("Continuing.");
  delay(1000);
}

/////////////////// loop ///////////////////
void loop()
{
  clientr.loop();
}

AWOL

#1
Jan 24, 2013, 11:45 am Last Edit: Jan 24, 2013, 11:47 am by AWOL Reason: 1
Quote
and I can confirm it receives by the mountain of serial debugging code I've put in,

That could be part of the problem.
Try to use the "F()" macro when printing constant strings, to make sure they don't use up your precious RAM.
Code: [Select]
 Serial.print(F("Current value is "));

Code: [Select]
String debugregVal = "registerValue is set to ";
That could be another part.
Please, don't use String.
"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Nick Gammon


And at other times (especially if I send commands to it quickly) it will just reset all on it's own.


Please note that, at present, the String library has bugs as discussed here and here.

In particular, the dynamic memory allocation used by the String class may fail and cause random crashes.

I recommend reworking your code to manage without String. Use C-style strings instead (strcpy, strcat, strcmp, etc.).
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

scouris

Thanks guys.
As a quick edit before I left for work this morning, I removed all un-necessary Serial.print()'s and the ones that were left I changed them to be Serial.print(F()) as AWOL described. The program seems to run fairly well now - frantic mashing of the keyboard to send commands to it fails to make it fall over.

Having said that, this evening I'm going to take your advice, Nick, and get rid of the Strings. One thing that I was trying to work out on the drive to work this morning was this section:

Code: [Select]
String strGetStatus="Status check: ";
    for(int i=0;i<16;i++){
      strGetStatus+=relayCheck[i];
    }
    strGetStatus.toCharArray(PublishData,100);

If I were to get rid of the String strGetStatus and instead make it a char*, would something like this be the way to go:

Code: [Select]
char* chrGetStatus[35]="Status check: ";
for(int i=0;i<16;i++){
chrGetStatus[(15+i)]=relayCheck[i];
}


Actually, writing that out it looks to me like double handling. the relayCheck is an int* - how would I convert this to a char array so I can publish it's value? At present, using the String method, I get a string of 0's or 1's that indicate which relay is on or off, and the web monitor currently looks at this, so I'd like it to come out the same.

PaulS

Code: [Select]
chrGetStatus[(15+i)]=relayCheck[i];
The compiler will accept that, but, if relayCheck[ i ] is 0 or 1, that is what will be store in your character array, not '0' or '1'. Fortunately, '0' = 0 + '0' and '1' = 1 + '0'.

Nick Gammon

What PaulS said, plus add a terminating 0x00 byte.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

PeterH


Code: [Select]
char* chrGetStatus[35]="Status check: ";



That would make more sense as:

Code: [Select]
char chrGetStatus[35]="Status check: ";

You want an array of characters, not an array of pointers.
I only provide help via the forum - please do not contact me for private consultancy.

Go Up