Wifi dpp fixes (IDFGH-9445) #10812
Wifi dpp fixes (IDFGH-9445) #10812
Conversation
|
|
| @@ -180,12 +180,12 @@ static int esp_dpp_handle_config_obj(struct dpp_authentication *auth, | |||
| wifi_config_t *wifi_cfg = &s_dpp_ctx.wifi_cfg; | |||
|
|
|||
| if (conf->ssid_len) { | |||
| os_memcpy(wifi_cfg->sta.ssid, conf->ssid, conf->ssid_len); | |||
| os_strlcpy((char*)wifi_cfg->sta.ssid, (char*)conf->ssid, sizeof(wifi_cfg->sta.ssid)); | |||
There was a problem hiding this comment.
I'd probably separate this stuff into two separate PRs. You have on one hand trying to fix ESP_ERR_DPP_TX_FAILURE and on the other hand trying to make it work to let you bootstrap and/or start_listen multiple times. I suspect the os_strlcpy fix is going to sale through more easily where as the MBO support is going to need a lot more careful thought to really understand what's going on there and why that would be relevant.
There was a problem hiding this comment.
Yeah, I still don't personally know why exactly MBO support seems to fix it as well as it does. And it doesn't help that once you get deep enough into wifi related stuff, there isn't much in the way of documentation or anything.
| } | ||
|
|
||
| if (dpp_akm_legacy(conf->akm)) { | ||
| if (conf->passphrase[0]) | ||
| os_memcpy(wifi_cfg->sta.password, conf->passphrase, | ||
| os_strlcpy((char*)wifi_cfg->sta.password, (char*)conf->passphrase, |
There was a problem hiding this comment.
This has a subtle bug in that if your first start_listen sequence yields a passphrase and your second one does not, you'll technically yield a wifi_config_t with a passphrase from the previous listen. I think the fix is overall more consistent if you wipe the wifi_cfg data structure on each call to start_listen. Same bug below with pmf_cfg.required = true.
There was a problem hiding this comment.
Actually there are other existing bugs in this code that suggest wiping s_dpp_ctx.wifi_cfg on each invocation of esp_dpp_handle_config_obj is better. The caller even runs this function in a loop which seems clearly broken to me...
There was a problem hiding this comment.
Ah, I must've missed this thanks to the two networks I tested with having the same passphrase, I'll have to look at wiping wifi_cfg. ty
|
Thanks for your contribution. |
| @@ -495,6 +495,7 @@ menu "Wi-Fi" | |||
| bool "Enable DPP support" | |||
| default n | |||
| select ESP_WIFI_MBEDTLS_CRYPTO | |||
| select ESP_WIFI_MBO_SUPPORT | |||
There was a problem hiding this comment.
We are trying to reproduce this issue and fix it. these are two separate features and should have no dependency on each other.
There was a problem hiding this comment.
I know they are very unrelated, however this change is the only one that's let dpp consistently work near perfectly, compared to barely working unless the listen channel was set to that of the ap and phone(dont know why its an issue since the qr has the channel + the esp sends on the correct channel anyways).
Without this fix, esp_wifi_action_tx_req would in most cases I tested, fail by sending the
WIFI_EVENT_ACTION_TX_STATUS event with a status=1, which in turn esp_dpp.c would send the ESP_ERR_DPP_TX_FAILURE event to the callback.
Without this fix, it would take making the environmental conditions near perfect for me to get a moderate success rate, but with this fix, even going out of my way I cant intentionally cause the WIFI_EVENT_ACTION_TX_STATUS related error.
There was a problem hiding this comment.
Fwiw on 4.4.4 I could not reproduce MBO having an effect. What helped for me most was fixing the fact that retry doesn't work as it is supposed to in the example code (s_dpp_stop_listening is not reset on start_listen), then just spamming retry on my phone. It is possible this MBO feature does something, but it's gonna take some tough work to figure out why. What isn't up for debate tho is that DPP is highly unreliable for more than just me, and we definitely need to dig in and understand why.
|
@gayafhannah checking in, are you planning to divide this PR so we can get the non-MBO fixes fast tracked? If not I might be able to tackle that as I'd really like to base the Rust DPP support on top of a fixed version of esp-idf. Honestly I'd think the severity of these bugs would warrant a new release in the 4.4.x branch given that this feature was mature in the 4.4.x series and 5.x is so new, though I'm not really sure what Espressif's policy is there. Also @kapilkedawat would be good to hear from you what your preferences are moving forward here. Even though we have some basic workarounds that make things work better, we still don't really have a fundamental understanding as to why it is so intermittent or why the tx failure occurs in the first place. I'm recommending we move forward with the obvious fixes (e.g. s_dpp_stop_listening is not cleared so even the example code never retries as it claims to), but punt to Espressif for more sleuthing on the root cause. I'm happy to help out there, but I don't have source code for the wifi driver and can't really tell why it would fail. |
|
Opened #10865 to tackle just the start_listen change |
|
Its just a coincidence that issue was not observed when MBO was enabled, MBO and DPP are independent features. therefore we cannot merge this PR. |
espressif-bot
added
Status: Done
Fix
ESP_ERR_DPP_TX_FAILUREerror caused by channel mismatch related issues(to the best of my knowledge) by settingESP_WIFI_MBO_SUPPORTbe a dependency ofESP_WIFI_DPP_SUPPORTThis has fixed all instances of the
ESP_ERR_DPP_TX_FAILUREerror and I have not been able to replicate the error with this fix yet.components/esp_wifi/Kconfigexamples/wifi/wifi-easy-connect/dpp-enrollee/sdkconfig.defaultsFix
esp_dpp_handle_config_objnot copying null terminator of ssid and password.Without this fix, I've quickly run into issues where unless I deinit and init DPP, subsequent attempts at configuring the network can lead to incorrect configs being sent to the callback.
An example of this is first using dpp to try connect to ExampleAP, but failing to connect due to signal reasons so you try connect it to SecondAP instead, without doing a deinit+init in-between, and as the null terminator of SecondAP isn't copied, on the second run you're still left with SecondAPP
components/wpa_supplicant/esp_supplicant/src/esp_dpp.cRelated Issue: #10615