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

Fix ESP8266 driver behavior on connection failures #10377

Merged
merged 2 commits into from Apr 18, 2019

Conversation

Projects
None yet
7 participants
@michalpasztamobica
Copy link
Contributor

commented Apr 11, 2019

Description

In case the application calls disconnect() the driver will only perform actual disconnection procedure (including a status update and event signalling) if the ESP was really disconnected. This was added in this commit, but in fact user might call disconnect() for example after a failed connection attempt. In this case we should stop trying to connect, so that a retry is possible. This is also how EMAC drivers work.

Furthermore, we did not make use of ESP8266 timeout message, because in fact it kept on trying to connect. Therefore we need to use our own timeout to return a proper error value if the connection really takes too long.

Pull request type

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

Reviewers

@VeijoPesonen
@SeppoTakalo

@ciarmcom ciarmcom requested review from SeppoTakalo, VeijoPesonen and ARMmbed/mbed-os-maintainers Apr 11, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@cmonr

cmonr approved these changes Apr 12, 2019

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_failures branch 3 times, most recently from 23267c8 to 0221ef4 Apr 12, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@kjbracey-arm , @SeppoTakalo , would you please provide any review remarks?

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Furthermore, we did not make use of ESP8266 timeout message, because in fact it kept on trying to connect. Therefore we need to use our own timeout to return a proper error value if the connection really takes too long.

I had a talk with Kevin and my understanding is that giving up after 30 seconds in blocking mode should be fine. The rest of this comment you should read as my opinion.

Assumption here is that each connection attempt takes at maximum ~15 seconds(ESP8266_CONNECT_TIMEOUT). There is a 15 second grace period between the attempts with the current implementation. In other words please increase ESP8266_INTERFACE_CONNECT_TIMEOUT_MS to 30s+some seconds so that connect procedure has time to run two times if necessary.

That timelimit tells how long the driver has time to initiate a new connect-call, not when it should be done. It might still take 15 seconds(ESP8266_CONNECT_TIMEOUT) before the procedure succeeds or fails. So altogether it might take ~45 seconds that we know has the connection been established or did it fail.

I guess the grace period could be tuned down a bit from that 15 seconds so that the documentation can say we give up after 45 seconds. Return value should be something which makes possible to break out from the loop in blocking-mode.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_failures branch from 0221ef4 to d4adad6 Apr 15, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@VeijoPesonen , thank you for this valuable comment. I fully agree with your opinion, I tried to embrace it in the code with the following line:

#define ESP8266_INTERFACE_CONNECT_TIMEOUT_MS (2 * ESP8266_CONNECT_TIMEOUT + 15000)

I hope this would make it cristal clear for anyone reading the code in the future what this timeout depends on and what to consider when adjusting it.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_failures branch from d4adad6 to 453e485 Apr 15, 2019

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_failures branch 2 times, most recently from 0386089 to 7472b19 Apr 16, 2019

ESP8266: Timeout if chip keeps returning errors
The ESP chip returns timeout, but keeps trying to connect, so we want to
keep track of time ourselves, instead of relying on the chip's timeout.
This fixes failing WIFI-CONNECT-SECURE-FAIL test.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:esp8266_connect_failures branch from 7472b19 to 3e70df7 Apr 16, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 17, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit 69b9f3f into ARMmbed:master Apr 18, 2019

26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 9287 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
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.