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.

Sign up for GitHub

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

Wifi dpp fixes (IDFGH-9445) #10812

Closed
wants to merge 2 commits into from
Closed

Wifi dpp fixes (IDFGH-9445) #10812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f1e166
Tweaked Kconfig and esp_dpp.c
gayafhannah Feb 21, 2023
2eeef72
Accidentally used old config option in prev commit
gayafhannah Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/esp_wifi/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ menu "Wi-Fi"
bool "Enable DPP support"
default n
select ESP_WIFI_MBEDTLS_CRYPTO
select ESP_WIFI_MBO_SUPPORT
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are trying to reproduce this issue and fix it. these are two separate features and should have no dependency on each other.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

help
Select this option to enable WiFi Easy Connect Support.

Expand Down
4 changes: 2 additions & 2 deletions components/wpa_supplicant/esp_supplicant/src/esp_dpp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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

sizeof(wifi_cfg->sta.password));
if (conf->akm == DPP_AKM_PSK_SAE) {
wifi_cfg->sta.pmf_cfg.required = true;
Expand Down
1 change: 1 addition & 0 deletions examples/wifi/wifi_easy_connect/dpp-enrollee/sdkconfig.defaults
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
CONFIG_ESP_WIFI_MBO_SUPPORT=y
CONFIG_ESP_WIFI_DPP_SUPPORT=y