Sorting of scanned wifi networks #661
Conversation
tyeth
force-pushed
the
wireless-rssi-sorting
branch
from
7d2aebe
to
f234c27
Compare
|
Works on Pico, which is what I was after as it was affecting my testing of other things. No known network sorted response details
And success (failing on connect attempt #0, but succeeding on # 1 aka 2nd attempt): Details
|
tyeth
changed the title
|
@brentru this is ready for review. I should retest on the other platforms, especially samd. |
There was a problem hiding this comment.
@tyeth I have a few review notes below and also some questions:
- Do we take the network with the highest RSSI?
- If we fail to connect to the defined network, do we try again with the next network in our list of networks?
- What is the purpose of sorting by RSSI if we are not going to take the network with the largest RSSI or if its not defined?
| /****************************************************************/ | ||
| struct WiFiNetwork { | ||
| char ssid[33]; /*!< SSID (Max 32 characters + null terminator */ | ||
| int rssi; /*!< Received Signal Strength Indicator */ |
There was a problem hiding this comment.
RSSI should be type int32_t to match the WiFi library
| @@ -119,22 +145,38 @@ class Wippersnapper_AIRLIFT : public Wippersnapper { | |||
| return false; | |||
| } | |||
|
|
|||
| // Was the network within secrets.json found? | |||
| // Dynamically allocate memory for the network list | |||
| std::vector<WiFiNetwork> networks(n); | |||
There was a problem hiding this comment.
I am not sure about using a vector/dynamic alloc. here, allowing too many possible networks will increase sort time.
Instead, could you define a global MAX_WIFI_NETWORK macro of 15 possible network options?
| */ | ||
| /****************************************************************/ | ||
| struct WiFiNetwork { | ||
| char ssid[33]; /*!< SSID (Max 32 characters + null terminator */ |
There was a problem hiding this comment.
Looks like SSID is a string in the WiFi library, so we should match the type
https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiScan.h#L48
| WS_DEBUG_PRINT("SSID found! RSSI: "); | ||
| WS_DEBUG_PRINTLN(WiFi.RSSI(i)); | ||
| strncpy(networks[i].ssid, WiFi.SSID(i), sizeof(networks[i].ssid) - 1); | ||
| networks[i].ssid[sizeof(networks[i].ssid) - 1] = |
There was a problem hiding this comment.
If you switch SSID to a String type, it will be null-terminated during compilation.
| @@ -26,6 +26,8 @@ | |||
| #include "SPI.h" | |||
| #include "WiFiNINA.h" | |||
| #include "Wippersnapper.h" | |||
| #include <algorithm> | |||
There was a problem hiding this comment.
Do we want this here, or moved to be included by WipperSnapper.h or another file (networking.h?) so it's included by all the drivers?
| std::sort(networks.begin(), networks.end(), compareByRSSI); | ||
|
|
||
| // Was the network within secrets.json found? | ||
| for (const auto &network : networks) { |
There was a problem hiding this comment.
Why are we using autotype assignment here? Instead of this overhead, we could iterate until the end of the array
Addresses #659