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

Abuse esp_mesh_scan_get_ap_record to reduce stack use of scan_results #224

Closed
wants to merge 1 commit into from

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Jul 29, 2023

Based on #221. I suspect relying on weird internal implementation details (i.e. that we can just call a mesh api function without actually using wifi mesh) might not be very elegant, but it works. We probably will need to flush the scan results still, I just wanted to upload the initial findings :)

It looks like C2 will need to remain on the non-mesh code.

My main worry is that any new binary blob may break this. Now, the binaries esp-wifi uses are in our hands, so it's possible to test for such breakage, but it is probably additional work on each blob update.

@bugadani bugadani marked this pull request as ready for review August 8, 2023 03:43
@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 11, 2023

Interesting but .... mhhhh - I'm not super convinced to be honest

The fact it doesn't work for all chips already doubles that. As you already said "My main worry is that any new binary blob may break this." - I agree we can handle that but it potentially adds additional trouble when updating the blobs (which sometimes is already enough trouble)

But the main thing is that it doesn't get us too much. With 10 records requested it looks like this

<esp_wifi::wifi::WifiController as embedded_svc::wifi::Wifi>::scan_n::hc2359ba1e996a506
code=1862 stack=Some(1504)

Given scan probably isn't used too much after wifi-provisioning (if at all) and the fact that there are much worse functions (I plan to work on that soon) I'm not sure if it's worth the potential trouble this might cause

@bugadani
Copy link
Contributor Author

bugadani commented Aug 11, 2023

Well I'm not fighting for this, though I really would like to see a non-heapless::Vec-bound "give me the results as an iterator" API eventually. This PR kind of reflects what I have in mind, if only we had a stable API from the wifi libs.

Oh well, one can hope :)

Also, I think this isn't too much trouble to keep rebased, so I might just do that in case someone wants it.

stack=Some(1504) is pretty substantial, given that we have 8kB of stack space defined in the .lds. This problem has been solved since writing this comment.

@bugadani bugadani force-pushed the meshscan branch 2 times, most recently from a17559d to 9a584ad Compare August 14, 2023 14:33
@bugadani bugadani force-pushed the meshscan branch 4 times, most recently from 2db70a2 to f3a69a4 Compare September 8, 2023 03:10
@bugadani
Copy link
Contributor Author

bugadani commented Oct 16, 2023

@MabezDev have you been able to find out anything about this? (i.e. if this function is planned to be further restricted or if the relevant team could make it instead more generally available for "normal wifi" and, e.g. for the C2?)

@MabezDev
Copy link
Member

Good news, with the latest WiFi blobs the API is available. The relevant pull request is yet to land in esp-idf, but the signature of the function is as follows:

esp_err_t esp_wifi_scan_get_ap_record(wifi_ap_record_t *ap_record);

The next time we update the blobs we can modify this PR and merge it :).

@MabezDev MabezDev mentioned this pull request Jan 8, 2024
@bugadani bugadani mentioned this pull request Jan 11, 2024
@MabezDev
Copy link
Member

The blobs have been updated and the API is now available.

@bugadani
Copy link
Contributor Author

Noted; I'm a bit swamped right now but if nobody jumps in I will get to this... eventually

@bjoernQ
Copy link
Contributor

bjoernQ commented May 27, 2024

Thanks! esp-wifi is moved into the esp-hal repository. If you want to continue working on this please consider re-opening the PR there

@bjoernQ bjoernQ closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants