Add compile-time WiFi hard-off mode (WLED_FORCE_WIFI_OFF)#5394
Add compile-time WiFi hard-off mode (WLED_FORCE_WIFI_OFF)#5394Stargazer2026 wants to merge 12 commits into
Conversation
…an-behavior-with-lan Add compile-time WiFi hard-off mode (WLED_FORCE_WIFI_OFF)
WalkthroughThis PR adds a WLED_FORCE_WIFI_OFF compile-time feature that hard-disables regular WiFi startup and scanning while preserving emergency AP recovery via button press. The feature requires Ethernet to be enabled and validates button configuration at build time. Runtime helpers gate startup, scans, and reconnect behavior when forced-off is active. ChangesWiFi-off mode feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
641-643: Consider adding a debug print for consistency with the other twoWLED_FORCE_WIFI_OFFblocks.
initAP()silently returns, whilesetup()(line 515) andinitConnection()(line 686) both emit aDEBUG_PRINTLNwhen forced off. This makes it harder to trace the suppression in debug logs.✨ Proposed addition
`#ifdef` WLED_FORCE_WIFI_OFF + DEBUG_PRINTLN(F("WLED_FORCE_WIFI_OFF: skipping AP init.")); return; `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.cpp` around lines 641 - 643, The initAP() function currently returns silently when WLED_FORCE_WIFI_OFF is defined; add a DEBUG_PRINTLN call consistent with the other blocks (setup() and initConnection()) so the suppression is visible in logs. Locate the WLED_FORCE_WIFI_OFF guard inside initAP() and insert a DEBUG_PRINTLN with a clear message (e.g., "WLED_FORCE_WIFI_OFF: AP init suppressed") immediately before the return so behavior matches the other WLED_FORCE_WIFI_OFF blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/wled.cpp`:
- Around line 641-643: The initAP() function currently returns silently when
WLED_FORCE_WIFI_OFF is defined; add a DEBUG_PRINTLN call consistent with the
other blocks (setup() and initConnection()) so the suppression is visible in
logs. Locate the WLED_FORCE_WIFI_OFF guard inside initAP() and insert a
DEBUG_PRINTLN with a clear message (e.g., "WLED_FORCE_WIFI_OFF: AP init
suppressed") immediately before the return so behavior matches the other
WLED_FORCE_WIFI_OFF blocks.
netmindz
added
connectivity
|
If we add this PR, then there should be a HUGE warning that "no wifi" means "no wifi connection at all"
|
|
(joke) maybe rename the build flag to WLED_FORCE_WIFI_OFF_AND_SHOOT_YOURSELF_IN_THE_FOOT 😉 I'm not saying NO to this PR - there might be special use cases where its helpful. But I would prefer a less "hard off" approach, maybe by adding a "wifi mode" user setting next to "AP mode", and keep at least "AP mode by button" operational for emergency cases. |
|
can you please elaborate on the advantages? |
|
Thank you for reviewing this. The intention of this PR is not to change default behavior. WiFi remains enabled unless explicitly excluded via a compile time flag. The primary motivation is deterministic Ethernet only deployments. In certain installations, especially fixed architectural or PoE based setups, it is desirable to completely exclude the WiFi stack at build time rather than simply not configuring it. This approach has several technical advantages:
The decision to implement this as a compile time exclusion instead of a runtime toggle was intentional. A runtime option would require:
By contrast, this implementation:
The feature is strictly opt in for specialized builds and does not affect existing users or standard firmware releases. If needed, I am happy to provide exact firmware size comparisons or adjust naming and placement of the build flag to better match existing conventions. |
|
The only point I am getting is the "radio restricted area" thing, but that is very niche. All other points can be addressed in a different way and the firmware size reduction is not really helpful unless you pack it full of usermods. |
|
Thanks for the clarification. I understand that this is a niche use case. I am not trying to position it as something broadly needed for typical installations. My main point is that the implementation cost for the project is extremely low:
This does not introduce additional behavioral complexity in the normal code path. If the flag is not set, the firmware behaves exactly as before. Regarding flexibility: yes, a recompile is required if the network topology changes. But since this is a compile-time exclusion, it is explicitly targeting fixed-purpose deployments where that trade-off is intentional. In other words, this is not meant as a user-facing feature but as a specialization hook for constrained or regulated environments. Given the very small and well-contained delta, I would argue that the long-term maintenance overhead should be close to zero, especially compared to runtime configuration features. If you still feel this increases project surface unnecessarily, I completely understand. I just wanted to clarify that the implementation was intentionally minimal and non-invasive. |
I do understand your intend and I can see its usefulness for some cases, however my concern is this: edit: edit2: |
| #endif | ||
|
|
||
| // You can choose some of these features to disable: | ||
| //#define WLED_FORCE_WIFI_OFF // saves 5kb, hard-disable WiFi radio entirely (WIFI_OFF). AP/STA will not start. |
There was a problem hiding this comment.
Its better to not add the flag here. It will be too tempting for "cool lets try this" users to brick their boards.
For documentation, i'd prefer a PR in WLED-Docs and describe the new flag as an expert option.
| #define CLIENT_PASS "Your_Password" | ||
| */ | ||
|
|
||
| //#define WLED_FORCE_WIFI_OFF // Hard disable WiFi radio (WIFI_OFF): no STA and no AP. |
There was a problem hiding this comment.
please remove the flag here, too (see my comment on wled.h)
|
A few use case questions that came to my mind:
|
|
@Stargazer2026 please don't get me wrong - I'm not trying to corner your PR. This is just an attempt to find out what could be the consequences of having WLED without wifi option. See it as a free "safety analysis", that can be used to improve the safety and availability in non-normal cases. Actually, this kind of analysis is part of my "day time job", so don't take my questions personally :-) |
|
I can completely understand that outside of the home setting, that to have devices that might run on ethernet 99% of the time, the suddenly swap to AP mode in an office, nightclub etc just because someone rebooted the router is a valid concern, so having the option to select ethernet only makes sense. I'm less clear on the use case of when you would use WLED in a setting where you couldn't (legally?) have an active radio and if this change would be robust enough to meet that compliance 100% - it's certainly a feature I wouldn't want the team to take responsibility for ensuring every release never activates the radio at all |
I agree, especially we know that many "non-expert admins" will simply use OTA to refresh their device with the latest software. Like "I trust HomeAssistent to pick the right update for me". After that point, the WLED device will start offering its WLED-AP against all "radio off" assumptions. As a counter-measure, removing OTA and ESP-NOW could work. But if hard "radio off" requirements need to be met, it also means to put the board into a hard-to-break enclosure with intruder alert, to prevent updating WLED by UART pins. |
|
First of all, thank you all for the detailed analysis! I genuinely appreciate the “safety review” perspective. These are exactly the right questions to ask :-) Let me clarify one important point: This feature is not intended as a regulatory compliance guarantee and should not be marketed or understood as “certified radio-off firmware”. It is simply a compile-time exclusion of the WiFi stack for controlled, fixed-purpose builds where the integrator fully owns the deployment. I completely agree that default WLED behavior must remain flexible, recoverable and safe for normal users. Given the concerns raised, I would propose tightening the scope further:
Regarding responsibility: From an implementation perspective, the delta remains small, isolated and fully opt-in. Default builds and OTA users remain completely unaffected. If the concern is mainly about inexperienced users experimenting with it, I am fully in favor of making access intentionally inconvenient (for example via forced acknowledgement in code), so it does not become a “cool let’s try this” toggle. My goal here is not to expand WLED’s general feature surface, but to provide a safe specialization hook for advanced deployments without affecting mainstream users. Happy to adjust the PR accordingly. |
|
Thanks for your response :-) I think "make access inconvenient" is the way to go 👍
What if we could have both? If we lower our expectations on "radio off", we could still allow "WLEP-AP by button" for dummy user recovery. The 5Kb firmware shrink is nothing we absolutely need, so keeping the AP code active but enforcing "AP_BEHAVIOR_BUTTON_ONLY" could be an option. Not saving 5Kb flash is really a small price, compared to a few "stuck users" who need help to get out of their "no Wi-Fi" problems, especially if the "inaccessible" board is sealed in epoxy and fixed to the roof ;-) |
|
Thanks again for the constructive feedback. I’ve reworked the implementation based on the discussion. Updated BehaviorWhen
In other words, WiFi is no longer part of normal network behavior, but intentional recovery via button-triggered AP is still possible. This addresses the main concern about “stuck” devices while still preventing unintended RF activity in Ethernet-first deployments. Compile-Time SafeguardsTo make this feature safer and clearly expert-only, I added the following safeguards:
This shifts prerequisite validation to compile time, where it is most useful for this type of feature. Scope ClarificationThis is not intended to provide regulatory “radio-off compliance”. Default firmware behavior remains completely unchanged. The implementation remains small and isolated:
If the naming should better reflect the soft-off semantics instead of “FORCE_WIFI_OFF”, I’m happy to adjust that as well. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
929-934: Avoid misleading scan log when scans are compiled out.Line 930 logs “scan initiated” even though
findWiFi(true)is omitted underWLED_FORCE_WIFI_OFF. Consider making the log conditional or updating the message.💡 Optional tweak
if (scanDone && multiWiFi.size() > 1) { - DEBUG_PRINTLN(F("WiFi scan initiated on disconnect.")); -#ifndef WLED_FORCE_WIFI_OFF - findWiFi(true); // reinit scan -#endif +#ifndef WLED_FORCE_WIFI_OFF + DEBUG_PRINTLN(F("WiFi scan initiated on disconnect.")); + findWiFi(true); // reinit scan +#else + DEBUG_PRINTLN(F("WiFi scan suppressed (WLED_FORCE_WIFI_OFF).")); +#endif scanDone = false; return; // try to connect in next iteration }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.cpp` around lines 929 - 934, The log message "WiFi scan initiated on disconnect." can be misleading when scans are compiled out under WLED_FORCE_WIFI_OFF; update the condition around the DEBUG_PRINTLN or change the message so it only appears when findWiFi(true) will actually be called. Concretely, move or wrap the DEBUG_PRINTLN(F("WiFi scan initiated on disconnect.")) so it is executed only when WLED_FORCE_WIFI_OFF is not defined (alongside the call to findWiFi(true)), or alter the text to say "WiFi scan skipped (scanning disabled) on disconnect" when WLED_FORCE_WIFI_OFF is defined; adjust the block that uses scanDone, multiWiFi, and findWiFi accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/wled.cpp`:
- Around line 929-934: The log message "WiFi scan initiated on disconnect." can
be misleading when scans are compiled out under WLED_FORCE_WIFI_OFF; update the
condition around the DEBUG_PRINTLN or change the message so it only appears when
findWiFi(true) will actually be called. Concretely, move or wrap the
DEBUG_PRINTLN(F("WiFi scan initiated on disconnect.")) so it is executed only
when WLED_FORCE_WIFI_OFF is not defined (alongside the call to findWiFi(true)),
or alter the text to say "WiFi scan skipped (scanning disabled) on disconnect"
when WLED_FORCE_WIFI_OFF is defined; adjust the block that uses scanDone,
multiWiFi, and findWiFi accordingly.
| #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." |
There was a problem hiding this comment.
both button defines should be mendatory IMHO
There was a problem hiding this comment.
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))
|
I refactored the implementation now to use less #ifdef blocks. Please have a look at commit d14643b and let me know if you prefer this implementation or the former implementation, with more explicit (inline) #ifdef blocks. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
how do the two versions compare regarding code size? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
They're almost identical: | Code Base / Build Target | WIFI ON | WIFI OFF |
|--------------------------|---------|----------|
| Before Refactoring | 1301777 | 1297105 |
| After Refactoring | 1301789 | 1297077 | |
|
Just a quick follow-up in case anything is still open on my side. If there are remaining concerns, adjustments, or preferred refinements to the current approach, I’m happy to work on them. Otherwise I’ll wait for further feedback :-) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
6915-6949: Extract the onset-tuning literals into named constants.The detector currently bakes several calibration values directly into the math (
96.0f,1.12f,4.0f,3.2f,0.18f, decay bounds). Hoisting them intoconstexprnames would make future retuning much easier without changing behavior.Based on learnings, added/modified WLED code should replace hardcoded numeric literals with meaningful constants when those constants improve maintainability.♻️ Example refactor
+ constexpr float kEnergyFloor = 96.0f; + constexpr float kFluxBaselineAlpha = 0.08f; + constexpr float kFluxBaselineScale = 1.12f; + constexpr float kFluxBaselineOffset = 4.0f; + constexpr float kOnsetGain = 3.2f; + constexpr float kBaseDriveScale = 0.18f; + float sensitivity = 0.45f + (float)SEGMENT.intensity / 128.0f; // slider 2 - float onsetNorm = (flux * 255.0f) / (energy + 96.0f); // robust against AGC level shifts - data->fluxAvg += 0.08f * (onsetNorm - data->fluxAvg); - float onsetOnly = fmaxf(0.0f, onsetNorm - (data->fluxAvg * 1.12f + 4.0f)); - float onsetDrive = fminf(255.0f, onsetOnly * sensitivity * 3.2f); - float baseDrive = fminf(255.0f, (energy / 12.0f) * 0.18f); // secondary fill component only + float onsetNorm = (flux * 255.0f) / (energy + kEnergyFloor); // robust against AGC level shifts + data->fluxAvg += kFluxBaselineAlpha * (onsetNorm - data->fluxAvg); + float onsetOnly = fmaxf(0.0f, onsetNorm - (data->fluxAvg * kFluxBaselineScale + kFluxBaselineOffset)); + float onsetDrive = fminf(255.0f, onsetOnly * sensitivity * kOnsetGain); + float baseDrive = fminf(255.0f, (energy / 12.0f) * kBaseDriveScale); // secondary fill component only🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6915 - 6949, Summary: Replace hardcoded onset-tuning literals with named constexpr constants to improve maintainability. Fix: add a small block of constexpr float definitions near the top of the FX.cpp scope (or above the spectral flux code) with meaningful names (e.g., kEnergyOffset = 96.0f, kFluxAvgGain = 0.08f is already used but add kFluxAvgFactor if desired, kFluxAvgMultiplier = 1.12f, kOnsetOffset = 4.0f, kOnsetDriveScale = 3.2f, kBaseDriveScale = 0.18f, kSensitivityBase = 0.45f, kSensitivityDiv = 128.0f, kDecayMin = 1.4f, kDecayMax = 16.0f) and replace the numeric literals in the spectral flux block (references: data->prevFft loop, onsetNorm computation, data->fluxAvg update, onsetOnly calculation, onsetDrive, baseDrive, and the mapf call using SEGMENT.speed) so the behavior does not change but tuning values are centralized and named.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/FX.cpp`:
- Around line 6915-6949: Summary: Replace hardcoded onset-tuning literals with
named constexpr constants to improve maintainability. Fix: add a small block of
constexpr float definitions near the top of the FX.cpp scope (or above the
spectral flux code) with meaningful names (e.g., kEnergyOffset = 96.0f,
kFluxAvgGain = 0.08f is already used but add kFluxAvgFactor if desired,
kFluxAvgMultiplier = 1.12f, kOnsetOffset = 4.0f, kOnsetDriveScale = 3.2f,
kBaseDriveScale = 0.18f, kSensitivityBase = 0.45f, kSensitivityDiv = 128.0f,
kDecayMin = 1.4f, kDecayMax = 16.0f) and replace the numeric literals in the
spectral flux block (references: data->prevFft loop, onsetNorm computation,
data->fluxAvg update, onsetOnly calculation, onsetDrive, baseDrive, and the mapf
call using SEGMENT.speed) so the behavior does not change but tuning values are
centralized and named.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 469a8745-66ed-48dd-b82e-1a9ae4b4f8dc
📒 Files selected for processing (2)
wled00/FX.cppwled00/FX.h
|
|
||
|
|
||
| ///////////////////////////// | ||
| // * MIDNOISE ONSET // |
There was a problem hiding this comment.
please put this in a new PR
There was a problem hiding this comment.
Oops, that's what happens when you use "main" as source for a PR. Sorry for that.
I removed the accidental commit that got looped into this PR.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
softhack007
added
the
AI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/wled.h (1)
32-32: ⚡ Quick winRename this feature flag to the standard
WLED_DISABLE_*/WLED_ENABLE_*form.
WLED_FORCE_WIFI_OFFbreaks the project's feature-toggle convention, so it adds a one-off compile-time API to maintain across headers, docs, and build flags. Please rename it consistently here and in the sample config/docs before merge.As per coding guidelines "Use WLED-specific feature flags:
WLED_DISABLE_*andWLED_ENABLE_*for feature toggling".Also applies to: 296-306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled.h` at line 32, The feature flag WLED_FORCE_WIFI_OFF breaks the project's naming convention; rename it to follow the WLED_DISABLE_* pattern (e.g., WLED_DISABLE_WIFI_OFF or WLED_DISABLE_WIFI) everywhere: update the define in wled.h (replace WLED_FORCE_WIFI_OFF), update all conditional checks (`#ifdef/`#ifndef/defined(...) uses) that reference WLED_FORCE_WIFI_OFF, and update sample config/docs and any build flags to use the new name; also apply the same renaming pattern to other non-conforming flags referenced around lines 296-306 so all feature toggles use WLED_DISABLE_* / WLED_ENABLE_* consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/wled.cpp`:
- Around line 55-59: The helper wifiScanRunning currently treats any negative
WiFi.scanComplete() value as “still running”; change it to return true only when
the status equals WIFI_SCAN_RUNNING so failed scans (WIFI_SCAN_FAILED) aren’t
considered running. Update the return in wifiScanRunning(bool wifiConfigured) to
check WiFi.scanComplete() == WIFI_SCAN_RUNNING and keep the existing
regularWiFiEnabled(), wifiConfigured and multiWiFi.size() > 1 checks so
findWiFi(), restartWiFiScanIfEnabled() and handleConnection() won’t be blocked
by a failed async scan.
---
Nitpick comments:
In `@wled00/wled.h`:
- Line 32: The feature flag WLED_FORCE_WIFI_OFF breaks the project's naming
convention; rename it to follow the WLED_DISABLE_* pattern (e.g.,
WLED_DISABLE_WIFI_OFF or WLED_DISABLE_WIFI) everywhere: update the define in
wled.h (replace WLED_FORCE_WIFI_OFF), update all conditional checks
(`#ifdef/`#ifndef/defined(...) uses) that reference WLED_FORCE_WIFI_OFF, and
update sample config/docs and any build flags to use the new name; also apply
the same renaming pattern to other non-conforming flags referenced around lines
296-306 so all feature toggles use WLED_DISABLE_* / WLED_ENABLE_* consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43591438-01d2-4c9d-902a-1c0d2b57800c
📒 Files selected for processing (4)
wled00/my_config_sample.hwled00/network.cppwled00/wled.cppwled00/wled.h
| bool wifiScanRunning(bool wifiConfigured) | ||
| { | ||
| if (!regularWiFiEnabled()) return false; | ||
| return wifiConfigured && multiWiFi.size() > 1 && WiFi.scanComplete() < 0; | ||
| } |
There was a problem hiding this comment.
Treat only WIFI_SCAN_RUNNING as “scan still running” here.
findWiFi() and the new restartWiFiScanIfEnabled() both treat WIFI_SCAN_FAILED as recoverable, but this helper still returns true for any negative status. In a multi-SSID setup, one failed async scan will keep handleConnection() returning early and can stall reconnect indefinitely.
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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/wled.cpp` around lines 55 - 59, The helper wifiScanRunning currently
treats any negative WiFi.scanComplete() value as “still running”; change it to
return true only when the status equals WIFI_SCAN_RUNNING so failed scans
(WIFI_SCAN_FAILED) aren’t considered running. Update the return in
wifiScanRunning(bool wifiConfigured) to check WiFi.scanComplete() ==
WIFI_SCAN_RUNNING and keep the existing regularWiFiEnabled(), wifiConfigured and
multiWiFi.size() > 1 checks so findWiFi(), restartWiFiScanIfEnabled() and
handleConnection() won’t be blocked by a failed async scan.
I agree with the bunny here - maybe better to rename the feature flag to |
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
Thanks for submitting this. My colleagues' concerns about safety are not ill-founded, but I also agree that that there are valuable use cases for being able to isolate the Wifi network interface from the rest of the system. ESP32-P4 support is the top of that list - we still need V5 and ESP-IDF support but we can make some steps like this to get ready.
I concur that we should be using WLED_DISABLE_WIFI as the flag name.
This dovetails a bit with #5650 .
At the limit, I think there's a bigger "network management" refactor waiting in the wings -- both with the "dependencies/network" name collision with V5, as well as issues like #4703 which points out that the two interfaces can't be treated as truly independent in some hardware configurations. The particular goal of treating wifi client support as a compile-time option would definitely be cleaner in that wider context.
|
|
||
| namespace { | ||
|
|
||
| bool restartWiFiScanIfEnabled(bool requireMultiWiFi) |
There was a problem hiding this comment.
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.
| bool regularWiFiEnabled() | ||
| { | ||
| #ifdef WLED_FORCE_WIFI_OFF | ||
| return false; | ||
| #else | ||
| return true; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
If you must expose this state as a variable, use a static constexpr variable to communicate clearly that it is a compile-time constant.
| 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 | ||
| } |
There was a problem hiding this comment.
As for restartWifiScan: gate the feature at the call site, not inside the implementing function. If a function is named initRegularWifiStartup(), then it should init the wifi any time it is called, not just sometimes.
| if (regularWiFiEnabled()) return false; | ||
|
|
||
| DEBUG_PRINTLN(F("WLED_FORCE_WIFI_OFF active. WiFi stays OFF unless AP emergency mode is opened by button.")); | ||
| apBehavior = AP_BEHAVIOR_BUTTON_ONLY; |
There was a problem hiding this comment.
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 (cfg.cpp, set.cpp).
|
|
||
| bool handleForceWiFiOffConnection() | ||
| { | ||
| if (regularWiFiEnabled()) return false; |
There was a problem hiding this comment.
This call path is unnecessarily convoluted.
- First, we unconditionally call this function - expressly named as being handling a specific build-time configuration value, regardless of its setting
- Then we call some other function to test the build time value
- ...and return, cancelling out the call, if it's not set.
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.
|
|
||
| bool wifiScanRunning(bool wifiConfigured) | ||
| { | ||
| if (!regularWiFiEnabled()) return false; |
| return wifiConfigured && multiWiFi.size() > 1 && WiFi.scanComplete() < 0; | ||
| } | ||
|
|
||
| int findWiFiIfEnabled(bool doScan = false) |
There was a problem hiding this comment.
Conditional in function name, again.
| void WLED::initConnection() | ||
| { | ||
| DEBUG_PRINTF_P(PSTR("initConnection() called @ %lus.\n"), millis()/1000); | ||
| if (handleForceWiFiOffConnection()) return; |
There was a problem hiding this comment.
The websocket line below is important for Ethernet too.
5 participants
Motivation
Description
WLED_FORCE_WIFI_OFFtowled.hand document it inmy_config_sample.hso users can enable it frommy_config.hor build flags.wled.cpp, change startup networking to callWiFi.mode(WIFI_OFF)whenWLED_FORCE_WIFI_OFFis defined instead of starting STA scanning andfindWiFi(true).initAP()to immediately return whenWLED_FORCE_WIFI_OFFis defined so no softAP is created.initConnection()early-exit whenWLED_FORCE_WIFI_OFFis defined after callingWiFi.disconnect(true)and forcingWiFi.mode(WIFI_OFF), and add debug prints to indicate the forced-off state.Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Chores