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

esp_dpp: Fix retry with esp_supp_dpp_start_listen after failure (IDFGH-9508) #10865

Closed
wants to merge 1 commit into from

Conversation

jasta
Copy link
Contributor

@jasta jasta commented Feb 28, 2023

This fixes a subtle bug in which ESP_ERR_DPP_TX_FAILURE errors would call esp_supp_dpp_stop_listen which sets the s_dpp_stop_listening flag to true. Subsequent attempts to restart listening with esp_supp_dpp_start_listen then only attempt to listen once more for 500ms before reading the s_dpp_stop_listening flag again and giving up.

The lack of this fix contributes greatly to #10615, but I don't think this fix addresses the root cause. With this fix, it typically works but sometimes requires manually retrying it in the phone UI a couple of times. Without this fix, any number of retries by deinit/init again will seemingly not work as the retries seem to lose too much state or are not done quickly enough to realistically work around the issue, most likely due to a timing race condition somewhere I can't identify.

This fixes a subtle bug in which ESP_ERR_DPP_TX_FAILURE errors would
call esp_supp_dpp_stop_listen which sets the s_dpp_stop_listening flag
to true.  Subsequent attempts to restart listening with
esp_supp_dpp_start_listen then only attempt to listen once more for
500ms before reading the s_dpp_stop_listening flag again and giving up.

This contributes greatly to espressif#10615, but the fix here is still largely
a work-around as it sometimes requires manually retrying a couple times
before it works.  Without this fix, any number of retries by
deinit/init again will seemingly not work as the retries for currently
unknown reasons.
@CLAassistant
Copy link

CLAassistant commented Feb 28, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jasta
Copy link
Contributor Author

jasta commented Feb 28, 2023

In my opinion it actually makes more sense to silently keep retrying for a longer period than 500ms (the spec seems to suggest 5s is reasonable). Further, upon looking into the DPP protocol it seems that there is a new version 2 available and stable that addresses some of the channel misalignment issues we seem to be experiencing. wpa_supplicant on Android is running this version and is even advertising that it wants to use version 2, but wpa_supplicant even in v5.0.1 is using a very old version with only DPP1 support. I recommend upgrading wpa_supplicant as a way of fixing the issue properly, though I can't say for sure this will fix it ATM.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Feb 28, 2023
@github-actions github-actions bot changed the title esp_dpp: Fix retry with esp_supp_dpp_start_listen after failure esp_dpp: Fix retry with esp_supp_dpp_start_listen after failure (IDFGH-9508) Feb 28, 2023
@jasta
Copy link
Contributor Author

jasta commented Mar 8, 2023

For context, the dpp-enrollee sample tells us that we should call esp_supp_dpp_start_listen upon error, but it doesn't work for the reasons outlined in this issue.

espressif-bot pushed a commit that referenced this pull request May 20, 2023
This fixes a subtle bug in which ESP_ERR_DPP_TX_FAILURE errors would
call esp_supp_dpp_stop_listen which sets the s_dpp_stop_listening flag
to true.  Subsequent attempts to restart listening with
esp_supp_dpp_start_listen then only attempt to listen once more for
500ms before reading the s_dpp_stop_listening flag again and giving up.

This contributes greatly to #10615, but the fix here is still largely
a work-around as it sometimes requires manually retrying a couple times
before it works.  Without this fix, any number of retries by
deinit/init again will seemingly not work as the retries for currently
unknown reasons.

Signed-off-by: Shreyas Sheth <[email protected]>

Closes #10865
espressif-bot pushed a commit that referenced this pull request Jun 3, 2023
This fixes a subtle bug in which ESP_ERR_DPP_TX_FAILURE errors would
call esp_supp_dpp_stop_listen which sets the s_dpp_stop_listening flag
to true.  Subsequent attempts to restart listening with
esp_supp_dpp_start_listen then only attempt to listen once more for
500ms before reading the s_dpp_stop_listening flag again and giving up.

This contributes greatly to #10615, but the fix here is still largely
a work-around as it sometimes requires manually retrying a couple times
before it works.  Without this fix, any number of retries by
deinit/init again will seemingly not work as the retries for currently
unknown reasons.

Signed-off-by: Shreyas Sheth <[email protected]>

Closes #10865
espressif-bot pushed a commit that referenced this pull request Jun 14, 2023
This fixes a subtle bug in which ESP_ERR_DPP_TX_FAILURE errors would
call esp_supp_dpp_stop_listen which sets the s_dpp_stop_listening flag
to true.  Subsequent attempts to restart listening with
esp_supp_dpp_start_listen then only attempt to listen once more for
500ms before reading the s_dpp_stop_listening flag again and giving up.

This contributes greatly to #10615, but the fix here is still largely
a work-around as it sometimes requires manually retrying a couple times
before it works.  Without this fix, any number of retries by
deinit/init again will seemingly not work as the retries for currently
unknown reasons.

Signed-off-by: Shreyas Sheth <[email protected]>

Closes #10865
espressif-bot pushed a commit that referenced this pull request Jun 14, 2023
This fixes a subtle bug in which ESP_ERR_DPP_TX_FAILURE errors would
call esp_supp_dpp_stop_listen which sets the s_dpp_stop_listening flag
to true.  Subsequent attempts to restart listening with
esp_supp_dpp_start_listen then only attempt to listen once more for
500ms before reading the s_dpp_stop_listening flag again and giving up.

This contributes greatly to #10615, but the fix here is still largely
a work-around as it sometimes requires manually retrying a couple times
before it works.  Without this fix, any number of retries by
deinit/init again will seemingly not work as the retries for currently
unknown reasons.

Signed-off-by: Shreyas Sheth <[email protected]>

Closes #10865
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants