ClientContext (tcp) updates #5089

d-a-v · 2018-08-28T15:54:35Z

make Nagle work properly (default is unchanged: disabled = NoDelay enabled)
add client.setSync(bool) (= "always flush" mode)
slower when enabled but allocates less memory and data are always flushed
client.stop() implicitely calls .flush() (flush has a small timeout anyway)
add static WiFiClient::setDefaultSync(bool)/::setDefaultNoDelay(bool)
internal: remove PuSH flag from data packet

earlephilhower

I think you need to compile with full debugging options. I'm seeing build failures due to variables you've done away with still setting in DEBUGV lines.

earlephilhower · 2018-09-17T19:55:31Z

libraries/ESP8266WiFi/src/include/ClientContext.h

-        if (_pcb->snd_queuelen >= TCP_SND_QUEUELEN) {
-            can_send = 0;
-        }
-        size_t will_send = (can_send < left) ? can_send : left;
        DEBUGV(":wr %d %d %d\r\n", will_send, left, _written);


The debug lines fail to compile due to them trying to dump variables that were just removed (left)

earlephilhower · 2018-09-17T19:55:56Z

libraries/ESP8266WiFi/src/include/ClientContext.h

+                // user data will not stay in place when data are sent but not acknowledged
+                flags |= TCP_WRITE_FLAG_COPY;
+            err_t err = tcp_write(_pcb, buf, next_chunk_size, flags);
+            DEBUGV(":wrc %d %d %d\r\n", next_chunk_size, will_send, (int)err);


will_send undefined, too

earlephilhower · 2018-09-17T20:01:18Z

Functionally, when compiled w/o debugging, this patch doesn't seem to cause any problems with BearSSL or axTLS in simple testing (bssl_validatiaon, https_request).

Simple refactor to make WiFiClientSecureAxTLS use an external header to define its SSLContext, just as it does for several other classes. Fixes esp8266#3648

As part of the "clear connection configuration for reused objects" patch, a ::stop would reset the self-signed, trust anchors, etc. WiFiClient, unfortunately, calls ::stop as part of the connection process, so all of these settings were lost. Now only clear the connection settings on ::stop if we've already been connected. Also update the github public key which changed yet again. Fixes esp8266#5086

devyte

Kudos on the constness!

libraries/ESP8266WiFi/src/WiFiClient.h

devyte · 2018-09-05T20:58:39Z

libraries/ESP8266WiFi/src/include/ClientContext.h

@@ -35,7 +35,7 @@ class ClientContext
 {
 public:
    ClientContext(tcp_pcb* pcb, discard_cb_t discard_cb, void* discard_cb_arg) :
-        _pcb(pcb), _rx_buf(0), _rx_buf_offset(0), _discard_cb(discard_cb), _discard_cb_arg(discard_cb_arg), _refcnt(0), _next(0)
+        _pcb(pcb), _rx_buf(0), _rx_buf_offset(0), _discard_cb(discard_cb), _discard_cb_arg(discard_cb_arg), _refcnt(0), _next(0), _sync(WiFiClient::getDefaultSync())


This is a bad idea, because it needlessly creates a circular dependency: ClientContext depends on WiFiClient, and WiFiClient depends on ClientContext. I suggest either factoring the sync into its own .h/.cpp, or pushing its declaration/definition into ClientContext.h/.cpp for reuse by WiFiClient class.

devyte · 2018-09-18T22:28:17Z

libraries/ESP8266WiFi/src/WiFiClient.cpp

@@ -42,6 +42,8 @@ extern "C"
 #include "c_types.h"

 uint16_t WiFiClient::_localPort = 0;
+bool WiFiClient::_defaultNoDelay = true;


I suggest a big fat comment here about what the four combinations of nodelay and sync do, because I can't for the life of me keep them straight in my head.

…ontext

…ed to 300ms

earlephilhower · 2018-09-23T20:17:49Z

@d-a-v, there's something weird in the tree and it's failing with the EthernetClient example builds. Very odd! Maybe it was using the ClientContext and something got broken?

earlephilhower

As far as the SSL side goes, looks fine to me. Minor niggle on some C operator early-terminator, but would not -1 the patch for it. Thanks for the heads-up!

earlephilhower · 2018-09-23T20:28:36Z

libraries/ESP8266WiFi/src/WiFiClient.cpp

 {
-    if (_client)
-        _client->wait_until_sent();
+    return !_client || _client->wait_until_sent(maxWaitMs?: WIFICLIENT_MAX_FLUSH_WAIT_MS);


I get the shorthand, but using the early-termination feature to avoid an if-then could be confusing

earlephilhower · 2018-09-23T20:32:26Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp

-  }
-  WiFiClient::stop();
+bool WiFiClientSecure::stop(unsigned int maxWaitMs) {
+  bool ret = WiFiClient::stop(maxWaitMs); // calls our virtual flush()


I see ::stop calls ::flush so we're OK there. Why I did _client->abort() as well, I couldn't tell you. LGTM.

earlephilhower · 2018-09-23T20:36:55Z

The build problem is the changes to the flush and stop signature. Need to update the EthernetClient sources to have the new timeout parameter w/defaults...

d-a-v · 2018-09-24T21:02:21Z

doc is visible there: client and server.

Added details for getters

d-a-v added 10 commits August 28, 2018 00:46

wip

6c64759

cc wip

746ec81

cc wip

53c9222

cc comments

6de50d4

wip cc

2dd152d

+sync, get/set default nodelay, sync

f87602a

default nodelay=1

6be4b6f

update flush()

8f7fb1e

fix return value

c0bea2b

ClientContext: put things together

d00ac35

earlephilhower requested changes Sep 17, 2018

View reviewed changes

d-a-v and others added 13 commits September 18, 2018 09:35

wip

53931c5

cc wip

81ec0f4

cc wip

fbf1dd1

cc comments

b4a918f

wip cc

c14eeef

+sync, get/set default nodelay, sync

238610e

default nodelay=1

d0036a1

update flush()

e4c187b

fix return value

d31d6f2

ClientContext: put things together

5cf60b6

Move SSLContext to its own header (esp8266#5121)

a058506

Simple refactor to make WiFiClientSecureAxTLS use an external header to define its SSLContext, just as it does for several other classes. Fixes esp8266#3648

ClientContext: fix debugging messages

d06cac2

d-a-v force-pushed the ClientContext branch from 2423ad3 to d06cac2 Compare September 18, 2018 07:50

Merge branch 'master' into ClientContext

e8c659e

earlephilhower approved these changes Sep 18, 2018

View reviewed changes

devyte requested changes Sep 18, 2018

View reviewed changes

WiFiClient: move static members out of the class, add comments

f271b2a

d-a-v added 2 commits September 19, 2018 01:46

Merge branch 'ClientContext' of github.com:d-a-v/Arduino into ClientC…

7f57694

…ontext

remove circular dependency

888bdb8

d-a-v mentioned this pull request Sep 19, 2018

IPv6 on esp8266-nonos-sdk and arduino #5136

Merged

parameter and return value for Client::flush&stop, flush timeout rais…

c59a91f

…ed to 300ms

d-a-v mentioned this pull request Sep 23, 2018

[Arduino OTA] Python consuming 25% of an I5 processor - Showing error in IDE after upload #4966

Closed

Merge branch 'master' into ClientContext

3c54c9e

earlephilhower approved these changes Sep 23, 2018

View reviewed changes

d-a-v added 9 commits September 23, 2018 22:47

tcp flush: restart timer on ack receive

8414e17

OTA protocol needs setNoDelay(true)

4d62879

fix Ethernet with Client changes

826b8b5

1 line unredable -> 5 lines readable code

3fa6a97

Merge branch 'master' into ClientContext

72b59d9

doc

7752c98

doc

c160d17

doc

db8fe6c

doc

8bb444d

Update client-class.rst

33dee32

Added details for getters

devyte approved these changes Sep 24, 2018

View reviewed changes

devyte merged commit 83a8076 into esp8266:master Sep 25, 2018

mongozmaki mentioned this pull request Sep 26, 2018

Slow WiFiClient after commit "ClientContext (tcp) updates #5089" #5170

Closed

ghmartin77 mentioned this pull request Jan 21, 2019

publish() needs an idle period to succeed? knolleary/pubsubclient#435

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ClientContext (tcp) updates #5089

ClientContext (tcp) updates #5089

d-a-v commented Aug 28, 2018

earlephilhower left a comment

earlephilhower Sep 17, 2018

earlephilhower Sep 17, 2018

earlephilhower commented Sep 17, 2018

devyte left a comment

devyte Sep 5, 2018

devyte Sep 18, 2018

earlephilhower commented Sep 23, 2018

earlephilhower left a comment

earlephilhower Sep 23, 2018

earlephilhower Sep 23, 2018

earlephilhower commented Sep 23, 2018

d-a-v commented Sep 24, 2018

ClientContext (tcp) updates #5089

ClientContext (tcp) updates #5089

Conversation

d-a-v commented Aug 28, 2018

earlephilhower left a comment

Choose a reason for hiding this comment

earlephilhower Sep 17, 2018

Choose a reason for hiding this comment

earlephilhower Sep 17, 2018

Choose a reason for hiding this comment

earlephilhower commented Sep 17, 2018

devyte left a comment

Choose a reason for hiding this comment

devyte Sep 5, 2018

Choose a reason for hiding this comment

devyte Sep 18, 2018

Choose a reason for hiding this comment

earlephilhower commented Sep 23, 2018

earlephilhower left a comment

Choose a reason for hiding this comment

earlephilhower Sep 23, 2018

Choose a reason for hiding this comment

earlephilhower Sep 23, 2018

Choose a reason for hiding this comment

earlephilhower commented Sep 23, 2018

d-a-v commented Sep 24, 2018