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

Add limit for returning the first 12 access points with high rssi #387

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabik111
Copy link

During the WiFi.scanNetworks() the access points found are stored on a vector container.

The allocation of memory for the vector doesn't grow linearly with the increasing of data stored, but logarithmically (view this ) and this can hang up the Arduino UNO R4 board for memory saturation when performing a scan inside an enviroment with many APs.

This PR limits the number of element stored inside the vector for returning only the APs with the highest RSSI.

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project and removed type: enhancement Proposed improvement labels Oct 23, 2024
@facchinm facchinm requested a review from maidnl November 5, 2024 13:30
Copy link
Contributor

@maidnl maidnl left a comment

Choose a reason for hiding this comment

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

@fabik111 just a little clarification about growth: anything that grows logarithmic-ally
grows less than linear.
That is impossible for any container: for 1 element (big as "space") the vector needs at least 1 space, for 2 elements at least 2 spaces, for 3 elements at least 3 spaces and
so on (this is a linear growth).
If the growth would be logarithmic-ally that would mean that for 100 elements less
than 100 spaces were required (which is obviously impossible).
What you mentioned in your PR said that "reallocations should only happen at
logarithmic-ally growing intervals of size" which has a completely different meaning
(although it is related to the fact that to store n elements more than n-space can be allocated, so the growth is more than linear and definitely more than logarithmic).

Your concern about saving memory is really appreciable, but is always
risky: what if you are in a environment with a lot of (good) Wifi Network available
and you have to connect with the 13-th RSSI strongest network? You cannot connect to the network. Did you run out memory due to the presence of so many Wifi Network?
Or is just a concern? That is something to think about with this kind of PR.

Last but most important your code does not work: my understanding is that you want to select the 12 strongest Wifi Networks, but your code does not achieve this result. I have tried it.
Here is the output of your algorithm (I limited the number of network to 4)
with some made up values:

adding AP with RSSI 384
adding AP with RSSI 887
adding AP with RSSI 778
adding AP with RSSI 916
adding AP with RSSI 2004
adding AP with RSSI 2005
adding AP with RSSI 2006
adding AP with RSSI 2007
adding AP with RSSI 2008
adding AP with RSSI 2009

ACCESS POINTS Selected: 
384
887
778
2009

Clearly not the best 4 RSSI are selected.

Pay attention to the fact that

std::vector<CAccessPoint>::reverse_iterator rit = access_points.rbegin();
access_points.insert(rit.base(), ap);

is equivalent to

access_points.push_back(ap);

just with a more convolute syntax.
You can verify that with something like:

vector<int> v;
vector<int> w;

for(int i = 0; i < 10 ; i++){
  auto rit = v.rbegin();
  v.insert(rit.base(), i);

  w.push_back(i);
}

for(int i = 0; i < 10 ; i++){
  cout << v[i] << " " << w[i] << endl;
}

At the end the 2 vectors (w,v) are equal.
So you are always working on the last elements of the vector, but this does not allow you to select the strongest n Wifi Networks.

My personal suggestion, if really you run out of memory, is to completely revise the strategy: perhaps the access_points vector might be erased when is no more needed
(perhaps when the board is connected to a Wifi network, but this is something that needs to be investigated in deep to see if there are not side effects).

Thank you

@fabik111
Copy link
Author

Hello @maidnl, thank you for your review.
I'm sorry I didn't express myself well in explaining the problem and I thank you for pointing out the error.

About the problem: this is not a concern, but actually what happens.
In my apartment (in a condominium) I run out of memory during a scan (the board freezes), more precisely, while inserting the APs in the vector.
Here the log:

15:17:10.442 -> Scanning
// All the print comes from the CWifi::scanNetworks() method
15:17:10.487 -> CWifi::scanNetworks() - before modem write
15:17:10.487 -> - SRAM left: 12115
15:17:17.177 -> CWifi::scanNetworks() - after modem write
15:17:17.177 -> - SRAM left: 8223
15:17:17.177 -> discovered APs:18
15:17:17.177 -> inserting AP:0
15:17:17.177 -> - SRAM left: 7247
15:17:17.177 -> inserted AP
15:17:17.177 -> - SRAM left: 7247
15:17:17.316 -> inserting AP:1
15:17:17.316 -> - SRAM left: 7247
15:17:17.316 -> inserted AP
15:17:17.316 -> - SRAM left: 6983
15:17:17.316 -> inserting AP:2
15:17:17.316 -> - SRAM left: 6983
15:17:17.316 -> inserted AP
15:17:17.441 -> - SRAM left: 6463
15:17:17.441 -> inserting AP:3
15:17:17.441 -> - SRAM left: 6463
15:17:17.441 -> inserted AP
15:17:17.441 -> - SRAM left: 6463
15:17:17.441 -> inserting AP:4
15:17:17.441 -> - SRAM left: 6463
15:17:17.565 -> inserted AP
15:17:17.565 -> - SRAM left: 5431
15:17:17.565 -> inserting AP:5
15:17:17.565 -> - SRAM left: 5431
15:17:17.565 -> inserted AP
15:17:17.565 -> - SRAM left: 5431
15:17:17.565 -> inserting AP:6
15:17:17.704 -> - SRAM left: 5431
15:17:17.704 -> inserted AP
15:17:17.704 -> - SRAM left: 5431
15:17:17.704 -> inserting AP:7
15:17:17.704 -> - SRAM left: 5431
15:17:17.704 -> inserted AP
15:17:17.704 -> - SRAM left: 5431
15:17:17.704 -> inserting AP:8
15:17:17.784 -> - SRAM left: 5431
15:17:17.784 -> inserted AP
15:17:17.784 -> - SRAM left: 3375
15:17:17.784 -> inserting AP:9
15:17:17.784 -> - SRAM left: 3375
15:17:17.784 -> inserted AP
15:17:17.784 -> - SRAM left: 3375
15:17:17.909 -> inserting AP:10
15:17:17.909 -> - SRAM left: 3375
15:17:17.909 -> inserted AP
15:17:17.909 -> - SRAM left: 3375
15:17:17.909 -> inserting AP:11
15:17:17.909 -> - SRAM left: 3375
15:17:17.909 -> inserted AP
15:17:18.034 -> - SRAM left: 3375
15:17:18.034 -> inserting AP:12
15:17:18.034 -> - SRAM left: 3375
15:17:18.034 -> inserted AP
15:17:18.034 -> - SRAM left: 3375
15:17:18.034 -> inserting AP:13
15:17:18.034 -> - SRAM left: 3375
15:17:18.204 -> inserted AP
15:17:18.204 -> - SRAM left: 3375
15:17:18.204 -> inserting AP:14
15:17:18.204 -> - SRAM left: 3375
15:17:18.204 -> inserted AP
15:17:18.204 -> - SRAM left: 3375
15:17:18.204 -> inserting AP:15
15:17:18.283 -> - SRAM left: 3375
15:17:18.283 -> inserted AP
15:17:18.283 -> - SRAM left: 3375
15:17:18.283 -> inserting AP:16
15:17:18.283 -> - SRAM left: 3375
// the function has never returned

For sure, as you mention, the solution proposed in the PR is not the best, maybe adding a new scanNetworks with an int argument for limiting the APs in list is better, what do you think?
I, also, tried my sketch with the MKR1010 without problems because, I believe, the list it's limited to 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants