esp8266 nonblocking connect/disconnect #11409

dmaziec · 2019-09-04T09:58:17Z

Description

ESP8266Interface::connect() and ESP8266Interface::disconnect() can be used in terms of asynchronous operations. Mainly, it is based on newly added private variable of type esp_connection_software_status which is used to keep tracking of state of connection.
wifi_connect_nonblock test was renamed and amended to test both connect() and disconnect() operation

Pull request type

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

Reviewers

@SeppoTakalo
@michalpasztamobica
@VeijoPesonen
@teetak01

Release Notes

This release contains changes for ESP8266 non-blocking operations, which make them compliant with documentation:

connect() and disconnect() will now return immediately in non-blocking mode and pass all blocking operations to asynchronous thread.
in case of consecutive calls of connect() (or disconnect()) returned value from the second one may be NSAPI_ERROR_BUSY
If ESP8266Interface::set_credentials(...) is called during connecting to the network, it is not executed and NSAPI_ERROR_BUSY is returned.

The change will not affect any users, unless they took advantage of the incorrect non-blocking behavior.

ciarmcom · 2019-09-04T11:00:23Z

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

VeijoPesonen · 2019-09-05T06:22:49Z

TESTS/network/wifi/README.md

@@ -385,7 +385,7 @@ Test `WiFiInterface::connect()` without parameters. Use `set_credentials()` for

 `connect()` calls return `NSAPI_ERROR_OK`.

-### WIFI_CONNECT_NONBLOCK
+### WIFI_NONBLOCK


Maybe WIFI_CONNECT_DISCONNECT_NONBLOCK would be more descriptive.

VeijoPesonen · 2019-09-05T06:25:58Z

TESTS/network/wifi/README.md

-7.  `Call WiFiInterface::get_connection_status()`
-8.  `Call WiFiInterface::set_blocking(true)`
+7.  `disconnect()`
+8. `Call WiFiInterface::set_blocking(true)`

 **Expected result:**

 In case of drivers which do not support asynchronous mode `set_blocking(false)` call returns `NSAPI_ERROR_UNSUPPORTED` and skips test case, otherwise:


This could be converted to list for readability.

components/wifi/esp8266-driver/ESP8266Interface.h

+        IFACE_STATUS_DISCONNECTED = 0,
+        IFACE_STATUS_CONNECTING = 1,
+        IFACE_STATUS_CONNECTED = 2,
+        IFACE_STATUS_DISCONNECTING = 3


michalpasztamobica · 2019-09-09T05:47:19Z

@VeijoPesonen , your review remarks have been addressed :)

VeijoPesonen · 2019-09-09T05:57:57Z

TESTS/network/wifi/main.cpp

@@ -75,7 +75,7 @@ Case cases[] = {
    Case("WIFI-CONNECT-PARAMS-VALID-UNSECURE", wifi_connect_params_valid_unsecure),
    Case("WIFI-CONNECT", wifi_connect),
    //Most boards are not passing this test, but they should if they support non-blocking API.
-    //Case("WIFI-CONNECT-NONBLOCKING", wifi_connect_nonblock),
+    //Case("WIFI_CONNECT_DISCONNECT_NONBLOCK", wifi_nonblock),


The function name should also be changed.

VeijoPesonen · 2019-09-09T06:32:30Z

components/wifi/esp8266-driver/ESP8266Interface.cpp

+            _connect_retval = NSAPI_ERROR_NO_CONNECTION;
+        }
+        // Power down the modem
+        _rst_pin.rst_assert();


Having two separate pins to power down the modem is already confusing enough, so these two lines should be pushed on their own function.

Something like a private ESP8266Interface::_power_off()?
off seems to be the stronger version of down, so let's use it for a function that would do both actions.
Or should we be explicit with _power_down_and_off()?

I really don't see the semantic difference between those two - so _power_off() looks good to me. Function's comment section could include the explanation what is the difference between these two methods.

VeijoPesonen · 2019-09-09T06:50:06Z

components/wifi/esp8266-driver/ESP8266Interface.cpp

-    nsapi_error_t status = _conn_status_to_error();
-    if (status == NSAPI_ERROR_NO_CONNECTION) {
-        return NSAPI_ERROR_NO_CONNECTION;
+    if (!_if_blocking) {


Is there a reason to acquire the lock so late? Earlier it was done immediatelly at the beginning of this function.

Good point. The mutex should protect the queue - we need to move the lock up, before line 432.

I think it should be acquired already before first check of _software_conn_stat as it is modified from elsewhere. That's at least my gut feeling.

Hmm, we haven't been protecting _conn_stat with a mutex so far, I think _software_conn_stat could be handled similarly?
I was looking into the documentation but I see no clear indication if ESP8266Interface or NetworkInterface are meant to be thread-safe or not. If so - then we would probably need to protect even further than we do right now and I would suggest to create a separate task and take care of this outside of this PR.
For example: we'd have to protect the set_credentials() and get_ip_address().
I also wonder if those functions should or shouldn't become properly non-blocking in case set_blocking(false) gets called? Somehow the documentation only mentioned connect() and disconnect()...
What do you think @VeijoPesonen ?

I was looking into the documentation but I see no clear indication if ESP8266Interface or NetworkInterface are meant to be thread-safe or not. If so - then we would probably need to protect even further than we do right now and I would suggest to create a separate task and take care of this outside of this PR.

A fair point and I would say we don't need to go so far. What I was thinking here was the interaction between disconnect_async() and disconnect() when same the caller calls disconnet() twice in a row.

We've agreed to put the mutex lock after the _softwatre_conn_stat checks, as the result will be the same (return of NSAPI_ERROR_BUSY either due to failed lock or _software_conn_stat value check) and we don't protect that variable elsewhere in the code... Is this ok?

Sounds good.

VeijoPesonen · 2019-09-09T08:48:04Z

components/wifi/esp8266-driver/ESP8266Interface.cpp

+        if (_conn_stat_cb) {
+            _conn_stat_cb(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, _conn_stat);
+        }
+        _software_conn_stat = IFACE_STATUS_DISCONNECTED;


Should this be updated before executing the callback? Better to have the internal state in order before calling any external code.

VeijoPesonen · 2019-09-10T07:15:59Z

components/wifi/esp8266-driver/ESP8266Interface.h

@@ -403,6 +412,11 @@ class ESP8266Interface : public NetworkStack, public WiFiInterface {
        mbed::DigitalOut  _pwr_pin;
    } _pwr_pin;

+    /** Assert the reset and power pins
+     *    ESP8266 has two pins serving similar purpose and this function asserts them both if they are configured in mbed_app.json.


Indentation. Lline length shouldn't exceed 120 characters [Docs › Contributing › Style]. Otherwise this PR looks good to me :)

You're right, I guess astyle config is taking advantage of that preferably keyword :D

Each line preferably has at most 120 characters.

I guess so too :D As we know opinions start to differ once you go beyond 80 characters.

`ESP8266Interface::connect()` and `ESP8266Interface::disconnect()` can be used in terms of asynchronous operations. Mainly, it is based on newly added private variable of type `esp_connection_software_status` which is used to keep tracking of state of connection.`wifi_connect_nonblock` test was renamed and amended to test both `connect()` and `disconnect()` operation

VeijoPesonen

Looks good to me :)

0xc0170 · 2019-09-11T13:21:18Z

Ci started

mbed-ci · 2019-09-11T14:35:08Z

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR

michalpasztamobica · 2019-09-11T14:43:22Z

@0xc0170 , I checked the failures, but I cannot see a relation to those changes... Is there some issue in CI now?

0xc0170 · 2019-09-12T05:50:11Z

CI restarted

mbed-ci · 2019-09-12T06:46:56Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR

0xc0170 · 2019-09-12T16:02:31Z

CI restarted

mbed-ci · 2019-09-12T17:17:26Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR

0xc0170 · 2019-09-13T10:32:59Z

test restarted

mbed-ci · 2019-09-13T12:36:12Z

Test run: SUCCESS

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

0xc0170 · 2019-10-16T05:55:35Z

@adbridge Why was this moved to 6.0?

dmaziec force-pushed the esp8266-nonblocking branch 2 times, most recently from 2620bbe to 98ba282 Compare September 4, 2019 10:53

ciarmcom requested review from michalpasztamobica, SeppoTakalo, teetak01, VeijoPesonen and a team September 4, 2019 11:00

ciarmcom added the needs: review label Sep 4, 2019

dmaziec force-pushed the esp8266-nonblocking branch from 98ba282 to ee453a6 Compare September 4, 2019 11:16

VeijoPesonen reviewed Sep 5, 2019

View reviewed changes

components/wifi/esp8266-driver/ESP8266Interface.h

IFACE_STATUS_DISCONNECTED = 0,

IFACE_STATUS_CONNECTING = 1,

IFACE_STATUS_CONNECTED = 2,

IFACE_STATUS_DISCONNECTING = 3

This comment was marked as off-topic.

Sign in to view

dmaziec force-pushed the esp8266-nonblocking branch from ee453a6 to 31bdab7 Compare September 5, 2019 09:15

VeijoPesonen reviewed Sep 9, 2019

View reviewed changes

dmaziec force-pushed the esp8266-nonblocking branch from 31bdab7 to 2726b3f Compare September 9, 2019 12:22

VeijoPesonen reviewed Sep 10, 2019

View reviewed changes

dmaziec force-pushed the esp8266-nonblocking branch from 2726b3f to f5ccbe7 Compare September 10, 2019 07:38

michalpasztamobica approved these changes Sep 10, 2019

View reviewed changes

VeijoPesonen approved these changes Sep 10, 2019

View reviewed changes

0xc0170 approved these changes Sep 10, 2019

View reviewed changes

SeppoTakalo approved these changes Sep 11, 2019

View reviewed changes

0xc0170 removed the needs: review label Sep 11, 2019

0xc0170 added the needs: CI label Sep 11, 2019

michalpasztamobica mentioned this pull request Sep 12, 2019

Component: Add qca400x wifi #11466

Closed

0xc0170 added ready for merge and removed needs: CI labels Sep 13, 2019

0xc0170 merged commit ac7b851 into ARMmbed:master Sep 13, 2019

0xc0170 added release-version: 5.15.0-rc1 and removed ready for merge labels Sep 13, 2019

adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.15.0-rc1 labels Oct 3, 2019

0xc0170 mentioned this pull request Oct 16, 2019

Release candidate for mbed-os-5.14.1 #11689

Closed

adbridge mentioned this pull request Nov 18, 2019

WiFi: Re-order Greentea tests with minor fixes #11841

Merged

0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019

esp8266 nonblocking connect/disconnect #11409

esp8266 nonblocking connect/disconnect #11409

Uh oh!

Conversation

dmaziec commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Sep 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

michalpasztamobica commented Sep 9, 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!

VeijoPesonen Sep 9, 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!

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!

VeijoPesonen Sep 10, 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!

VeijoPesonen left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 11, 2019

Uh oh!

mbed-ci commented Sep 11, 2019

Uh oh!

michalpasztamobica commented Sep 11, 2019

Uh oh!

0xc0170 commented Sep 12, 2019

Uh oh!

mbed-ci commented Sep 12, 2019

Uh oh!

0xc0170 commented Sep 12, 2019

Uh oh!

mbed-ci commented Sep 12, 2019

Uh oh!

0xc0170 commented Sep 13, 2019

Uh oh!

mbed-ci commented Sep 13, 2019

Uh oh!

0xc0170 commented Oct 16, 2019

Uh oh!

Uh oh!

dmaziec commented Sep 4, 2019 •

edited

Loading

VeijoPesonen Sep 9, 2019 •

edited

Loading

VeijoPesonen Sep 10, 2019 •

edited

Loading