Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiFiSever - Arduino API available() and accept() #7605

Closed
wants to merge 1 commit into from
Closed

WiFiSever - Arduino API available() and accept() #7605

wants to merge 1 commit into from

Conversation

JAndrassy
Copy link
Contributor

@JAndrassy JAndrassy commented Sep 17, 2020

According to documentation, the server.available() should return first client with data available.
https://www.arduino.cc/en/Reference/WiFiServerAvailable (esp8266 doc points there).

ESP8266WiFi library now does what a new EthernetServer.accept() method does: https://www.arduino.cc/en/Reference/EthernetServerAccept. It was added to Ethernet library by Paul Stoffregen in 2018 for Ethernet 2.0. I added it to UIPEthernet in February and I have it in my WiFiEspAT library too.

This PR shows a draft of a possible implementation of available(). (This quick implementation could let some client wait to long.)

My opinion is that it is strange to have a function like this available(). My explanation how they defined it like this, is that the networking libraries by Arduino are always remote APIs for a firmware and there it doesn't look so strange.

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 20, 2020

It seems to be a fair request, in the name of compatibility with Arduino API.

However, it would break many sketches and libraries because of the significant WiFiServer::available() semantic change.

There will be no change for simple TCP server answering to a simple request then closing.
There will be an undefined behavior / a total mess when several clients can be managed in parallel: such code would believe to have several independent connections, while they would be in fact all the same one. I would call this a collapsing change.

We can't really fully deprecate WiFiServer::available(), can we ?

What could be done:

  • in all case, ::accept() must be ::available() old implementation
  • add an optional user #define like ESP8266_ARDUINO_WIFISERVER_OLD_API to keep up with current = old API
  • without this define, make WiFiServer::available() unavailable
  • another #define is needed, like ESP8266_ARDUINO_WIFISERVER_LEGAL_API would enable official Arduino implementation for enabling ::available() like the one proposed in this PR.

pros:

  • current sketches/libraries would not compile (= will not silently break) after these changes, unless by using ESP8266_ARDUINO_WIFISERVER_OLD_API which is harmless with older versions of this core.
  • Arduino compatibility is possible but requires another define.

cons:

  • Arduino compatibility is not totally transparent and requires a define: using ESP8266_ARDUINO_WIFISERVER_LEGAL_API in a sketch / a library also requires from the user to check that this define is supported
#ifndef ESP8266_ARDUINO_WIFISERVER_LEGAL_API_SUPPORTED
#error must use core 3.0.0+
#else 
#define ESP8266_ARDUINO_WIFISERVER_LEGAL_API
#endif

This proposal raises several potential issues depending on how these defines are really defined by the user/sketch/library.
Before moving forward, we must all reach an agreement about the above.
Any other proposal is welcome. The condition is that an existing sketch must not silently break.

@JAndrassy
Copy link
Contributor Author

In the years esp8266 Arduino exists, missed someone the true Arduino behaviour of available()? I think not.
If it would be my call, it would add accept() as alias to available() and document available() as it is now.

@JAndrassy
Copy link
Contributor Author

now I realized that the Arduino implementation of available() is tied with the write-to-all-clients functionality of Server base class as Print implementation. this is not implemented in ESP8266WiFi library.

WiFiServer telnetLogging; where every telnetLogging.print goes to all connected clients (or is ignored if there is no connected client) could be passed to any function or class which would take Print& log.

maybe a WiFiArduinoServer could inherit from WiFiServer and add the Arduino style available() function and the write-to-all-clients functionality. (same for SSL server)

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 21, 2020

maybe a WiFiArduinoServer could inherit from WiFiServer and add the Arduino style available() function and the write-to-all-clients functionality. (same for SSL server)

That would be a very good first step.

@JAndrassy
Copy link
Contributor Author

maybe a WiFiArduinoServer could inherit from WiFiServer and add the Arduino style available() function and the write-to-all-clients functionality. (same for SSL server)

That would be a very good first step.

I rework this PR. Should I use WiFiClient or ClientContext to store the clients? (ClientContext with ref and unref)

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 21, 2020

I think you have noticed that WiFiClient is just a wrapper around ClientContext.
If you don't intend to indeed use but only store the client, just use the simple version.

@JAndrassy
Copy link
Contributor Author

this will be the example called PagerServer, a simple server that echoes any incoming messages to all connected telnet clients.

void loop() {
  WiFiClient client = server.available(); // returns first client which has data to read
  if (client) { // client is true only if it is connected and has data to read
    String s = client.readStringUntil('\n'); // read the message incoming from one of the clients
    Serial.println(s); // print the message to Serial Monitor
    client.print("echo: "); // this is only for sending client
    server.println(s); // send the message to all connected clients
    server.flush(); // flush the buffers
  }
}

I think it is Arduino way elegant.

https://github.com/jandrassy/WiFiEspAT/blob/master/examples/Basic/PagerServer/PagerServer.ino

@JAndrassy JAndrassy closed this Sep 26, 2020
@JAndrassy
Copy link
Contributor Author

next attempt #7612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants