Go Down

Topic: Revised Ethernet Library Client code (Read 10 times) previous topic - next topic

etracer

Apr 02, 2009, 04:53 am Last Edit: Sep 27, 2014, 06:55 am by etracer Reason: 1
I've spent the last couple of days beating my head against a number of problems with the official Ethernet library. There are a variety of problems with the Client code that manifest themselves as connections that fail to close, and the inability to open multiple connections without running into a 35 second delay.

While there were a number of issues I addressed, the primary problem was in the stop() method that is meant to disconnect a connected client. The code was erroneously closing the connection without sending a FIN to gracefully disconnect, and then followed that immediately with a disconnect() which tries to gracefully shutdown with a FIN. This was causing the connection to get stuck in a FIN_WAIT state which made it unavailable for use until the state timed out after 35 seconds.

There were also a few scary opportunities where an array out-of-bounds condition could be triggered if the sketch called some of the client methods out of order. This could have led to memory corruption in the sketch.

There were too many changes to list the parts to patch here, so I'm simply making the revised Client.cpp file available.  You can download it from:

*EDIT* Link removed because it is no longer valid *EDIT*

NOTE! This code is based on the Ethernet library included with Arduino-0015. Compatibility with previous versions is unknown.

There are a lot of comments detailing the changes included in the code.

Simply replace (after you back up) the Client.cpp file in your Ethernet library (hardware/libraries/Ethernet). Be sure to delete the old Client.o file or the code won't recompile.

Here's a sample sketch that demonstrates the improved client code by making 50 connections to a webserver as quickly as possible. Be sure to update the ip[] and server[] with addresses that are appropriate for you.

Code: [Select]
// Sample sketch to demonstrate the fixes to the Ethernet library Client
// Connect to a webserver 50 times as quickly as possible and read the resulting page

#include <Ethernet.h>

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };
byte ip[] = { xxx, xxx, xxx, xxx };
byte server[] = {yyy, yyy, yyy, yyy };

uint8_t connectStatus;
uint8_t closeStatus;
int ignore;

Client client(server, 80);

void setup()
{
 Ethernet.begin(mac, ip);
 Serial.begin(9600);
}

void loop()
{
 for (int i=1; i<=50; i++) {
   Serial.print("Connection Attempt ");
   Serial.print(i, DEC);
   ConnectToWebserver();
   delay(1);
 }

// Enter an infinite loop to "stop" the sketch
 while (true);
}


void ConnectToWebserver(){

 uint8_t connectStatus;
 
 if (client.connect()) {
   Serial.print(" - Connected");
   
// Send the HTTP GET to the server
   client.println("GET / HTTP/1.0");
   client.println();

// Read the response
   Serial.print(" - ");
   Serial.print(ReadResponse(), DEC);
   Serial.println(" bytes received");

// Disconnect from the server
   client.flush();
   client.stop();
   
 } else {
// Connection failed
   Serial.println(" - CONNECTION FAILED!");
 }
}


int ReadResponse(){
 int totalBytes=0;
 unsigned long startTime = millis();

// First wait up to 5 seconds for the server to return some data.
// If we don't have this initial timeout period we might try to
// read the data "too quickly" before the server can generate the
// response to the request

 while ((!client.available()) && ((millis() - startTime ) < 5000));

 while (client.available()) {
   char c = client.read();
   totalBytes+=1;
 }
 return totalBytes;
}


I've successfully tested a run of 5000 consecutive connections successfully so the code appears solid. The only thing I've noticed is an occasional failure to connect on the initial attempt. I think this might be related to a reset and how quickly you start trying to make connections. The reality is you need to write your code to be "tolerant" of failed connections. Connection failures happen in the real world - even in a controlled environment with a local web server.

Another common mistake I see in peoples' code is not allowing time for the server to generate a response before attempting to read from the client. Sometimes web servers take time to respond. They almost always take longer than the time it takes the MCU to execute the few lines of code people typically have between their connect() and read()'s. The example sketch demonstrates how to wait for response with a timeout.

Let me know how testing goes.  ;)

mellis

Great!  Nice work.  Can you post this to the developers mailing list: http://arduino.cc/mailman/listinfo/developers_arduino.cc?  I'll try to incorporate it into the distribution soon.

etracer

I made a complete (but unfortunately long) post to the developer list. So all of the details are there for discussion.

follower

Thanks for looking into all of this.

Is this a good time to merge it into Ethernet2 and use that instead? I don't think this code actually touches the low-level stuff that the Ethernet2 changes do.

How much of an issue is the "SPI busyness" concern? It's not like anything else will be using the SPI bus at the same time is it? Does the WIZnet chip have a problem if it's called repeatedly? If it's not absolutely required I'd suggest removing it as an unnecessary complication.

I wonder about having a private 'isValid()' method rather than repeating the check in the code? I guess this is probably a stylistic question.

The whole close/disconnect thing did always seem to be odd and I remember running into issues with it back when I first worked on WIZnet stuff, so it's good to see it might finally work reliably! :-)

--Phil.

etracer

I'm not really that familiar with the Ethernet2 library, but after a quick look it seems like most of the enhancements apply. From what I see the Client.cpp looks basically unchanged.

My main concerns on the "SPI busyness" have to do with:

1. The impact on the bus if other SPI devices were being used. The tight loops continually checking the status of the socket would monopolize the bus. Maybe not an issue because the master is busy anyway?

2. Potential impact on interrupt handlers. I'm not sure about the interrupt priority of the hardware SPI or if it's even an issue. It just didn't seem like a good idea to be thrashing the bus.

Since a loop is needed to keep checking the status anyway, the only issue is whether a delay(1) serves any value. The only negative is adding a millisecond to the connect or disconnect. There's so much other overhead that I wasn't able to make more than a few connections a second anyway so I can't see 1ms making much difference. Maybe it's not enough delay to help items 1 & 2 anyway and therefore moot?

Anyway, that's the reasoning behind the code.

Go Up