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

Connection result feedback in STA #528

Closed
gepd opened this issue Feb 18, 2018 · 28 comments
Closed

Connection result feedback in STA #528

gepd opened this issue Feb 18, 2018 · 28 comments
Labels
Discussion Further Discussion ongoing enhancement Feature Request QA Quality Assurance testing needed
Milestone

Comments

@gepd
Copy link

gepd commented Feb 18, 2018

From #513

Requesting an option to display the reason why the connection wasn't able to be performed in the last try (STA mode). You will no always have access to read the debug output.

What could be the best way to implement this?
EEPROM, SPIFFS?

@tablatronix

Do you need to store it, or can it be a lifetime variable with a getter?

Can be a lifetime variable
I was thinking in storing it because how will you know the connection result after a possible reset?

@tablatronix
Copy link
Collaborator

True but I dont have the library doing resets anymore. That typically is not needed after 2.1.0 i think. Unless you mean uexpected resets?

@tablatronix tablatronix added enhancement Feature Request Discussion Further Discussion ongoing labels Feb 18, 2018
@tablatronix tablatronix added this to the dev milestone Feb 18, 2018
@gepd
Copy link
Author

gepd commented Feb 18, 2018

Yes, I mean assuming that it happens.
I don't know if it will ever happen or if storing it will be really useful. Today I just needed to know the reason why it wasn't connecting

tablatronix added a commit that referenced this issue Feb 18, 2018
@tablatronix
Copy link
Collaborator

tablatronix commented Feb 18, 2018

ok I added this easiest as possible, so let me know if its not specific enough.

caveats

  • I am saving all connection results, not just failures, it was easier
  • I am saving conx result for autoconnect AND wifisave
  • no wifi saved uses WL_NO_SSID_AVAIL as a flag, maybe disconnected would be better, or 255, not sure

If this is not satisfactory, I can change to getLastConxFailure, and add if(!WL_CONNECTED) conditions and only log failures, I wasn't sure if that was needed.

I made getWLStatusString() public also

also I am just using uint8_t instead of wl_status_t, just cause all the internals would have to be changed, and it probably doesn't matter

@gepd
Copy link
Author

gepd commented Feb 18, 2018

I was doing something similar, and I can say now there is no need to store the result value.

I'll check your implementation, in my tests I was getting the value from waitForConnectResult and then I shown it when I was in AP mode again.

I guess, know if the problem was the ssid or password will be enough in most of the cases.

@tablatronix
Copy link
Collaborator

also note
afaik STATION_WRONG_PASSWORD is not implemented in SDK last time I checked.
that is why
WL_CONNECT_FAILED is used for both STATION_CONNECT_FAIL and STATION_WRONG_PASSWORD

@gepd
Copy link
Author

gepd commented Feb 19, 2018

What do you think to add this value in HTTP_STATUS_OFF? But only when SSID or password is wrong

@tablatronix
Copy link
Collaborator

Not sure I understand

@gepd
Copy link
Author

gepd commented Feb 19, 2018

I mean, at this moment you have a message info showing the state of the network.

When it's not connected (HTTP_STATUS_OFF), it only shows **Not Connected** to networkname.
It would be more useful to display

Not Connected to networkname
SSID not valid / Wrong Password

But only if WL_NO_SSID_AVAIL or WL_CONNECT_FAILED

@tablatronix
Copy link
Collaborator

hmm, good idea

@tablatronix
Copy link
Collaborator

It looks like esp32 doesnt give much info on failure, just disconnected, ill keep looking but i didnt even see any useful events in the event system

@gepd
Copy link
Author

gepd commented Feb 20, 2018

I've tested your last changes and WL_NO_SSID_AVAIL works as expected, with a wrong password I only get WL_CONNECT_FAILED

Is the WL_STATION_WRONG_PASSWORD flag recently introduced by espressif?

@tablatronix
Copy link
Collaborator

In esp8266 it has always been there but not working. There is no equiv in esp32, there is an auth fail, but i only see it triggered when ap doesnt exist not on password falure, in fact i see no events for wrong password, just the default which is disconnected.

@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

In step 4.2, the Wi-Fi connection may fail because, for example, the password is wrong, the AP is not found, etc. In a case like this, <SYSTEM_EVENT_STA_DISCONNECTED> will arise and the reason for such a failure will be provided. For handling events that disrupt Wi-Fi connection, please refer to phase 6.

I tried debugging the reasoncodes and got nothing

@gepd
Copy link
Author

gepd commented Feb 20, 2018

I got it, the event needs an extra argument to get the reason:

#include <WiFi.h>

void WiFiEventHandler(WiFiEvent_t event, system_event_info_t info)
{
  Serial.printf("Got Event: %d\n", event);

  switch(event) {
    case SYSTEM_EVENT_STA_DISCONNECTED:
        Serial.printf("Reason: %d\n", info.disconnected.reason);
        break;
    }
}

void setup()
{
  Serial.begin(115200);
  
  WiFi.begin();
  WiFi.onEvent(WiFiEventHandler);
  while (WiFi.status() != WL_CONNECTED) {
    Serial.print(".");
    delay(500);
  }
  Serial.println();
}

void loop()
{
}

@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

I just commited that, lol

The question now is how the fuck do you use this to do anything since its a static member function pointer..

I also spent most of my time debugging this damn thing, since nothing was making sense in debug logs
espressif/arduino-esp32#1128

Earlier today , I got no meaning ful codes, today on another dev machine and network I was able to test all of this, and got logical reason codes.

@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

oddly sometimes I get no events at all, which seems odd, even if the router was not compatible or adhoc, I would think it would fire something.

[D][WiFiGeneric.cpp:293] _eventCallback(): Event: 2 - STA_START
2
*WM: wifi station disconnect
*WM: Connecting to saved AP
*WM: connectTimeout not set, ESP waitForConnectResult...
??????
*WM: Connection result: WL_DISCONNECTED
*WM: lastconxresult: WL_DISCONNECTED
*WM: AccessPoint set password is VALID
*WM: 12345678
*WM: wifi station disconnect
*WM: Disabling STA
*WM: Enabling AP
*WM: StartAP with SSID:  ESP32_94678576
[D][WiFiGeneric.cpp:293] _eventCallback(): Event: 3 - STA_STOP

tablatronix added a commit that referenced this issue Feb 20, 2018
using a static var for esp32 event for now to get this working and test. Probably not the best, not sure what the best option is here for esp32 onevent static member function callbacks
@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

ok I just hacked it in using a static uint8_t _lastconxresulttmp; and copying it into _lastconxresulttmp as needed. Should work for now.

@tablatronix
Copy link
Collaborator

sometimes I get this instead

          // [D][WiFiGeneric.cpp:298] _eventCallback(): Reason: 2 - AUTH_EXPIRE
          // *WM: Connection result: WL_DISCONNECTED          

not sure if this counts as wrong_password or not, probably not

@gepd
Copy link
Author

gepd commented Feb 20, 2018

I think it does. When I set a wrong pass I always get:

*WM: connectTimeout not set, ESP waitForConnectResult...
*WM: EVENT: WIFI_REASON: AUTH_EXPIRE
*WM: EVENT: WIFI_REASON: AUTH_FAIL
*WM: Connection result: WL_CONNECT_FAILED

@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

But this is WL_DISCONNECTED not WL_CONNECT_FAILED

hmm

It seem to be a uncaught event, if i set the password correct it connects, so it is a auth failure, not sure why the wl status is wrong

@tablatronix
Copy link
Collaborator

oh ok i see now

        } else if(reason == WIFI_REASON_AUTH_EXPIRE) {
            if(WiFi.getAutoReconnect()){
                WiFi.begin();
            }

This is changing the event status to disconnect, because it is auto reconnecting and losing the event somehow

@tablatronix
Copy link
Collaborator

I am trying to propose a fix for this, but I cannot figure out why this only runs 3 times, and not in a loop.
if WIFI_REASON_AUTH_EXPIRE status never gets updated, and there is no other event to stop the loop, so why does it only run 3 times ?

[D][WiFiGeneric.cpp:293] _eventCallback(): Event: 5 - STA_DISCONNECTED
[W][WiFiGeneric.cpp:298] _eventCallback(): Reason: 2 - AUTH_EXPIRE
WM: EVENT: WIFI_REASON: 2
[D][WiFiGeneric.cpp:293] _eventCallback(): Event: 5 - STA_DISCONNECTED
[W][WiFiGeneric.cpp:298] _eventCallback(): Reason: 2 - AUTH_EXPIRE
WM: EVENT: WIFI_REASON: 2
[D][WiFiGeneric.cpp:293] _eventCallback(): Event: 5 - STA_DISCONNECTED
[W][WiFiGeneric.cpp:298] _eventCallback(): Reason: 2 - AUTH_EXPIRE
WM: EVENT: WIFI_REASON: 2
*WM: Connection result: WL_DISCONNECTED

@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

ohhh duh, i never actually looked at this code

waitforconnectresult is the only thing preventing infinite loops in the event handler.. ugh
while((!status() || status() >= WL_DISCONNECTED) && i++ < 100) {

@tablatronix
Copy link
Collaborator

Yeah so this actually runs forever, unless you change the mode which we do. hmm

@tablatronix
Copy link
Collaborator

well anyways, auto_expire could be auth took too long and other reasons, so it is probably not a good idea to tell users wrong password unless we know it for certain

@tablatronix
Copy link
Collaborator

@tablatronix
Copy link
Collaborator

tablatronix commented Feb 20, 2018

I suppose we could change wrong password to "Auth Failure" to cover more bases

@gepd
Copy link
Author

gepd commented Feb 20, 2018

Yes, It may be better.
I tried the latest changes and all seems to be working well

@tablatronix tablatronix added the QA Quality Assurance testing needed label Feb 21, 2018
isots-code added a commit to isots-code/WiFiManager that referenced this issue May 31, 2018
* update library.properties

* clean up parameters

* oops

* clean up .h file

* fix tabs

* fix up parameters

* fixup debugging

* fixes paramid issues

paramid must be alphanumeric and adds {I} token support to use ids as input names if we make that optional.

* debugging clean

* oops

* adds exit to readme

* moved http strings to include, added token flags

* typo

* cleanup

* moved all html strings out of code

* more string tokens

* fix paramscount, oops

* doh...

* tzapu#454 adds WIFI_MANAGER_OVERRIDE_STRINGS

I dont feel like resolving conflicts

* fixup merge

* ajax test

* Bug fix for problem discovered when setting length to 1 for additional parameters

The initialization of the buffer holding the value stopped before the last character and storage did not take into account the extra-byte for end of string.

* debugsoftapconfig for esp32

* make debug funcs public

* fix esp32 examples

* fix esp32 wifi.ssid

* Update README.md

Esp32 warning

* camelcase _staShowStaticFields

* readme

* adds webclient checking for timeout delay

* remove reset, no reason for it

* converting token strings, in progress broken

* all tokens now in strings

* implemented info strings for esp32

* fix some html bugs

* change uptime

* infohelp align html

* added webserver precompiler warning

updates readme

* readme

* template

* Lower RAM usage of library

Place remaining debug messages into Flash using F().

* Update WiFiManager.cpp

* Update WiFiManager.cpp

* Update WiFiManager.cpp

* Create contributing.md

* Create ISSUE_TEMPLATE.md

* rename

* rename

* fix esp32 esp8266 bug

* moved strings to flash

* refactor ip form outputs

* adds disconnect method for users

non persistent disconnect

* publicized debug helpers

* ugh aliases

* moved all tokens to strings.h

* adds toggle for scan quality as percentage

`setScanDispPerc` until we have decent remplating

* oops

* defaulting to icons

* cleanup

* some more strings in progmem

* fix sketch size

* editing strings

* more strings cleanup

* refix memsketch

* more string cleanup

* left in debug

* comments

* typo

* dup define

* moved _wifissidprefix to progmem

* esp32 strings progmem

* strings

* file headers

* missed one file header

* Update ISSUE_TEMPLATE.md

* Update ISSUE_TEMPLATE.md

* documenting source

* change wifiscan detaisl to css for toggling

* more strings progmem

* oops broke lock icons

* bugged

* tzapu#528 adds getLastConxResult

* gitignore

no gitignore? add more stuff

* fix info page and adds esp32 aphostname

* testing tzapu#527

* fixes tzapu#527

* fix tzapu#527 for esp8266

* adds getWiFiIsSaved()

* doc blocks, reformatting code

* stability tzapu#527

* clean up

* remove debugging

* add result checking tzapu#527

* remove debugging

* change some uneeded ints

* feature tzapu#533 failure details

* allow empty ssids, bail earlier

* template update

* debuggin esp32 auth fail events

* tzapu#528

using a static var for esp32 event for now to get this working and test. Probably not the best, not sure what the best option is here for esp32 onevent static member function callbacks

* tzapu#528 WL_DISCONNECTED support for testing

* tzapu#534

* change webserver pointer to using

* remove warning

* fix some minor warnings

* adding menu control, in progress

* fix bug

* finish up menu customization

* fix template

* add param help

* fix up menus

* fix param crash

* fix menu and param save

* fix resetsettings

* adds mdns support

* change defs

* simplify debug

* comments

* fix menuids

* add wifiscan preloading with cache

This uses core scan object and assumes its lifetime, I am not sure what the scope is or how long it lives, will have to convert to local copy if its being lost, caching for 60s after startweb portal and 10s after root load, refresh bypasses cache

* fix order

* add preloadscan enable

* fix webserver pointer

* comments

* tzapu#542 handler

* tzapu#546 fix?

Since I do not know how to fix the delegated constructors properly, this should fix the issue for now

* oops tzapu#546

* adds erase

* add debugging

* remove dnsserver from readme

* adds nvs erase

// #define WM_ERASE_NVS // esp32 erase() will erase NVS

* oops

* adds opt erase flag

* fix savecallback tzapu#543

* fixup tzapu#548

* fix constructor

* move routes to strings

* oops fix constructor again

* remove preallocation of params memory

* init params as null

I started gettting odd exceptions, so I thought this should be null and not on the stack or pointing to garbage. I might be totally in the wrong here.

* fix customhtml parameters

* remove debug

* fixes tzapu#551

* disable ssid input autoformatting

autocorrect='off' autocapitalize='none'

* remove serial.prints

* fix up connection result for ondemand portal

* fix autoreconnect on auth failure

* notes

* fixes tzapu#555 oops

* added debug levels

* add esp shields

* Update README.md

* add WM_RTC def for rtc.h

* Added Custom DNS, fix newlines

* fix tabs

* comments

* oops

* remove testing

* minor layout fix to config

* add setShowDnsFields, adds edge case force hide ip fields

* adds section hrs

* showinfoerase toggle

* kludge setmenu with size so it works

Prboably going to add a vector for this, I do not want to add the overhead of std::array for something optional

* debug remove

* menuids as vector to deal with size

This kind of makes it a pain in the ass for users, not sure how to make this easier, I can take in an array of string ids, with a count arg, makes it easier, but then users can pass nonsense in, this way its enum ids, but damn it sure is convoluted. Welcome to ideas here...

```c++
  std::vector<WiFiManager::menu_page_t> menu = {wm.MENU_WIFI,wm.MENU_INFO,wm.MENU_PARAM,wm.MENU_CLOSE,wm.MENU_SEP,wm.MENU_ERASE,wm.MENU_EXIT};
  wm.setMenu(menu);
```

* adds handleClose

* add bug workaround for esp8266 softap

* oops

* fix debug blank

* travis testing

* change setmenu to take vector or array of strnig tokens

* travis testing

* change varnames

* fixing travis

* datatype fixes

* fix section compile error

* fix compiler warnings

* add esp32

* add esp32

* move menutokens

* add esp32

* add esp32

* fix debug levels

* ugh

* add esp32

* add esp32

* add esp32

* add esp32

* add esp32

* make travis build again

* Delete OnDemandConfigPortal.ino.cpp

Sigh

* fix fsparam spiffs examples for esp32

* spiffs case

* refactor connectwifi, adds save timeout

wifi save now no longer uses settimeout timeout

* fixes tzapu#573

* ugh remove test example

* fix parenths tzapu#573

* add setEnableConfigPortal(boolean enable) - if false dont open condig portal from autoConnect()

* getmodestr, handleerase(bool)

* fix handleerase add softapconfig catch fail

* fix nvs erase

* fix erroneous error debug

* adds chip revision efuse

not sure what use this is, as getChipRevision is usually the same anyway... shrug

* clean up defines, removed stuff for memory

* workaround for esp32 webserver parsearguments bug

* fixup scnanetworks

* fix some error logging

* fixes tzapu#581

adds htmlentities to ssid outputs, limited replacements for now

* comment

* fix savetimeout, too short, made optional

* fix menu output

removed test menu output, removed ref , not sure why I put that there....

* comments, debugging

* fixing up setmenu docblocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Further Discussion ongoing enhancement Feature Request QA Quality Assurance testing needed
Projects
None yet
Development

No branches or pull requests

2 participants