Smal Bug in Ethernet Lib, String var produces 1 byte packets

Hi everyone,

I want to use my arduino for some ethernet applications. When testing ethernet I always use wireshark to see if the quality of the libraries are up to standards.

Altough everything is working I noticed that every "client.print" produces a seperate telegram. That is ok for a small microprocessor without a big memory. But I also found out that using a String instead of a char[] produces 1 byte packets.

I put the following code inside the standard webserver example.

1) char touw[] = "The quick brown fox jumped over the lazy dog"; 2) client.print(touw); 3) String s=touw; 4) client.print(s);

so line 2 produces one telegram with 42 netto bytes line 4 produces 42 telegrams with 1 byte netto.

why is that? Shure it still is a valid html, my browser doesn't complain but it takes 5 times a long to transfer and is a lot of overhead for a smal cpu.

Hardware Mega 2560 & Ethernetshield (w5100)

Where can I put in a bug report?

cheers

John

Can you post a full sketch that demonstrates this? I'd like to see exactly how it is structured.

Where can I put in a bug report?

How about on your monitor, so YOU are reminded not to use Strings?

Can you post a full sketch that demonstrates this? I'd like to see exactly how it is structured.

There was a thread about this, earlier in the week. The overload of the print() method that accepts a String actually does call write() for one character at a time, as opposed to using toCharArray() and writing the whole string in one step.

I suspect the culprit is Print.cpp which has this function:

size_t Print::print(const String &s)
{
  size_t n = 0;
  for (uint16_t i = 0; i < s.length(); i++) {
    n += write(s[i]);
  }
  return n;
}

As you can see, printing a String involves calling write() repeatedly for each character.

write() is a virtual method which would be overridden by the Client class.

However printing a char does this:

size_t Print::print(const char str[])
{
  return write(str);
}

A single call to the write() method.

You might want to change the function in Print.cpp to be:

size_t Print::print(const String &s)
{
  return write (s.c_str ());
}

See if that gives better results.

Bug reporting here:

PaulS: How about on your monitor, so YOU are reminded not to use Strings?

That String class will be the death of us.

http://www.gammon.com.au/forum/?id=12153#trap23

Best avoid the String class anyway.

This clearly isn't a bug in the Ethernet library, but (if anything) the Print library.

Johnvanzijl: why is that? Shure it still is a valid html, my browser doesn't complain but it takes 5 times a long to transfer and is a lot of overhead for a smal cpu.

Actually, using String is a lot of overhead for the poor CPU.

http://www.gammon.com.au/concat

Two ways to send HYPNO.JPG, with the large packet being 4x times faster.

//zoomkat 12/26/12
//SD server test code
//open serial monitor to see what the arduino receives
//address will look like http://192.168.1.102:84 when submited
//for use with W5100 based ethernet shields

#include <SD.h>
#include <SPI.h>
#include <Ethernet.h>

byte mac[] = { 
  0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED }; //physical mac address
byte ip[] = { 192, 168, 1, 102 }; // ip in lan
byte gateway[] = { 192, 168, 1, 1 }; // internet access via router
byte subnet[] = { 255, 255, 255, 0 }; //subnet mask
EthernetServer server(84); //server port
String readString; 

//////////////////////

void setup(){

  Serial.begin(9600);

  // disable w5100 while setting up SD
  pinMode(10,OUTPUT);
  digitalWrite(10,HIGH);
  Serial.print("Starting SD..");
  if(!SD.begin(4)) Serial.println("failed");
  else Serial.println("ok");

  Ethernet.begin(mac, ip, gateway, gateway, subnet);

  //delay(2000);
  server.begin();
  Serial.println("Ready");

}

void loop(){
  // Create a client connection
  EthernetClient client = server.available();
  if (client) {
    while (client.connected()) {
      if (client.available()) {
        char c = client.read();

        //read char by char HTTP request
        if (readString.length() < 100) {
          //store characters to string 
          readString += c; 
          //Serial.print(c);
        } 
        //if HTTP request has ended
        if (c == '\n') {

          ///////////////
          Serial.println(readString); //print to serial monitor for debuging 

            client.println("HTTP/1.1 200 OK"); //send new page
          client.println("Content-Type: text/html");
          //client.println("Content-Type: image/jpeg");
          //client.println("Content-Type: image/gif");
          //client.println("Content-Type: application/x-javascript");
          //client.println("Content-Type: text");

          client.println();

          //File myFile = SD.open("boom.htm");
          File myFile = SD.open("HYPNO.JPG");
          //File myFile = SD.open("BLUEH_SL.GIF");
          //File myFile = SD.open("SERVOSLD.HTM");
          if (myFile) {
            //Serial.println("test.txt:");
            // read from the file until there's nothing else in it:
            while (myFile.available()) {
              client.write(myFile.read());
            }
            // close the file:
            myFile.close();

          }
          delay(1);
          //stopping client
          client.stop();
          readString="";
        }
      }
    }
  } 
}

Bigger packet

//zoomkat 12/26/12
//SD server test code
//open serial monitor to see what the arduino receives
//address will look like http://192.168.1.102:84 when submited
//for use with W5100 based ethernet shields

#include <SD.h>
#include <SPI.h>
#include <Ethernet.h>

byte mac[] = { 
  0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED }; //physical mac address
byte ip[] = { 
  192, 168, 1, 102 }; // ip in lan
byte gateway[] = { 
  192, 168, 1, 1 }; // internet access via router
byte subnet[] = { 
  255, 255, 255, 0 }; //subnet mask
EthernetServer server(84); //server port
String readString; 

//////////////////////

void setup(){

  Serial.begin(9600);

  // disable w5100 while setting up SD
  pinMode(10,OUTPUT);
  digitalWrite(10,HIGH);
  Serial.print("Starting SD..");
  if(!SD.begin(4)) Serial.println("failed");
  else Serial.println("ok");

  Ethernet.begin(mac, ip, gateway, gateway, subnet);
  //delay(2000);
  server.begin();
  Serial.println("Ready");
}

void loop(){
  // Create a client connection
  EthernetClient client = server.available();
  if (client) {
    while (client.connected()) {
      if (client.available()) {
        char c = client.read();

        //read char by char HTTP request
        if (readString.length() < 100) {
          //store characters to string 
          readString += c; 
          //Serial.print(c);
        } 
        //if HTTP request has ended
        if (c == '\n') {

          ///////////////
          Serial.println(readString); //print to serial monitor for debuging 

            client.println("HTTP/1.1 200 OK"); //send new page
          //client.println("Content-Type: text/html");
          client.println("Content-Type: image/jpeg");
          //client.println("Content-Type: image/gif");
          //client.println("Content-Type: application/x-javascript");
          //client.println("Content-Type: text");

          client.println();

          //File myFile = SD.open("boom.htm");
          File myFile = SD.open("HYPNO.JPG");
          //File myFile = SD.open("BLUEH_SL.GIF");
          //File myFile = SD.open("SERVOSLD.HTM");

          if (myFile) {

            byte clientBuf[64];
            int clientCount = 0;

            while(myFile.available())
            {
              clientBuf[clientCount] = myFile.read();
              clientCount++;

              if(clientCount > 63)
              {
                // Serial.println("Packet");
                client.write(clientBuf,64);
                clientCount = 0;
              }
            }
            //final <64 byte cleanup packet
            if(clientCount > 0) client.write(clientBuf,clientCount);            
            // close the file:
            myFile.close();
          }
          delay(1);
          //stopping client
          client.stop();
          readString="";
        }
      }
    }
  } 
}

Does the Due do a better job?... With 96 K of Sram?

Doc

Docedison: Does the Due do a better job?... With 96 K of Sram?

Doc

Give it a try and let us know! :)

I don't know how much better the transfer rate will be. It really gets down to the SPI bus speed after a point. But the bigger the packet, the faster the transfer.

I selected 64 bytes for the buffer so the payload was at least bigger than the header, and it will work with an Uno without running out of SRAM. 1400 bytes is about the max packet size with the W5100.

I bought a Digix, A Due compatible board and it comes with a low power WiFi board and an nRF24L01+ radio too.. I would Hope? that with the new breed of Arduino products with semi adequate sram capacity.. The multiple problems with the String class and it's dependencies on other broken String handling... like the fix that Nick Gammon suggested for the print class will be fixed completely.. So that they warn on small footprint Sram devices of possible trouble and on the larger... Due, Tre and that Intel Galileo thing would work properly... However if the progress towards making the compiler even a little less 'dated' is any indication of forward progress... then it's anyone's guess as to the real future of the Arduino idiom... IMNSHO.. I guess that many things have to change before the "New" devices and boards are "Properly" supported by the Arduino IDE... I intend only to speak of the ones that Arduino.cc "Officially" sells and supports. The "Variants should be supported by the designer of the Variant or a caveat about the level of official IDE support. Teensy's, Chipkit, Maple... The list is long and the real data is Mostly Very short..

Doc

I was hoping Johnvanzijl would get back to us with an update. Then we can file a proper bug report for the Print class. At least that can get fixed one day.

I don't agree this is an issue with Print-class. The function send() in socket.cpp calles W5100.execCmdSn(s, Sock_SEND) no matter how much data was previously send to the socket, so calling write() with a single char sends out a single-char packet. That actually could be implemented smarter (using a combination of timer and W5100 free memsize to decide whether to send the packet immediatly or wait for more data to be added). It shouldn't be the responsibility of the application to decide on packet-sizes.

That actually could be implemented smarter (using a combination of timer and W5100 free memsize to decide whether to send the packet immediatly or wait for more data to be added). It shouldn't be the responsibility of the application to decide on packet-sizes.

How will it decide you are finished sending? If it waits for more data and nothing else is sent, what then? I don't have a problem with packet size. I can control the size of the packets myself, and would like to keep it that way for now.

I would be pleased if a way could be implemented to store the data in the w5100 tx buffer until you called a client.send() function that would then send the W5100.execCmdSn(s, Sock_SEND) . Something like this:

client.print("This ");
client.print("is ");
client.write('a');
client.println(" test.");
client.send();

Then all that gets sent in one packet. But then it would be up to the programmer to not overflow the w5100 socket tx buffer.

edit: Even better (to keep compatibility with previous code), you would also need a client.nosend() command. Like this:

client.nosend();
client.print("This ");
client.print("is ");
client.write('a');
client.println(" test.");
client.send();

SurferTim: How will it decide you are finished sending? If it waits for more data and nothing else is sent, what then?

you need a maintain-function that is called by every Client, Server or UDP-method that tracks the time since last packet sent per socket and sends out the data whenever a given timeout has expired. This and the send-method would trigger the package-send whenever the MSS-size is reached. That way no RAM of the µC is wasted by buffering data as it's allready stored in W5x00. Of course this could result in data waiting indefinitly if you happen to call Print.write(..) with no Client.stop() or available() or anything related to network afterwards, but what kind of app should do that, as you are required to stop any Client you don't intend to read from at a later time before it goes out of scope anyway? (The alternative would be to use Timer-interupts to trigger the housekeeping).

Personally, I don't want a timeout delay before sending, and I don't want the ethernet functions using an interrupt. That can really screw up other SPI devices.

On a reall OS you will not find a socket-implementation that does not buffer and sends by timeout. But they all have a flush-method that ensures buffered data is send immediatly. (Being used to this behaviour I never understood why the flush on Arduino does delete the incoming data though). BTW: in my UIPEthernet-libray I implemented such housekeeping sending packets whenever mss is reached, incomming-data is to be acknowledged or socket-timeout has expired. No interupts being used. Works flawlessly.

You may want to read a bit about this optimization here: http://en.wikipedia.org/wiki/Nagle%27s_Algorithm. A 200ms-timeout is the widely accepted default here. The socket has an option TCP_NODELAY to override and bypass the delay (that's what should be set when an higher-level Socket-streams flush()-method is called).