itsanjan / arduino

Automatically exported from code.google.com/p/arduino
Other
0 stars 0 forks source link

TCP Client should allow buffering the outbound packet in w5100 memory. #563

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What change would like to see?
Using the standard TCP Client library with the standard Ethernet Shield, every 
single call to client.write(single_byte) results in an entire frame being 
constructed and sent.  This is terribly inefficient.  The underlying socket.cpp 
library already provides support for starting a packet, adding data, and 
finally sending the packet, but only for UDP.  This should absolutely be 
available for TCP as well.  I would argue that it's even more important for TCP 
than for UDP.

The problem _can_ be worked around by constructing the entire desired TCP 
packet in ram first, but this imposes serious restrictions on possible packet 
size, and completely ignores the 16k of packet buffer memory on the w5100 chip.

While this lack of efficiency is not really a huge problem on a local network, 
it can dramatically change the network bandwidth required for a sensor node 
connected over the internet for example.

Would this cause any incompatibilities with previous versions?  If so, how
can these be mitigated?

I believe they should be extra options. I believe the simplistic existing code 
is suitable for simplistic use cases, and should be kept, so I do not see any 
incompatibilities with previous versions.

Original issue reported on code.google.com by tweakne...@googlemail.com on 23 Jun 2011 at 6:16

GoogleCodeExporter commented 9 years ago
Pull request on github at https://github.com/arduino/Arduino/pull/34 

Original comment by tweakne...@googlemail.com on 24 Jun 2011 at 12:51

GoogleCodeExporter commented 9 years ago
Could this be done by creating beginPacket() / endPacket() calls, within which 
write(), print(), and println() would add their data to the W5100 buffer rather 
than sending it immediately?  Instead of creating the new addData() functions?  
This could also be something like buffer() / flush().

Original comment by dmel...@gmail.com on 24 Aug 2011 at 7:16

GoogleCodeExporter commented 9 years ago
I don't think we should add this at all.  

Adding anything to work with packets or buffers of data in TCP will make people 
think that they can (or that they should) directly affect where the packet 
breaks occur, and they can't (and shouldn't).

I know if seems inefficient to be sending single byte packets, but that's 
exactly what telnet will do if you're in an interactive terminal session to a 
remote server.  If the acknowledgement doesn't come back from the remote TCP 
layer before the next byte is ready to be sent then the Nagle algorithm will 
kick in and subsequent data will be buffered until either the acknowledgement 
arrives, or there's a full packet ready to send.

I haven't double-checked that the Wiznet TCP layer implements Nagle, but it's a 
pretty standard and old bit of TCP, and I don't think we should pollute the API 
just for the Wiznet chip.

Original comment by amce...@bluefountain.com on 27 Aug 2011 at 10:48

GoogleCodeExporter commented 9 years ago
This is mostly up to Adrian (as maintainer of the Ethernet library) but I, at 
least, am open to further arguments for why this is a good idea.

Original comment by dmel...@gmail.com on 28 Aug 2011 at 6:30

GoogleCodeExporter commented 9 years ago
http://www.google.com/url?sa=t&source=web&cd=7&ved=0CF4QFjAG&url=http%3A%2F%2Far
duino.cc%2Fpipermail%2Fdevelopers_arduino.cc%2F2009-March%2F000693.html&ei=ByeLT
q7SDIqL0AWl2IHcBQ&usg=AFQjCNGcaG4cvsKnO4EveqlX4atTKWrw8g&sig2=IsQaEwN7HCfg5DwLWl
WWRw

It seems this discussion came to the conclusion that there is no naggle in 
w5100. Also reading the whole topic they have gotten the improvement numbers. I 
think the discussion died just allowing the transmission of 8 bit binary write. 
I also think that the user should be faced with this question, but also as 
sugested in the mailing list the compromise would come by making undocumended.

Correct me if i am wrong

Original comment by ptsne...@gmail.com on 4 Oct 2011 at 4:19

GoogleCodeExporter commented 9 years ago
Ah, let me take back that "I haven't double-checked that the Wiznet TCP layer 
implements Nagle", but looking through the thread that ptsneves linked to, it 
seems I *did* find out that it *doesn't* implement Nagle, but had forgotten 
that in the intervening two years.  See 
http://arduino.cc/pipermail/developers_arduino.cc/2009-March/000756.html for my 
findings.

However, I still agree with my comment in that thread, that the bulk binary 
write is useful, but that we shouldn't be confusing the API at the Client level 
by exposing packet details to a stream API.  

I still haven't seen any argument for why we should add this other than "I've 
looked at a packet trace and it's inefficient".  When I did some timings (see 
elsewhere in the comment linked to above) adding this improved the speed of 
sending an HTTP request by "less than quarter of a millisecond".  I suspect 
there are better ways to find that sort of speed improvement just by optimising 
the Ethernet library code...

Original comment by adrian.m...@gmail.com on 9 Dec 2011 at 12:51

GoogleCodeExporter commented 9 years ago
Adrian, the efficiency argument had nothing to do with speed.  It's about 
datasize on the wire. (and the real $ cost of that datasize on certain networks)

Using something like MQTT, and trying to write out a 10 byte tcp payload would 
hopefully result in only 10+1*(tcp headers + ether headers) bytes on the wire.  
Depending on how the MQTT library is written, it actually becomes 6-8 entire 
frames.  (I can't remember anymore)  TCP correctly reassembles, and on a local 
ethernet segment, no-one ever knows or cares.  But if this is going over 3g, or 
something where bandwidth costs real money, then this starts to matter more.

Of course, the library can construct the whole packet in memory, and write just 
once, but now the whole packet has to be buffered in memory, probably in 
addition to the user data.  

Original comment by tweakne...@googlemail.com on 22 Dec 2011 at 3:41