Add WiFi band preference for dual-band ESP32 chips#5367
Add WiFi band preference for dual-band ESP32 chips#5367softhack007 merged 1 commit intowled:V5-C6from
Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds WiFi 5GHz band support and broad platform-target updates: introduces wifiBandMode handling (UI, JSON, request parsing), renames Network → WLEDNetwork, adds board capability header and target-specific capability flags, expands ESP32-C* / P4 target guards across drivers and build scripts, and migrates many ESP-IDF v5 build configuration items. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wled00/cfg.cpp`:
- Around line 160-162: After deserializing cfg.json, validate the wifiBandMode
value (symbol wifiBandMode) to ensure it's within the allowed 1–3 range before
passing it to esp_wifi_set_band_mode(); follow the same pattern used for txPower
validation — check if wifiBandMode is out of range or missing, clamp or replace
with a safe default (e.g., 1) and log or correct the value in the config
structure so only valid values reach esp_wifi_set_band_mode().
In `@wled00/set.cpp`:
- Around line 126-131: The BM query param is parsed and stored directly into
wifiBandMode (used later with esp_wifi_set_band_mode()), but its value isn't
validated; validate the incoming int from request->arg(F("BM")).toInt() to
ensure it is one of the allowed values (1, 2, or 3) before assigning to
wifiBandMode and toggling forceReconnect; if the value is invalid, do not change
wifiBandMode (and optionally log or ignore the parameter). Update the branch
that checks request->hasArg(F("BM")) to parse, validate against {1,2,3}, set
forceReconnect only when the validated value differs from the current
wifiBandMode, and only then assign the new value so esp_wifi_set_band_mode()
always receives a valid wifi_band_mode_t.
🧹 Nitpick comments (2)
wled00/wled.h (1)
378-380: Replace the magic default with the enum constant.
Using literal2ties behavior to enum ordering; preferWIFI_BAND_MODE_5G_ONLYto stay aligned with ESP-IDF.♻️ Proposed change
`#ifdef` SOC_WIFI_SUPPORT_5G -WLED_GLOBAL byte wifiBandMode _INIT(2); // WIFI_BAND_MODE_5G_ONLY default for dual-band chips +WLED_GLOBAL byte wifiBandMode _INIT((byte)WIFI_BAND_MODE_5G_ONLY); // default for dual-band chips `#endif`Based on learnings: "In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase."
wled00/wled.cpp (1)
772-774: Check esp_wifi_set_band_mode result and confirm call order.
Please verify (per ESP-IDF docs) that this call is valid afterWiFi.begin()and log failures to aid diagnostics.🔍 Suggested error handling
`#ifdef` SOC_WIFI_SUPPORT_5G - esp_wifi_set_band_mode((wifi_band_mode_t)wifiBandMode); + esp_err_t err = esp_wifi_set_band_mode((wifi_band_mode_t)wifiBandMode); + if (err != ESP_OK) { + DEBUG_PRINTF_P(PSTR("esp_wifi_set_band_mode failed: %d\n"), err); + } `#endif`
|
@jonny190 as already stated - please align to latest "V5-C6" code, and use |
wled00/wled.h
Outdated
| WLED_GLOBAL bool force802_3g _INIT(false); | ||
| #endif // WLED_SAVE_RAM | ||
| #ifdef SOC_WIFI_SUPPORT_5G | ||
| WLED_GLOBAL byte wifiBandMode _INIT(2); // WIFI_BAND_MODE_5G_ONLY default for dual-band chips |
There was a problem hiding this comment.
I would prefer to have WIFI_BAND_MODE_AUTO as the default. This is most compatible to all possible setups.
|
I think the default should be auto rather than 5Ghz only |
Adds a user-configurable "WiFi band" dropdown to the WiFi settings page: 2.4 GHz only, 5 GHz only, or Auto (both). Defaults to Auto. Uses WiFi.setBandMode() from the Arduino ESP32 core (requires IDF 5.4.2+), guarded by #ifdef SOC_WIFI_SUPPORT_5G. Replaces the hardcoded WIFI_BAND_MODE_AUTO call in V5-C6 with a user-selectable setting. Changes: - wled.h: new wifiBandMode variable (default WIFI_BAND_MODE_AUTO) - cfg.cpp: serialize/deserialize with input validation (1-3 range) - set.cpp: handle BM form param with validation, trigger reconnect - xml.cpp: populate dropdown value, hide on non-5G chips - wled.cpp: pass wifiBandMode to WiFi.setBandMode() in initConnection() - settings_wifi.htm: dropdown in Experimental section Tested on ESP32-C5 hardware - all three modes work correctly.
74adf02 to
c17d630
Compare
|
Updated based on review feedback:
|
|
I'm not sure why the usermods ci is triggering when the pr has changes to the usermods, we can ignore those failures in this pr |
softhack007
left a comment
There was a problem hiding this comment.
looks good to me, thank you 👍
ready to merge
Summary
WiFi.setBandMode()from the Arduino ESP32 core (IDF 5.4.2+)WIFI_BAND_MODE_AUTOin V5-C6 with a user-selectable setting#ifdef SOC_WIFI_SUPPORT_5G- zero impact on single-band buildsChanges (6 files, 32 lines added, 4 removed)
wled00/wled.hwifiBandModeglobal (default:WIFI_BAND_MODE_AUTO)wled00/cfg.cppwled00/set.cppBMform param with validation, trigger reconnectwled00/xml.cppwled00/wled.cppwifiBandModetoWiFi.setBandMode()ininitConnection()wled00/data/settings_wifi.htmTest plan
Tested on ESP32-C5 hardware:
Summary by CodeRabbit
New Features
Improvements