arduino-libraries / Ethernet

Ethernet Library for Arduino
http://arduino.cc/
258 stars 264 forks source link

EthernetServer available() is confusing #13

Open agdl opened 8 years ago

agdl commented 8 years ago

From @lestofante on March 15, 2013 10:29

Copied from original issue: arduino/Arduino#1320

EthernetServer.available() returns the first socket connected (or waiting for close) with data in RX buffer.

This is bad because:

We have found 3 solution:

  1. Add a flag into EthernetClient: when returned by available() this client will never be returned thanks to this flag. The client is returned even is RX buffer is empty.
    • Can break some code.
  2. Add an EthernetServer.available(int) where user can choose what Socket to use. This should be invisible to the user, as it can overflow the array or receive non-connected EthernetClient,
    • Advantage is that this is an overloaded method, so old code will work.
  3. Add a new method like EthernetServer.getNextClient() which will use the flag method as in solution (1).
    • No retro-compatibility problem.
    • An easy way to get all client connected.
    • A method to get maximum number of client connected should be provided, or the constant MAX_SOCK_NUM should be public, so user can create their own EthernetClient array

(the last is probably the best)

Additional Context

Related discussion (in Italian):

https://forum.arduino.cc/t/accettare-piu-connessioni-in-ingresso-su-server-ethernet/150498

PaulStoffregen commented 6 years ago

@cmaglie - I believe we need this in the 2.0 API. Protocols like FTP can't be implemented with the existing API. I'm leaning towards EthernetServer.connected(), like these:

http://forum.arduino.cc/index.php?topic=146795.0 http://forum.arduino.cc/index.php?topic=169165.msg1346247#msg1346247 https://github.com/gallegojm/Arduino-Ftp-Server/blob/master/FtpServer/FtpServer.cpp

Any thoughts on the API. Is EthernetServer.connected() ok with you?

cmaglie commented 6 years ago

The additional API is OK, please just use accept() instead of connected().

The reason is that connected() looks more like a function to query the state of the socket like, say, availble(), instead accept represents better the action of accepting incoming connections and it's also used in API of other languages.

cmaglie commented 6 years ago

Also the older available() API would became orthogonal to this one, in a sketch you would use available() or accept() but not both. Also this change should find his way up to the cores and the documentation.

PaulStoffregen commented 6 years ago

@cmaglie If we're calling this "accept", maybe the behavior should be changed to mark the client as accepted, so it won't be returned again? Currently it's implemented very much like available(), where it will be returned repeatedly.

PaulStoffregen commented 6 years ago

Let me make sure I understand how we're intending these functions to work?

Using server.available(), the library keeps a list of all the clients and returns whatever client has recently sent data. Multiple clients may be connected, but a "false" client is returned if none have sent any data. The intended usage model is simple code which repeatedly calls server.available() to get the next client data.

Using server.accept(), the user's sketch must keep track of which client(s) are connected. When a client connects, it is returned once regardless of whether it has sent any data. The sketch must keep track of the client, because server.accept() will not return the same client again.

Does that sound right?

cmaglie commented 6 years ago

Using server.accept(), the user's sketch must keep track of which client(s) are connected. When a client connects, it is returned once regardless of whether it has sent any data. The sketch must keep track of the client, because server.accept() will not return the same client again.

Correct. I thought that this was the behaviour of the new connected() function, sorry for the misunderstanding.

PaulStoffregen commented 6 years ago

Sounds good. I'll change it to this.

PaulStoffregen commented 6 years ago

I rewrote the AdvancedChatServer example to use EthernetServer accept(). It's working very nicely. :)

PaulStoffregen commented 6 years ago

@cmaglie - Who should we get involved in this conversation to update the website with the new functions?

cmaglie commented 6 years ago

/cc @SimonePDA

SimonePDA commented 6 years ago

I have already added the Ethernet new APIs using the massive work done by per1234 in https://github.com/arduino-libraries/Ethernet/issues/67.

You can see them online here: https://www.arduino.cc/en/Reference/Ethernet

Let me know if something needs fixes or further information.