Update ESP8266WiFiSTAClass::waitForConnectResult() for the same behavior with ESP32. #4985

ShinDaiIchi · 2018-07-30T06:51:46Z

Add counter i for trying WIFi-connection 100 times

…ior to ESP32 add counter i for trying WIFi-connection 100 times

split Arduino's String to Arduino's String Array

d-a-v · 2018-07-30T14:11:28Z

You seem to have committed something else along with this proposal. Please do another PR for this.

To not break very precious backward compatibility,
what do you think of adding an optional parameter that would not change behavior for existing sketches ?

uint8_t ESP8266WiFiSTAClass::waitForConnectResult(uint8_t maxSeconds = 0) { // 0 is infinity

You'd propose the same fix to esp-idf with 10 instead of 0.
@me-no-dev what do you think ?

devyte · 2018-07-30T14:37:20Z

cores/esp8266/WString.cpp

@@ -781,6 +781,57 @@ void String::trim(void) {
    buffer[len] = 0;
 }

+/************************************************************/


Please remove this String addition.
On a personal note, the implementation makes me want to tear my eyes out. Why didn't the author use a C++ reference instead of a double pointer?

Sorry, I just tried something on String addition but not for this PR.
Already remove them.

devyte · 2018-07-30T14:37:32Z

cores/esp8266/WString.h

@@ -237,7 +237,8 @@ class String {
        void toLowerCase(void);
        void toUpperCase(void);
        void trim(void);
-
+
+        int td_split(String delimiter, String** str_array); // v2.3   split String to String Array by multi-delimiters


Please remove this change.

Already removed this String addition change.

devyte · 2018-07-30T16:32:54Z

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp

@@ -374,7 +374,8 @@ uint8_t ESP8266WiFiSTAClass::waitForConnectResult() {
    if((wifi_get_opmode() & 1) == 0) {
        return WL_DISCONNECTED;
    }
-    while(status() == WL_DISCONNECTED) {
+    int i = 0;
+    while((!status() || status() >= WL_DISCONNECTED) && i++ < 100) {


This is a max of 10s time. Some connections could take longer, especially under poor signal conditions. Some users could not want to spend so much time trying to connect and want to give up sooner. I suggest taking an argument that defaults to this when not set.

If default time trying to connect when not set is 15 secs. What do you think ?

What would happen with a default non infinite value when user's network connectivity / the router is for any reason powered down or disconnected for a while (a longer while than this default value), or takes long to power on, or whatever ?

My point is that usually sketches patiently wait for connectivity. With this change, all sketches would need to be updated to loop on the connectivity loop.

I don't question this PR, only the historical default behavior / backward compatibility.

Oops @d-a-v is correct: either we maintain backwards compatibility and merge now, or we keep this breaking change and wait until release 3.0.0 to merge. I prefer the former.
So:

the function should take a timeout argument, in ms, which defaults to some value that disables the timeout (std::numeric_limits?)

If the user wants a timeout, he can specify a value for the argument.

In a future release we'll change the default to some value like 10 or 15s.

@ShinDaiIchi Could you please implement the changes and update the PR?

Already update with WiFi connecting's wait-timeout with backwards compatibility.

devyte · 2018-08-01T00:53:46Z

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp

@@ -374,7 +374,8 @@ uint8_t ESP8266WiFiSTAClass::waitForConnectResult() {
    if((wifi_get_opmode() & 1) == 0) {
        return WL_DISCONNECTED;
    }
-    while(status() == WL_DISCONNECTED) {
+    int i = 0;
+    while((!status() || status() >= WL_DISCONNECTED) && i++ < 100) {


Oops @d-a-v is correct: either we maintain backwards compatibility and merge now, or we keep this breaking change and wait until release 3.0.0 to merge. I prefer the former.
So:

the function should take a timeout argument, in ms, which defaults to some value that disables the timeout (std::numeric_limits?)

If the user wants a timeout, he can specify a value for the argument.

In a future release we'll change the default to some value like 10 or 15s.

@ShinDaiIchi Could you please implement the changes and update the PR?

…meout and compatable old version.

devyte

Minor changes requested. Will merge once fixed.

devyte · 2018-08-09T21:40:15Z

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp

    //1 and 3 have STA enabled
    if((wifi_get_opmode() & 1) == 0) {
        return WL_DISCONNECTED;
    }
-    while(status() == WL_DISCONNECTED) {
+    int i = 0; int try_times = wait_secs*10;
+    while((!status() || status() >= WL_DISCONNECTED) && ((wait_secs!=0)? (i++ < try_times): true )) {


don't use inverted logic please:
... && ((wait_secs==0) ? true : (i++ < try_times) )

devyte · 2018-08-09T21:40:40Z

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp

    //1 and 3 have STA enabled
    if((wifi_get_opmode() & 1) == 0) {
        return WL_DISCONNECTED;
    }
-    while(status() == WL_DISCONNECTED) {
+    int i = 0; int try_times = wait_secs*10;


PLease declare/init try_times on its own line

ShinDaiIchi

Update

ShinDaiIchi · 2018-08-02T12:26:12Z

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp

@@ -374,7 +374,8 @@ uint8_t ESP8266WiFiSTAClass::waitForConnectResult() {
    if((wifi_get_opmode() & 1) == 0) {
        return WL_DISCONNECTED;
    }
-    while(status() == WL_DISCONNECTED) {
+    int i = 0;
+    while((!status() || status() >= WL_DISCONNECTED) && i++ < 100) {


Already update with WiFi connecting's wait-timeout with backwards compatibility.

yoursunny · 2018-10-09T23:44:18Z

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp

-    while(status() == WL_DISCONNECTED) {
+    int i = 0; 
+    int try_times = wait_secs*10;
+    while((!status() || status() >= WL_DISCONNECTED) && ((wait_secs==0)? true : (i++ < try_times))) {


This line is difficult to read.

!status() tests whether the status is non-zero, but status is an enum and it’s unobvious which value is zero.

The part after && is really complicated.

How about:

for (int timeout = wait_secs * 10; wait_secs == 0 || timeout > 0; --timeout) { auto st = status(); switch (st) { case WL_CONNECTED: case WL_NO_SSID_AVAIL: case WL_CONNECT_FAILED: return st; default: delay(100); break; } }

Agree with @yoursunny . However:

I have a PR with a class that encapsulates the polled timeout. I suggest waiting a bit until it's merged, then using it, instead of reinventing the wheel for the nth time.
the switch should define cases for all enum values, and leave the default case for values not in the enum (error).

devyte · 2019-11-06T19:55:21Z

@ShinDaiIchi the polledTimeout class has been available for a while now. Could you please resolve conflicts and update with that?

ShinDaiIchi added 3 commits July 30, 2018 13:49

Update ESP8266WiFiSTAClass::waitForConnectResult() for the same behav…

0be5be6

…ior to ESP32 add counter i for trying WIFi-connection 100 times

Add td_split() for spliting String to String Array by multi-delimiters

d2c4cc7

split Arduino's String to Arduino's String Array

Add td_split() for spliting String to String Array

b39fc18

devyte requested changes Jul 30, 2018

View reviewed changes

ShinDaiIchi added 2 commits July 31, 2018 05:16

Update WString.h

579a9a6

Update WString.cpp

5c4ff27

devyte requested changes Aug 1, 2018

View reviewed changes

devyte and others added 4 commits August 1, 2018 15:36

Merge branch 'master' into master

8abc913

Update ESP8266WiFiSTAClass::waitForConnectResult with connection's ti…

8544d30

…meout and compatable old version.

Update ESP8266WiFiSTAClass::waitForConnectResult with connection's ti…

956510e

…meout and compatable old version.

Merge branch 'master' into master

52fcfa3

devyte requested changes Aug 9, 2018

View reviewed changes

Update (wait_secs==0)? etc...

c5fec54

ShinDaiIchi commented Aug 11, 2018

View reviewed changes

Merge branch 'master' into master

cba45ea

yoursunny reviewed Oct 9, 2018

View reviewed changes

d-a-v assigned devyte Oct 10, 2018

earlephilhower added the merge-conflict PR has a merge conflict that needs manual correction label Dec 10, 2019

Update ESP8266WiFiSTAClass::waitForConnectResult() for the same behavior with ESP32. #4985

Are you sure you want to change the base?

Update ESP8266WiFiSTAClass::waitForConnectResult() for the same behavior with ESP32. #4985

Uh oh!

Conversation

ShinDaiIchi commented Jul 30, 2018

Uh oh!

d-a-v commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-a-v Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devyte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShinDaiIchi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devyte commented Nov 6, 2019

Uh oh!

Uh oh!

d-a-v Jul 30, 2018 •

edited

Loading