Ethernet2 library enhancement

I was disappointed with the Ethernet library's performance, then I heard of Ethernet2, but it was pretty much the same speed. I was able to get 4.70KB/s over an HTTP connection with the standard Client.read() interface. I thought that the hardware must be able to do better, so I wrote a small addition to the library, which allowed me to achieve 81.92KB/s... See if you can beat my speed!

Addition to Client.cpp:

// returns # of bytes read, n is # of bytes requested, 
// b is a pointer to at least n bytes.
uint16_t Client::readn(uint16_t n, uint8_t *b) {
  if (!available())
    return -1;
  return recv(_sock, b, n < available() ? n : available());
}

Addition to Client.h:

//in public:
  uint16_t readn(uint16_t, uint8_t*);

Here is my RSS reader (early):

#include <Ethernet2.h>

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };
byte ip[] = { 192, 168, 1, 200 };
byte server[] = { 74,125,113,121 }; // rss.slashdot.org

const int BUFSZ (1024);

char buf[BUFSZ];

Client client(server, 80);

void setup()
{
  Ethernet.begin(mac, ip);
  Serial.begin(115200);
  
  delay(2000);
  
  Serial.println("connecting...");
  
  if (client.connect()) {
    Serial.println("connected");
    client.println("GET /Slashdot/slashdot HTTP/1.1");
    client.println("Host: rss.slashdot.org");
    client.println();
  } else {
    Serial.println("connection failed");
  }
}

void loop()
{
  unsigned long bytes(0);
  unsigned long beg(millis());
  if(client.available()) {
    while (client.available()) {
      bytes += client.readn(BUFSZ, (uint8_t*)buf);
      //Serial.print(buf);
    }
    Serial.print("Bytes: ");
    Serial.println(bytes);
    Serial.print("millis: ");
    Serial.println(millis()-beg);
    Serial.print("Speed: ");
    Serial.println(bytes/(float)(millis()-beg));
  }
  
  if (!client.connected()) {
    Serial.println();
    Serial.println("disconnecting.");
    client.stop();
    while(1);
  }
}

I know, my sketch is rough around the edges still, but the performance improvement I thought was worth sharing...

What do you think?

I noticed you have three calls to available() in your readn() method; does the compiler optimize this? If not, then do a single call to available, storing its return value (in the if() would work), then pass the value to recv(). That should be quicker (if it isn't, or is slower, then the compiler is likely optimizing the logic).

Actually:

// returns # of bytes read, n is # of bytes requested,
// b is a pointer to at least n bytes.
uint16_t Client::readn(uint16_t n, uint8_t *b) {
  if (!n = (n < available() ? n : available()))
    return -1;
  return recv(_sock, b, n);
}

This could probably be compacted more, but it would quickly become unreadable. This might all be moot though if the compiler is already optimizing the code...

:slight_smile:

I took cr0sh's advice, and made that change to Client.cpp, but I cannot test it until late tonight...

I also modified my code to attempt to get more accurate transfer statistics, but I don't have my Arduino with me to test. I should have grabbed it this morning. :frowning:

#include <Ethernet2.h>

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };
byte ip[] = { 192, 168, 1, 200 };
byte server[] = { 74,125,113,121 }; // rss.slashdot.org

const int BUFSZ (1024);

char buf[BUFSZ];

Client client(server, 80);

void setup()
{
  Ethernet.begin(mac, ip);
  Serial.begin(115200);
  
  delay(2000);
}

void loop()
{
  unsigned long bytes(0), beg(millis()), calls(0);
  
  Serial.println("connecting...");
  if (client.connect()) {
    Serial.println("connected");
    client.println("GET /Slashdot/slashdot HTTP/1.1");
    client.println("Host: rss.slashdot.org");
    client.println();
    while(client.connected()) {
      while (client.available()) {
        bytes += client.readn(BUFSZ, (uint8_t*)buf);
        ++calls;
        //Serial.print(buf);
      }
    }
    unsigned long dur(millis() - beg);
    Serial.print("Called readn() ");
    Serial.print(calls);
    Serial.println(" times.");
    Serial.print("Transferred ");
    Serial.print(bytes);
    Serial.println(" Bytes.");
    Serial.print("Took ");
    Serial.print(dur);
    Serial.println("ms.");
    Serial.print("Speed: ");
    Serial.println(bytes/(float)dur);
  
    Serial.println();
    Serial.println("disconnecting.");
    client.stop();  
  } else {
    Serial.println("connection failed");
  }
  delay(30000);
}

Would anybody be willing to test this out for me and see how it performs?

I lied slightly, the code I added to Client.cpp is actually slightly different from cr0sh's suggestion...

// returns # of bytes read, n is # of bytes requested,
// b is a pointer to at least n bytes.
uint16_t Client::readn(uint16_t n, uint8_t *b) {
  if (n = (n < available() ? n : available()))
    return recv(_sock, b, n);
  return -1;
}

I was thinking another possible way to write it is just:

uint16_t Client::readn(uint16_t n, uint8_t *b) {
  return recv(_sock, b, n < available() ? n : available());
}

This also allows you to treat the returned number of bytes read as a boolean.

New Code, added new feature of being able to select the source port.

Here's a patch against the latest Ethernet2 library:

Index: Client.cpp

===================================================================

--- Client.cpp      (revision 151)

+++ Client.cpp      (working copy)

@@ -17,6 +17,12 @@

   _port = port;  
 }
 
+Client::Client(uint8_t *ip, uint16_t port, uint16_t srcport) {
+  _ip = ip;
+  _port = port;
+  _srcport = srcport;
+}
+
 uint8_t Client::connect() {
   _sock = 255;
   
@@ -30,7 +36,7 @@

     return 0;
     
   // XXX: what port should we connect from?
-  socket(_sock, Sn_MR_TCP, _port, 0);
+  socket(_sock, Sn_MR_TCP, _srcport, 0);
   
   if (!::connect(_sock, _ip, _port))
     return 0;
@@ -59,6 +65,18 @@

   return b;
 }
 
+uint16_t Client::readn(uint16_t n, uint8_t *b) {
+  if (!available())
+    return 0;
+  return recv(_sock, b, n < available() ? n : available());
+}
+
+/*
+uint16_t Client::readn(uint16_t n, uint8_t *b) {
+  return recv(_sock, b, n < available() ? n : available());
+}
+*/
+
 void Client::flush() {
   while (available())
     read();
Index: Client.h

===================================================================

--- Client.h      (revision 151)

+++ Client.h      (working copy)

@@ -8,14 +8,17 @@

   uint8_t _sock;
   uint8_t *_ip;
   uint16_t _port;
+  uint16_t _srcport;
 public:
   Client(uint8_t);
   Client(uint8_t *, uint16_t);
+  Client(uint8_t *, uint16_t, uint16_t);
   uint8_t status();
   uint8_t connect();
   virtual void write(uint8_t);
   int available();
   int read();
+  uint16_t readn(uint16_t, uint8_t*);
   void flush();
   void stop();
   uint8_t connected();
Index: Server.cpp

===================================================================

--- Server.cpp      (revision 151)

+++ Server.cpp      (working copy)

@@ -4,7 +4,7 @@

   #include "socket.h"
 }
 
-#include "Ethernet.h"
+#include "Ethernet2.h"
 #include "Client.h"
 #include "Server.h"
 
@@ -77,4 +77,4 @@

       client.write(b);
     }
   }
-}

\ No newline at end of file

+}

Here's the latest iteration of my code.

I have also noticed that my connections don't really terminate when the webserver is finished sending, so I am detecting the end of input.

#include <Ethernet2.h>

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xE5 };
byte ip[] = { 192,168,1,206 };
byte server[] = { 74,125,113,121 }; // rss.slashdot.org

const int BUFSZ (1025);
char buf[BUFSZ];
unsigned int src_port(10240);

void setup()
{
  Ethernet.begin(mac, ip);
  Serial.begin(115200);
  Serial.println("\n/. RSS Reader\n");
  delay(2000);
}

void loop()
{
  unsigned int  bytes = 0, 
                beg = 0, 
                calls = 0, 
                n = 0, 
                dur = 0;
  Client client(server, 80, ++src_port);
  Serial.print("connecting...\nSource port:");
  Serial.println(src_port);
  if (client.connect()) {
    Serial.println("connected");
    client.println("GET /Slashdot/slashdot HTTP/1.1");
    client.println("Host: rss.slashdot.org");
    client.println();
    beg = millis();
    while(client.connected()) {
      while (client.available()) {
        bytes += (n = client.readn(BUFSZ - 1, (uint8_t*)buf));
        buf[n] = '\0';
        ++calls;
        if(n > 7) // marks end of xmit, sizeof("\r\n0\r\n\r\n") - 1
        {
          if(0 == strcmp((char *)(buf + n - 7), "\r\n0\r\n\r\n"))
          {
            Serial.println();
            Serial.println("disconnecting.");
            client.stop();
            client.flush();
          }
        }
        //Serial.print(buf);
        /*
        if(!(calls % 10)) 
        {
          dur = millis() - beg;
          Serial.println(bytes);
          Serial.println(dur);
          Serial.println(bytes/(float)dur);
        }
        */
      }
    }
    unsigned long dur(millis() - beg);
    Serial.println();
    Serial.print("Called readn() ");
    Serial.print(calls);
    Serial.println(" times.");
    Serial.print("Transferred ");
    Serial.print(bytes);
    Serial.println(" Bytes.");
    Serial.print("Took ");
    Serial.print(dur);
    Serial.println("ms.");
    Serial.print("Speed: ");
    Serial.println(bytes/(float)dur);
  } else {
    Serial.println("connection failed");
  }
  delay(20000);
}

Anyone know how I would see about getting these changes into the arduino distribution?