Go Down

Topic: Ethernet (w5100) sketch hangs (Read 25 times) previous topic - next topic

fungus


So I currently use Digital Pin 4 as an output to control a mechanical relay.  Just so I dont have to rewrite some code, can I use the following and leave Digital Pin 4 alone??

Or is it hardwired so I have to use Pin 4?


It's hardwired.
No, I don't answer questions sent in private messages (but I do accept thank-you notes...)

SurferTim

#106
Oct 19, 2011, 11:40 am Last Edit: Oct 19, 2011, 03:11 pm by SurferTim Reason: 1
@Cruelkix and draythomp: Approximately how many downloads did they do before failing? When I stopped mine last night, it was around 4500 downloads. I'm doing one download every 20 seconds. I just started my test program again. Maybe my good run was a fluke? It had made it 2700 downloads before failing with the bug, so....

Do either of you have access to the logs in the server you are using? Are there any entries there that may help pinpoint a problem?

Edit: Just to insure it wasn't something else I had done, I removed the "correction" code I was using for testing. Now mine is back to almost normal, except the register checks.

Edit2: This "correction code" undo has me a bit concerned. The ethernet shield is still violating one of my prime directives of circular queues. It changes the TX_WR register. That should be only done by the Arduino code.

This was the code that prevented another write until the w5100 was finished sending everything. So if the w5100 is using this pointer to build packets, the worst case scenario would be to use client.print() or client.println() to send data. That should cause that failure quickest, because all characters are sent in their own packet. The chances of failure due to this error should be greatest in that condition. I use client.write(). It sends the request in one packet. So I send a couple packets every 20 seconds. If you use the client.print(), it sends about 30 packets for the same request.

This is the code that concerns me. w5100.cpp. I left my "correction code" there but remarked out. It waited until both the TX_RD and TX_WR registers were equal before writing more data.
Code: [Select]
void W5100Class::send_data_processing(SOCKET s, uint8_t *data, uint16_t len)
{
 uint16_t ptr;
//   uint16_t rptr;
 
//  do{

// Arduino gets original TX_WR here.
   ptr = readSnTX_WR(s);

//    rptr = readSnTX_RD(s);
//  }while(ptr != rptr);
 
 uint16_t offset = ptr & SMASK;
 uint16_t dstAddr = offset + SBASE[s];

// OK, picture this. About here, the w5100 decides it needs to build a packet for the last character you sent,
//   so it changes TX_WR here to give it enough memory to build it.


// Now the Arduino code starts writing data into the memory the w5100 is using to build a packet. OUCH!!

 if (offset + len > SSIZE)
 {
   // Wrap around circular buffer
   uint16_t size = SSIZE - offset;
   write(dstAddr, data, size);
   write(SBASE[s], data + size, len - size);
 }
 else {
   write(dstAddr, data, len);
 }

 ptr += len;

// OK, the Arduino now writes the original value increased by the length of this send.
 writeSnTX_WR(s, ptr);
}

Kinda scary to me.

Cruelkix

@Surfer - I don't have any sort of test software running.  I am using the code below:

Code: [Select]

#include <SPI.h>
#include <Ethernet.h>
#include <OneWire.h>
#include <DallasTemperature.h>
#define maxLength 60
#define ONE_WIRE_BUS 3

OneWire ourWire(ONE_WIRE_BUS);
DallasTemperature sensors(&ourWire);

byte mac[] = { 0x90, 0xA2, 0xDA, 0x00, 0x5A, 0xFA };
byte ip[] = { 192,168,0,177 };
Server server(80);

String inString = String(maxLength);
long previousMillis = 0;
int setferm[4] = {300,300,300,300};
String Whereto[4] = {"a","b","c","d"};
int TempSensor[4] = {0,0,0,0};
char tempBuff[4];
int whichTank = -1;
int chillValue = 10;
int curTankDiff = 0;
int maxTankDiff = 0;
int checkvalve = -1;
int valveToOpenIdx = -1;
boolean chilling = false;
double volts = 0.486328125;


void setup()
{
  Ethernet.begin(mac, ip);  // start the Ethernet connection and the server:
  server.begin();
  //Serial.begin(9600); // initialize serial communications at 9600 bps:
 
 
 
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  pinMode(6, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT);
   
  digitalWrite(4, HIGH);
  digitalWrite(5, LOW);
  digitalWrite(6, LOW);
  digitalWrite(7, LOW);
  digitalWrite(8, LOW);
  digitalWrite(9, LOW);
 
  sensors.begin();
 
}

void loop()
{
 
  /*Valve and Pump operation*/
 
  //Serial.print("Requesting temperature...");
  sensors.requestTemperatures(); // Send the command to get temperatures
  //Serial.println("DONE");
  for (int j = 0; j<4; j++)//Get Temps off sensors
    {
      TempSensor[j] = sensors.getTempFByIndex(j);
      //Serial.println(sensors.getTempFByIndex(j));
      }     
 
 
  switch ( whichTank ) //Calculate the difference between the Tank actual temperature and the tank set point
    {
      case 0: chillValue = TempSensor[0]-setferm[0];
              break;
      case 1: chillValue = TempSensor[1]-setferm[1];
              break;
      case 2: chillValue = TempSensor[2]-setferm[2];
              break;
      case 3: chillValue = TempSensor[3]-setferm[3];
              break;   
    }
   
  if (chillValue < -1 && valveToOpenIdx != -1) //If the difference is below -1, turn off the pump and close the valve
    {
      digitalWrite(9, LOW);
      delay(500);
      digitalWrite(valveToOpenIdx + 5, LOW);
      chilling = false;
    }   
   
   
  if (chilling == false) //Currently the pump is off and no valves are open, should they be?  Check to see if the difference for any of the tanks is greater than 0, open valve and turn pump on if they are.
    {   
      curTankDiff;
      maxTankDiff = 0;

      valveToOpenIdx = -1;
     
      for (int i = 0; i < 4; i++) {
        curTankDiff = TempSensor[i] - setferm[i];
        if (curTankDiff > maxTankDiff) {
          valveToOpenIdx = i;
          whichTank = i;
          maxTankDiff = curTankDiff;
        }
      }
     
      if (valveToOpenIdx != -1)
        { 
          digitalWrite(valveToOpenIdx + 5, HIGH);
          digitalWrite(9, HIGH);
          chilling = true;
      } 
    }
 
 
  /*Web Server and User Interface*/
  int bufLength = 4;
 
  Client client = server.available(); // listen for incoming clients
  if (client)
    {
       
      boolean currentLineIsBlank = true; // an http request ends with a blank line
      while (client.connected())
      {
        if (client.available())
          {
            char c = client.read();
            if (inString.length() < maxLength)
              {
                inString += c;
              }       
           
            // if you've gotten to the end of the line (received a newline character) and the line is blank, the http request has ended, so you can send a reply
            if (c == '\n' && currentLineIsBlank)
              {
         
                if (inString.indexOf("?") > -1) //If the request has new fermenter set points, assign them to the correct tank number
                  {
                    int Posa = inString.indexOf("a");
                    int Posb = inString.indexOf("b");
                    int Posc = inString.indexOf("c");
                    int Posd = inString.indexOf("d");
           
                    inString.substring((Posa+2), (Posb-1)).toCharArray(tempBuff, bufLength);  //transfer substring to buffer
                    setferm[0] = atoi(tempBuff);
                    inString.substring((Posb+2), (Posc-1)).toCharArray(tempBuff, bufLength);  //transfer substring to buffer
                    setferm[1] = atoi(tempBuff);
                    inString.substring((Posc+2), (Posd-1)).toCharArray(tempBuff, bufLength);  //transfer substring to buffer
                    setferm[2] = atoi(tempBuff);
                    inString.substring((Posd+2), (Posd+5)).toCharArray(tempBuff, bufLength);  //transfer substring to buffer
                    setferm[3] = atoi(tempBuff);
           
                    maxTankDiff = 0;  // check to see if this causes a valve to open or close, if so which one?
                    checkvalve = -1;     
                    for (int i = 0; i < 4; i++)
                      {
                        curTankDiff = TempSensor[i] - setferm[i];
                        if (curTankDiff > maxTankDiff)
                          {
                            checkvalve = i;
                            maxTankDiff = curTankDiff;
                          }
                      }
           
           
                 }
         
            client.println("HTTP/1.1 200 OK"); //Print information to webpage
            client.println("Content-Type: text/html");
            client.println();
           
            client.println("<font size=32 color=CDAD00><b><u>HGB Fermenter Control</u></b></font><br /><br /> ");
         
            for (int j = 0; j<4; j++) // Display the current termperature of the Fermenters
              {
                client.print("<font size=32>Temperature of Fermenter ");client.print(j + 1);client.print(" is ");client.print(TempSensor[j]);client.println("</font><br />");
              }
                           
            client.print("<br /><br /><form method=get><font size=32>");
         
             for (int j = 0; j<4; j++) //Display the current Set points for the Fermenters
              {
                client.print("Setpoint Fermenter ");client.print(j + 1);client.print(": <input type=text style=font-size:32pt size=3 name=");client.print(Whereto[j]);client.print(" value="); client.print(setferm[j]);client.print(" />");
                if (checkvalve == j) // if a valve is open from our check, change the text to display which one
                  {
                    client.print(" Open");
                  }
                else
                  {
                    client.print(" Closed");
                  }
               
                client.print("<br />");
              }
         
            client.print("<br /><br /><input type=submit value=Update-Temps style=height:6em; width:24em /></font></form>");
                                     
            break;
        }
       
        if (c == '\n')
          {
            currentLineIsBlank = true; // you're starting a new line
          }
        else if (c != '\r')
          {
            currentLineIsBlank = false;  // you've gotten a character on the current line
          }
         
        }
    }
 
    // give the web browser time to receive the data
    delay(100);
    inString = " ";
    // close the connection:
    client.stop();
  }   

}


I use a good amount of client.print, so if you think that may be a problem, that might be a big one for me.  I would gladly throw some test code on my arduino & ethernet shield to make sure it is working ok.  Really, my freezes up when no requests are even sent to it.  Just sitting there running the temperature sensing loop, it freezes.

SurferTim

#108
Oct 19, 2011, 04:05 pm Last Edit: Oct 19, 2011, 04:30 pm by SurferTim Reason: 1
That would be the highest risk of the failure if this is the case. Each character is sent in its own packet. That request is gonna send a bunch of packets in a hurry. Then it waits until the next send, then sends a bunch of packets in a hurry.

The more packets you build together back-to-back like that, the better chance you have of this type of data collision.

If you know where your w5100.cpp file is, you could try the "correction code" I was using. Edit your file to the new function. Here is is with the correction code working:
Code: [Select]
void W5100Class::send_data_processing(SOCKET s, uint8_t *data, uint16_t len)
{
 uint16_t ptr;
 uint16_t rptr;
 
 do{
      ptr = readSnTX_WR(s);
      rptr = readSnTX_RD(s);
 }while(ptr != rptr);
 
 uint16_t offset = ptr & SMASK;
 uint16_t dstAddr = offset + SBASE[s];

 if (offset + len > SSIZE)
 {
   // Wrap around circular buffer
   uint16_t size = SSIZE - offset;
   write(dstAddr, data, size);
   write(SBASE[s], data + size, len - size);
 }
 else {
   write(dstAddr, data, len);
 }

 ptr += len;

writeSnTX_WR(s, ptr);
}


Add: Just as a precaution, back up your original w5100.cpp file before editing it.

Cruelkix

I'm not sure where the w5100.cpp file is but I'm sure I can find it.  I'll give your pathced file a go and see if it helps.

Also, is there a better way for me to write the code for sending all of those client.print requests?

This chunk of my code is where they all sit.

Code: [Select]

client.println("HTTP/1.1 200 OK"); //Print information to webpage
            client.println("Content-Type: text/html");
            client.println();
           
            client.println("<font size=32 color=CDAD00><b><u>HGB Fermenter Control</u></b></font><br /><br /> ");
         
            for (int j = 0; j<4; j++) // Display the current termperature of the Fermenters
              {
                client.print("<font size=32>Temperature of Fermenter ");client.print(j + 1);client.print(" is ");client.print(TempSensor[j]);client.println("</font><br />");
              }
                           
            client.print("<br /><br /><form method=get><font size=32>");
         
             for (int j = 0; j<4; j++) //Display the current Set points for the Fermenters
              {
                client.print("Setpoint Fermenter ");client.print(j + 1);client.print(": <input type=text style=font-size:32pt size=3 name=");client.print(Whereto[j]);client.print(" value="); client.print(setferm[j]);client.print(" />");
                if (checkvalve == j) // if a valve is open from our check, change the text to display which one
                  {
                    client.print(" Open");
                  }
                else
                  {
                    client.print(" Closed");
                  }
               
                client.print("<br />");
              }
         
            client.print("<br /><br /><input type=submit value=Update-Temps style=height:6em; width:24em /></font></form>");
                                     
            break;
        }


SurferTim

#110
Oct 19, 2011, 05:11 pm Last Edit: Oct 19, 2011, 06:03 pm by SurferTim Reason: 1
I haven't tried anything that large, so you might need to break that up into separate packets. About 1000 bytes per packet is all you can send. I can't try this because my test unit is running this test code.

This uses a large buffer, but doesn't write that much. You might want to use a small buffer to build each iteration of those loops, then append them to outBuf.
Code: [Select]
char outBuf[1000];
int thisNum = 25;

sprintf(outBuf,"This is a test. All this will be sent as one packet. Including this number: %d\r\n",thisNum);
// This call sends the whole thing as one packet
client.write(outBuf);


In your case, I would:
build the "header stuff" into outBuf[], and send it.
build each iteration of the loop into outBuf[], and send them.
build the "footer stuff" into outBuf[], and send it.

DCContrarian


That would be the highest risk of the failure if this is the case. Each character is sent in its own packet. That request is gonna send a bunch of packets in a hurry. Then it waits until the next send, then sends a bunch of packets in a hurry.

The more packets you build together back-to-back like that, the better chance you have of this type of data collision.



Just to expand a little more on what SurferTim is saying here: the designers of the ethernet library decided to derive the Client class from Print and Stream so that it could use Print() and PrintLn() for formatted output. The problem with that is that Print() ends up calling write(byte) once per byte output, which causes one socket send for each byte sent. It's possible that the W5100 does some sort of defragmentation internally, but even so this is a poor way of implementing it.  One solution would have been to implement some sort of buffering in Client::write(), but that is non-trivial, consumes memory and increases latency.

Failing a re-write of Client:write(), it seems that it would be imprudent to use Client::print() or printLn().  Rather, you should format a buffer yourself and use either Client::write(const char *str) or Client::write(const uint8_t *buf, size_t size) instead.

DCContrarian



Edit2: This "correction code" undo has me a bit concerned. The ethernet shield is still violating one of my prime directives of circular queues. It changes the TX_WR register. That should be only done by the Arduino code.

This was the code that prevented another write until the w5100 was finished sending everything. So if the w5100 is using this pointer to build packets, the worst case scenario would be to use client.print() or client.println() to send data. That should cause that failure quickest, because all characters are sent in their own packet. The chances of failure due to this error should be greatest in that condition. I use client.write(). It sends the request in one packet. So I send a couple packets every 20 seconds. If you use the client.print(), it sends about 30 packets for the same request.



I want to clarify something here. The connection between the shield and the Arduino is SPI, which is a serial interface. The shield has no access to the memory or registers of the Arduino and doesn't write to or change anything.   (See http://en.wikipedia.org/wiki/Serial_Peripheral_Interface_Bus for details).

It is the Arduino SPI library that is changing things behind the scenes. This is interrupt-driven, so it is going on in the background of your sketch. If there is unpredictable behavior it is because of some interaction between the Ethernet and SPI libraries.

Cruelkix

#113
Oct 19, 2011, 08:49 pm Last Edit: Oct 19, 2011, 08:53 pm by Cruelkix Reason: 1


In your case, I would:
build the "header stuff" into outBuf[], and send it.
build each iteration of the loop into outBuf[], and send them.
build the "footer stuff" into outBuf[], and send it.



I got it all coded before I saw your message.  This is what I did.  I don't have a board at work that I can test it on, but it compiles at least :P

Code: [Select]

           ptrS = servBuffS;
           for (int j = 0; j<4; j++) // Display the current termperature of the Fermenters
             {
               ptrS += sprintf(ptrS,"<font size=32>Temperature of Fermenter %d is %d </font><br />", j + 1, TempSensor[j]);
             }
           
           ptrM = servBuffM;
           for (int j = 0; j<4; j++) //Display the current Set points for the Fermenters
             {
               ptrM += sprintf(ptrM,"Setpoint Fermenter %d: <input type=text style=font-size:32pt size=3 name=%d value=%d />", j + 1, Whereto[j], setferm[j]);
               if (checkvalve == j) // if a valve is open from our check, change the text to display which one
                 {
                   ptrM += sprintf(ptrM," Open");
                 }
               else
                 {
                   ptrM += sprintf(ptrM," Closed");
                 }
               
               ptrM += sprintf(ptrM,"<br />");
             }
             
           sprintf(servBuffB,"HTTP/1.1 200 OK <br />Content-Type: text/html <br /><br /><font size=32 color=CDAD00><b><u>HGB Fermenter Control</u></b></font><br /><br /> [%s] <br /><br /><form method=get><font size=32> [%s] <br /><br /><input type=submit value=Update-Temps style=height:6em; width:24em /></font></form>", ptrS, ptrM);
                     
           client.write(servBuffB);          


I guess I need a line of code or 3 that will clear all the arrays ebfore the void() loops again tho....  how do you clear a char array??

SurferTim

#114
Oct 19, 2011, 09:04 pm Last Edit: Oct 19, 2011, 09:21 pm by SurferTim Reason: 1
Try something simple. Here is a really simple response. I can't test it right now. I have another test running.
Code: [Select]
client.write("HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n<html><body>TEST</body></html>");


When you load the webpage, it should just say "TEST". Does that work?

Then you can try this:
Code: [Select]
char outBuf[128];

//This is the header stuff
sprintf(outBuf,"HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n<html><body>TEST<br>");
client.write(outBuf);

// This is the loop
for(int x = 0; x < 4; x++)
{
 sprintf(outBuf,"This is iteration %d<br>\r\n",x);
 client.write(outBuf);
}

// This is the footer stuff
sprintf(outBuf,"</body></html>\r\n");
client.write(outBuf);



Cruelkix


Try something simple. Here is a really simple response. I can't test it right now. I have another test running.
Code: [Select]
client.write("HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n<html><body>TEST</body></html>");


When you load the webpage, it should just say "TEST". Does that work?

Then you can try this:
Code: [Select]
char outBuf[128];

//This is the header stuff
sprintf(outBuf,"HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n<html><body>TEST<br>");
client.write(outBuf);

// This is the loop
for(int x = 0; x < 4; x++)
{
 sprintf(outBuf,"This is iteration %d<br>\r\n",x);
 client.write(outBuf);
}

// This is the footer stuff
sprintf(outBuf,"</body></html>\r\n");
client.write(outBuf);





Yeah, I did prety much what you show in the second chunk of code, I just used some pointers and crap.  I crammed it all into one client.write.  Its not very pretty in the code, but as far as packets transfers it should be efficient.

Like i said.  I dont have a board at work that I can test it on.  I'll see how it goes when I get home.

On a seperate note: how ar eyour tests going.

SurferTim

#116
Oct 19, 2011, 09:32 pm Last Edit: Oct 19, 2011, 09:53 pm by SurferTim Reason: 1
The test is going fine. It just passed download 2244.  :)

I like to send a "header" packet back to the client just to let it know you are responding. That way if it takes a while to generate the rest of the page, the client will wait.

draythomp

Cruelkix, do you have a memory used check in your code?  If you don't you should add one.  Strings and me don't get along; everytime I use them I run out of memory on the board and it fails.  It's probably me and my chaotic coding style, but nevertheless, it's good to know how much memory is being used.  I try to keep the available memory over 200 at all times.  That's roughly the overhead you need to keep from clobbering the stack and wandering off into space code wise.

The testing of available memory can be put almost anywhere just to be sure you're not doing that and just confusing yourself.

Don't misunderstand, this is a problem I have constantly and I code against now.  You may not have a problem at all, it's just good to know.

It's easy to check the memory, just include:

#include <MemoryFree.h>

and then print it so you can check what's going on.

Serial.println(freeMemory());

You can sprinkle these prints around to see what code does what and so forth. 
Trying to keep my house under control http://www.desert-home.com/

Cruelkix


The test is going fine. It just passed download 2244.  :)

I like to send a "header" packet back to the client just to let it know you are responding. That way if it takes a while to generate the rest of the page, the client will wait.



That makes sense.  I modified my code to send a header out first.  

If this doesnt work to keep things from locking up, I think it's important that I really isolate where the problem is coming from.  I think I'll go barebones and load up some different sketches that work on just one thing at a time.

Isolate the webserver and make sure that works with just a simple send/reciever

then isolate the tempsensors and relay controls.

Cruelkix

#119
Oct 19, 2011, 10:28 pm Last Edit: Oct 19, 2011, 10:59 pm by Cruelkix Reason: 1

Cruelkix, do you have a memory used check in your code?  If you don't you should add one.  Strings and me don't get along; everytime I use them I run out of memory on the board and it fails.  It's probably me and my chaotic coding style, but nevertheless, it's good to know how much memory is being used.  I try to keep the available memory over 200 at all times.  That's roughly the overhead you need to keep from clobbering the stack and wandering off into space code wise.

The testing of available memory can be put almost anywhere just to be sure you're not doing that and just confusing yourself.

Don't misunderstand, this is a problem I have constantly and I code against now.  You may not have a problem at all, it's just good to know.

It's easy to check the memory, just include:

#include <MemoryFree.h>

and then print it so you can check what's going on.

Serial.println(freeMemory());

You can sprinkle these prints around to see what code does what and so forth.  



Interesting.  I'll throw that in as well and see if I'm having an issue.  I would think that it can't be that though, just because of how random the crashes are.

EDIT: As a side note, anyone trying to use MemoryFree.h, it is not actually include in the standard libraries and is not a standard library download.  You have to create it.  Follow the instructions here in Reply #19:

http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1213583720/15

Go Up