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

on 2.1.0 server.setNoDelay(true); generate exception(2) at start #1695

Closed
luc-github opened this Issue Feb 28, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@luc-github
Copy link
Contributor

luc-github commented Feb 28, 2016

On 2.1.0 my project crash at start when wasn't using 2.0.0
I narrow down using the sample sketch WiFiTelnetToSerial.ino (https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/examples/WiFiTelnetToSerial/WiFiTelnetToSerial.ino), which generate exception(2) at start.
commenting the line :

server.setNoDelay(true);

solve the issue.

This do not happen on 2.0.0.

2.1.0 sample sketch is compiled under windows 10 / IDE 1.6.5 and using nodemcu 1.0

@miky2k

This comment has been minimized.

Copy link

miky2k commented Mar 4, 2016

Confirmed for me.
there is possibility to back-port correction to 2.1.0 without to wait next version?

@tablatronix

This comment has been minimized.

Copy link
Contributor

tablatronix commented Mar 5, 2016

yup same here latest master, WiFiServer.setNoDelay(true) causes wdt resets.

#include <ESP8266WiFi.h>
#include <Arduino.h>

WiFiServer TelnetServer(23);
WiFiClient Telnet;

void setup(){
  Serial.begin(115200);
  Serial.setDebugOutput(true);
  TelnetServer.begin();
//  TelnetServer.setNoDelay(true);
}

void loop(){
  if (TelnetServer.hasClient()){
    if (!Telnet || !Telnet.connected()){
      if(Telnet) Telnet.stop();
      Telnet = TelnetServer.available();
    } else {
      TelnetServer.available().stop();
    }
  }
  if (Telnet && Telnet.connected() && Telnet.available()){
    while(Telnet.available())
      Serial.write(Telnet.read());
  }
}
@igrr

This comment has been minimized.

Copy link
Member

igrr commented Mar 10, 2016

After a short investigation, setNoDelay on a WiFiServer instance had always resulted in undefined behaviour. But before we upgraded to 1.5.1 it didn't cause run-time issues (apparently).

For those who are interested, here is what's happening. We call tcp_nagle_disable on a server tcp_pcb structure. This macro expands into ((pcb)->flags |= TF_NODELAY). Flags is a member of tcp_pcb structure. The trouble is that tcp_listen function, which we call to start listening on a port, replaces tcp_pcb structure with a smaller tcp_pcb_listen structure.
https://github.com/kadamski/esp-lwip/blob/esp8266-1.4.1/src/core/tcp.c#L519-L570
https://github.com/kadamski/esp-lwip/blob/esp8266-1.4.1/src/include/lwip/tcp.h#L284-L294
Now tcp_pcb_listen structure doesn't have flags member! When we do tcp_nagle_disable, we write out of bounds of this structure.

So it seems that we have to remove setNoDelay function from WiFiServer class and update examples accordingly.

@luc-github

This comment has been minimized.

Copy link
Contributor Author

luc-github commented Mar 10, 2016

Thanks a lot @igrr - very clear
Additional question: if the flags TF_NODELAY is never used, should it be removed also ?
or do we expect in a future that tcp_pcb_listen function to support this flag ?

@igrr

This comment has been minimized.

Copy link
Member

igrr commented Mar 10, 2016

We will save no_delay flag in WiFiServer, and it will set this flag on each new client connection automatically. I'll post a commit soon. (i.e. will not remove setNoDelay function from WiFiServer).

@luc-github

This comment has been minimized.

Copy link
Contributor Author

luc-github commented Mar 10, 2016

Excellent - thanks a lot - you are the best as usual ^_^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.