ESP32: Port net module to netconn API. #2047

devsaurus · 2017-07-18T19:08:01Z

Fixes #1970, #2014, #2044.

This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/en/*.

This PR ports the net module from raw API to netconn API in order to safeguard the inter-process communication between the NodeMCU task and lwip's tcpip_thread tasks. Refer to the Multithreading section in http://git.savannah.nongnu.org/cgit/lwip.git/tree/doc/rawapi.txt.

While this generated a high number of changes in the source file, the overall code structure almost completely remained the same:

calls to tcp_* and udp_* functions were replaced with their netconn_ counterparts
events are instantly routed through the task interface to their l*_cb functions
high-level DNS functionality untouched

The netconn API provides only one single callback function for event handling to the application code in contrast to the dedicated callbacks of tcp_accept, tcp_send etc. This generated the main structural difference since all events are fed into this single callback function first and have to be de-muxed to the specific handlers (which already existed and were re-used).

I tried to consider @djphoenix's results regarding sockets in TIME_WAIT state from #1838 but could only implement one of the two aspects: LWIP_SO_REUSE is enabled by default in sdkconfig.defaults but there's currently no clean way to decrease TCP_MSL in the sdk source tree. There's pending PR espressif/esp-idf#783 to expose TCP_MSL in menuconfig - once it's done we can set it to 5000 in sdkconfig.defaults as well.
Thus expect the default timeout of ~2 minutes until a closed server socket is finally deleted and heap is freed. Manually modify sdk/esp32-esp-idf/components/lwip/include/lwip/lwip/priv/tcp_priv.h if you need a workaround.

jmattsson

Overall looks great, just a handful of comments and questions. Looks like using netconn simplifies things in this module, which is a nice change. Dare I ask about the whole PLUS / MINUS thing of the events? Or am I better off not knowing? :)

jmattsson · 2017-07-19T02:22:21Z

components/modules/net.c

 {
-  lnet_event *ev = (lnet_event *)malloc (sizeof (lnet_event) + p->len);
+  lnet_event *ev = (lnet_event *)malloc (sizeof (lnet_event) /*+ p->len*/);


Nuke the + p->len completely? :)

jmattsson · 2017-07-19T02:23:04Z

components/modules/net.c


  switch (type) {
    case TYPE_TCP_CLIENT:
      ud->client.cb_connect_ref = LUA_NOREF;
      ud->client.cb_reconnect_ref = LUA_NOREF;
      ud->client.cb_disconnect_ref = LUA_NOREF;
      ud->client.hold = 0;
+      ud->client.num_held = 0;
+      ud->client.connecting = false;


Should ud->client.closing also be initialised here, or is that only relevant for UDP?

It's initialized for TCP by fall-through to UDP case ;)

Whoops, missed the fall-through, sorry!

jmattsson · 2017-07-19T02:25:02Z

components/modules/net.c

+{
+  SYS_ARCH_DECL_PROTECT(lev);
+
+  if (!netconn) return;


I'd suggest moving the early return up above the SYS_ARCH_DECL_PROTECT - at first reading it had me worried about leaving something locked.

jmattsson · 2017-07-19T02:26:42Z

components/modules/net.c

+        // connection established, trigger Lua callback
+        ud->client.connecting = false;
+        post_net_connected(ud);
+


This line left intentionally blank? :)

Yep, to take a deep breath 😁

jmattsson · 2017-07-19T02:30:49Z

components/modules/net.c

+      // new connection available from netconn_listen()
+      if (ud->netconn &&
+          ud->self_ref != LUA_NOREF &&
+          ud->server.cb_accept_ref != LUA_NOREF) {


What happens if we haven't got the self_ref or cb_accept_ref? Do we leak connections? Should we do accept+immediate-close if we can't accept for real?

Hm, good point, maybe. Never checked that. Is it possible to trigger this corner case at all? Not with net.listen(), it enforces a callback. Probably with a race when a client connects while the sever is being closed...

I can't accept right here, would need to feed this event down the complete task pipeline and handle in laccept_cb(). I observed nasty things happening when netconn_ functions are called from within lwip's thread. Symptoms are

I (25309) wifi: bcn_timout,ap_probe_send_start
I (27819) wifi: ap_probe_send over, resett wifi status to disassoc

I'll remove the check for self_ref since it's not used at all in laccept_cb. It would open up a race condition in net_listen() when netconn_listen() was executed before the ref is actually set.

The check for cb_accept_ref fails when the server is in the process of being closed / shut down. Not accepting is ok in this case since the pending client request was stored in the server's netconn accept-mailbox and is going to be tcp_abort'ed & freed when the server netconn is finally closed and deleted.

jmattsson · 2017-07-19T02:36:56Z

components/modules/net.c

    if (ud->client.cb_sent_ref != LUA_NOREF) {
      lua_rawgeti(L, LUA_REGISTRYINDEX, ud->client.cb_sent_ref);
      lua_rawgeti(L, LUA_REGISTRYINDEX, ud->self_ref);
      lua_call(L, 1, 0);
    }
  } else if (ud->type == TYPE_TCP_CLIENT) {
-    err = tcp_write(ud->tcp_pcb, data, datalen, TCP_WRITE_FLAG_COPY);
+    size_t bytes_written;
+    err = netconn_write_partly(ud->netconn, data, datalen, NETCONN_COPY, &bytes_written);


Presumably bytes_written should be checked and we should do something if we can't send it all?

I need to check what happens in this case.

jmattsson · 2017-07-19T02:38:25Z

components/modules/net.c

+  if (ud->netconn && ud->type == TYPE_TCP_CLIENT && !ud->client.connecting) {
+    ud->client.connecting = true;
+    netconn_connect(ud->netconn, addr, ud->port);
+  } else if (!ud->netconn && ud->client.wait_dns == 0) {


I think you really want ud->netconn, not !ud->netconn here!

That's existing logic, I just replaced !ud->pcb with !ud->netconn.
Not sure which corner case is covered here: no pcb/netconn and no pending dns request.

Ah, I misread that line! I somehow saw it as !ud->netconn && ud->netconn.wait_dns and thus took the first part to be a null pointer guard.

jmattsson · 2017-07-19T02:44:11Z

components/modules/net.c

+
+  if (p) {
+    netbuf_delete(p);
+


Does payload need to be free'd explicitly, or is it linked to the netbuf?

The latter. netbuf_data() just provides the data pointer and length of the embedded pbuf, while netbuf_delete() frees both the netbuf and the pbuf.

jmattsson · 2017-07-19T02:45:28Z

components/modules/net.c

+
+    case NETCONN_EVT_ERROR:
+      post_net_err(ud, netconn_err(ud->netconn));
+      ud->client.closing = true;


I see this flag being set, but I don't see us acting on it to force a close. Is that not needed?

netconn triggers the callback multiple times in case of an error and when the connection goes down in general. The flag is used some lines above to prevent us from further posting to the Lua task. Lua cbs would operate on outdated/closed/freed userdata otherwise.

jmattsson · 2017-07-19T02:47:00Z

components/modules/net.c

+  if (!netconn) return;
+
+  SYS_ARCH_PROTECT(lev);
+  if (netconn->socket < 0) {


Nothing initialises netconn->socket to -1 from what I can see? Or does the netconn layer do that implicitly?

Yes, that's even a "feature" of netconn to support the socket layer.
I borrowed this logic to record early receive events from there.

devsaurus · 2017-07-19T18:46:54Z

Thanks for the review, Johny. I'll follow up on the pending topics soon.

accept event triggered on incomplete userdata
stress/corner tests against server sockets
use closing flag also for server sockets to block any callback activity upon error or intentional close
what if netconn_write_partly() actually writes partly?

Dare I ask about the whole PLUS / MINUS thing of the events?

The PLUS/MINUS event mimic appears to be specifically tailored to the needs of the socket API. netconn layer makes assumptions about the upper layer's implementation with select() on file descriptors etc. and delivers events accordingly.

SENDPLUS / SENDMINUS are signaled to toggle a binary flag in socket layer, upon specific events from raw API.
RCVPLUS / RCVMINUS shall increment / decrement an event counter in socket upon other specific events.

Raw events from tcp_error(), tcp_sent(), tcp_accept() etc. are muxed into this scheme and one needs to understand the socket layer implementation to make sense out of netconn's events.

Or am I better off not knowing? :)

Err, this was the worst part in the porting job, really 😃

and set it consistently also for intended netconn close to prevent any cb activity

devsaurus · 2017-07-21T21:02:47Z

I've pushed the last update to your review feedback. net.send() will throw an error now if payload is not completely queued to lwip since this situation can't be handled cleanly with the current design. We would need to ref the data strings in a list and unref from the complementing sent callback.
This is overkill IMO and is not needed if the application sticks to the guideline to call the next net.send() from the callback and not call send multiple times in a row.
However, if a valid use case proves me wrong then net_send() would need to ref the string objects until the payload is consumed.

AadiMehta · 2017-07-22T18:17:02Z

Can someone please point me towards the documentation of this newly committed net module for esp32. Thanks!

jmattsson · 2017-07-23T03:28:30Z

It's the same Lua interface as before, just the underlying implementation that changed. Just keep on using net :)

marcelstoer · 2017-07-25T18:54:22Z

Except there's no net.md in https://github.com/nodemcu/nodemcu-firmware/blob/dev-esp32/mkdocs.yml ... because it doesn't exist yet.

devsaurus · 2017-07-25T19:39:44Z

Except there's no net.md

See #2053.

marcelstoer · 2017-07-25T20:27:50Z

@AadiMehta thanks to @devsaurus there's now https://nodemcu.readthedocs.io/en/dev-esp32/en/modules/net/

AadiMehta · 2017-07-26T05:45:16Z

@devsaurus @marcelstoer Thanks a lot! I am in the process of migrating nodemcu lua code from ESP8266 to ESP32, and this helps a lot to debug issues. Also eagerly waiting for other modules support in ESP32!
Thanks a lot 👍

Port net module to netconn API.

070973e

devsaurus requested a review from jmattsson July 18, 2017 19:08

jmattsson requested changes Jul 19, 2017

View reviewed changes

devsaurus added 3 commits July 20, 2017 21:04

first batch of review feedback

abbd384

promote closing flag to serve all socket types

d26c8ea

and set it consistently also for intended netconn close to prevent any cb activity

signal an error in case TCP send data was only written partly

f501b84

jmattsson approved these changes Jul 22, 2017

View reviewed changes

jmattsson merged commit 977dab2 into nodemcu:dev-esp32 Jul 22, 2017

devsaurus deleted the esp32-netconn branch July 22, 2017 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESP32: Port net module to netconn API. #2047

ESP32: Port net module to netconn API. #2047

devsaurus commented Jul 18, 2017

jmattsson left a comment

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 20, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 20, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017 •

edited

Loading

jmattsson Jul 19, 2017

devsaurus Jul 19, 2017

devsaurus commented Jul 19, 2017 •

edited

Loading

devsaurus commented Jul 21, 2017

AadiMehta commented Jul 22, 2017

jmattsson commented Jul 23, 2017

marcelstoer commented Jul 25, 2017 •

edited

Loading

devsaurus commented Jul 25, 2017 •

edited by marcelstoer

Loading

marcelstoer commented Jul 25, 2017

AadiMehta commented Jul 26, 2017

ESP32: Port net module to netconn API. #2047

ESP32: Port net module to netconn API. #2047

Conversation

devsaurus commented Jul 18, 2017

jmattsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsaurus Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsaurus commented Jul 19, 2017 • edited Loading

devsaurus commented Jul 21, 2017

AadiMehta commented Jul 22, 2017

jmattsson commented Jul 23, 2017

marcelstoer commented Jul 25, 2017 • edited Loading

devsaurus commented Jul 25, 2017 • edited by marcelstoer Loading

marcelstoer commented Jul 25, 2017

AadiMehta commented Jul 26, 2017

devsaurus Jul 19, 2017 •

edited

Loading

devsaurus commented Jul 19, 2017 •

edited

Loading

marcelstoer commented Jul 25, 2017 •

edited

Loading

devsaurus commented Jul 25, 2017 •

edited by marcelstoer

Loading