TCP keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) #3937

d-a-v · 2017-12-08T14:50:40Z

(not yed automatic) tcp-keep-alive for all new connections with standard default values
#2750

igrr · 2017-12-08T15:05:20Z

libraries/ESP8266WiFi/src/WiFiClient.h

+  void disableKeepAlive () { keepAlive(0, 0, 0); }
+
+  static void setKeepAlive (void* pcb, uint16_t idle_sec, uint16_t intv_sec, uint8_t count);
+  static void getKeepAlive (void* pcb, uint16_t* idle_sec, uint16_t* intv_sec, uint8_t* count);


How about moving these two into ClienContext? Exposing details such as pcb does not look right...
If you move it into ClienContext, then the first argument can also be changed from void* to tcp_pcb*.

WiFiClient.cpp has <lwip/.h>
ClientContext has private _pcb
You make me realize that since ClientContext knows tcp_pcb, it has <lwip/.h> too.
will fix :)

igrr · 2017-12-08T15:07:41Z

libraries/ESP8266WiFi/src/include/ClientContext.h

@@ -31,6 +31,10 @@ extern "C" void esp_schedule();

 #include "DataSource.h"

+#define TCP_DEFAULT_KEEPALIVE_IDLE_SEC          7200 // 2 hours
+#define TCP_DEFAULT_KEEPALIVE_INTERVAL_SEC      75   // 75 sec
+#define TCP_DEFAULT_KEEPALIVE_COUNT             9    // fault after 9 failures


Do you think that releasing 2.4.0 with keepalives enabled by default, after two release candidates didn't have them, may cause some issues? How about adding methods to enable keepalives first, and then enable them by default after the release?

I set up an example with 5s idle-keep-alive (default value is 2 hours), logs are:

16:41:47.308767 IP 10.43.1.163.23 > 10.43.1.254.51006: Flags [.], ack 28, win 2117, length 0 16:41:47.308797 IP 10.43.1.254.51006 > 10.43.1.163.23: Flags [.], ack 1, win 29200, length 0 16:41:52.808481 IP 10.43.1.163.23 > 10.43.1.254.51006: Flags [.], ack 28, win 2117, length 0 16:41:52.808512 IP 10.43.1.254.51006 > 10.43.1.163.23: Flags [.], ack 1, win 29200, length 0

.163 is esp with tcpserial example. We can see that tcp sends empty packets which are answered by peer. WiFiClient::connected() will answer false once keep-alive-count packets are not answered. Libraries and Sketchs already take care of ::connected() result, so yes I think there will be no harm.
I understand and agree that we can never predict side effects. I made this PR so that #2750 can see and test, it can wait 2.5, or I can disable it by default for 2.4.

d-a-v · 2017-12-08T18:19:55Z

keepalive PR is squash-updated ~~and still enabled by default~~.

devyte · 2017-12-12T03:42:06Z

libraries/ESP8266WiFi/src/include/ClientContext.h

+            _pcb->so_options &= ~SOF_KEEPALIVE;
+    }
+
+    void getKeepAlive (uint16_t* idle_sec, uint16_t* intv_sec, uint8_t* count)


Maybe use of references instead of pointers would be better here?

This way with pointers let the user choose which value to get, like with gettimeofday(&tv, NULL) we never want the second one. This can't be done with references
I put this getKeepAlive() method mainly for the user to be informed if keep-alive is activated through WiFiClient:: since ClientContext is not accessible.
I'm open to any other method than this one.

I missed your original intent. In that case, I think I'd prefer three inline methods, one for each value, each returning the requested value, plus a 4th inline method that just returns the enabled flag as a bool, something along the lines of:
getKeepAliveCount()
getKeepAliveInterval()
getKeepAliveIdle()
isKeepAliveEnabled()

Body could be like this:
return isKeepAliveEnabled() ? _pcb-> blah : 0;

keepalive-api is updated, PR squashed-rebased

devyte · 2017-12-12T03:44:40Z

libraries/ESP8266WiFi/src/include/ClientContext.h

@@ -325,6 +328,26 @@ class ClientContext
        return _write_from_source(new BufferedStreamDataSource<ProgmemStream>(stream, size));
    }

+    void keepAlive (uint16_t idle_sec = TCP_DEFAULT_KEEPALIVE_IDLE_SEC, uint16_t intv_sec = TCP_DEFAULT_KEEPALIVE_INTERVAL_SEC, uint8_t count = TCP_DEFAULT_KEEPALIVE_COUNT)
+    {
+        if (idle_sec && intv_sec && count) {


What happens if any 2 of the 3 values are non-zero? Are all combinations allowed?

(mostly thinking of sanity checks, if necessary)

any 0 is an invalid value for keep-alive api, so any 0 disables keep-alive.

devyte · 2017-12-12T03:46:05Z

libraries/ESP8266WiFi/src/include/ClientContext.h

+    void getKeepAlive (uint16_t* idle_sec, uint16_t* intv_sec, uint8_t* count)
+    {
+        bool enabled = _pcb->so_options & SOF_KEEPALIVE;
+        if (idle_sec) *idle_sec = enabled? (_pcb->keep_idle + 500) / 1000: 0;


Indentation (body of the if statement on a new line)

function removed

devyte · 2017-12-12T03:49:59Z

libraries/ESP8266WiFi/src/WiFiClient.h

@@ -30,6 +30,10 @@

 #define WIFICLIENT_MAX_PACKET_SIZE 1460

+#define TCP_DEFAULT_KEEPALIVE_IDLE_SEC          7200 // 2 hours


Do #defines count against available heap, or is it better to make these static const and put them in flash?
(I'm thinking of the free heap project, every byte counts)

unlike static const, #define count for nothing in ram nor flash.

devyte · 2018-01-02T15:13:59Z

@d-a-v if you add the getter, I think this can be merged

d-a-v · 2018-01-02T15:19:12Z

The getters are already present (I push-squashed to reduce the number of commits).
But now we are at pre-2.5, should'nt we enable keepalive by default ?
The default idle value is 2h, It could be 10s (it sends empty packets).
Regarding #2330 what good value could we use for a good start ?

devyte · 2018-01-02T15:58:22Z

@d-a-v isn't WiFiClient missing a getter isKeepAliveEnabled()?

d-a-v · 2018-01-02T21:39:14Z

right, this is fixed

devyte · 2017-12-28T02:36:35Z

libraries/ESP8266WiFi/src/WiFiClient.h

+  uint16_t getKeepAliveIdle () const;
+  uint16_t getKeepAliveInterval () const;
+  uint8_t  getKeepAliveCount () const;
+  void     disableKeepAlive () { keepAlive(0, 0, 0); }


What about a getter for keepAliveEnabled?
bool isKeepAliveEnabled() {return _client->isKeepAliveEnabled();}

devyte · 2018-01-05T03:17:27Z

Still pending: verify if the defaults are ok, but that can be a different issue.

Androbin · 2018-09-15T14:23:05Z

Why isn't keepAlive enabled by default?
And does WiFiServer support keepAlive?
(or ESP8266WebServer respectively)

d-a-v · 2018-09-15T14:45:34Z

Why isn't keepAlive enabled by default?

Because we did not check all side effects yet.
But you can enable it as soon as you have a valid WiFiClient.

And does WiFiServer support keepAlive?

keepAlive is for TCP stream (WiFiClient here).
This is irrelevant for WiFiServer: WiFiServer is awaiting for those streams.

d-a-v · 2018-09-15T21:12:14Z

This is unrelated. This pull-request is about keep-alive TCP's feature, not connection type in HTTP header.
Please open a new issue and fill the issue template.

d-a-v force-pushed the keepalive branch from fc4e5c1 to 1b2d850 Compare December 8, 2017 15:02

igrr reviewed Dec 8, 2017

View reviewed changes

d-a-v force-pushed the keepalive branch 2 times, most recently from 82b7d6e to 7e607e5 Compare December 8, 2017 18:18

d-a-v changed the title ~~setup keepalive(2h,75s,9) by default + status helper + change/disable methods~~ keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) Dec 8, 2017

devyte requested changes Dec 12, 2017

View reviewed changes

d-a-v force-pushed the keepalive branch from 08b66ab to c91220d Compare December 26, 2017 23:14

keepalive api (default 2h,75s,9 not enabled)

2524141

d-a-v force-pushed the keepalive branch from c91220d to 2524141 Compare January 2, 2018 21:38

devyte approved these changes Jan 2, 2018

View reviewed changes

devyte added 2 commits January 3, 2018 13:15

Merge branch 'master' into keepalive

661ac56

Merge branch 'master' into keepalive

184fe3b

devyte merged commit 9cfbbc7 into esp8266:master Jan 5, 2018

d-a-v deleted the keepalive branch January 5, 2018 17:42

devyte mentioned this pull request Jan 7, 2018

TCP-server keepalive #3886

Closed

incosystem pushed a commit to incosys/Arduino that referenced this pull request Jan 8, 2018

keepalive api (default 2h,75s,9 not enabled) (esp8266#3937)

a797d3f

d-a-v changed the title ~~keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default)~~ TCP keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) Sep 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TCP keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) #3937

TCP keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) #3937

d-a-v commented Dec 8, 2017 •

edited

Loading

igrr Dec 8, 2017

d-a-v Dec 8, 2017 •

edited

Loading

igrr Dec 8, 2017

d-a-v Dec 8, 2017

d-a-v commented Dec 8, 2017 •

edited

Loading

devyte Dec 12, 2017

d-a-v Dec 12, 2017

devyte Dec 26, 2017

devyte Dec 26, 2017

d-a-v Dec 26, 2017

devyte Dec 12, 2017

devyte Dec 12, 2017

d-a-v Dec 12, 2017

devyte Dec 12, 2017

d-a-v Dec 12, 2017 •

edited

Loading

devyte Dec 12, 2017

d-a-v Dec 12, 2017

devyte commented Jan 2, 2018

d-a-v commented Jan 2, 2018

devyte commented Jan 2, 2018

d-a-v commented Jan 2, 2018

devyte Dec 28, 2017

devyte commented Jan 5, 2018

Androbin commented Sep 15, 2018 •

edited

Loading

d-a-v commented Sep 15, 2018

d-a-v commented Sep 15, 2018

		@@ -30,6 +30,10 @@

		#define WIFICLIENT_MAX_PACKET_SIZE 1460

		#define TCP_DEFAULT_KEEPALIVE_IDLE_SEC 7200 // 2 hours

TCP keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) #3937

TCP keepalive(2h,75s,9) API + status helper + change/disable methods (not enabled by default) #3937

Conversation

d-a-v commented Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

d-a-v Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-a-v commented Dec 8, 2017 • edited Loading

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

d-a-v Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devyte commented Jan 2, 2018

d-a-v commented Jan 2, 2018

devyte commented Jan 2, 2018

d-a-v commented Jan 2, 2018

Choose a reason for hiding this comment

devyte commented Jan 5, 2018

Androbin commented Sep 15, 2018 • edited Loading

d-a-v commented Sep 15, 2018

d-a-v commented Sep 15, 2018

d-a-v commented Dec 8, 2017 •

edited

Loading

d-a-v Dec 8, 2017 •

edited

Loading

d-a-v commented Dec 8, 2017 •

edited

Loading

d-a-v Dec 12, 2017 •

edited

Loading

Androbin commented Sep 15, 2018 •

edited

Loading