Skip to content

Eliminate gigantic blocking time inside ESP8266WiFiMulti::run #4888

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Eliminate gigantic blocking time inside ESP8266WiFiMulti::run #4888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1b9491
ESP8266WiFiMulti: refactor ESP8266WiFiMulti::run in preparation for r…
braddr Jul 4, 2018
eb12b49
ESP8266WiFiMulti: change the connection loop to be async
braddr Jul 4, 2018
3ff102b
Merge branch 'master' into 2.4.1-braddr
d-a-v Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 105 additions & 78 deletions libraries/ESP8266WiFi/src/ESP8266WiFiMulti.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include <limits.h>
#include <string.h>

ESP8266WiFiMulti::ESP8266WiFiMulti() {
ESP8266WiFiMulti::ESP8266WiFiMulti() : connect_timeout_ms(0) {
}

ESP8266WiFiMulti::~ESP8266WiFiMulti() {
Expand All @@ -38,8 +38,104 @@ bool ESP8266WiFiMulti::addAP(const char* ssid, const char *passphrase) {
return APlistAdd(ssid, passphrase);
}

void ESP8266WiFiMulti::handleConnectComplete(wl_status_t status) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has no dependencies on other class members or methods, and is not public. Please make it a static free standing function.

#ifdef DEBUG_ESP_WIFI
IPAddress ip;
uint8_t * mac;
switch(status) {
case WL_CONNECTED:
ip = WiFi.localIP();
mac = WiFi.BSSID();
DEBUG_WIFI_MULTI("[WIFI] Connecting done.\n");
DEBUG_WIFI_MULTI("[WIFI] SSID: %s\n", WiFi.SSID().c_str());
DEBUG_WIFI_MULTI("[WIFI] IP: %d.%d.%d.%d\n", ip[0], ip[1], ip[2], ip[3]);
DEBUG_WIFI_MULTI("[WIFI] MAC: %02X:%02X:%02X:%02X:%02X:%02X\n", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
DEBUG_WIFI_MULTI("[WIFI] Channel: %d\n", WiFi.channel());
break;
case WL_NO_SSID_AVAIL:
DEBUG_WIFI_MULTI("[WIFI] Connecting Failed AP not found.\n");
break;
case WL_CONNECT_FAILED:
DEBUG_WIFI_MULTI("[WIFI] Connecting Failed.\n");
break;
default:
DEBUG_WIFI_MULTI("[WIFI] Connecting Failed (%d).\n", status);
break;
}
#else
(void)status;
#endif
}

struct BestNetwork {
WifiAPEntry Network;
int NetworkDb;
uint8 BSSID[6];
int32_t Channel;

BestNetwork() : Network({NULL, NULL}), NetworkDb(INT_MIN) {}
};

void ESP8266WiFiMulti::selectBest(BestNetwork& best, int8_t scanResult) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only external dependency of this function is on WiFi. It doesn't depend on any WiFiMulti method or member. Therefore, there is no point of having the function in the class as a method.
Please make it a static free-standing function in the .cpp.

for(int8_t i = 0; i < scanResult; ++i) {

String ssid_scan;
int32_t rssi_scan;
uint8_t sec_scan;
uint8_t* BSSID_scan;
int32_t chan_scan;
bool hidden_scan;

WiFi.getNetworkInfo(i, ssid_scan, sec_scan, rssi_scan, BSSID_scan, chan_scan, hidden_scan);

bool known = false;
for(auto entry : APlist) {
if(ssid_scan == entry.ssid) { // SSID match
known = true;
if(rssi_scan > best.NetworkDb) { // best network
if(sec_scan == ENC_TYPE_NONE || entry.passphrase) { // check for passphrase if not open wlan
best.NetworkDb = rssi_scan;
best.Channel = chan_scan;
best.Network = entry;
memcpy((void*) &best.BSSID, (void*) BSSID_scan, sizeof(best.BSSID));
}
}
break;
}
}

if(known) {
DEBUG_WIFI_MULTI(" ---> ");
} else {
DEBUG_WIFI_MULTI(" ");
}

DEBUG_WIFI_MULTI(" %d: [%d][%02X:%02X:%02X:%02X:%02X:%02X] %s (%d) %c\n", i, chan_scan, BSSID_scan[0], BSSID_scan[1], BSSID_scan[2], BSSID_scan[3], BSSID_scan[4], BSSID_scan[5], ssid_scan.c_str(), rssi_scan, (sec_scan == ENC_TYPE_NONE) ? ' ' : '*');
delay(0);
}
}

wl_status_t ESP8266WiFiMulti::connectLoop() {
wl_status_t status = WiFi.status();
// wait for connection, fail, or timeout
if (status == WL_CONNECTED ||
status == WL_NO_SSID_AVAIL ||
status == WL_CONNECT_FAILED ||
(millis() > connect_timeout_ms))
{
handleConnectComplete(status);
connect_timeout_ms = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constructor-inited member, yet it is cleared here. Will it never be used again? Is there a need to init it again?

}

return status;
}

wl_status_t ESP8266WiFiMulti::run(void) {

if (connect_timeout_ms != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout value is cleared above and never set again. Is that correct behavior?

return connectLoop();
}

wl_status_t status = WiFi.status();
if(status == WL_DISCONNECTED || status == WL_NO_SSID_AVAIL || status == WL_IDLE_STATUS || status == WL_CONNECT_FAILED) {

Expand All @@ -66,98 +162,29 @@ wl_status_t ESP8266WiFiMulti::run(void) {
}

if(scanResult > 0) {
static const uint32_t connectTimeout = 5000; // 5s timeout

// scan done, analyze
WifiAPEntry bestNetwork { NULL, NULL };
int bestNetworkDb = INT_MIN;
uint8 bestBSSID[6];
int32_t bestChannel;
BestNetwork best;

DEBUG_WIFI_MULTI("[WIFI] scan done\n");
delay(0);

DEBUG_WIFI_MULTI("[WIFI] %d networks found\n", scanResult);
for(int8_t i = 0; i < scanResult; ++i) {

String ssid_scan;
int32_t rssi_scan;
uint8_t sec_scan;
uint8_t* BSSID_scan;
int32_t chan_scan;
bool hidden_scan;

WiFi.getNetworkInfo(i, ssid_scan, sec_scan, rssi_scan, BSSID_scan, chan_scan, hidden_scan);

bool known = false;
for(auto entry : APlist) {
if(ssid_scan == entry.ssid) { // SSID match
known = true;
if(rssi_scan > bestNetworkDb) { // best network
if(sec_scan == ENC_TYPE_NONE || entry.passphrase) { // check for passphrase if not open wlan
bestNetworkDb = rssi_scan;
bestChannel = chan_scan;
bestNetwork = entry;
memcpy((void*) &bestBSSID, (void*) BSSID_scan, sizeof(bestBSSID));
}
}
break;
}
}

if(known) {
DEBUG_WIFI_MULTI(" ---> ");
} else {
DEBUG_WIFI_MULTI(" ");
}

DEBUG_WIFI_MULTI(" %d: [%d][%02X:%02X:%02X:%02X:%02X:%02X] %s (%d) %c\n", i, chan_scan, BSSID_scan[0], BSSID_scan[1], BSSID_scan[2], BSSID_scan[3], BSSID_scan[4], BSSID_scan[5], ssid_scan.c_str(), rssi_scan, (sec_scan == ENC_TYPE_NONE) ? ' ' : '*');
delay(0);
}
selectBest(best, scanResult);

// clean up ram
WiFi.scanDelete();

DEBUG_WIFI_MULTI("\n\n");
delay(0);

if(bestNetwork.ssid) {
DEBUG_WIFI_MULTI("[WIFI] Connecting BSSID: %02X:%02X:%02X:%02X:%02X:%02X SSID: %s Channel: %d (%d)\n", bestBSSID[0], bestBSSID[1], bestBSSID[2], bestBSSID[3], bestBSSID[4], bestBSSID[5], bestNetwork.ssid, bestChannel, bestNetworkDb);
if(best.Network.ssid) {
DEBUG_WIFI_MULTI("[WIFI] Connecting BSSID: %02X:%02X:%02X:%02X:%02X:%02X SSID: %s Channel: %d (%d)\n", best.BSSID[0], best.BSSID[1], best.BSSID[2], best.BSSID[3], best.BSSID[4], best.BSSID[5], best.Network.ssid, best.Channel, best.NetworkDb);

WiFi.begin(bestNetwork.ssid, bestNetwork.passphrase, bestChannel, bestBSSID);
status = WiFi.status();
WiFi.begin(best.Network.ssid, best.Network.passphrase, best.Channel, best.BSSID);
connect_timeout_ms = millis() + connectTimeout;

static const uint32_t connectTimeout = 5000; //5s timeout

auto startTime = millis();
// wait for connection, fail, or timeout
while(status != WL_CONNECTED && status != WL_NO_SSID_AVAIL && status != WL_CONNECT_FAILED && (millis() - startTime) <= connectTimeout) {
delay(10);
status = WiFi.status();
}

#ifdef DEBUG_ESP_WIFI
IPAddress ip;
uint8_t * mac;
switch(status) {
case WL_CONNECTED:
ip = WiFi.localIP();
mac = WiFi.BSSID();
DEBUG_WIFI_MULTI("[WIFI] Connecting done.\n");
DEBUG_WIFI_MULTI("[WIFI] SSID: %s\n", WiFi.SSID().c_str());
DEBUG_WIFI_MULTI("[WIFI] IP: %d.%d.%d.%d\n", ip[0], ip[1], ip[2], ip[3]);
DEBUG_WIFI_MULTI("[WIFI] MAC: %02X:%02X:%02X:%02X:%02X:%02X\n", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
DEBUG_WIFI_MULTI("[WIFI] Channel: %d\n", WiFi.channel());
break;
case WL_NO_SSID_AVAIL:
DEBUG_WIFI_MULTI("[WIFI] Connecting Failed AP not found.\n");
break;
case WL_CONNECT_FAILED:
DEBUG_WIFI_MULTI("[WIFI] Connecting Failed.\n");
break;
default:
DEBUG_WIFI_MULTI("[WIFI] Connecting Failed (%d).\n", status);
break;
}
#endif
} else {
DEBUG_WIFI_MULTI("[WIFI] no matching wifi found!\n");
}
Expand Down
8 changes: 8 additions & 0 deletions libraries/ESP8266WiFi/src/ESP8266WiFiMulti.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ struct WifiAPEntry {

typedef std::vector<WifiAPEntry> WifiAPlist;

struct BestNetwork;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If selectBest is removed from the class and made a static free standing function, there is no need to declare this here, because the definition in the .cpp is enough. Please remove.


class ESP8266WiFiMulti {
public:
ESP8266WiFiMulti();
Expand All @@ -58,6 +60,12 @@ class ESP8266WiFiMulti {

private:
WifiAPlist APlist;
unsigned long connect_timeout_ms;

void selectBest(BestNetwork& best, int8_t scanResult);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be removed from the class, see comment above.

void handleConnectComplete(wl_status_t status);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a debug method, and there is also no need for it to be part of the class, or exported in the .h. Please remove and make the function a static free standing function in the .cpp.

wl_status_t connectLoop();

bool APlistAdd(const char* ssid, const char *passphrase = NULL);
void APlistClean(void);

Expand Down