Pages: 1 ... 6 7 [8] 9 10 ... 18   Go Down
Author Topic: Ethernet (w5100) sketch hangs  (Read 21732 times)
0 Members and 1 Guest are viewing this topic.
Valencia, Spain
Offline Offline
Faraday Member
**
Karma: 144
Posts: 5358
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

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

Miramar Beach, Florida
Offline Offline
Faraday Member
**
Karma: 140
Posts: 5873
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

@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:
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.
« Last Edit: October 19, 2011, 08:11:09 am by SurferTim » Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 53
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Code:
#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.
Logged

Miramar Beach, Florida
Offline Offline
Faraday Member
**
Karma: 140
Posts: 5873
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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.
« Last Edit: October 19, 2011, 09:30:06 am by SurferTim » Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 53
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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;
        }

Logged

Miramar Beach, Florida
Offline Offline
Faraday Member
**
Karma: 140
Posts: 5873
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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.
 
« Last Edit: October 19, 2011, 11:03:01 am by SurferTim » Logged

Offline Offline
Full Member
***
Karma: 11
Posts: 147
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Offline Offline
Full Member
***
Karma: 11
Posts: 147
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


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.
Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 53
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


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 smiley-razz

Code:

            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??
« Last Edit: October 19, 2011, 01:53:48 pm by Cruelkix » Logged

Miramar Beach, Florida
Offline Offline
Faraday Member
**
Karma: 140
Posts: 5873
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Try something simple. Here is a really simple response. I can't test it right now. I have another test running.
Code:
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:
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);

« Last Edit: October 19, 2011, 02:21:01 pm by SurferTim » Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 53
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Try something simple. Here is a really simple response. I can't test it right now. I have another test running.
Code:
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:
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.
Logged

Miramar Beach, Florida
Offline Offline
Faraday Member
**
Karma: 140
Posts: 5873
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

The test is going fine. It just passed download 2244.  smiley

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.
« Last Edit: October 19, 2011, 02:53:27 pm by SurferTim » Logged

New River, Arizona
Offline Offline
God Member
*****
Karma: 19
Posts: 931
Arduino rocks
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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. 
Logged

Trying to keep my house under control http://www.desert-home.com/

Offline Offline
Jr. Member
**
Karma: 0
Posts: 53
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

The test is going fine. It just passed download 2244.  smiley

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.
Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 53
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
« Last Edit: October 19, 2011, 03:59:13 pm by Cruelkix » Logged

Pages: 1 ... 6 7 [8] 9 10 ... 18   Go Up
Jump to: