Ethernet connection crashing

[AlbertoLopSie]

Hi Lennart!

I'm having troubles trying to start a Telnet Server over a wired Ethernet connection (W5100 shield). Already checked #22 and #27 but I can't find my problem related to them.

I'm passing false as second argument to telnet.begin() and it's avoiding the WiFi check as expected, but later the code throws:

assert failed: tcpip_send_msg_wait_sem IDF/components/lwip/lwip/src/api/tcpip.c:455 (Invalid mbox)

Backtrace: 0x40084109:0x3ffb1e90 0x4008e535:0x3ffb1eb0 0x40094161:0x3ffb1ed0 0x4011bb55:0x3ffb2000 0x4012c6b5:0x3ffb2030 0x4012c715:0x3ffb2050 0x4011b2f1:0x3ffb20a0 0x400e9576:0x3ffb20c0 0x400e95f4:0x3ffb2110 0x400f9917:0x3ffb2130 0x400e02fb:0x3ffb2170 0x400de137:0x3ffb21a0 0x400dee07:0x3ffb21c0 0x400dfeb2:0x3ffb2240 0x400e0f54:0x3ffb2270 0x40101751:0x3ffb2290

  #0  0x40084109:0x3ffb1e90 in panic_abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/panic.c:408
  #1  0x4008e535:0x3ffb1eb0 in esp_system_abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/esp_system.c:137
  #2  0x40094161:0x3ffb1ed0 in __assert_func at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/newlib/assert.c:85
  #3  0x4011bb55:0x3ffb2000 in tcpip_send_msg_wait_sem at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/lwip/lwip/src/api/tcpip.c:455 (discriminator 1)
  #4  0x4012c6b5:0x3ffb2030 in netconn_apimsg at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/lwip/lwip/src/api/api_lib.c:136
  #5  0x4012c715:0x3ffb2050 in netconn_new_with_proto_and_callback at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/lwip/lwip/src/api/api_lib.c:166
  #6  0x4011b2f1:0x3ffb20a0 in lwip_socket at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/lwip/lwip/src/api/sockets.c:1774
  #7  0x400e9576:0x3ffb20c0 in WiFiServer::begin(unsigned short, int) at /Users/alberto/.platformio/packages/framework-arduinoespressif32/tools/sdk/esp32/include/lwip/lwip/src/include/lwip/sockets.h:656
      (inlined by) WiFiServer::begin(unsigned short, int) at /Users/alberto/.platformio/packages/framework-arduinoespressif32/libraries/WiFi/src/WiFiServer.cpp:80
  #8  0x400e95f4:0x3ffb2110 in WiFiServer::begin(unsigned short) at /Users/alberto/.platformio/packages/framework-arduinoespressif32/libraries/WiFi/src/WiFiServer.cpp:70
  #9  0x400f9917:0x3ffb2130 in ESPTelnetBase::begin(unsigned short, bool) at .pio/libdeps/esp32dev/ESP Telnet/src/ESPTelnetBase.cpp:21
  #10 0x400e02fb:0x3ffb2170 in vserial_listen(int, int (*)(char*), bool) at src/VSerial.cpp:256
  #11 0x400de137:0x3ffb21a0 in InitVTerm() at src/Trh32_Term.cpp:330
  #12 0x400dee07:0x3ffb21c0 in Trh32_Term_tick() at src/Trh32_Term.cpp:394
  #13 0x400dfeb2:0x3ffb2240 in profile(char const*, unsigned long, void (*)()) at src/UTILS.cpp:534
  #14 0x400e0f54:0x3ffb2270 in loop() at src/main.cpp:128
  #15 0x40101751:0x3ffb2290 in loopTask(void*) at /Users/alberto/.platformio/packages/framework-arduinoespressif32/cores/esp32/main.cpp:50

ELF file SHA256: 3ecc7d947ae65388

Rebooting...

The stack trace shows:

Using the debugger I could follow the offending line to the WiFiServer::begin method:

void WiFiServer::begin(uint16_t port, int enable){
  if(_listening)
    return;
  if(port){
      _port = port;
  }
  struct sockaddr_in server;
  sockfd = socket(AF_INET , SOCK_STREAM, 0);                        // <<<=== Panic is thrown HERE
  if (sockfd < 0)
    return;
  setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &enable, sizeof(int));
  server.sin_family = AF_INET;
  server.sin_addr.s_addr = _addr;
  server.sin_port = htons(_port);
  if(bind(sockfd, (struct sockaddr *)&server, sizeof(server)) < 0)
    return;
  if(listen(sockfd , _max_clients) < 0)
    return;
  fcntl(sockfd, F_SETFL, O_NONBLOCK);
  _listening = true;
  _noDelay = false;
  _accepted_sockfd = -1;

The full code is huge but the library initialization portion is something like:

      static byte_8  EthMAC[6];

     // MAC init code here...

      Ethernet.init(ETH_CS);            // ETH_CS: chip select
      Ethernet.begin(EthMAC);

     telnet.onConnect(onTelnetConnect);
     telnet.setLineMode(true);

     telnet.begin(port, false);
     ...

Due to some related comment read somewhere I also tried pre-initializing Esp Wifi code with the following code with no luck:

const wifi_init_config_t wconfig = WIFI_INIT_CONFIG_DEFAULT();
esp_wifi_init(&wconfig);  

The telnet server is working fine on Wifi either in AP or STA modes.

Any hint would be greatly appreciated!

Alberto

[AlbertoLopSie]

Comment...

I've got the Telnet server working making some dirty changes in ESPTelnetBase.h:

...
#include <WiFi.h>
#include <Ethernet.h>
...

using TCPClient = EthernetClient;      // using TCPClient = WiFiClient;
using TCPServer = EthernetServer;      // using TCPServer = WiFiServer;

and tweaking some WiFiServer/Client-related code in ESPTelnetBase.cpp

I still need to make the code to be able to switch between WiFi and Ethernet on runtime but I don't think it would possible to do it without rewriting the whole ESPTelnet class.

Would you consider a PR if I can produce a working code?

[LennartHennigs]

Hey Alberto, Thanks for reaching out. As I don't have the Arduino Shield I a hard time replicating your problem. To answer your question: Yes, in general I don't mind reviewing and accepting a PR changes to the lib.

I have a few principles for the libs I share:

• I try not to introduce breaking changes to existing users
• changes relevant for a subset of users should not lower the overall experience
• After the changes made, the lib will still need to work for users with different setups (e.g. WiFi vs. Ethernet)

If you feel, that your ideas are in line with there, feel free to tinker with it. If not, let's discuss 😀

Also, feel free to describe your solution before changing code if that is faster or easier to do...

Cheers Lennart

[AlbertoLopSie]

Thanks for considering it Lennart,

My first attempt was to replace in ESPTelnetBase.h, WiFiClient and WSiFiServer for its base classes Client and Server:

using TCPClient = Client;
using TCPServer = Server;

but, they are abstract classes and cannot be instantiated statically as you do in:

...
 protected:
  TCPServer server = TCPServer(23);  // must be initalized here
  TCPClient client;
...

Also, some of the methods that need rewrite are private, so overriding is not possible. For example:

void ESPTelnetBase::connectClient(TCPClient c, bool triggerEvent) {
  client = c;
  ip = client.remoteIP().toString();
  client.setNoDelay(true);                               // ==> client.setNoDelay() does not exist in EthernetClient
  if (triggerEvent && on_connect != NULL) on_connect(ip);
  emptyClientStream();
  connected = true;
}

The other method I've found that need rewrite is:

void ESPTelnetBase::processClientConnection() {
  // if (!server.hasClient()) return;        // ===> server.hasClient() does not exist in EthernetServer

  TCPClient newClient = server.accept();

   if (!newClient) return;                   // The former check needs to be rewritten this way for an EthernetServer
                                             // Don't really know if works on WiFiServer

  if (!connected) {
    connectClient(newClient);
  } else {
    handleExistingConnection(newClient);
  }
}

With these changes ESPTelnet works fine over the Ethernet@2.0.2 library in my W5100 shield. Of course these changes broke the WiFi functionality, but I was only trying to probe the concept.

I guess that supporting both libraries would require separate constructors or begin methods, to instruct ESPTelnet library to use one or another backend.

I need this code working ASAP and therefore I'm going to fork and modify it for my project, but as this baby has been grown by you, any suggestion would be greatly appreciated. Of course I'll propose you as PR the results.

Thanks!

[LennartHennigs]

Hey Alberto, that when inheritance gets back to haunt me. :-)

I made the base class to share the common methods for either the stream or non-stream version. Adding additional classes and inheritance to implement WiFi or Ethernet sounds complicated.

I think, I‘d then would want to add the code for both wifi and ethernet handling inside the BaseClass and determine the „mode“ via begin() .

This would mean I need to change begin() and all other places where I do WiFi stuff. But, then I‘d also check whether the Ethernet class works the same across Arduino, ESP8266 and ESP32. As the WiFi class has slight differences, this might very well be the case for Ethernet as well…

This sounds like the easiest, least complicated way to implement this. What do you think?

Cheers

[AlbertoLopSie]

Hi Lennart!

Hey Alberto, that when inheritance gets back to haunt me. :-) Jajaja!!! Life is just a matter of waiting for the unavoidable!!!

I think, I‘d then would want to add the code for both wifi and ethernet handling inside the BaseClass and determine the „mode“ via begin() . Agreed

This would mean I need to change begin() and all other places where I do WiFi stuff. But, then I‘d also check whether the Ethernet class works the same across Arduino, ESP8266 and ESP32. As the WiFi class has slight differences, this might very well be the case for Ethernet as well…

This sounds like the easiest, least complicated way to implement this. What do you think?

Agreed. Personally, I would't bother too much avoid Arduino, the library is named ESPTelnet, isn't it ;-) ?

My current work is on a ESP32 and although I do have a couple of ESP8266 somewhere, preparing a test bed would take me a time that I don't have right now. I'd rather let it for later.

I'm not completely aware of some of your thoughts when writing the library so, my suggestions may be invalid. Please let me know if that happens.

On my previous message I pinned the 2 functions that (at least) need to be changed. I agree with you that the matter of switching between WiFi and Ethernet servers should be made at constructor or begin()

I guess that doing it at ESPTelnetBase constructor would break some of the code in the derived class, but to be able to do it at begin() you should handle the instantiation of the TCPServer server and TCPClient client protected properties different. They cannot be static except if you are willing to keep 2 set of static props, WiFiServer/WiFiClient and EthernetServer/EthernetClient.

Tho other way could be dynamic instantiating through new and keeping its pointers as Server * / Client *. I don't really like dynamic memory on embedded systems but being a CPP environment it is very hard to avoid. On the other hand, this dynamic allocation is done just once, on begin()

I'll start exploring some of these paths today and what I find.

Cheers!

[AlbertoLopSie]

Hi Lennart,

I've created a new PR with the suggested changes fixing the Ethernet support.

Please let me know what do you think about!

Cheers!

[LennartHennigs]

Hey, wow! Thank you. Will review the PR next week, as i am on holidays and without a computer. Only brought a tablet :-) Thanks again!

[LennartHennigs]

Hey @AlbertoLopSie, please check if the code now still works on your end using this branch and the WS100:

https://github.com/LennartHennigs/ESPTelnet/tree/pr-ethernet-static

I will do the same with the WiFi classes for the ESP32 and the ESP8266

[AlbertoLopSie]

Yes!!!

It works like a charm!! (or... at least as good as it was working with my PR)

Moving the TCPClient class to a new file was the right move, I didn't do it by lazyness ;-)

Good to be able to help!!

Cheers!!

[LennartHennigs]

The code does not work with WiFi. I’ll take a stab at it in the next couple of days.

[AlbertoLopSie]

The code does not work with WiFi. I’ll take a stab at it in the next couple of days.

Mmm... I had it tested on Ethernet, WiFi STA and WiFi AP.

What problem did you get?

[LennartHennigs]

Hey, The ESP8266 D1 Mini does does not allow connections.

Trying 192.168.0.66...
telnet: connect to address 192.168.0.66: Connection refused
telnet: Unable to connect to remote host

...but it works fine on an ESP32 M5Stack Core 2. Haven't figured out why yet.

[AlbertoLopSie]

Sorry I can’t help… Don’t have any 8266 at hand

[LennartHennigs]

:-) let’s see what I can find. I also ordered a W5100 addon board. So I can give that a spin, too.