udp-receiver-sender stops working

Hi,
i have written following sketch.

Platform is Arduino Nano with W5100 Ethernet Shield and 4-Relais-Shield.

The Arduino stops receiving and answering UDP-Packets after a while, the duration is every time total different.
I did build an function that echoes me the millis()-Value, so i can see that the loop is still running, but there are no new packets arriving.

Can someone give a hint, how i can debug this?

thx,
Stephan

// 21.09.2017: Version für 4 Relais entwickelt.
// 26.09.2017: hängt sich bei 994942 millis auf

#include <SPI.h>         // needed for Arduino versions later than 0018
#include <Ethernet.h>
#include <EthernetUdp.h>         // UDP library from: bjoern@cs.stanford.edu 12/30/2008


// Enter a MAC address and IP address for your controller below.
// The IP address will be dependent on your local network:
byte mac[] = {
  0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED
};
IPAddress ip(192, 168, 0, 98);

unsigned int localPort = 8887;      // local port to listen on

// buffers for receiving and sending data
char packetBuffer[UDP_TX_PACKET_MAX_SIZE];  //buffer to hold incoming packet,
char  ReplyBuffer[] = "acknowledged";       // a string to send back
unsigned int R1 = 5;
unsigned int R2 = 6;
unsigned int R3 = 7;
unsigned int R4 = 8;
unsigned long lR1off;
unsigned long iDauerR1;
unsigned int R1State = HIGH;
unsigned long lR2off;
unsigned long iDauerR2;
unsigned int R2State = HIGH;
unsigned long lR3off;
unsigned long iDauerR3;
unsigned int R3State = HIGH;
unsigned long lR4off;
unsigned long iDauerR4;
unsigned int R4State = HIGH;

// Generally, you should use "unsigned long" for variables that hold time
// The value will quickly become too large for an int to store
unsigned long previousMillis = 0;        // will store last time LED was updated

// constants won't change:
const long interval = 1000;           // interval at which to blink (milliseconds)


// An EthernetUDP instance to let us send and receive packets over UDP
EthernetUDP Udp;
/*  If you are using an Arduino Ethernet shield you cannot use the following pins on
/ *  the following boards. Firmata will ignore any requests to use these pins:
 *
 *  - Arduino Uno or other ATMega328 boards: (D4, D10, D11, D12, D13)
 *  - Arduino Mega: (D4, D10, D50, D51, D52, D53)
 *  - Arduino Leonardo: (D4, D10)
 *  - Arduino Due: (D4, D10)
 *  - Arduino Zero: (D4, D10)
//*/
void setup() {
  // start the Ethernet and UDP:
  Ethernet.begin(mac, ip);
  Udp.begin(localPort);

  Serial.begin(9600);

  pinMode(R1, OUTPUT);
  digitalWrite(R1, HIGH);
  pinMode(R2, OUTPUT);
  digitalWrite(R2, HIGH);
  pinMode(R3, OUTPUT);
  digitalWrite(R3, HIGH);
  pinMode(R4, OUTPUT);
  digitalWrite(R4, HIGH);
}

void loop() {

  int packetSize = Udp.parsePacket();
  if (packetSize) {
    Serial.println(packetSize);
    //IPAddress remote = Udp.remoteIP();
  	//memset(packetBuffer, 0, sizeof(packetBuffer));
	  Udp.read(packetBuffer, UDP_TX_PACKET_MAX_SIZE);
	  Serial.println(packetBuffer);
	  Udp.beginPacket(Udp.remoteIP(), Udp.remotePort());
    Udp.write(packetBuffer);
    Udp.endPacket();
    String strBuffer= packetBuffer; // 1R5000
  	String strRelais=strBuffer.substring(0, 1);// welches Relais wird angesprochen
  	int iRelais=strRelais.toInt();
  	Serial.print(strRelais);
  	Serial.print(" - ");
    //delay(10);
    
  if(iRelais == 1){
  	String strDAUER=strBuffer.substring(2);
  	long iTemp= strDAUER.toInt();
    iDauerR1= abs(iTemp);
    // Sooo, da wir ja schon wissen, dass Relais 1 gemeint ist, schalten wir doch direkt ein: 
    R1State = LOW;
    digitalWrite(R1, R1State);
    //aktuelle Zeit:
    Serial.print("on: ");
    Serial.println(millis());
    // und wann müssen wir wieder ausschalten? 
    lR1off=millis()+iDauerR1;
  } // end if iRelais
  
  if(iRelais == 2){
  	String strDAUER=strBuffer.substring(2);
  	long iTemp= strDAUER.toInt();
    iDauerR2= abs(iTemp);
    // Sooo, da wir ja schon wissen, dass Relais 1 gemeint ist, schalten wir doch direkt ein: 
    R2State = LOW;
    digitalWrite(R2, R2State);
    //aktuelle Zeit:
    Serial.print("on: ");
    Serial.println(millis());
    // und wann müssen wir wieder ausschalten? 
    lR2off=millis()+iDauerR2;
  } // end if iRelais
  
  if(iRelais == 3){
  	String strDAUER=strBuffer.substring(2);
  	long iTemp= strDAUER.toInt();
    iDauerR3= abs(iTemp);
    // Sooo, da wir ja schon wissen, dass Relais 1 gemeint ist, schalten wir doch direkt ein: 
    R3State = LOW;
    digitalWrite(R3, R3State);
    //aktuelle Zeit:
    Serial.print("on: ");
    Serial.println(millis());
    // und wann müssen wir wieder ausschalten? 
    lR3off=millis()+iDauerR3;
  } // end if iRelais
  
  if(iRelais == 4){
  	String strDAUER=strBuffer.substring(2);
  	long iTemp= strDAUER.toInt();
    iDauerR4= abs(iTemp);
    // Sooo, da wir ja schon wissen, dass Relais 1 gemeint ist, schalten wir doch direkt ein: 
    R4State = LOW;
    digitalWrite(R4, R4State);
    //aktuelle Zeit:
    Serial.print("on: ");
    Serial.println(millis());
    // und wann müssen wir wieder ausschalten? 
    lR4off=millis()+iDauerR4;
  } // end if iRelais
} // end if (packetSize)

if (millis() > lR1off  && R1State == LOW){
  R1State = HIGH;
  digitalWrite(R1, R1State);
  Serial.print("off: ");
  Serial.println(millis());
} 

if (millis() > lR2off  && R2State == LOW){
  R2State = HIGH;
  digitalWrite(R2, R2State);
  Serial.print("off: ");
  Serial.println(millis());
} 

if (millis() > lR3off  && R3State == LOW){
  R3State = HIGH;
  digitalWrite(R3, R3State);
  Serial.print("off: ");
  Serial.println(millis());
} 

if (millis() > lR4off  && R4State == LOW){
  R4State = HIGH;
  digitalWrite(R4, R4State);
  Serial.print("off: ");
  Serial.println(millis());
} 



  unsigned long currentMillis = millis();

  if (currentMillis - previousMillis >= interval) {
    // save the last time you blinked the LED
    previousMillis = currentMillis;
    Serial.print("##");
    Serial.print(millis());
    Serial.println("##");
    
  }



} // end loop

Can someone give a hint, how i can debug this?

Quit using Strings.

Hi, i have no idea how i can split, print and use the Datagram else.

Using for always ended in a desaster. I will try again and post my code.

Do you think this is the Problem why my arduino/Ethernet-Board crashes?

Stephan

Do you think this is the Problem why my arduino/Ethernet-Board crashes?

If "this" is the fact that you don't know how to parse a packet, then no.

If "this" is the fact that you piss away resources using the String class, then yes.

PaulS: If "this" is the fact that you piss away resources using the String class, then yes.

Thanks for this Information!

Okay, so far i got changed a bit on my code. A friend of mine helped me, i hope it’s better code now.

First, i thought, it was better, but then the failures occurred again.

I discovered, that it might be a problem with the ethernet chip, as the arduino is still running well, when no more ethernet messages are received.

In the Documentation of the W5100-Chip i found the information:

By asserting this pin low for at least 2us, all internal
registers will be re-initialized to their default states.

So i built a “watchdog”, if a time passes without receiving any packets, pin 3 is set to low and resettes the Ethernet chip.
The reset is working, but doesn’t cause the W5100 to answer.

With tcpdump i can see the udp messages leaving, i can also see the arp-requests leaving, and i can reproduce that, if i see no answers from the arduino to the arp requests, i can’t reach the arduino.

Are there more issues in my code?

#include <SPI.h>         // needed for Arduino versions later than 0018
#include <Ethernet.h>
#include <EthernetUdp.h>         // UDP library from: bjoern@cs.stanford.edu 12/30/2008


// Enter a MAC address and IP address for your controller below.
// The IP address will be dependent on your local network:
byte mac[] = {
  0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0x61
};

IPAddress ip(192, 168, 0, 97);

unsigned int localPort = 8887;      // local port to listen on

// buffers for receiving and sending data
char packetBuffer[UDP_TX_PACKET_MAX_SIZE];  //buffer to hold incoming packet,
char  ReplyBuffer[] = "acknowledged";       // a string to send back

unsigned int pinRelais[4]= { 5, 6, 7, 8 };
unsigned int uiResetETH = 3;
unsigned long ulDauerR[4]= { 0, 0, 0, 0};
unsigned int uiRState[4]= { HIGH, HIGH, HIGH, HIGH };
unsigned long ulROff[4]= {0, 0, 0, 0};
unsigned int uiWatchdogCounter = 0;

// Generally, you should use "unsigned long" for variables that hold time
// The value will quickly become too large for an int to store
unsigned long previousMillis = 0;        // will store last time LED was updated

// constants won't change:
const long interval = 1000;           // interval at which to blink (milliseconds)


// An EthernetUDP instance to let us send and receive packets over UDP
EthernetUDP Udp;
/*  If you are using an Arduino Ethernet shield you cannot use the following pins on
/ *  the following boards. Firmata will ignore any requests to use these pins:
 *
 *  - Arduino Uno or other ATMega328 boards: (D4, D10, D11, D12, D13)
 *  - Arduino Mega: (D4, D10, D50, D51, D52, D53)
 *  - Arduino Leonardo: (D4, D10)
 *  - Arduino Due: (D4, D10)
 *  - Arduino Zero: (D4, D10)
//*/
void setup() {
  // start the Ethernet and UDP:
  Ethernet.begin(mac, ip);
  Udp.begin(localPort);

  Serial.begin(9600);

  // TI: Initialisieren
  //int i= 0;
  for(int i= 0; i < 4; i++) 
  {
    pinMode(pinRelais[i], OUTPUT);
    digitalWrite(pinRelais[i], HIGH);
  }
  
  //Watchdog/reset ETH
  pinMode(uiResetETH, OUTPUT);
  digitalWrite(uiResetETH,LOW);
  delay(1000);
  digitalWrite(uiResetETH,HIGH);
  delay(1000);
  
  Serial.println("Hello, this is");
  Serial.println("HV_TIM_V02_UDP_UNO_4Relais_Millis");
  Serial.println("my IP Adress is:");
  Serial.println(ip);
//  Serial.println("my MAC Adress is:");
//  Serial.println(mac);
  Serial.println("my Port is: ");
  Serial.println(localPort);
}

void loop() {

  
  
  int packetSize = Udp.parsePacket();
  if (packetSize) 
  {
    Serial.println(packetSize);
    //IPAddress remote = Udp.remoteIP();
    memset(packetBuffer, 0, UDP_TX_PACKET_MAX_SIZE);
    Udp.read(packetBuffer, UDP_TX_PACKET_MAX_SIZE);
    Serial.println(packetBuffer);
    
    
    // TI:
  	// -48: ( 48 = '0'), sonst wandelt er in den ASCII-Code der Zahl um
  	// -1: Index von Relais 1 ist 0 (Relais1, Relais2, Relais3, Relais4 ist jetzt Relais[0], Relais[1], Relais[2], Relais[3])
  	int iRelais= (int)packetBuffer[0] - 48 - 1;
  	
  	
  	// Dauer wird auch mehrfach gebraucht, also schon an dieser Stelle extrahieren
  	// das macht die Funktion atol für uns, allerdings soll erst der Buffer ab Stelle '2' gewandelt werden
  	unsigned long ulDauer = abs(atol((&packetBuffer[2])));
  	
  	if (iRelais > 3)
  	{
  	  packetBuffer[6] = packetBuffer[6] + 1;
  	}
  	
  	if (ulDauer > 5000)
  	{
       packetBuffer[6] = packetBuffer[6] + 2;
       ulDauer = 5000;
  	}
  	
  	
  	Serial.print(" - ");
    //delay(10);

    // wir senden die Antwort erst nach der Fehlerüberprüfung:
    Udp.beginPacket(Udp.remoteIP(), Udp.remotePort());
    Udp.write(packetBuffer);
    Udp.endPacket();
    // Watchdog-Counter zurücksetzen, da Paket empfangen
    uiWatchdogCounter = 0;



    
    if(iRelais > 3)
    {
      Serial.print("Relais-Index out of Range!");
      // hier noch irgendwas machen?
    }
    else
    {
          ulDauerR[iRelais]= ulDauer;
          uiRState[iRelais]= LOW;
          digitalWrite(pinRelais[iRelais], uiRState[iRelais]);
      
          //aktuelle Zeit:
          Serial.print("on: ");
          Serial.println(millis());
      
          // und wann müssen wir wieder ausschalten? 
          ulROff[iRelais]= millis() + ulDauerR[iRelais];
  		  
    }
    
  } // end if (packetSize)

  for(int i= 0; i < 4; i++)
  {
    // TI: Wichtig: erst den State abfragen, dann die Zeit vergleichen.
    // Wenn die State-Abfrage FALSE ist, wird die Zeit gar nicht erst verglichen
    // Da für den Zeitvergleich jedesmal ein Funktionsaufruf erfolgt, ist dieser Vergleich
    // zeit- und rechenintensiver. Das macht für Deine Heizungssteuerung vermutlich keinen merkbaren
    // Unterschied, aber generell ist das Best-Practice (falls Du später mal eine Rakete steuern möchtets, oder so ;-)
    if((uiRState[i] == LOW) && (millis() > ulROff[i])) 
    {
      uiRState[i]= HIGH;
      digitalWrite(pinRelais[i], uiRState[i]);
      Serial.print("off: ");
      Serial.println(millis());
    }
    
    
  }
  
  unsigned long currentMillis = millis();

  if (currentMillis - previousMillis >= interval)
  {
    uiWatchdogCounter++;
    // save the last time you blinked the LED
    previousMillis = currentMillis;
    Serial.print("##");
    Serial.print(millis());
    Serial.print("##");
    Serial.println(uiWatchdogCounter);
  }
  // wenn mehr als 60 Sekunden lang keine Anweisung kam
  if (uiWatchdogCounter > 115)
  {
    Serial.println("resette ethshield");
    //resette das Ethernet-Shield
    digitalWrite(uiResetETH,LOW);
    delay(2);
    digitalWrite(uiResetETH,HIGH);
    delay(1000);
    uiWatchdogCounter=0;
  }
} // end loop

Thanks,
Stephan