Wifi scan process redesign

[teamplayer3]

Scanning for access points is completely blocking implemented. To make it more async compatible, I think the scan process should be divided into start scan and get scan results and listen for the wifi scan done event.

To make it memory save, I wrapped the process into a ScanProcess struct. This mimics the style of WifiWait.

A ScanProcess can be used as follows:

let result = ScanProcess::new(&sysloop, &mut wifi_driver).scan(&config);

New functions scan_start() and get_scan_result() are private and must be used through ScanProcess::scan().

While implementing these changes, I added the ScanConfig to configure the scan process.

I didn't change the currently available scan function on the driver. The new implemented functions can be fused with them.

I implemented a public set_mode() function to set the wifi mode manual. This should be automated, but for now, it's manual to configure for scanning. I thought of a state tracking by types as generics for the WifiDriver to switch from config state to started state.

I'm open for discussion.

[ivmarkov]

But ScanProcess::scan() is as blocking as it gets, so I'm kind of missing the point of your redesign? You end up with another way to do blocking scan?

[MabezDev]

I think this PRs intention is that ScanProcess::scan() replicates the current behavior but uses non-blocking methods under the hood, whilst also exposing these non-blocking methods publically.

Personally, I think it would be better to expose the scanning API via async and keep the current blocking api the same, and it's actually something we want for esp-wifi. (Sorry if I've missed these traits somewhere, but I looked through embedded-svc and didn't see any).

[teamplayer3]

I thought this could be a first step to async/await.

ScanProcess could be made async (only the Waitable must be changed) and under the hood it uses the same functions on the driver.

Or in general, what's the idea to make the whole esp wifi async?

[ivmarkov]

I think this PRs intention is that ScanProcess::scan() replicates the current behavior but uses non-blocking methods under the hood, whilst also exposing these non-blocking methods publically.

But that's the point: start_scan, stop_scan and get_scan_result (the last one BTW should have extra overrides - just like the current blocking scan variations - so that folks can use it without alloc too) are not public. :)

How the async story goes so far for everything esp-idf-svc related is as follows:

• No direct async methods on the API which lives in the non-asynch modules. If we want to expose any asynchronicity in the main modules, this is done via callbacks and event loops. Since EspWifi and friends are already tied to the system event loop, simply making start_scan, stop_scan and the alloc and no-alloc variations of get_scan_result public would be good enough. @teamplayer3 However, you need to ensure that using these APIs does not result in crashes of ESP IDF due to invalid state, etc. I.e., they must be safe, in the Rust sense.
• By the way, start, connect and disconnect are - in fact - "async" already per the above definition, because the fact that you called start does not necessarily mean, that the wifi had started. It might happen later, and you must listen the event loop or use some of the helpers that do it for you
• Inside the wifi::asynch module, you might use async and expose true async APIs, which are layered on top of listening the event loop etc. But this might be an optional next step

@teamplayer3 ^^^

[ivmarkov]

@zRedShift ^^^ pinging you as you also had a stake in the "async" scan, if I remember correctly.

[ivmarkov]

And ScanProcess probably needs to go. The current blocking scan which is in fact implemented inside ESP IDF is likely doing the same anyway.

[teamplayer3]

@ivmarkov my concern of exposing the start_scan, stop_scan and get_scan_result is that get_scan_result must be called after scanning to free the allocated memory. Or must this not be considered at this point.

@attention If this API is called, the found APs are stored in WiFi driver dynamic allocated memory and the will be freed in esp_wifi_scan_get_ap_records, so generally, call esp_wifi_scan_get_ap_records to cause the memory to be freed once the scan is done

[ivmarkov]

@ivmarkov my concern of exposing the start_scan, stop_scan and get_scan_result is that get_scan_result must be called after scanning to free the allocated memory. Or must this not be considered at this point.

@attention If this API is called, the found APs are stored in WiFi driver dynamic allocated memory and the will be freed in esp_wifi_scan_get_ap_records, so generally, call esp_wifi_scan_get_ap_records to cause the memory to be freed once the scan is done

As long as it does not crash, I don't think this is a problem. And this memory should be free-d when the wifi driver is dropped anyway.

[zRedShift]

I'm currently using a fork with some modifications and calls to esp-idf-sys to do async scanning, I would love to upstream it but I never have the time :/
Basically, what's missing is the sync implementation, and I think it could be done using ScanProcess in this PR.
Other than that, there are some intricacies vis-a-vis connection to an AP while having a scan would result in aborting the scan etc. that need to be dealt with gracefully.

[ivmarkov]

Basically, what's missing is the sync implementation, and I think it could be done using ScanProcess in this PR.

Don't want to sound stubborn, but the sync implementation is already there - scan & variants, and it is likely using - under the hood - what ScanProcess manually reimplements.

So @teamplayer3 if you can:

• make public start_scan, stop_scan, get_scan_result
• then make a few additional get_scan_result which are no-alloc (in the spirit of the scan* variants)
• generalize scan* to also take your new ScanConfig struct
• remove set_mode as it is dupe-ing set_configuration

... we might have a great start. Truly async-ifying the above is kind of trivial and probably an optional next step, as listening to Wifi events in an async way is already implemented of course.

[ivmarkov]

@teamplayer3 I must say, code looks quite OK!
I have only one concern: isn't it too early to expose the ScanConfig API in embedded-svc? If we do this, it means we have a relative certainty that:

• The ScanConfig configuration can be implemented/supported for different systems. I.e. esp32 bare-metal, but maybe also embedded Linux with wpa_supplicant and others
• Ditto for the scan_start/scan_stop

@MabezDev @zRedShift What do you think?

• If we would like to post-pone extending the Wifi trait API as of yet, then ScanConfig needs to be moved back to esp_idf_svc, and the scan method from the Wifi trait needs to call WifiDriver::scan with default scan configuration.

[ivmarkov]

usize was there for a reason: to indicate to you that you've fetched potentially less number of aps than found. I'm not sure we want to get rid of it.

Moreover, you've removed it from some signatures, but kept it in others. Why?

[teamplayer3]

Moreover, you've removed it from some signatures, but kept it in others. Why?

I didn't removed it from the trait signatures because I agree with you to don't change the public API at this point.

Ah the returned size should be the found count. Ok, makes sense. I change it back.

[MabezDev]

usize was there for a reason: to indicate to you that you've fetched potentially less number of aps than found. I'm not sure we want to get rid of it.

Doesn't heapless essentially encode this information already? len() being the number of results actually found, N or capacity() being the max you could search for?

[teamplayer3]

Doesn't heapless essentially encode this information already? len() being the number of results actually found, N or capacity() being the max you could search for?

It isn't restricted by N how many the esp searches. It searches always all (dynamically allocated on the heap intern). The usize gives how many are found. And the heapless len() how many are fetched. If N is larger than the amount of found aps len() is the same as usize. If N is smaller than the amount of found aps usize tells the user how many could be fetched.

I will test if it's possible to fetch the remaining aps if not all are fetched at first.

[ivmarkov]

Ditto.

[ivmarkov]

@teamplayer3 Let's proceed with my suggestion, and fold back the ScanConfig API back into esp-idf-svc for now.
The scan method of the Wifi trait would remain no-arg for now, and it would call the scan method of WifiDriver with default ScanConfig.