-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add compile-time WiFi hard-off mode (WLED_FORCE_WIFI_OFF) #5394
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
base: main
Are you sure you want to change the base?
Changes from all commits
f8eb607
e3af1ad
83f45cd
f2e7d79
8913e53
1ad958f
700849b
4382e41
d14643b
ef88e49
0558bda
03cb56b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,58 @@ | |
|
|
||
| extern "C" void usePWMFixedNMI(); | ||
|
|
||
| namespace { | ||
|
|
||
| bool regularWiFiEnabled() | ||
| { | ||
| #ifdef WLED_FORCE_WIFI_OFF | ||
| return false; | ||
| #else | ||
| return true; | ||
| #endif | ||
| } | ||
|
Comment on lines
+19
to
+26
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you must expose this state as a variable, use a |
||
|
|
||
| void initRegularWiFiStartup() | ||
| { | ||
| if (!regularWiFiEnabled()) { | ||
| DEBUG_PRINTLN(F("WLED_FORCE_WIFI_OFF: disabling regular WiFi startup (AP button emergency remains available).")); | ||
| apBehavior = AP_BEHAVIOR_BUTTON_ONLY; | ||
| WiFi.mode(WIFI_OFF); | ||
| return; | ||
| } | ||
|
|
||
| WiFi.mode(WIFI_STA); // enable scanning | ||
| findWiFi(true); // start scanning for available WiFi-s | ||
| } | ||
|
Comment on lines
+28
to
+39
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for |
||
|
|
||
| bool handleForceWiFiOffConnection() | ||
| { | ||
| if (regularWiFiEnabled()) return false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call path is unnecessarily convoluted.
This needlessly obfuscates what's being done. Please instead #ifdef the call at the call site, so the function (a) do exactly what it says, and (b) clearly get called only when needed. |
||
|
|
||
| DEBUG_PRINTLN(F("WLED_FORCE_WIFI_OFF active. WiFi stays OFF unless AP emergency mode is opened by button.")); | ||
| apBehavior = AP_BEHAVIOR_BUTTON_ONLY; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not appropriate to mysteriously overwrite system config values in the main loop. (I'm not saying the code we have is clean: just that we shouldn't repeat existing mistakes.) For handling the configuration value, instead set an appropriate bootup default and dike out the reader sites that set it ( |
||
| lastReconnectAttempt = millis(); | ||
| if (!apActive) { | ||
| WiFi.disconnect(true); | ||
| WiFi.mode(WIFI_OFF); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| bool wifiScanRunning(bool wifiConfigured) | ||
| { | ||
| if (!regularWiFiEnabled()) return false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this check necessary? |
||
| return wifiConfigured && multiWiFi.size() > 1 && WiFi.scanComplete() < 0; | ||
| } | ||
|
Comment on lines
+55
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Treat only
Suggested fix bool wifiScanRunning(bool wifiConfigured)
{
if (!regularWiFiEnabled()) return false;
- return wifiConfigured && multiWiFi.size() > 1 && WiFi.scanComplete() < 0;
+ const int scanStatus = WiFi.scanComplete();
+ return wifiConfigured && multiWiFi.size() > 1 && scanStatus == WIFI_SCAN_RUNNING;
}🤖 Prompt for AI Agents |
||
|
|
||
| int findWiFiIfEnabled(bool doScan = false) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conditional in function name, again. |
||
| { | ||
| if (!regularWiFiEnabled()) return selectedWiFi; | ||
| return findWiFi(doScan); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| /* | ||
| * Main WLED class implementation. Mostly initialization and connection logic | ||
| */ | ||
|
|
@@ -523,8 +575,7 @@ void WLED::setup() | |
| WiFi.persistent(false); // on ESP8266 using NVM for wifi config has no benefit of faster connection | ||
| #endif | ||
| WiFi.onEvent(WiFiEvent); | ||
| WiFi.mode(WIFI_STA); // enable scanning | ||
| findWiFi(true); // start scanning for available WiFi-s | ||
| initRegularWiFiStartup(); | ||
|
|
||
| // all GPIOs are allocated at this point | ||
| serialCanRX = !PinManager::isPinAllocated(hardwareRX); // Serial RX pin (GPIO 3 on ESP32 and ESP8266) | ||
|
|
@@ -686,6 +737,8 @@ void WLED::initAP(bool resetAP) | |
| void WLED::initConnection() | ||
| { | ||
| DEBUG_PRINTF_P(PSTR("initConnection() called @ %lus.\n"), millis()/1000); | ||
| if (handleForceWiFiOffConnection()) return; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The websocket line below is important for Ethernet too. |
||
|
|
||
| #ifdef WLED_ENABLE_WEBSOCKETS | ||
| ws.onEvent(wsEvent); | ||
| #endif | ||
|
|
@@ -892,12 +945,13 @@ void WLED::handleConnection() | |
|
|
||
| // ignore connection handling if WiFi is configured and scan still running | ||
| // or within first 2s if WiFi is not configured or AP is always active | ||
| if ((wifiConfigured && multiWiFi.size() > 1 && WiFi.scanComplete() < 0) || (now < 2000 && (!wifiConfigured || apBehavior == AP_BEHAVIOR_ALWAYS))) | ||
| const bool wifiScanRunning = ::wifiScanRunning(wifiConfigured); | ||
| if (wifiScanRunning || (now < 2000 && (!wifiConfigured || apBehavior == AP_BEHAVIOR_ALWAYS))) | ||
| return; | ||
|
|
||
| if (lastReconnectAttempt == 0 || forceReconnect) { | ||
| DEBUG_PRINTF_P(PSTR("Initial connect or forced reconnect (@ %lus).\n"), nowS); | ||
| selectedWiFi = findWiFi(); // find strongest WiFi | ||
| selectedWiFi = findWiFiIfEnabled(); // find strongest WiFi | ||
| initConnection(); | ||
| interfacesInited = false; | ||
| forceReconnect = false; | ||
|
|
@@ -930,12 +984,12 @@ void WLED::handleConnection() | |
| if (interfacesInited) { | ||
| if (scanDone && multiWiFi.size() > 1) { | ||
| DEBUG_PRINTLN(F("WiFi scan initiated on disconnect.")); | ||
| findWiFi(true); // reinit scan | ||
| findWiFiIfEnabled(true); // reinit scan | ||
| scanDone = false; | ||
| return; // try to connect in next iteration | ||
| } | ||
| DEBUG_PRINTLN(F("Disconnected!")); | ||
| selectedWiFi = findWiFi(); | ||
| selectedWiFi = findWiFiIfEnabled(); | ||
| initConnection(); | ||
| interfacesInited = false; | ||
| scanDone = true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| #endif | ||
|
|
||
| // You can choose some of these features to disable: | ||
| //#define WLED_FORCE_WIFI_OFF // disable regular WiFi (STA/AP fallback). Requires a configured momentary button 0 for emergency AP long-press recovery. | ||
| //#define WLED_DISABLE_ALEXA // saves 11kb | ||
| //#define WLED_DISABLE_HUESYNC // saves 4kb | ||
| //#define WLED_DISABLE_INFRARED // saves 12kb, there is no pin left for this on ESP8266-01 | ||
|
|
@@ -291,6 +292,18 @@ WLED_GLOBAL char otaPass[33] _INIT(DEFAULT_OTA_PASS); | |
| #ifndef BTNTYPE | ||
| #define BTNTYPE BTN_TYPE_PUSH | ||
| #endif | ||
|
|
||
| #ifdef WLED_FORCE_WIFI_OFF | ||
| #ifndef WLED_USE_ETHERNET | ||
| #error "WLED_FORCE_WIFI_OFF requires WLED_USE_ETHERNET." | ||
| #endif | ||
| #if (BTNPIN < 0) | ||
| #warning "WLED_FORCE_WIFI_OFF: BTNPIN is disabled. Emergency AP long-press on button 0 will not be available unless configured in cfg.json." | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both button defines should be mendatory IMHO
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback. How do you feel about a change as outlined below? It moves the default definitions of BTNPIN and BTNTYPE below the checks for the WLED_FORCE_WIFI_OFF use case. This results in a hard enforcement that the button is explicitly configured by the user in the intended way. It also changes from warning to error in case it's missing. diff --git a/wled00/wled.h b/wled00/wled.h
index 207848d2671fda94e14b671d2d61fb30c7e178e8..7e29a4b231df4fe9baeb652fa4ee85c38a432b0f 100644
--- a/wled00/wled.h
+++ b/wled00/wled.h
@@ -263,69 +263,75 @@ using PSRAMDynamicJsonDocument = BasicJsonDocument<PSRAM_Allocator>;
#define _INIT(x)
#define _INIT_N(x)
#define _INIT_PROGMEM(x)
#else
#define WLED_GLOBAL
#define _INIT(x) = x
//needed to ignore commas in array definitions
#define UNPACK( ... ) __VA_ARGS__
#define _INIT_N(x) UNPACK x
#define _INIT_PROGMEM(x) PROGMEM = x
#endif
#define STRINGIFY(X) #X
#define TOSTRING(X) STRINGIFY(X)
#define WLED_CODENAME "Niji"
// AP and OTA default passwords (for maximum security change them!)
WLED_GLOBAL char apPass[65] _INIT(WLED_AP_PASS);
#ifdef WLED_OTA_PASS
WLED_GLOBAL char otaPass[33] _INIT(WLED_OTA_PASS);
#else
WLED_GLOBAL char otaPass[33] _INIT(DEFAULT_OTA_PASS);
#endif
-// Hardware and pin config
-#ifndef BTNPIN
- #define BTNPIN 0
-#endif
-#ifndef BTNTYPE
- #define BTNTYPE BTN_TYPE_PUSH
-#endif
-
#ifdef WLED_FORCE_WIFI_OFF
#ifndef WLED_USE_ETHERNET
#error "WLED_FORCE_WIFI_OFF requires WLED_USE_ETHERNET."
#endif
+ #ifndef BTNPIN
+ #error "WLED_FORCE_WIFI_OFF requires BTNPIN to be explicitly defined."
+ #endif
+ #ifndef BTNTYPE
+ #error "WLED_FORCE_WIFI_OFF requires BTNTYPE to be explicitly defined."
+ #endif
#if (BTNPIN < 0)
- #warning "WLED_FORCE_WIFI_OFF: BTNPIN is disabled. Emergency AP long-press on button 0 will not be available unless configured in cfg.json."
+ #error "WLED_FORCE_WIFI_OFF requires BTNPIN >= 0 so emergency AP long-press can be triggered on button 0."
#endif
#if !((BTNTYPE == BTN_TYPE_PUSH) || (BTNTYPE == BTN_TYPE_PUSH_ACT_HIGH) || (BTNTYPE == BTN_TYPE_TOUCH))
- #warning "WLED_FORCE_WIFI_OFF: BTNTYPE is not momentary by default. Emergency AP long-press on button 0 requires a momentary button type."
+ #error "WLED_FORCE_WIFI_OFF requires a momentary BTNTYPE (BTN_TYPE_PUSH / BTN_TYPE_PUSH_ACT_HIGH / BTN_TYPE_TOUCH)."
#endif
#endif
+
+// Hardware and pin config
+#ifndef BTNPIN
+ #define BTNPIN 0
+#endif
+#ifndef BTNTYPE
+ #define BTNTYPE BTN_TYPE_PUSH
+#endif
#ifndef RLYPIN
WLED_GLOBAL int8_t rlyPin _INIT(-1);
#else
WLED_GLOBAL int8_t rlyPin _INIT(RLYPIN);
#endif
//Relay mode (1 = active high, 0 = active low, flipped in cfg.json)
#ifndef RLYMDE
WLED_GLOBAL bool rlyMde _INIT(true);
#else
WLED_GLOBAL bool rlyMde _INIT(RLYMDE);
#endif
//Use open drain (floating pin) when relay should be off
#ifndef RLYODRAIN
WLED_GLOBAL bool rlyOpenDrain _INIT(false);
#else
WLED_GLOBAL bool rlyOpenDrain _INIT(RLYODRAIN);
#endif
#ifndef IRPIN
#define IRPIN -1
#endif
#ifndef IRTYPE
#define IRTYPE 0
#endif
#if defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S2) || (defined(RX) && defined(TX)) |
||
| #endif | ||
| #if !((BTNTYPE == BTN_TYPE_PUSH) || (BTNTYPE == BTN_TYPE_PUSH_ACT_HIGH) || (BTNTYPE == BTN_TYPE_TOUCH)) | ||
| #warning "WLED_FORCE_WIFI_OFF: BTNTYPE is not momentary by default. Emergency AP long-press on button 0 requires a momentary button type." | ||
| #endif | ||
| #endif | ||
| #ifndef RLYPIN | ||
| WLED_GLOBAL int8_t rlyPin _INIT(-1); | ||
| #else | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Factoring the common logic out to a function is a good idea, but conditionals in function names are an antipattern.
In a feature-toggle case like this, it's better to #ifdef gate the call sites instead. Blanking out function bodies makes it hard to reason about what's actually happening when reading at the call sites.