ESP8266: connect() checks errors from ESP chip #9639

michalpasztamobica · 2019-02-07T16:17:50Z

Description

ESP8266Interface::connect() checks ESP8266::connect() return values and returns if unrecoverable errors are returned.
Increased timeout for network-wifi greentea tests.
Fixes failing network-wifi-WIFI-CONNECT-SECURE-FAIL tests for ESP8266.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@VeijoPesonen

ciarmcom · 2019-02-07T18:00:35Z

@michalpasztamobica, thank you for your changes.
@VeijoPesonen @SeppoTakalo @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

VeijoPesonen · 2019-02-08T08:14:40Z

components/wifi/esp8266-driver/ESP8266Interface.cpp

@@ -244,7 +244,10 @@ int ESP8266Interface::connect()
    }

    while (_if_blocking && (_conn_status_to_error() != NSAPI_ERROR_IS_CONNECTED)) {
-        _if_connected.wait();
+        if (_if_connected.wait_for(ESP8266_INTERFACE_CONNECT_TIMEOUT)) {


I have made this way too simplistic(read - I made a mistake) and that's why there is need to introduce one more variable to see what ESP8266::connect returns, sorry for that. Timeout here wouldn't work because the condition might get raised spuriously. Instead of returning NSAPI_ERROR_CONNECTION_TIMEOUT we would want to return what ever ESP8266::connect spits out.

Just to elaborate - there is not much of a reason to continue if ESP8266::connect() returns, e.g., NSAPI_ERROR_NO_SSID.

The process can continue on the background but call should return.

VeijoPesonen · 2019-02-08T08:14:52Z

components/wifi/esp8266-driver/ESP8266Interface.h

@@ -34,6 +34,10 @@

 #define ESP8266_SOCKET_COUNT 5

+#ifndef ESP8266_INTERFACE_CONNECT_TIMEOUT


The whole block is not needed

michalpasztamobica · 2019-02-08T12:31:27Z

@VeijoPesonen , I modified the code to handle the errors recognized by ESP8266::connect() (AUTH_FAILURE, CONNECTION_TIMEOUT and NO_SSID). If any of these or OK are detected the ESP8266Interface::connect() will be signalled to return a proper return value.
network-wifi suite is passing with this solution.
Let me know if this is more or less what you meant? :)

I also updated the commit message and title of this PR, leaving the PR's Description as was, to maintain history.

VeijoPesonen · 2019-02-08T14:36:29Z

components/wifi/esp8266-driver/ESP8266Interface.cpp

-    if (_esp.connect(ap_ssid, ap_pass) != NSAPI_ERROR_OK) {
+    _connect_retval = _esp.connect(ap_ssid, ap_pass);
+    if (_connect_retval == NSAPI_ERROR_OK || _connect_retval == NSAPI_ERROR_AUTH_FAILURE
+            || _connect_retval == NSAPI_ERROR_CONNECTION_TIMEOUT || _connect_retval == NSAPI_ERROR_NO_SSID) {


I think we should continue trying with NSAPI_ERROR_CONNECTION_TIMEOUT. In other words with that return value we should end up in the else-statement

You are right, although checking this led me to a sad conclusion, that ESP does not realize that "aaaaaaaa" network does not exist and it never returns with "3: cannot find the target AP" error code, as the documentation promises.
Instead after two minutes it returns a "2: wrong password", which we later translate to AUTH_FAILURE.
I even logged into ruka and loaded a cliapp to run "wlan0 scan" to make double sure "aaaaaaaa" does not exist. It doesn't.
To make up for the 2 minute wait I will also extend the wifi testsuite duration.

Sounds like a good idea.

There is also influence from previous tests - when I run the whole suite the ESP will never return anything else than 1 in WIFI-CONNECT-SECURE-FAIL. I am investigating.

ESP8266Interface::connect() checks the exact return value from the underlying ESP8266::connect() call. Increased timeout for network-wifi greentea tests to 6 minutes.

michalpasztamobica · 2019-02-11T15:27:46Z

I updated the code according to Veijo's remark (not to return if ESP advertises timeout). Unfortunately, the network-wifi-WIFI-CONNECT-SECURE-FAIL was not passing after this last change. I tried many things: the way we set up and tear down the test, the reset line assertion on disconnect, the order of tests in the suite, etc.
In the end I provided the json config with credentials for our local office WiFi and run the wifi suite locally. It is passing just fine (I did not have an unsecured WiFi, so I did not test that part).
I think there is some issue with the ruka wifi network. If the CI keeps failing with these changes I will raise a proper ticket. With my local, stable, uncrowded WiFi the tests are passing fine.
@VeijoPesonen , @SeppoTakalo , if you agree with the changes and the plan for future, please approve.

michalpasztamobica · 2019-02-13T15:15:32Z

@cmonr , @0xc0170 , this is approved. Can we merge this in to see its influence on the nightly CI builds?

cmonr · 2019-02-13T20:52:11Z

@michalpasztamobica Need to start CI first. Looking at the current CI queue, it looks like merging it tonight might be a hard sell.

cmonr · 2019-02-13T20:52:20Z

CI started

mbed-ci · 2019-02-14T01:49:22Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

ciarmcom requested review from SeppoTakalo, VeijoPesonen and a team February 7, 2019 18:00

ciarmcom added the needs: review label Feb 7, 2019

VeijoPesonen suggested changes Feb 8, 2019

View reviewed changes

michalpasztamobica force-pushed the esp8266_connect_timeout branch from a0d0131 to 66e8a67 Compare February 8, 2019 12:26

michalpasztamobica changed the title ~~ESP8266: connect() timeout introduction~~ ESP8266: connect() checks errors from ESP chip Feb 8, 2019

VeijoPesonen suggested changes Feb 8, 2019

View reviewed changes

ESP8266: connect() can handle ESP's errors.

6a184da

ESP8266Interface::connect() checks the exact return value from the underlying ESP8266::connect() call. Increased timeout for network-wifi greentea tests to 6 minutes.

michalpasztamobica force-pushed the esp8266_connect_timeout branch from 66e8a67 to 6a184da Compare February 11, 2019 15:20

VeijoPesonen approved these changes Feb 13, 2019

View reviewed changes

SeppoTakalo approved these changes Feb 13, 2019

View reviewed changes

cmonr approved these changes Feb 14, 2019

View reviewed changes

cmonr merged commit f2abdcb into ARMmbed:master Feb 14, 2019

cmonr added release-version: 5.11.5 and removed needs: review labels Feb 14, 2019

ccli8 mentioned this pull request Feb 14, 2019

Inconsistent symbol generation with target.mbed_ram_start override #9716

Closed

michalpasztamobica mentioned this pull request Feb 15, 2019

ESP8266: connect() returns OK in non-blocking calls #9741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ESP8266: connect() checks errors from ESP chip #9639

ESP8266: connect() checks errors from ESP chip #9639

Uh oh!

michalpasztamobica commented Feb 7, 2019 •

edited

Loading

Uh oh!

ciarmcom commented Feb 7, 2019

Uh oh!

VeijoPesonen Feb 8, 2019

Uh oh!

VeijoPesonen Feb 8, 2019

Uh oh!

VeijoPesonen Feb 8, 2019

Uh oh!

VeijoPesonen Feb 8, 2019

Uh oh!

michalpasztamobica commented Feb 8, 2019

Uh oh!

VeijoPesonen Feb 8, 2019 •

edited

Loading

Uh oh!

michalpasztamobica Feb 11, 2019

Uh oh!

VeijoPesonen Feb 11, 2019

Uh oh!

michalpasztamobica Feb 11, 2019

Uh oh!

michalpasztamobica commented Feb 11, 2019

Uh oh!

michalpasztamobica commented Feb 13, 2019

Uh oh!

cmonr commented Feb 13, 2019

Uh oh!

cmonr commented Feb 13, 2019

Uh oh!

mbed-ci commented Feb 14, 2019

Uh oh!

Uh oh!

		@@ -34,6 +34,10 @@

		#define ESP8266_SOCKET_COUNT 5

		#ifndef ESP8266_INTERFACE_CONNECT_TIMEOUT

ESP8266: connect() checks errors from ESP chip #9639

ESP8266: connect() checks errors from ESP chip #9639

Uh oh!

Conversation

michalpasztamobica commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Feb 7, 2019

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!

michalpasztamobica commented Feb 8, 2019

Uh oh!

VeijoPesonen Feb 8, 2019 • 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!

michalpasztamobica commented Feb 11, 2019

Uh oh!

michalpasztamobica commented Feb 13, 2019

Uh oh!

cmonr commented Feb 13, 2019

Uh oh!

cmonr commented Feb 13, 2019

Uh oh!

mbed-ci commented Feb 14, 2019

Uh oh!

Uh oh!

michalpasztamobica commented Feb 7, 2019 •

edited

Loading

VeijoPesonen Feb 8, 2019 •

edited

Loading