Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 14, 2019

Conversation

Projects
None yet
6 participants
@michalpasztamobica
Copy link
Contributor

michalpasztamobica commented Feb 7, 2019

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 ciarmcom requested review from SeppoTakalo , VeijoPesonen and ARMmbed/mbed-os-maintainers Feb 7, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Feb 7, 2019

components/wifi/esp8266-driver/ESP8266Interface.cpp Outdated
@@ -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)) {

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Feb 8, 2019

Contributor

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.

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Feb 8, 2019

Contributor

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

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Feb 8, 2019

Contributor

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

components/wifi/esp8266-driver/ESP8266Interface.h Outdated
@@ -34,6 +34,10 @@

#define ESP8266_SOCKET_COUNT 5

#ifndef ESP8266_INTERFACE_CONNECT_TIMEOUT

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Feb 8, 2019

Contributor

The whole block is not needed

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_timeout branch Feb 8, 2019

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

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 8, 2019

@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.

components/wifi/esp8266-driver/ESP8266Interface.cpp Outdated
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) {

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Feb 8, 2019

Contributor

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

This comment has been minimized.

@michalpasztamobica

michalpasztamobica Feb 11, 2019

Author Contributor

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.

This comment has been minimized.

@VeijoPesonen

VeijoPesonen Feb 11, 2019

Contributor

Sounds like a good idea.

This comment has been minimized.

@michalpasztamobica

michalpasztamobica Feb 11, 2019

Author Contributor

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.

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

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_timeout branch to 6a184da Feb 11, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

michalpasztamobica commented Feb 13, 2019

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

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 13, 2019

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

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 13, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 14, 2019

Test run: SUCCESS

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

@cmonr

cmonr approved these changes Feb 14, 2019

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

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9225 cycles (-12 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.