added Enable/Disable WiFi functionality #4670
Conversation
* added isEnabled() to Usermod base class * standardized mod_id parameter in UsermodManager (some were uint8_t, some were uint16_t)
disable wifi
WalkthroughA new boolean flag, Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
569-841: Consider adding explicit WiFi shutdown when disabled.While the current implementation correctly prevents WiFi operations when WiFi is disabled, it might be beneficial to explicitly turn off the WiFi hardware when
wifiEnabledis false to save power.Currently, the code skips operations but doesn't actively shut down WiFi if it was previously enabled before the flag was changed.
You could add code like this near the beginning of
handleConnection():void WLED::handleConnection() { static bool scanDone = true; static byte stacO = 0; const unsigned long now = millis(); const unsigned long nowS = now/1000; const bool wifiConfigured = WLED_WIFI_CONFIGURED; + // Ensure WiFi is completely turned off when disabled + if (!wifiEnabled && (WiFi.getMode() != WIFI_OFF)) { + DEBUG_PRINTLN(F("WiFi disabled, turning off radio")); + WiFi.mode(WIFI_OFF); + return; + } // 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))) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
package.json(1 hunks)wled00/button.cpp(1 hunks)wled00/cfg.cpp(2 hunks)wled00/const.h(1 hunks)wled00/data/settings_wifi.htm(2 hunks)wled00/fcn_declare.h(3 hunks)wled00/json.cpp(2 hunks)wled00/network.cpp(1 hunks)wled00/set.cpp(1 hunks)wled00/um_manager.cpp(1 hunks)wled00/wled.cpp(8 hunks)wled00/wled.h(2 hunks)wled00/xml.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
wled00/um_manager.cpp (2)
wled00/fcn_declare.h (4)
getUMData(454-454)data(419-419)data(419-419)isModEnabled(474-474)usermods/audioreactive/audio_reactive.cpp (2)
data(1438-1443)data(1438-1438)
wled00/wled.cpp (3)
wled00/wled.h (1)
resetAP(1068-1068)wled00/util.cpp (2)
prepareHostname(118-139)prepareHostname(118-118)wled00/udp.cpp (4)
espNowSentCB(945-947)espNowSentCB(945-945)espNowReceiveCB(950-1033)espNowReceiveCB(950-950)
wled00/fcn_declare.h (1)
usermods/mpu6050_imu/usermod_gyro_surge.h (1)
um_data(163-197)
🔇 Additional comments (34)
package.json (2)
28-29: Reordering dependencies in package.json.The order of dependencies in package.json doesn't affect functionality. This change simply reorders "nodemon" and "web-resource-inliner" alphabetically.
34-35: Added trailing newline.Adding a trailing newline at the end of JSON files is a standard best practice.
wled00/const.h (1)
213-213: Added new usermod ID for WiFi enable/disable feature.The new usermod ID is properly defined with a sequential ID (58) and references the implementation file "disable_wifi.h". This aligns with the PR objective of adding WiFi enable/disable functionality.
wled00/set.cpp (1)
23-23: Added WiFi enablement flag processing.This line captures the WiFi enabled state from the HTTP request parameter "EN" when handling WiFi settings updates. It follows the same pattern used for other WiFi settings in this function.
wled00/xml.cpp (2)
201-201: Added WiFi enabled checkbox to settings form.Properly adds the checkbox for enabling/disabling WiFi to the settings page, using the same pattern as other settings.
202-206: Added dynamic UI control for WiFi-related elements.These lines dynamically enable or disable WiFi-related UI controls based on the WiFi enabled status. This provides appropriate UI feedback when WiFi is disabled. The implementation targets:
- The scan button
- SSID input field
- Password input field
- BSSID input field
This ensures users can't attempt to configure WiFi settings when the feature is disabled.
wled00/cfg.cpp (2)
100-100: WiFi enabled flag integration in config deserializationThe new code correctly adds support for loading the wifiEnabled flag from configuration JSON. The bitwise OR operation (
|) preserves the current value if the key is missing in the JSON.
769-769: WiFi enabled flag integration in config serializationThis change properly adds the wifiEnabled flag to the WiFi configuration JSON object during serialization, completing the configuration persistence feature.
wled00/network.cpp (1)
322-322: Properly integrated WiFi enabled check in configuration verificationThe modification correctly adds the WiFi enabled state as a prerequisite for considering WiFi configured. This ensures WiFi operations won't be attempted when disabled.
wled00/data/settings_wifi.htm (2)
159-162: Added WiFi enable/disable UI control with restart noticeGood addition of the WiFi enable checkbox with a clear message that the change requires a device restart.
108-116: Well-implemented UI state management for WiFi controlsThe function effectively toggles the disabled state of relevant WiFi configuration controls based on the checkbox state. This is a good UX improvement that prevents users from configuring WiFi while it's disabled.
wled00/um_manager.cpp (3)
34-34: Expanded usermod ID range by changing parameter typeChanging
uint8_ttouint16_tformod_idexpands the range of supported usermod IDs from 256 to 65536, providing room for future growth.
36-36: Consistency improvement for mod_id comparisonUpdated comparison logic to match the wider
uint16_ttype. This change maintains consistency with the parameter type change.
41-47: New method to check if a usermod is enabledThis method properly implements checking if a specified usermod is enabled, which is essential for the WiFi enable/disable functionality. The implementation correctly returns the usermod's enabled state when found.
However, there's a potential issue: if the requested usermod is not found, the function returns false. Consider adding a comment to clarify this behavior or returning a more explicit result like
-1to distinguish between "not found" and "found but disabled".wled00/json.cpp (2)
480-486: Thorough implementation of WiFi-dependent AP mode logic.The change correctly integrates the new
wifiEnabledflag with AP mode activation logic. AP mode will only be started if currently inactive, explicitly requested, and WiFi is enabled. Similarly, AP mode will be stopped if it's active but either WiFi is disabled or AP mode is no longer requested.
727-727: Good addition of WiFi enabled state to JSON information.Adding the WiFi enabled state to the serialized info JSON output is appropriate and necessary for clients to properly reflect the current WiFi state.
wled00/fcn_declare.h (3)
433-433: Good default implementation for isEnabled() method.Adding this virtual method to the
Usermodbase class provides a clean extension point for implementing enable/disable functionality in usermod classes. The default implementation returningtrueensures backward compatibility with existing usermods.
454-454: Parameter type change for usermod ID.Changing the
mod_idparameter fromuint8_ttouint16_tallows for a larger range of usermod IDs. The comment correctly explains thatUSERMOD_ID_RESERVEDwill poll all usermods.
474-474: New function to check if a usermod is enabled.This function provides a way to check if a specific usermod is enabled, which is essential for the new WiFi enable/disable functionality.
wled00/wled.h (5)
348-348: Good addition of WiFi enable flag to WiFiOptions structure.The 1-bit flag for WiFi enabled state is memory-efficient and properly integrated into the WiFiOptions structure.
350-358: Updated constructor to initialize new WiFi enabled flag.The constructor properly initializes the new
wifiEnabledflag.
361-363: Consistent initialization of wifiEnabled to true by default.The flag is initialized to
truefor both ESP32 and other architectures, which is a good backward-compatible default.
371-371: Added access macro for the WiFi enabled flag.This macro provides a consistent way to access the WiFi enabled flag regardless of the underlying implementation.
383-383: Added global wifiEnabled variable for non-RAM-saving configurations.This complements the packed structure approach for configurations without
WLED_SAVE_RAM.wled00/button.cpp (2)
325-346: Changed button handling logic for button 0.The restructured code now has a clearer logic flow with nested if-else blocks:
Long press > WLED_LONG_AP (5 seconds):
- If press > WLED_LONG_FACTORY_RESET (10 seconds): performs factory reset
- Otherwise: starts AP mode with
initAP(true)Short press ≤ WLED_LONG_AP: sets
doReboot = trueThe major change here is that short presses now trigger a reboot rather than the previous behavior. This seems intentional as it fits with the PR description of long press to start AP mode.
Please confirm that the change in behavior for short press on button 0 (now triggering a reboot) is intentional.
334-341: Commented usermod code for WiFi management.There's commented code that seems to be related to enabling WiFi through a usermod. Since it's commented out, it has no functional impact, but it provides useful context about potential future integration with a dedicated WiFi management usermod.
wled00/wled.cpp (8)
569-570: Good implementation of WiFi enablement check in initAP().The code properly prevents Access Point initialization when WiFi is disabled or when AP behavior is set to button-only mode, which aligns with the PR's objective to control WiFi functionality.
572-574: Enhanced debugging for AP behavior.Adding debug output for the current AP behavior setting is helpful for troubleshooting WiFi connectivity issues.
637-639: Well-placed early return for disabled WiFi.This early return properly prevents all WiFi connection attempts when WiFi is disabled, ensuring the device respects the user's preference to disable WiFi functionality.
674-691: Properly gated ESP-NOW initialization.The ESP-NOW initialization is now correctly only performed when WiFi is enabled, while maintaining all the original functionality. The indentation changes make the code more readable.
765-765: Correct WiFi enablement check in handleConnection().Adding the WiFi enabled condition here prevents unnecessary reconnection attempts when WiFi is deliberately disabled.
796-796: Properly gated disconnection handling.This check ensures the system doesn't try to handle WiFi disconnections when WiFi is intentionally disabled, which is the correct behavior.
823-825: Improved debug information for AP startup.The enhanced debug message now includes the AP behavior setting, which is helpful for troubleshooting AP initialization issues.
841-841: Properly conditioned improv response handling.The code correctly prevents sending improv responses when WiFi is disabled, ensuring consistent behavior when WiFi functionality is turned off.
|
One thing that I forgot to note... I do not have an Ethernet board to test the USER_ETHERNET features. I think that I covered that area correctly, however, if someone with a board could test that Ethernet still works correct, I would greatly appreciate it. |
|
I'd also be interested in others' thoughts regarding the "turn WiFi off" comments from code rabbit. |
There was a problem hiding this comment.
I'm afraid I'll have to say no to this PR.
First, if you disable WiFi, how do you enable it again? Even AP mode on button press is disabled so there will be no way to enable it again. Or did I miss such behaviour?
Second, even though ESP-NOW uses WiFi circuitry it is not a WiFi protocol and should not be disabled. There is an option for enabling/disabling ESP-NOW in the settings.
Thirdly, there are unnecessary changes.
the AP still starts with a button0 longpress (see 1st line in initAP().
I did not realize that ESP-NOW was not a WiFi protocol. I'll remove those code changes. That was my mistake.
|
|
ESPNow and wifi hardware are tightly entangled, it is not possible to disable wifi and have ESPNow enabled, at least in arduino framework afaik. |
The setting initializes to Active, can be enabled via b0 longpress, ETH (if installed), the BLE usermod (once I finish it) OK, @blazoncek said in an earlier PR of mine that he felt that disable WiFi should be integrated, not a usermod. So, I'm not sure which direction to go there... I already ripped out the usermod code. |
|
I've made the code changes noted above. But, I still have to test the rebuild. I'll do that tonight and refresh the PR. |
|
My point is that it should not be possible to accidentally disable wifi and brick a setup |
Yes, I said that. @DedeHai the approach to include possibility to enable WiFi as a core feature is IMO acceptable and preferred. Also it is possible to disconnect STA and not enable AP and still use ESP-NOW. There is no need for WiFi stack to be enabled (apart from being initialised) to use ESP-NOW. However, @rhkean not every device has a button and it may be impossible to recover form accidentally setting WiFi to be disabled. Also, reporting that WiFi is disabled in JSON may be unnecessary as nobody will be able to get that info (as web server will be inaccessible). I would suggest to remove configuration items and only use JSON API to enable/disable WiFi. It can be scheduled as a boot preset to allow disabling WiFi on device start and may also allow to use IR (or other remote) to enable WiFi back on. My own experiments (while doing ESP-NOW code) showed that if you do: WiFi.disconnect();
WiFi.softAPdisconnect();WiFi will be disabled (it will not connect to configured SSID and it will not open AP). However it will still allow ESP-NOW to function if desired. There is one catch, though. WLED will periodically check if it is connected or not and will try to reconnect if WiFi is configured. You need to skip this check in |
|
I am not against making it a core feature if done right, was just thinking out loud. What I do want to avoid is many complaints about bricked setups |
|
I've made the changes noted above and improved the WiFi settings page by moving the disable setting to the bottom of the page and included a warning with required confirmation checkbox. |
|
Did not test yet but from reading the code the double-check to disable it seems acceptable. |
Can you point me to where the wifi-sleep is not respected? I can't find any of my modifications that impacted that. |
|
* insured wifiSleep is respected for ESPNow when WiFi is disabled
AH! good catch! thank you. fixed. |
There was a problem hiding this comment.
I'm still not satisfied with proposed changes and I still do not like the fact that WiFi enabled state is saved into configuration (I will never approve that, no matter what but other team members may vote otherwise and I will accept that).
WLED has W in its name which stands for "wireless" (or WiFi).
There are also some irrelevant changes that need to be reverted.
I've worked with WiFi logic for several months while preparing ESP-NOW sync and have to say that it is still not ready for prime time due to the fact that I want WiFi to remain available at all times.
Let's put it this way:
- if Ethernet is connected it will be used and WiFi will not be used (effectively disabled, no need for any change)
- if you do not configure WiFi and set AP mode to never start, WiFi will not be enabled (no need for any change)
- if you want to temporarily disable WiFi, add JSON handling (that's on my TODO list)
- if your use case is to have device run on battery and reduce power consumption create a boot preset (or a playlist) that will temporarily disable WiFi
|
fully agree with @blazoncek, if there were a JSON command to disable wifi instead, that would be a much more elegant solution. |
|
wow! Great feedback. Thank you so much. My use case is BLE. My intentions were to implement disabling WiFi, then implement BLE, then eventually upgrade the BLE module with COEXISTENCE so the WiFi did not have to be disabled. I'll make the changes you noted above, remove the config save and settings page changes, and add a JSON API call |
|
If you are trying to implement BLE (I assume you also developed BLE enabled mobile app), be prepared to have ESP with at least 8MB of flash as otherwise your image will be too big (except if you'll fiddle with partitions which I do not recommend). As for enabling and disabling WiFi (to prolong battery life for example) it would be sufficient (IMO) just to have JSON API (which can be triggered via various means like saved preset, HTTP, MQTT, IR, Wiz remote and UDP). It is true that you may only be able to disable WiFi via network methods, but playlists or IR or reboot will be able to enable it back on (or BLE command). The important trick is how to properly handle disabled WiFi in |
yes, next step after getting BLE working from WLED, I plan to modify the Android App to include BLE support
actually, using the IDF4 env that's included in the platformio.ini and the NimBLE-Arduino package (I need the newer g++ compiler in the IDF4 toolchain for NimBLE 2.x ... datatypes that aren't recognized by the g++ compiler that is included with the arduino toolchain), I have a build that is only about 1.35MB... don't even have to disable OTA. (granted at this point of only sent a few words back and forth, so.... we'll see. LOL ). That leaves me with about 200k to work with... fingers-crossed that it'll be enough
I haven't done much with the JSON API to date, so I didn't think of that option when I started modifying the code. That's the route I'll take this week. I'm assuming much of the valid parts of my PR would stay, and I only need to the triggering method to JSON API instead of Settings pages. my real goal would be to get COEXISTENCE working so the wifi and ble work at the same time, but... I'll focus on getting the communication working first... LOL
Thanks for the pointer. I'll take a look. |
|
OK.... made all of the changes mentioned above. I've tested by putting a I'll add the JSON API call next. |
|
For JSON API I'm thinking something in the lines of: if (!wifi[F("on")].isNull()) {
bool sta = getBoolVal(wifi[F("on")], wifiEnabled);
if (Network.isConnected() && !sta) WiFi.disconnect();
if (sta) forceReconnect = true;
wifiEnabled = sta;
}And simple logic in if (wifiConfigured && !wifiEnabled) return;This will allow firing up AP on boot if WiFi is not configured (assuming AP was not disabled). The logic behind the above is that you |
|
now that's an elegant solution... I'll code it up and start testing after work! (I've got to figure out how to use SerialJSON for now, bc I don't have an IR remote lying about) :/ |
|
Create two presets: |
Summary by CodeRabbit