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

WifiServer - Connection reset #119

Closed
Dennis650 opened this issue Jan 7, 2017 · 26 comments
Closed

WifiServer - Connection reset #119

Dennis650 opened this issue Jan 7, 2017 · 26 comments
Labels
Status: Test needed Issue needs testing

Comments

@Dennis650
Copy link

Hi,

i wanted to implement a WifiServer on the ESP32 with Arduino. I tried different examples from ESP8266, but i didn't get it working.

I updated the latest Version of Arduino for ESP32 from github today.

I setup the ESP32 as AccessPoint and startet the WifiServer. I can connect and send a request. The ESP32 sends the response, but then resets the connection, according to curl. So InternetBrowsers and curl are showing me errors.

Can you please have a look what I have done wrong? Or is this a bug?

Error with curl:

curl http://192.168.4.1
<!DOCTYPE HTML>
<html><h1><Hello World!</h1></html>
curl: (56) Recv failure: Connection reset by peer

Here is my Sketch:

#include <WiFi.h>

const char* WIFI_SSID = "ESP-Accesspoint";
const char* WIFI_PASSWORD = "geheimPasswort";
WiFiServer server(80);

void setup(void) {
	Serial.begin(115200);

	WiFi.mode(WIFI_AP);
	WiFi.softAP(WIFI_SSID, WIFI_PASSWORD);

	server.begin();
}

void loop(void) {

	// Check if a client has connected
	WiFiClient client = server.available();
	if (!client) {
		return;
	}

	// Wait until the client sends some data
	Serial.println("New client");
	delay(1);

	// Read the first line of the request
	String request = client.readStringUntil('\r');
	Serial.print("Request: ");
	Serial.println(request);

	client.flush();

	String s = "HTTP/1.1 200 OK\r\n";
	s += "Content-Type: text/html\r\n\r\n";
	s += "<!DOCTYPE HTML>\r\n";
	s += "<html><h1><Hello World!</h1></html>\r\n";

	Serial.println("Response:");
	Serial.println(s);
	client.println(s);
	client.flush();

	delay(1);
	Serial.println("Client disonnected"); //When method ends
}

Thanks in advance and Greetings
Dennis

@me-no-dev
Copy link
Member

You did not specify "Content-Length" in your response so the browser does not have an idea how much data to expect/read, then it throws you the error because it was expecting more data.
TCP Server part is OK, but your HTTP implementation is not :)

@Dennis650
Copy link
Author

Dennis650 commented Jan 7, 2017

Hi me-no-dev,

thanks for your quick response.

In all of the examples of WifiServer with arduino, the Content-Length isn't set in the header. RFC says, this should be set, but not must.
Examples:
https://www.arduino.cc/en/Tutorial/WiFiWebServer
https://learn.sparkfun.com/tutorials/esp8266-thing-development-board-hookup-guide/example-sketch-web-server
http://esp8266-server.de/

So i thought, there must be something in the ESP32 Implementation, what is differnet to other Arduino Cores.

Greetings
Dennis

@me-no-dev
Copy link
Member

I'm glad you looked through RFC but I guess you did not understand my explanation above :)
I tested the basic servers and they perform exactly as expected. I mean the browser is waiting and the sketch is the one that closes the connection after it sends the data.
Now I have written a few web servers and can for sure tell you that that is what is happening with your sketch :) If you construct the HTML response properly, I will not error on you.

String response = "<!DOCTYPE HTML>\r\n<html><h1><Hello World!</h1></html>\r\n";
client.printf("HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: %u\r\n\r\n%s",
    response.length(), response.c_str());
client.flush();
//........

@Dennis650
Copy link
Author

Hi,

i did understand your explanation. I got it working yesterday after your hint with the content-length. Thanks.

My question is, why "content-lentgth" isn't set in all of the Arduino examples i found and postet above. With your explamation, all of these examples must be wrong, because they dont't set content-length. But it seems that they are working on other cores.

Greetings
Dennis

@me-no-dev
Copy link
Member

are you saying that the same example looks different to the browser?

@Dennis650
Copy link
Author

Hi,

i used the code from these examples 1:1 to have a working sketch. I tried 3 oder 4 examples, everytime i got "Connection reset by peer".
I don't have other Arduino Devices, so i can't test it. But in all these examples, the content length is not set in the Sketch. Because there are so many examples in the internet without setting content length, i think that they are working. But on esp32 they don't work 1:1 without setting content-length.

Greetings
Dennis

@andrei-ivanov
Copy link

That's what I know too, that the content length header is optional and just a suggestion for the browsers to show how much they have to download.
Also the client examples don't seem to use this header: https://www.arduino.cc/en/Reference/WiFiClientAvailable
I think Stream.available() is HTTP agnostic.

@me-no-dev
Copy link
Member

ok guys, but please do test on other platforms and do show the different output so we know what and where the problem might be. Right now this is all looking like speculation or something that changed in the browser. Neither TCP nor HTTP are any complicated :)
When you say "optional", yes it is optional, but when not there and when not using chunked output, the browser is keeping the connection open waiting for new data. When you close the connection like you do, the browser is not expecting it. It's expecting more data ;)

Let's get some hard data on different platforms before we keep digging into this

@Dennis650
Copy link
Author

Hi,

i found an unused ESP8266 here. Normaly, i run them with LUA.

I uploaded the sketch I postet in the first comment on this discussion and just had to change

#include <Wifi.h>

to

#include <ESP8266Wifi.h>

Output of curl 192.168.4.1:

<!DOCTYPE HTML>
<html><h1><Hello World!</h1></html>

On ESP32 it is:

<!DOCTYPE HTML>
<html><h1><Hello World!</h1></html>
curl: (56) Recv failure: Connection reset by peer

I am totaly new to Arduino. Can i switch on debug for additional information?

Greetings
Dennis

@tPirath
Copy link

tPirath commented Jan 11, 2017

First of, thank you Me-No-Dev for your awesome contributions!

However, I can confirm issues with the ESP32-Implementation of WiFiServer. All code I wrote for ESP8266 and also the above mentioned examples reset connections after the first communication attempts.

Moreover, I have a simple example that receives bytes from the client and produces an error message:
[E][WiFiClient.cpp:196] available(): 9

Here is the main loop code:
`void loop()
{
// put your main code here, to run repeatedly:
if(!client.connected())
{
//try to connect to a new client
client = server.available();
}
else

if(client.available() > 0)
{
  while(client.available())
  {
    data[ind] = client.read();
    ind++;
  }

client.flush();
//Serial.println(data);
for(int j=0;j < ind; j++)
{
Serial.print(data[j]);
}
ind = 0;
//client.print("OK!");
}
}`
The issue appears not only if data is send by the server but also while reading available bytes from a client.

@schrod
Copy link
Contributor

schrod commented Feb 25, 2017

I've been troubleshooting a similar issue for a while now, and I think I may understand why this is happening. It has to do with how the file handle is stored/copied in WiFiClient. The server creates the WiFiClient object in available(). When it returns it, by value, a copy of the object is made. The original object is then destroyed when available() exits resulting in the destructor being called which calls close on the file handle. The copied client now has a file handle that is invalid.

I can think of several ways to resolve this but in order to maintain api compatibility, the file handle probably needs to be stored in such a way that it isn't closed until all references to it are destroyed. I've started trying to reworking WiFiClient to do this but I still need to flush it out and test it.

@schrod
Copy link
Contributor

schrod commented Feb 26, 2017

I've done some testing with my reworked WiFiClient and it solves the problem I was seeing, but doesn't fix the initial reported problem. Digging into it further, it looks like if you don't include the content-length header you need to wait for the client to timeout and close the connection first. Adding a delay of a couple of seconds after client.println call achieves this and causes it to behave the same way it does on ESP8266. The ESP8266 write function includes a fixed delay of 5 seconds which explains why it works there without the additional delay. Delaying in the write function is going to cause pretty poor performance when sending data larger than a single packet, or on an application that correctly uses the content-length header and really is fundamentally the wrong approach.

I think the correct answer is to use the content-length header and if that isn't possible for some reason then it should be up to the application to add the delay. Unfortunately that seams to break a lot of the existing example code. A possible work around would be to add a delay to the flush function, that will only help if the example uses it though. Currently the flush call doesn't do anything, and even on the ESP8266 it only effects the receive buffer.

@hcs-svn
Copy link

hcs-svn commented Feb 26, 2017

@schrod: I also get most of my web things not working because of the "disconnect" problem (not the content-length problem).
If you are interested, I would test your solution in my scenarios.

@schrod
Copy link
Contributor

schrod commented Feb 26, 2017

@hcs-svn I created a pull request, go ahead and try it and let me know.
#238

@hcs-svn
Copy link

hcs-svn commented Feb 26, 2017

That looks good.
My trivial test sketch works and the huge web fontend of my sketch that I migrated from ESP8266 also works (the first time)

@me-no-dev
Copy link
Member

I'm really puzzled by the fact that the following code works and emits no errors from curl on my end:

WiFiServer server(80);

void setup(){
  //start wifi and connect
  server.begin();
}

void loop() {
    WiFiClient client = server.available();
    if (!client) {
        return;
    }
    while(!client.available()){
        delay(1);
    }
    while(client.available()){
        size_t a = client.available();
        uint8_t buf[a];
        client.readBytes(buf, a);
    }
    client.flush();
    client.print("HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n\r\n<!DOCTYPE HTML><html><body><h2>Hello World</h2></body></html>");
}

@schrod
Copy link
Contributor

schrod commented Mar 1, 2017

The difference seams to be in how the request is being read. If you change your example to use the readStringUntil used in the original example it starts failing as well. The readStringUntil approach doesn't actually read all available data, perhaps the client is interpreting this as the server disconnecting prematurely and therefore generates the error? The ESP8266 flush() routine does do a read of any available data (up to the buffer length).

If I modify the flush function here to do the same, the it starts working again...

void WiFiClient::flush() {
        size_t bytesAvaliable;
        while((bytesAvaliable = available()) > 0){
            uint8_t buf[bytesAvaliable];
            readBytes(buf, bytesAvaliable);
        }
  }

@me-no-dev
Copy link
Member

then maybe the flush thing is connected as well :)
Though we need a better way to flush than this, because we do not know the size of available data and if too much, can bring memory issues with buffer above.
Client close should flush as well.

@schrod
Copy link
Contributor

schrod commented Mar 1, 2017

Yeah, I think it's probably worth doing. That was intended as a quick test, not as a final solution. In addition to the buffer memory issue I don't like the fact that it has the potential, however unlikely, to turn into an endless loop. The ESP8266 code only attempts one read, which would eliminate both of these problems, but doesn't seam particularly robust to me. I think I would prefer something like:

#define WIFI_CLIENT_FLUSH_BUFFER_SIZE	(1024)
#define WIFI_CLIENT_FLUSH_MAX_RETRY  	(10)

void WiFiClient::flush() {
	uint8_t buf[WIFI_CLIENT_FLUSH_BUFFER_SIZE];
	int retryCount = WIFI_CLIENT_FLUSH_MAX_RETRY;

	while((recv(fd(), buf, WIFI_CLIENT_FLUSH_BUFFER_SIZE, MSG_DONTWAIT) > 0) && (retryCount-- > 0)) {
	}
}

@me-no-dev
Copy link
Member

me-no-dev commented Mar 2, 2017

@schrod your code assumes that data left is less than 10K. Also I do not see the possibility of endless loop. Could you please show me that?

Maybe something like that?

#define WIFI_CLIENT_FLUSH_BUFFER_SIZE	(1024)
void WiFiClient::flush() {
  size_t a = available();
  if(!a){
    return;//nothing to flush
  }
  uint8_t * buf = (uint8_t *)malloc(WIFI_CLIENT_FLUSH_BUFFER_SIZE);
  if(!buf){
    return;//memory error
  }
  while(a){
    size_t toRead = min(a, WIFI_CLIENT_FLUSH_BUFFER_SIZE);
    if(recv(fd(), buf, toRead, MSG_DONTWAIT) < 0 && errno != EWOULDBLOCK) {
      log_e("%d", errno);
      stop();
      break;
    } else if(errno == EWOULDBLOCK){
      delay(1);//give some time
    }
    a = available();
  }
  free(buf);
}

@schrod
Copy link
Contributor

schrod commented Mar 2, 2017

Yes, it's limited to 10k, that is more than the ESP8266 implementation, and it avoids the possibility of an endless loop, which could occur if the client continuously sends data. Like I said its unlikely but the possibility exists. In your code, the call to available is unnecessary overhead, recv will either read all that's available if its less that buffer size, or up to buffer size. Knowing what is available ahead of time doesn't change anything. Also EWOULDBLOCK indicates that there is no more data available, given that that is what we're trying to accomplish the delay seams unnecessary and inefficient.

Also, you shouldn't really be using malloc/free in c++, new/delete are more appropriate. Is there an advantage here to allocating the memory from the heap vs the stack (local variable)?

@me-no-dev
Copy link
Member

I see no problem in using malloc. Sure, should skip EWOULDBLOCK and break there. For the rest I'm not that sure. I do not understand arguments like "that is more than the ESP8266 implementation". This is not ESP8266 and in fact it's far more than ESP8266 so it's normal do be able to handle "more" conditions. That infinite loop you are talking about is such a case, where the user should know that the client sends data all of the time and find proper way around it. Should not be handled "half-way" by the flush code and leave the client half-flushed, instead the user should bang head and think of proper implementation.
I prefer heap because it's much more possible to run out of heap than any of the other assumptions. I've seen it enough :) I do not like large buffers on the stack and do my best to stay away from those. 1K here, 1K there and no more left.
As for available... I'm not positive that lwip will give you the total amount of data available every time. It keeps stuff in queues and I don't think it gives all data in some conditions. Overhead is not the issue here. The whole thing if you ask me is overhead ;) I like raw tcp/udp.

@me-no-dev
Copy link
Member

@schrod btw if I'm proven wrong about available, I would gladly remove the redundant calls and replace them with decrement.

@schrod
Copy link
Contributor

schrod commented Mar 2, 2017

I wasn't trying to hold the ESP8266 code up as the gold standard, that is absolutely not the case. It was more an example of a working implementation, I agree this should be better. There is nothing wrong with your implementation, I just feel it could be more efficient. Like all design decisions there are trade offs to be made, you've chosen to error on the side of being absolutely through while I tend to lean more towards efficiency. We can agree to disagree. Since it's your project it's ultimately up to you anyway :)

@me-no-dev
Copy link
Member

If we talk about efficiency, which I'm much more about that you probably know, this lib should be written on top of raw TCP, like ESP8266 libs are and like my amazing AsyncTCP libs are (and like mdns currently is on ESP32), but I was told implement the the network based on POSIX, and if you go through the code, you will forget about efficiency immediately ;) so I don't bother to worry about that.
I can check after reading if all data is read and save the call to available then, which in 99% of the cases will be the only read that will be necessary to be made and the code will be efficient 99% of the time.

@me-no-dev
Copy link
Member

@schrod I did some tests with simple request to a local web server and here is what I got.

ESP8266 request takes 3-6ms to complete (connect, request, read, close)
ESP32 request takes 63-69ms to complete

Let's talk about optimizations now :) The whole stack need to move to RAW TCP or we will move at snail speed.

Lzw655 pushed a commit to Lzw655/arduino-esp32 that referenced this issue Oct 12, 2023
* Rework the lib-builder for ESP-IDF v5.1

* Update package json with tolls matching the ESP-IDF version

* fix: rainmaker examples crashing on s3 due to low stack memory. (espressif#106) (espressif#107)

* Update scripts with the latest requirements

* Update configs + SR Support

* Add esp-elf-gdp to the list of packages

* Fix RainMaker builds and new sr models path

* Temporary force arduino branch for CI to work

* fix target branch

* Delete esp-dl component manifest for requiring IDF 4.4.x

* Temporary changes to allow Cron CI to run

* Support builds based on ESP-IDF tag

* Push to esp32-arduino-libs

* Update repository_dispatch.sh

* Rework scripts to allow build when either dst needs it

* Github complains when pushing to the libs repo

* Authenticate to the libs repo

* Attempt at splitting SDK from Arduino

* Archive only the result and reorder deploy commands

* Update cron.sh

* Fix script and zip paths

* Fix download URL and json merger

* Change sdk folder structure and fix json generation

* Switch output folder from sdk to esp32-arduino-libs

* arduino_tinyusb: compile support for DFU mode (espressif#116)

* Update PlatformIO build script templates (espressif#118)

Adds support for new package with precompiled SDK libraries

* Autogenerate PlatformIO manifest file for prebuilt SDK libs (espressif#119)

* Autogenerate PlatformIO manifest file for prebuilt SDK libs

- Add a special Python script that generates "package.json" with IDF revision from the "version.txt" according to SemVer

* Tidy up

* Refactor manifest generator

Now IDF version and commit hash are passed directly from Git client instead of reading from a pregenerated "version.txt" file

* Move IDF definitions to be available with any build

* Use more components from registry and add mp3 decoder

* esp-sr component requires clearing before each build

* revert ESP_SR from component manager

* Build ESP_SR only for ESP32-S3 for now

* [TinyUSB] Update esp32sx dcd and add dwc2 option

* Workaround for recent change in ESP-Insights

* Add initial support for ESP32-C6

* Run build tests on ESP32-C6

* Remove -mlongcalls for non-xtensa chips

* Temp fix for ESP-Insights on C6

* Add support for ESP32H2

* Added tflite-micro component (espressif#128)

* Update build badge in README.md

* Added tflite-micro component

---------

Co-authored-by: Me No Dev <[email protected]>

* Make cron rebuild the libs if they need to be pushed to Arduino

For when we change something in the lib-builder, but no new updates are available in ESP-IDF

* Update build actions

* Fix permissions

* Do not build for obsolete Flash modes

* Try separate detect for cron builds

* Add permissions to github api

* Try more basic commit detection

* another try to pass vars and get commit

* Update push.yml

* Update config.sh

* Enable builds again

* Update build.sh

* Combine the artifacts from all jobs

* fix and test deploy check

* Update push.yml

* Disable Memprot to allow loading external elfs

* Fix archive name

* Disable coredump to flash

* Enable SPI ETH KSZ8851SNL

* Add temporary support for Arduino SPI Ethernet

* Add a temporary fix for relative include in BT lib

* Enable Classic BT HID Host and Device for ESP32

* Revert "Enable Classic BT HID Host and Device for ESP32"

This reverts commit aa0040ad271d00ac507fd2b478ee143b6c118615.

* C6 was added to ESP-SR

* Update Ethernet and remove SR workaround

* Pin RainMaker version

* Update target branch

* Add back cron.sh

---------

Co-authored-by: Sanket Wadekar <[email protected]>
Co-authored-by: Luca Burelli <[email protected]>
Co-authored-by: Valerii Koval <[email protected]>
blue-2357 pushed a commit to blue-2357/arduino-esp32 that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Test needed Issue needs testing
Projects
None yet
Development

No branches or pull requests

6 participants