Skip to content

WifiMulti resilience updates #2775

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 13 commits into from
Closed

WifiMulti resilience updates #2775

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f8eaa42
Improvements to WiFiMulti resilience
Daemach May 13, 2019
d5b229c
Added example file for new options
Daemach May 13, 2019
0fab2f8
Modified to include connection testing at startup
Daemach May 14, 2019
fa7d1be
Merge remote-tracking branch 'upstream/master' into wifiMultiExpand
Daemach May 14, 2019
a406a3f
Changed strictmode function, default testPhrase
Daemach May 15, 2019
f2b2ff9
Stability fixes
Daemach Jun 6, 2019
3c5ecab
Merge remote-tracking branch 'upstream/master' into wifiMultiExpand
Daemach Jun 6, 2019
15af6ef
Added upstream
Daemach Jun 6, 2019
592079e
testConnection timeout fix
Daemach Jun 7, 2019
d0d4d17
Removed vscode folders
Daemach Jun 21, 2019
838b075
More vscode cleanup
Daemach Jun 21, 2019
c587b44
Merge remote-tracking branch 'upstream/master' into wifiMultiExpand
Daemach Jul 9, 2019
19ad37e
Rebase
Daemach Jul 10, 2019
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
45 changes: 28 additions & 17 deletions libraries/WiFi/src/WiFiMulti.cpp
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please merge the master branch and resolve the conflicts ?

Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,19 @@ uint8_t WiFiMulti::run(uint32_t connectTimeout)
int8_t scanResult;
uint8_t status = WiFi.status();
if(status == WL_CONNECTED) {
for(uint32_t x = 0; x < APlist.size(); x++) {
if(!_bStrict || WiFi.SSID()==APlist[x].ssid){
if (_bTestConnection && !_bWFMInit){
if (testConnection()) {
_bWFMInit = true;
return status;
}
} else if (!_bStrict) {
return status;
} else {
for(uint32_t x = 0; x < APlist.size(); x++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

since you don't need the index value this can be written as:

for(auto ap : APlist) {
  if(WiFi.SSID() == ap.ssid) {
    return status;
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

Much more elegant. I need to play with vectors more...

if(WiFi.SSID()==APlist[x].ssid){
return status;
}
}
}
WiFi.disconnect(false,false);
delay(10);
Expand All @@ -62,7 +71,7 @@ uint8_t WiFiMulti::run(uint32_t connectTimeout)
if(scanResult == WIFI_SCAN_RUNNING) {
// scan is running
return WL_NO_SSID_AVAIL;
} else if(scanResult >= 0) {
} else if (scanResult >= 0) {
// scan done analyze
int32_t bestIndex = 0;
WifiAPlist_t bestNetwork { NULL, NULL, NULL };
Expand Down Expand Up @@ -157,11 +166,12 @@ uint8_t WiFiMulti::run(uint32_t connectTimeout)

WiFi.begin(bestNetwork.ssid, (_bAllowOpenAP && bestNetworkSec == WIFI_AUTH_OPEN) ? NULL:bestNetwork.passphrase, bestChannel, bestBSSID);
status = WiFi.status();
_bWFMInit = true;

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);
delay(100);
status = WiFi.status();
}

Expand All @@ -175,14 +185,12 @@ uint8_t WiFiMulti::run(uint32_t connectTimeout)

if (_bTestConnection){
// We connected to an AP but if it's a captive portal we're not going anywhere. Test it.
if (testConnection(bestIndex)){
log_d("[WIFI] TestConnection returned true...");
if (testConnection()){
resetFails();
} else {
log_e("[WIFI] TestConnection returned false...");
markAsFailed(bestIndex);
WiFi.disconnect();
delay(500);
WiFi.disconnect(false,false);
delay(100);
status = WiFi.status();
}
} else {
Expand Down Expand Up @@ -250,7 +258,7 @@ void WiFiMulti::setTestURL(String testURL){
_testURL = testURL;
}

bool WiFiMulti::testConnection(int32_t i){
bool WiFiMulti::testConnection(){
Copy link
Member

Choose a reason for hiding this comment

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

why not add the URL as an argument (defaulting to NULL) and then parse it to get the port and so on?

Copy link
Author

Choose a reason for hiding this comment

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

I figured most people would just want to turn it on and forget it so I defaulted the test URL and phrase to amazon which I thought would be universal.

//parse url
int8_t split = _testURL.indexOf('/',7);
String host = _testURL.substring(7, split);
Expand All @@ -261,12 +269,10 @@ bool WiFiMulti::testConnection(int32_t i){
WiFiClient client;
const int httpPort = 80;
Copy link
Collaborator

Choose a reason for hiding this comment

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

port needs to be dynamically determined from the test url, it is not guaranteed that all servers will expose port 80 (most are moving to https as a default)

Copy link
Author

Choose a reason for hiding this comment

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

Hmm. Can I make an https request directly from wifiClient? I'm trying to keep it simple by not including more libraries than necessary. I'm assuming anyone using this function would set the test page manually. Still, if you feel this is important I'll modify it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

true, but if a user sets a url with https it would likely fail here as well

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. In my example I specify that it must be http://. Need to think it through a bit more.

if (!client.connect(host.c_str(), httpPort)) {
log_e("connection failed");
Serial.println("connection failed");
markAsFailed(i);
log_e("Connection failed");
return false;
} else {
log_i("connection to test host successful");
log_i("Connected to test host");
Copy link
Collaborator

@atanisoft atanisoft May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

at this point it should be possible to close the connection without reading the actual data since the point of this is to check that the connection to the internet is successful.

Copy link
Author

Choose a reason for hiding this comment

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

See above. I would rather skip the text check to make it simpler. Just need to make sure we don't get trapped. If this device gets onto xfinitywifi here it doesn't want to get off.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, ideally this would exit once it receives the initial http header details (all lines until a blank line IIRC). Once the header has been received the connection can be closed. I'd recommend read by line and check the line for known keywords (Location: XXX, HTTP XXX 200|302 etc...) That way you can abort earlier from the line by line checking.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @atanisoft here :) there are some codes that show all is good.

Copy link
Member

Choose a reason for hiding this comment

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

actually what is the target? which codes would mean that something went bad?

Copy link
Author

Choose a reason for hiding this comment

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

All I am trying to do is ensure that the device can actually reach the internet - captive portals are the problem. If you're connected to Wifi but behind a captive portal you're not going anywhere. This functionality is most useful when you have bAllowOpenAP, which automatically connects to any and all open APs in addition to the ones in your list just to get online. In my case, once it's online it connects to my server, grabs a config file with the correct SSID information then turns bAllowOpenAP off and bStrictMode on. It's good functionality to put 1000 brand new devices online just by plugging them in and setting your AP to open mode for 15 minutes.

I'm looking for anything but a 304 by default, but if that isn't good enough, you can set your own URL and look for any test phrase that you want, including text in a webpage. By default I don't want to look for 200's either in case a captive portal happens to return that. Looking for a permanent redirect seems like the best solution because a captive portal wouldn't do that and a site like google or amazon should be available everywhere.

}

// This will send the request to the server
Expand All @@ -277,17 +283,22 @@ bool WiFiMulti::testConnection(int32_t i){
while (client.available() == 0) {
if (millis() - timeout > 5000) {
log_e(">>>Client timeout!");
markAsFailed(i);
client.stop();
return false;
}
}

bool bSuccess = false;
// Read all the lines of the reply from server and print them to Serial
while(client.available()) {
// String line = client.readStringUntil('\r');
// Serial.print(line);
return client.find(_testPhrase.c_str());
bSuccess = client.find(_testPhrase.c_str());
if (bSuccess){
log_i("Success. Found test phrase");
} else {
log_e("Failed. Can't find test phrase");
}
return bSuccess;
}

}
Expand Down
5 changes: 3 additions & 2 deletions libraries/WiFi/src/WiFiMulti.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,14 @@ class WiFiMulti
bool _bStrict = true;
bool _bAllowOpenAP = false;
bool _bTestConnection = false;
String _testPhrase = "This is a very simple HTML file.";
String _testPhrase = "simple HTML file.";
String _testURL = "http://www.brainjar.com/java/host/test.html";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be better to default this to www.google.com or similar that returns a very small page. Checking the actual content is not super critical to test that the connection was successful to the remote endpoint, just that a connection to the remote was successful.

Copy link
Author

Choose a reason for hiding this comment

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

I was/am concerned that a captive portal would return 200 and fool me. This test is almost purely to skip captive portals. I test it with the xfinitywifi AP at the office, but it could be the Starbucks next door. Thoughts? I wonder if looking for a 302 header would be better...

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps checking for a 302 would work as well. The ultimate goal would be to avoid downloading the full response when it is not needed in most cases. Only the first few lines of the response will be needed to detect a redirect or if it is a general 200. Having client.find() will result in a lot of heap usage for the duration of the find and it may not even find the string.

Copy link
Author

Choose a reason for hiding this comment

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

will client.find() abort as soon as it finds the target string? 302 Found in this case

Copy link
Author

Choose a reason for hiding this comment

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

Make that 200 OK... I updated the h file.

bool _bWFMInit = false;

void markAsFailed(int32_t i);
void resetFails();

bool testConnection(int32_t i);
bool testConnection();
bool APlistAdd(const char* ssid, const char *passphrase = NULL);


Expand Down