loop stops working after a few hours

Hi everyone! I am new to the arduino world, I am trying to control a light through an app and the Pub/sub model through a public Mqtt broker.

I am using an arduino UNO and an Arduino 2 Ethernet shield. I found a sketch- attached below - which runs perfectly and allows me to control the light through the developed app which publish on the broker. The light is powerd by an external battery.

After a few hours, however, it stops working and I have to restart arduino, to make it work again.

It seems that arduino after some time “stops checking” any new messages published on the broker by the app.

I am not sure if the issue might be the broker that i am using (test.mosquitto.org) or something in the loop or something else.

[code]
#include <SPI.h>
#include <Ethernet2.h>
#include <PubSubClient.h>

byte mac[] = { 0xD4, 0x23, 0x76, 0x87, 0x90, 0x87 };
IPAddress staticIP( 192, 168, 1, 123);
EthernetClient client;
void connectToInternet()
{
  // Attempt to connect to Ethernet with DHCP
  if (Ethernet.begin(mac) == 0)
  {
    Serial.print("[ERROR] Failed to Configure Ethernet using DHCP");
    // DHCP failed, attempt to connect to Ethernet with static IP
    Ethernet.begin(mac, staticIP);
  }
  // Delay to let Ethernet shield initialize
  delay(1000);
  // Connection successful
  Serial.println("[INFO] Connection Successful");
  Serial.print("");
  printConnectionInformation();
  Serial.println("-----------------------------------------------");
  Serial.println("");
}
void printConnectionInformation()
{
  // Print Connection Information
  Serial.print("[INFO] IP Address: ");
  Serial.println(Ethernet.localIP());
  Serial.print("[INFO] Subnet Mask: ");
  Serial.println(Ethernet.subnetMask());
  Serial.print("[INFO] Gateway: ");
  Serial.println(Ethernet.gatewayIP());
  Serial.print("[INFO] DNS: ");
  Serial.println(Ethernet.dnsServerIP());
}
// IP address of the MQTT broker
char server[] = { "test.mosquitto.org"};
int port = 1883;
char topic[] = { "codifythings/lightcontrol"};

PubSubClient pubSubClient(server, 1883, callback, client);
void callback(char* topic, byte* payload, unsigned int length)
{
  // Print payload
  String payloadContent = String((char *)payload);
  Serial.println("[INFO] Payload: " + payloadContent);

  // Turn lights on/off
  turnLightsOnOff();
}

void turnLightsOnOff()
{
  // Check if lights are currently on or off
  if (digitalRead(9) == LOW)
  {
    //Turn lights on
    Serial.println("[INFO] Turning lights on");
    digitalWrite(9, HIGH);
  }
  else
  {
    // Turn lights off
    Serial.println("[INFO] Turning lights off");
    digitalWrite(9, LOW);
  }
}
void setup()
{
  // Initialize serial port
  Serial.begin(9600);
  // Connect Arduino to internet
  connectToInternet();

  //Connect MQTT Broker
  Serial.println("[INFO] Connecting to MQTT Broker");
  if (pubSubClient.connect("arduinoClient"))
  {
    Serial.println("[INFO] Connection to MQTT Broker Successful");
    pubSubClient.subscribe(topic);
    Serial.println("[INFO] Successfully Subscribed to MQTT Topic ");

    // Set LED pin mode
    pinMode(9, OUTPUT);
  }
  else
  {
    Serial.println("[INFO] Connection to MQTT Broker Failed");
  }
}
void loop()
{
  // Wait for messages from MQTT broker
  pubSubClient.loop();
}

[/code]

Thank you for any help!

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. This can happen after the program has been running perfectly for some time. Just use cstrings - char arrays terminated with '\0' (NULL).

...R

One issue I see is that the client.loop() is running, now this is a good one, way too fast.

Another issue, I see, is there is not checking to see if the network is still valid and if the MQTT connection is still valid and if they are not valid make-em so.

Ok, about that being too fast. The way your code is once client loop is done, start client loop again. Client loop causes the callback trigger, then your program needs time to see if their is a publication to process, process the publication and then check client loop again.

What you got doing is that once client loop has done its things, say received a message and stored it in a buffer for the callback to receive, you tell client loop to start again. And what is one of the first things client loop does is to clear that buffer.

Here is some code that makes the connection, holds the connection open

////
/*
    Important to not set vTaskDelay to less then 10. Errors begin to develop with the MQTT and network connection.
    makes the initial wifi/mqtt connection and works to keeps those connections open.
*/
void MQTTkeepalive( void *pvParameters )
{
  sema_MQTT_KeepAlive   = xSemaphoreCreateBinary();
  xSemaphoreGive( sema_MQTT_KeepAlive ); // found keep alive can mess with a publish, stop keep alive during publish
  // setting must be set before a mqtt connection is made
  MQTTclient.setKeepAlive( 90 ); // setting keep alive to 90 seconds makes for a very reliable connection, must be set before the 1st connection is made.
  for (;;)
  {
    //check for a is-connected and if the WiFi 'thinks' its connected, found checking on both is more realible than just a single check
    if ( (wifiClient.connected()) && (WiFi.status() == WL_CONNECTED) )
    {
      xSemaphoreTake( sema_MQTT_KeepAlive, portMAX_DELAY ); // whiles MQTTlient.loop() is running no other mqtt operations should be in process
      MQTTclient.loop();
      xSemaphoreGive( sema_MQTT_KeepAlive );
    }
    else {
      log_i( "MQTT keep alive found MQTT status %s WiFi status %s", String(wifiClient.connected()), String(WiFi.status()) );
      if ( !(WiFi.status() == WL_CONNECTED) )
      {
        connectToWiFi();
      }
      connectToMQTT();
    }
    vTaskDelay( 250 ); //task runs approx every 250 mS
  }
  vTaskDelete ( NULL );
}
////
void connectToMQTT()
{
  // create client ID from mac address
  byte mac[6];
  WiFi.macAddress(mac);
  log_i( "mac address %d.%d.%d.%d.%d", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]  );
  String clientID = String(mac[0]) + String(mac[5]) ;
  log_i( "connect to mqtt as client %s", clientID );
  while ( !MQTTclient.connected() )
  {
    MQTTclient.connect( clientID.c_str(), mqtt_username, mqtt_password );
    log_i( "connecting to MQTT" );
    vTaskDelay( 250 );
  }
  log_i("MQTT Connected");
}
//
void connectToWiFi()
{
  log_i( "connect to wifi" );
  while ( WiFi.status() != WL_CONNECTED )
  {
    WiFi.disconnect();
    WiFi.begin( SSID, PASSWORD );
    log_i(" waiting on wifi connection" );
    vTaskDelay( 4000 );
  }
  log_i( "Connected to WiFi" );
  WiFi.onEvent(  );
}

Note this message

/*
Important to not set vTaskDelay to less then 10. Errors begin to develop with the MQTT and network connection.
makes the initial wifi/mqtt connection and works to keeps those connections open.
*/

And the part in bold. The message means if client.loop() runs too fast it messes up things.

If client loop is running, then do not send or receive a message during client loop operation.


The data published to the MQTT Broker should have the retain flag set. As per the Eclipse documentation, when a message has been taken off the broker and the retain flag is not set and a request is made to have the message reread then a null payload will be passed to the client. It is up to the user to code for a possible null payload. I just use the retain flag.

@mattrra

Other post/duplicate DELETED
Please do NOT cross post / duplicate as it wastes peoples time and efforts to have more than one post for a single topic.

Continued cross posting could result in a time out from the forum.

Could you also take a few moments to Learn How To Use The Forum.

Other general help and troubleshooting advice can be found here.
It will help you get the best out of the forum in the future.

handling c_strings needs using commands that do storing values based on pointers.
This means a wrong pointer will store bytes to the given adress absolutely mercyless.
Even if this means to overwrite other variables.
The compiler has no chance to check if the target-adress makes sense or not.

This is one of the flaws - at least for newbees - about C++

So I prefer using the PString-library which offers even some more comfort for adding integers and floats to strings
in a much more compact way than dealing with c_strings
You can install the PString-library with the Arduino-IDE-livrary manager

here is a demo-code how you can use PStrings

#include <PString.h>

//my personal naming-convemtion adding suffixes "_AoC" for ArrayOfChar
// and _PS  for PString for easy indicating the variabletype
char    MyDemo_AoC[20 + 1]; // 20 chars for characters one extra-char for terminating zero 
PString MyDemo_PS(MyDemo_AoC, sizeof(MyDemo_AoC));

int MyInt = -12345;
float MyFloat = -9.87654321;

void ShowPStringDemo() {  
  MyDemo_PS = "";
  Serial.print("MyDemo_PS = ''; contains #");
  Serial.print(MyDemo_PS);
  Serial.print("#");
  Serial.println();

  MyDemo_PS = "A";
  Serial.print("MyDemo_PS = 'A'; contains #");
  Serial.print(MyDemo_PS);  
  Serial.print("#");
  Serial.println();

  MyDemo_PS += "2345678";
  Serial.print("MyDemo_PS += '2345678'; contains #");
  Serial.print(MyDemo_PS);  
  Serial.print("#");
  Serial.println();

  MyDemo_PS += "9";
  Serial.print("MyDemo_PS += '9'; contains #");
  Serial.print(MyDemo_PS);  
  Serial.print("#");
  Serial.println();

  MyDemo_PS += "ABC";
  Serial.print("MyDemo_PS += 'ABC'; contains #");
  Serial.print(MyDemo_PS);  
  Serial.print("#");
  Serial.println();


  MyDemo_PS = "123456789012345678901234567890";
  Serial.print("MyDemo_PS = '123456789012345678901234567890'; contains #");
  Serial.print(MyDemo_PS);  
  Serial.print("#");
  Serial.println();

  MyDemo_PS = MyInt;
  MyDemo_PS += " ";
  MyDemo_PS += MyFloat;

  Serial.print("MyDemo_PS = MyInt + MyFloat ; contains #");
  Serial.print(MyDemo_PS);    
  Serial.print("#");
  Serial.println();
  Serial.println();
  Serial.println();
}

boolean TimePeriodIsOver (unsigned long &expireTime, unsigned long TimePeriod) {
  unsigned long currentMillis  = millis();  
  if ( currentMillis - expireTime >= TimePeriod )
  {
    expireTime = currentMillis; // set new expireTime
    return true;                // more time than TimePeriod) has elapsed since last time if-condition was true
  } 
  else return false;            // not expired
}

unsigned long MyTestTimer = 0;                   // variables MUST be of type unsigned long

void setup()
{
  Serial.begin(115200);
  Serial.println("setup start");  
}

void loop() {
  if ( TimePeriodIsOver(MyTestTimer,1000) ) {
    ShowPStringDemo();
  }  
}

best regards Stefan