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
Esp32 WiFi AP+STA fix and WiFi scan support #312
Conversation
1. When operating as a STATION and SOFTAP mDNS should only be initialized once, the second start will cause a restart of the node. 2. Start of async SSID scan support. For SSID scan it is required to start the station interface, so for SoftAP only mode the code will set the interface mode AP+STA but only start the AP.
example VERBOSE output from scan:
An example of it's usage can be seen here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this API is fine; I have two general comments:
- we should not have APIs that leak
#include
s of the underlying host system. - we probably want to refactor the CC32xxWiFi and the ESP32WiFiManager to share the same APIs wherever possible.
2 is only possible if we keep to 1; the CC32xxWiFi was implemented according to the requirement of 1.
The wifi scan APIs are however mildly incompatible, because the 3220 does not have a notification when the scan is complete (which is a huge deficiency of the 3220 but we can't do anything to fix that).
I believe we should merge this now, and at some point in the future work on abstracting the wifi APIs, thereafter we can rework the ESP32WiFiManager as well.
return ssidScanResults_.size(); | ||
} | ||
|
||
wifi_ap_record_t Esp32WiFiManager::get_ssid_scan_result(size_t index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const wifiap_record_t& as return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry this was a wrong advice. it does need to be returned by value.
Yes, I have the old proposed API which I'll put on a branch and we can adjust and migrate in parallel to a common "generic" API.
Yes, the common API will remove the system types from the API and allow both sync/async APIs where possible. For systems which do not support an async API it can either raise an error or perform the action as sync, in the case of the scan the proposed API is:
In the case of the ESP32 the network_scan_start()/network_scan_stop() methods would likely return a generic error code as un-supported since there is no continuous scan support on the ESP32 that I have found (unless it is done via a background task that calls async scan repeatedly)
Yes, I don't think we need to hold this (outside of the above feedback items). The rework can be done in parallel to a new common API (PR coming shortly for discussion on the interface only) |
…rn of temp ref type)
This has a bug in the mDNS code which I've fixed in my CS copy of this. I'll be submitting an update to this branch after cleaning it up further. |
This is necessary when using STA+AP mode since the mDNS sytem will not start the UDP listener until at least one IP address has been assigned.
@balazsracz this is now up-to-date with latest tested changes from the CS side, can you do a review on this for anything that needs adjustment prior to merge? |
Set the mDNS initialized flag before publishing deferred service entries. Narrow the scope on mdnsInitLock_ in start_mdns_system.
size_t Esp32WiFiManager::get_ssid_scan_result_count() | ||
{ | ||
OSMutexLock l(&ssidScanResultsLock_); | ||
return ssidScanResults_.size(); | ||
} | ||
|
||
// Returns one SSID record from the last scan. | ||
const wifi_ap_record_t& Esp32WiFiManager::get_ssid_scan_result(size_t index) | ||
{ | ||
OSMutexLock l(&ssidScanResultsLock_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi this code is not correct, because the mutex is released but a reference to the storage area is returned to the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me send in an update PR with this changed to return by value.
@balazsracz @bakerstu feedback on SSID scan API, it is pretty raw currently and we talked about the possibility of a common API layer and I'd like to work towards that but this is a first pass using the IDF native object types.