Telnet server: alternative to ESPAsyncTCP

[Niek]

This is a rewrite of the telnet module, it's no longer using ESPAsyncTCP but the regular WiFiServer/WiFiClient libraries. Tested it quite a bit, but more testing and feedback is appreciated.

[mcspr]

I will try this later today, but I think there are two issues:

• the way logging works, we can't issue any writes for the wificlient. For example, try async-mqtt-client active or connect to the webui
• is data[2048] part really needed? stack on esp is pretty limited, unless we are in SYS context (ticker, async callbacks etc.). ref. CONT_STACKSIZE, which is 4k by default

edit: and just to be clear, logging part is easily fixed externally, if debugSend uses internal buffering and flushes inside loop()

• btw, are there any issues with nodelay? i remember enabling it in the async client, and that resulted in a pretty big problem. Packets were not buffered, so we ended up flooding the net 1 character at the time

[Misiu]

@Niek @mcspr why this is needed? I'm not an expert and I don't try to disagree with the changes, but I want to understand why this is needed and what benefits it has.
I always used ESPurna as a "best practice" reference project, I learned A LOT from it.
ESPAsyncTCP was used because of async support, and I always thought that we should write code in an async way to avoid the need to relay on loop.
I'll be grateful for the explanation :)

[Niek]

@Misiu ESPAsyncTCP has not been updated in over a year. It also doesn't play well with Arduino core >= 2.5 especially on the SSL side of things.

Someone made a fork to work with BearSSL (https://github.com/Adam5Wu/ESPAsyncTCP) but it requires a bunch other forked stuff like the Arduino core and it hasn't been updated in a year either.

@mcspr can you elaborate on the first point? I don't seem to run any issues with logging (but in my tests I'm not using any ESPAsyncTCP modules).

The 2048 char array is way overkill indeed for incoming data, maybe we can limit it to 512 or something similar.

[mcspr]

Practical reason is to allow to build without it (using pubsubclient for mqtt, for example) and possibly use Core's server + BearSSL. Not yet though, and I kind of missed removal of the existing one. @Niek We might want to guard new telnet server the same way as #1751 , eg TELNET_SERVER == ...

not to get ot... 🉑

Async stuff is kind of cool, but sometimes we hit surprising things that were either already handled by WiFiClient abstraction¹ or completely avoided by it. One benefit, however, since we implement it :) - we can fix some quicks between Core versions, while this still applies. We kind of have two ways now - create more robust approach for clients / servers than current one e.g. manually supply all callbacks and streamline buffering (AsyncPrinter / AsyncBufferred... ?).
edit: If we were to try to add bearssl, one problem is that BearSSL specifically employs yields (search ref:) inside some heavy calculations, so they need to be offloaded to loop somehow :/ Might be wrong though.
ESPAsyncWebServer & AsyncMqttClient would need patches too, but that part is seemingly easier. Certs would be easier in the feature, since would be able to add pre-parsed static structs in the source.

Or just use WiFiClient stuff and try to not do all network activity all at once.

Or, maybe try to patch Core WiFiClient::write timeouts to make them behave the same way AsyncClient::write does - just try to write something to the network layer and do nothing otherwise (if there's already too much stuff). I think I saw some issues at the esp8266/Arduino, but not much traction there. Wonder how hard it is.

¹ ClientContext aka Core's ESPAsyncTCP

[Niek]

I have updated the PR, supporting 2 telnet server implementations now. I tested a lot and it seems to work fine on my setup - please review.

[mcspr]

Regarding client timeout - it does not disconnect and only indication of any problems is write() returning 0. You can try running telnet client on a phone, for example, and disconnect WiFi connection. After running info in serial a couple of times, buffer is finally filled and every consecutive write() will timeout. Managing that will require some "write success" counter, but I'm not yet sure where it should go.

[Niek]

The docs mention using flush(timeout) in order to check if the connection has timed out: https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/client-class.html?highlight=wificlient#flush-and-stop

I haven't been able to get it to work though (always seem to return true).

[mcspr]

IDK why travis does not pick up the latest commit when restarting the PR build.
Since the basics work, this can be added and the timeout thing worked out after that?

[Niek]

Seems like a Travis CI issue. Do you have rights to restart jobs?

In any case, yes this can be merged, we'll figure out the timeout later.

[Niek]

I pushed a last fix now (allocate only required buffer, not 512 chars] - travis also succeeded this time.

[mcspr]

And following up on the #1799 (comment) (since we don't track ESPAsyncTCP issue yet and this is not really worth mentioning in the library issue tracker)... I have a sort-of working bearssl port based on the work from the https://github.com/Adam5Wu/ESPAsyncTCP
Diff would be unreadable though... And the only thing tested was https://canihazip/s with SyncClient example

Turns out, insecure option is a Core-specific thing. But, it can be safely used just by adding 2 declarations so they get linked from the Core's WiFiClient helpers
Thunk / Non-thunk part was not really a problem, and original code works straight away. If needed, thunk macro can remap required functions and builds with WPS will still work fine.
MFLN works too! Because it is as easy as adding a different buffer size for the bearssl API call. Quick solution is to have AsyncClient::setSSLParams(...) method to supply new BearSSL-specific options.
Certificates from SPIFFS are kind of a tricky thing to have, but basic X509List arg should work as it is right now (untested)
Duplication is not really as severe as expected. Maybe some things can be safely moved to be reusable.

Tracking ESPAsyncTCP repo, there are some problems with the classes that I had mentioned (AsyncPrinter and Buffer) regarding lwip2 buffer sizes.

[mhightower83]

Tracking ESPAsyncTCP repo, there are some problems with the classes that I had mentioned (AsyncPrinter and Buffer) regarding lwip2 buffer sizes.

@mcspr - could you point me to more information on this. I recently posted a PR at me-no-dev/ESPAsyncTCP, I fixed numerous problems in TCPAsyncTCP and SyncClient. Many of the issues in SyncClient were generic in nature and were applied to the other modules; however, there wasn't any test code to verify the results.

[mcspr]

My bad! I read the me-no-dev/ESPAsyncTCP#115 patch briefly a couple of days ago and made a mental note about those classes for some reason. I do see now that those changes are for the default values, so disregard my previous comment.