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

Issues with both documentation and implementation of WiFi.status() in ESP8266WiFi library #7005

Closed
thierer opened this issue Jan 9, 2020 · 5 comments
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.

Comments

@thierer
Copy link

thierer commented Jan 9, 2020

Platform

  • Hardware: Wemos D1 mini
  • Core Version: 2.6.3 (3d128e5)

Problem Description

The docs for WiFi.status() state

Function returns one of the following connection statuses:

WL_CONNECTED after successful connection is established
WL_NO_SSID_AVAIL in case configured SSID cannot be reached
WL_CONNECT_FAILED if password is incorrect
WL_IDLE_STATUS when Wi-Fi is in process of changing between statuses
WL_DISCONNECTED if module is not configured in station mode

The code just does a mapping from the return values of wifi_station_get_connect_status() to Arduino constants:

wl_status_t ESP8266WiFiSTAClass::status() {
    station_status_t status = wifi_station_get_connect_status();

    switch(status) {
        case STATION_GOT_IP:
            return WL_CONNECTED;
        case STATION_NO_AP_FOUND:
            return WL_NO_SSID_AVAIL;
        case STATION_CONNECT_FAIL:
        case STATION_WRONG_PASSWORD:
            return WL_CONNECT_FAILED;
        case STATION_IDLE:
            return WL_IDLE_STATUS;
        default:
            return WL_DISCONNECTED;
    }
}

IMHO the documentation and the code have the following issues:

  1. WL_NO_SSID_AVAIL and WL_CONNECT_FAILED

From what I experienced, both states only happen with setAutoReconnect(true). That's mostly an issue with the underlying esp8266 sdk, but I think the docs should at least state the fact. Also, looking at the code suggests that a wrong password isn't the only possible cause though I don't know under which circumstances wifi_station_get_connect_status() would return STATION_CONNECT_FAIL.

  1. WL_IDLE_STATUSand WL_DISCONNECTED

I think the two status values are mixed up. The implementation should probably reflect the semantics of the original Arduino library, quoted below:

  • WL_IDLE_STATUS: it is a temporary status assigned when WiFi.begin() is called and remains active until the number of attempts expires (resulting in WL_CONNECT_FAILED) or a connection is established (resulting in WL_CONNECTED);
  • WL_DISCONNECTED: assigned when disconnected from a network;

But as it is implemented now, it's excatly the other way round: WL_IDLE_STATUS is reported before and after WiFi.begin() resp. WiFi.disconnect() are called and WL_DISCONNECTED in the time between the two calls before a connection has been made:

Demo sketch

Output:

Status before WiFi.begin(): WL_IDLE_STATUS
Status while connecting: WL_DISCONNECTED
Status after connecting: WL_CONNECTED
Status after WiFi.disconnect(): WL_IDLE_STATUS
#include <ESP8266WiFi.h>

#define SSID "xxx"
#define PW "xxx"

const char *wiFiStatus() {
  switch (WiFi.status()) {
    case WL_IDLE_STATUS:
      return "WL_IDLE_STATUS";
    case WL_CONNECTED:
      return "WL_CONNECTED";
    case WL_DISCONNECTED:
      return "WL_DISCONNECTED";
    default:
      return "Unhandled value";
  }
}

void setup() {
  Serial.begin(115200);

  WiFi.persistent(false);
  WiFi.setAutoConnect(false);
  WiFi.disconnect();

  Serial.printf("Status before WiFi.begin(): %s\n", wiFiStatus());

  WiFi.begin(SSID, PW);

  while (WiFi.status() != WL_CONNECTED) {
    Serial.printf("Status while connecting: %s\n", wiFiStatus());
    delay(5000);
  }

  Serial.printf("Status after connecting: %s\n", wiFiStatus());

  WiFi.disconnect();

  Serial.printf("Status after WiFi.disconnect(): %s\n", wiFiStatus());
}

void loop() {}

I'd be happy to prepare a PR, but I'd like to discuss the prefered solution first:

  1. The obvious fix would be to swap the two symbols in the switch statement, but that would break backwards compatibilty with programs that rely on the current behaviour.

  2. At very least the docs should be updated: I have the impression that for WL_IDLE_STATUS and WL_DISCONNECTED they try to represent what the implementation would be if it matched the definition of the original library, so if 1. isn't an option, then the annotations for these two values should be swapped.

  3. The longer term goal should imho be to fix WiFi.status() to better represent the current state of the connection. That would include making WL_NO_SSID_AVAIL and WL_CONNECT_FAILED work with setAutoReconnect(false) and maybe add the currently unsupported state WL_CONNECTION_LOST. i won't promise anything :), but would such a PR be considered given that it would break backwards compatibility even more?

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 9, 2020

I think users mainly rely on WL_CONNECTED (at least that's what is in the examples).
Only CaptivePortalAdvanced.ino (WL_IDLE_STATUS, WL_NO_SSID_AVAIL) and interactive.ino do not.

I also think that we will allow some (minor) backward compatibility issues for release 3.0.0.

@thierer
Copy link
Author

thierer commented Jan 10, 2020

I also think that we will allow some (minor) backward compatibility issues for release 3.0.0.

So what's your suggestion based on that assessment?

  1. Only fix the docs now, small solution (swap WL_IDLE_STATUS and WL_DISCONNECTED) for 3.0.0 and full solution (rework WiFi.status()) never or
  2. small solution now and full solution for 3.0.0?

@devyte
Copy link
Collaborator

devyte commented Jan 28, 2020

@thierer we've discussed internally, and what you put forth here has merit. Like you pointed out, this impacts compatibility, so we keep status quo for now and any changes must be targeted for v3.
Our concensus was basically point 1 in your previous comment, but Id prefer to not change docs now either, and just do everything for v3. However, I'm intrigued about what you consider a rework of wifi. status? Can you make a pr with your idea, or at least a gist, so that we can better discuss it?

@devyte devyte added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Jan 28, 2020
@thierer
Copy link
Author

thierer commented Feb 2, 2020

Fair enough.

I don't have a PR, but IMHO the way to go is to not rely on wifi_station_get_connect_status() for WiFi.status() but instead track the status from WiFi.begin(), WiFi.disconnect() and an onStationModeDisconnected() handler to detect WL_NO_SSID_AVAIL and WL_CONNECT_FAILED.

(I suspect the reason why wifi_station_get_connect_status() only reports WL_NO_SSID_AVAIL and WL_CONNECT_FAILED with auto-reconnect enabled is that it then installs a callback for itself to trigger the reconnect and as a side-effect has a better understanding of the current connection status).

WIFI_EVENT_STAMODE_DISCONNECTED defines a lot of poorly documented disconnect reasons. So that would require some more research to get the mapping to wl_status_tright.

I'll see if I find time to come up with a PR.

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 31, 2021

Closing as duplicate #7432, let's follow-up there !

@d-a-v d-a-v closed this as completed Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

No branches or pull requests

3 participants