Skip to content

RainMaker - Improve sysProvEvent function #7932

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

Closed
wants to merge 10 commits into from
Closed

RainMaker - Improve sysProvEvent function #7932

wants to merge 10 commits into from

Conversation

FernandoGarcia
Copy link

Improve sysProvEvent function to prevent this type of error.

@FernandoGarcia FernandoGarcia changed the title Improve sysProvEvent function RainMaker - Improve sysProvEvent function Mar 5, 2023
Copy link
Contributor

@sanketwadekar sanketwadekar left a comment

Choose a reason for hiding this comment

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

Uploading actual logs while provisioning will help.

#endif
break;
case ARDUINO_EVENT_PROV_INIT:
wifi_prov_mgr_disable_auto_stop(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

This code was added to handle provisioning failures when the log level is set to "Debug". This shouldn't be removed.

switch (sys_event->event_id)
{
case ARDUINO_EVENT_WIFI_STA_GOT_IP:
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please look at the code format in the example and format it accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update other examples as well.

Copy link
Author

Choose a reason for hiding this comment

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

Please look at the code format in the example and format it accordingly.

Hi!

Could you add a lint with clang-format or similar to Github action to auto format according to Espressif style on PR?

Best regards.

wifi_prov_mgr_disable_auto_stop(10000);
break;
case ARDUINO_EVENT_PROV_CRED_SUCCESS:
wifi_prov_mgr_stop_provisioning();
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be removed

Add ARDUINO_EVENT_PROV_INIT and ARDUINO_EVENT_PROV_CRED_SUCCESS back
@VojtechBartoska VojtechBartoska added Area: Rainmaker Issue is related to ESP Rainmaker. Resolution: Awaiting response Waiting for response of author labels Mar 8, 2023
@FernandoGarcia
Copy link
Author

Hi @VojtechBartoska !

Which answer are you waiting for?

Best regards.

@VojtechBartoska VojtechBartoska added Status: Review needed Issue or PR is awaiting review and removed Resolution: Awaiting response Waiting for response of author labels Mar 13, 2023
@VojtechBartoska
Copy link
Contributor

Hello @FernandoGarcia , excuse me but I've missed your last commit with updates up to requested changes. We'll review your code soon.

@VojtechBartoska VojtechBartoska added this to the 2.0.8 milestone Mar 22, 2023
@sanketwadekar
Copy link
Contributor

This PR does provide more useful information to the user but does not help in solving the WiFi connection failure issue addressed here. Solving that issue would require changes in the Arduino WiFi library.

The WiFi disconnection error code 202 (AUTH_FAIL) is considered a non-reconnectable reason and only one reconnection attempt is made. Adding the reconnection code in the rainmaker sketch won't solve the issue.
One solution to this would be to try reconnection for all disconnection reasons.
Other would be to add this error code 202 to the list of reconnectable reasons. This still leaves us with reconnection issues with error codes not on the list.
@me-no-dev @VojtechBartoska @SuGlider @shahpiyushv what do you think about this?

@me-no-dev
Copy link
Member

@sanketwadekar AUTH_FAIL means that PASS was wrong. No? If so, then how is that reconnectable?

@sanketwadekar
Copy link
Contributor

sanketwadekar commented Mar 30, 2023

@sanketwadekar AUTH_FAIL means that PASS was wrong. No? If so, then how is that reconnectable?

You're right, it should not be reconnectable. However, @FernandoGarcia reported that the device did not connect to WiFi after a reboot. See comment here. I am not sure what could be the reason behind it.

@me-no-dev
Copy link
Member

@sanketwadekar if the ESP returns AUTH_FAIL and then later on connect with the same credentials, then the issue is somewhere else. If we patch this, it would fix the issue for @FernandoGarcia but it will break the general functionality for everyone else. I would rather have this resolved upstream in ESP-IDF and have it working correctly.

@shahpiyushv
Copy link
Contributor

@sanketwadekar if the ESP returns AUTH_FAIL and then later on connect with the same credentials, then the issue is somewhere else. If we patch this, it would fix the issue for @FernandoGarcia but it will break the general functionality for everyone else. I would rather have this resolved upstream in ESP-IDF and have it working correctly.

@me-no-dev , AUTH_FAIL is sometimes returned even when the signal strength is poor. The esp-idf examples' common component does not have any check for disconnect reason as you can see here

@FernandoGarcia
Copy link
Author

@me-no-dev , AUTH_FAIL is sometimes returned even when the signal strength is poor. The esp-idf examples' common component does not have any check for disconnect reason as you can see here

Hi!

It isn't the case because I was pretty close from router and my board has external antenna.

Best regards.

@shahpiyushv
Copy link
Contributor

It isn't the case because I was pretty close from router and my board has external antenna.

@FernandoGarcia What I mean here is that wrong credentials may not necessarily be the reason for AUTH_FAIL. It could be received in other cases too.

@me-no-dev
Copy link
Member

It isn't the case because I was pretty close from router and my board has external antenna.

@FernandoGarcia What I mean here is that wrong credentials may not necessarily be the reason for AUTH_FAIL. It could be received in other cases too.

But you will get it when credentials are wrong, so that would cause pointless reconnect on loop.

@me-no-dev
Copy link
Member

how about we address AUTH_FAIL issue separately. Seems unconnected to this PR. BTW @FernandoGarcia please fix the conflicts.

AUTH_FAIL: I propose a method to be added to WiFiSTA to control wether reconnects will be attempted on AUTH_FAIL. This way it can help the folks with that issue, while not causing pointless reconnects to the rest of the users.

@FernandoGarcia
Copy link
Author

BTW @FernandoGarcia please fix the conflicts.

Done!

@me-no-dev
Copy link
Member

@FernandoGarcia please fix the build errors so we can merge it :)

@me-no-dev
Copy link
Member

@shahpiyushv @sanketwadekar I am looking at our code and do not see a reason why WiFi would not reconnect on AUTH_FAIL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Rainmaker Issue is related to ESP Rainmaker. Status: Review needed Issue or PR is awaiting review
Projects
Development

Successfully merging this pull request may close these issues.

5 participants