Juerd / ESP-WiFiSettings

WiFi Manager for the ESP32 Arduino environment
Other
166 stars 34 forks source link

Check writes on filesystem #10

Closed d-a-v closed 3 years ago

d-a-v commented 3 years ago

This proposal makes the webserver emit a temporary page indicating that an error happened during writes when this occurs.

also:

Juerd commented 3 years ago

Hi, thanks for taking the time to improve this library!

I definitely like the changes to the library itself, but I'm not a big fan of the changes to the example code. I'd prefer to keep the ESP8266 as a second-class citizen in this project; I'll tolerate it but it shouldn't mess with the primary goal of having nice code when we can have it. Besides that, I'm convinced that newbies will copy/paste from the examples, and thus have a preamble that suggests that the code will be portable across ESP8266 and ESP32, without taking the necessary precautions in the rest of their programs.

Juerd commented 3 years ago

I can't merge this PR because there are changes that I want and changes that I don't want within commits. Instead I'll just apply the changes manually.

(Another thing that I don't like is the #ifndef ESP8266 for the WIFI_AUTH_WPA2_ENTERPRISE. I'll agree that your version is technically better as it is more semantically correct, but I really, absolutely, dislike littering code with ifdefs and ifndefs. My condition for accepting the ESP8266 support contributed by @Pwuts into the project was that it could be mostly contained in the big definition block at the top of the file.

Juerd commented 3 years ago

How would you feel about the error message being final, instead of redirecting back to the configuration page? I think any FS error is going to persist anyway. Most likely it will be either missing code (like .begin()) or broken flash, neither of which can be fixed from the web interface.

Juerd commented 3 years ago

Manually merged (in part)

Juerd commented 3 years ago

Released, with changes, as v3.5.2

d-a-v commented 3 years ago

Thanks ! I'm fine with your review and changes.

... However there's still this warning:

.../ESP-WiFiSettings/WiFiSettings.cpp: In lambda function:
.../ESP-WiFiSettings/WiFiSettings.cpp:313:40: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  313 |             opt.replace("{1x}",   mode == WIFI_AUTH_WPA2_ENTERPRISE ? "(won't work: 802.1x is not supported)" : "");
      |                                        ^

which is solved by this non-invasive change:

--- a/WiFiSettings.cpp
+++ b/WiFiSettings.cpp
@@ -16,7 +16,7 @@
     #define esp_task_wdt_reset wdt_reset
     #define wifi_auth_mode_t uint8_t    // wl_enc_type
     #define WIFI_AUTH_OPEN ENC_TYPE_NONE
-    #define WIFI_AUTH_WPA2_ENTERPRISE -1337 // not available on ESP8266
+    constexpr auto WIFI_AUTH_WPA2_ENTERPRISE = -1337; // not available on ESP8266
     #define setHostname hostname
     #define INADDR_NONE IPAddress(0,0,0,0)