Conversation
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…/LOW text, rename Caps to Functions Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…ti Relay support Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…ns and platforms Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…t in btnPin array Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…nfo tab Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…isplay Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new "Pin Info" settings subpage and HTML asset, exposes per-GPIO data via a new JSON "pins" target and serializePins(), defines SUBPAGE_PINS and updates subpage bounds, and wires the new page into server routing and GPIO info output. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
DedeHai
added
enhancement
hardware
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@wled00/data/settings_pininfo.htm`:
- Around line 28-31: The page references d.Sf.action which will throw because
there is no form named "Sf"; remove or guard that line (check that d && d.Sf &&
d.Sf.action exist before assigning) and keep behavior consistent with this being
a read-only info page; also reduce the polling frequency by changing the
setInterval call that assigns pinsTimer (currently setInterval(loadPins, 250))
to a much less aggressive interval (e.g., setInterval(loadPins, 2000)) to lower
embedded HTTP server load; locate these changes around the pinsTimer/setInterval
and the d.Sf.action reference in this block and update accordingly.
In `@wled00/json.cpp`:
- Around line 1092-1097: The loop that enumerates pins on ESP8266 skips A0
(GPIO17) because ENUM_PINS is 17 and the for-loop uses gpio < ENUM_PINS; change
the ESP8266 constant so the loop includes gpio==17 (e.g., set constexpr int
ENUM_PINS = 18) so the for (int gpio = 0; gpio < ENUM_PINS; gpio++) iteration
will reach GPIO17 and make the existing special-case code checking if (gpio ==
17) effective; update any related comment to reflect the new upper bound.
- Around line 1080-1085: The comment in getPinOwnerName is wrong: change the
comment that currently reads "low bit not set" to "high bit not set" (or "bit 7
not set") to reflect the check !(static_cast<uint8_t>(owner) & 0x80), and since
getPinOwnerName is only used from serializePins within this translation unit,
give getPinOwnerName internal linkage by marking it static; update the function
declaration/definition accordingly (refer to getPinOwnerName and serializePins).
🧹 Nitpick comments (7)
wled00/xml.cpp (1)
193-193:subPage < 0is always false sincebyteis unsigned.
subPageis declared asbyte(i.e.,uint8_t), so the< 0check is dead code. Not a functional issue, but worth noting for clarity.Proposed fix
- if (subPage <0 || subPage >SUBPAGE_LAST) return; + if (subPage > SUBPAGE_LAST) return;wled00/data/settings_pininfo.htm (4)
16-16:gpioInfois declared but never used.The global variable
gpioInfois initialized as an empty object but never referenced anywhere in the file.Proposed cleanup
- var pinsTimer=null, gpioInfo={}; + var pinsTimer=null;
58-83: Missing cleanup of the polling timer.When navigating away from this page (e.g., via the Back button), the
setIntervaltimer is never cleared. While navigation typically unloads the page, explicitly clearing the timer inB()would be cleaner and prevent any edge-case where requests continue after the user navigates away.Proposed fix
- function B(){window.open(getURL('/settings'),'_self');} // back button + function B(){if(pinsTimer)clearInterval(pinsTimer);window.open(getURL('/settings'),'_self');} // back button
62-82: Consider reducing DOM churn from fullinnerHTMLreplacement every poll cycle.Every 250ms (or whatever the interval), the entire table HTML is rebuilt and assigned via
innerHTML. On embedded devices with limited resources, this creates unnecessary GC pressure and visual flicker. A lighter approach would be to only update the dynamic parts (state dots, touch values) after the initial table is built, or at minimum check if the pin list structure has changed before rebuilding.
85-94: Styles duplicatestyle.cssbase theme — consider relying on the loaded stylesheet.The page already loads
style.css(line 12), but then re-declaresbodybackground, colors, etc. in the inline<style>block. The table-specific styles (.bs,.tv) are fine, but the body/general styles may conflict with or duplicate the shared theme. Consider keeping only the page-specific additions inline. As per coding guidelines, use tabs for indentation in web files. The inline styles appear to use tabs correctly.wled00/json.cpp (2)
1111-1129: Inconsistent indentation inside#elifblocks.Lines 1118–1128 use 8-space indentation while the rest of the function and the coding guideline require 2-space indentation. The
#if CONFIG_IDF_TARGET_ESP32block at lines 1114–1116 uses 4-space indentation which is also inconsistent.As per coding guidelines: "Use 2-space indentation for C++ source files (.cpp)".
Proposed fix (showing ESP32-S2 block as example; apply same to S3 and C3)
`#elif` CONFIG_IDF_TARGET_ESP32S2 - // ESP32-S2: ADC1 channels 0-9 (GPIO 1-10) - int adc_channel = digitalPinToAnalogChannel(gpio); - if (adc_channel >= 0 && adc_channel <= 9) caps |= PIN_CAP_ADC; + // ESP32-S2: ADC1 channels 0-9 (GPIO 1-10) + int adc_channel = digitalPinToAnalogChannel(gpio); + if (adc_channel >= 0 && adc_channel <= 9) caps |= PIN_CAP_ADC;
1184-1188: Usegpioinstead ofrlyPinfor consistency and correctness.
rlyPinis a global for the primary relay, but the loop iterates all GPIOs. If WLED ever supports multiple relay-owner pins, this would read the wrong pin's state. Sincegpiois already known to be the relay pin at this point, use it directly.Proposed fix
if (owner == PinOwner::Relay) { pinObj["m"] = 1; // mode: output - pinObj["s"] = digitalRead(rlyPin); // read state from hardware (digitalRead returns output state for output pins) + pinObj["s"] = digitalRead(gpio); // read state from hardware }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wled00/json.cpp`:
- Around line 1111-1129: Fix the indentation and add a fallback ADC branch:
reformat the `#elif` blocks that call digitalPinToAnalogChannel(gpio) so they use
2-space indentation to match .cpp coding guidelines, and add a final `#else`
branch after the CONFIG_IDF_TARGET checks that calls
digitalPinToAnalogChannel(gpio) and sets caps |= PIN_CAP_ADC when adc_channel >=
0 (so newer ESP32 variants get ADC capability detected); ensure you reference
the same symbols (digitalPinToAnalogChannel, gpio, adc_channel, caps,
PIN_CAP_ADC) and keep the per-variant explicit checks for
ESP32/ESP32S2/ESP32S3/ESP32C3 intact.
- Around line 1137-1150: Add an explicit CONFIG_IDF_TARGET_ESP32C6 branch so
ESP32-C6 doesn't fall through to the legacy "classic" defaults: under an `#elif
defined(CONFIG_IDF_TARGET_ESP32C6)` check, mark the boot pin with `if (gpio ==
9) caps |= PIN_CAP_BOOT;` and set the bootstrap pins with `if (gpio == 2 || gpio
== 8) caps |= PIN_CAP_BOOTSTRAP;` (same pattern as the other target-specific
branches using the gpio variable and PIN_CAP_BOOT / PIN_CAP_BOOTSTRAP symbols).
🧹 Nitpick comments (2)
wled00/json.cpp (2)
1052-1057:PIN_CAP_PWMis defined but never used.The macro is defined at line 1054 but the only usage site is commented out (line 1132). Remove it to avoid dead code, or add a comment that it's reserved for future use.
1184-1188: Consider usinggpioinstead ofrlyPinfor the relay digitalRead.Line 1187 reads
digitalRead(rlyPin)using the global variable, but at this pointgpiois the pin being serialized and its owner is confirmed to bePinOwner::Relay. UsingdigitalRead(gpio)would be more self-consistent and wouldn't break if the relay pin ownership model ever changes.Proposed fix
if (owner == PinOwner::Relay) { pinObj["m"] = 1; // mode: output - pinObj["s"] = digitalRead(rlyPin); // read state from hardware (digitalRead returns output state for output pins) + pinObj["s"] = digitalRead(gpio); // read state from hardware }
|
@coderabbitai ESP8266 has the analog input pin A0, it is not selectable as an analog button in WLED dropdowns. Was this ever available as an analo pin or where can it be used through the UI? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 52203 🏁 Script executed: Length of output: 17063
A0 was never selectable in WLED UI dropdowns for ESP8266. This is by design. On ESP8266, the hardware only has one analog input pin, and the code handles this specially:
How to use analog buttons on ESP8266:
About the commit message "remove A0 for ESP8266": This refers to not displaying A0/GPIO17 in the new Pin Info page you added in this PR, since it's marked as input-only with a TODO to "display as A0 pin" - meaning there's still work to improve how it's shown in the UI. So A0 is available for analog buttons through the button type setting, but it's not a selectable pin in dropdowns because it's automatically used when needed. 🧠 Learnings used |
|
Good idea, reminds me of something similar that I did for wled-mm some time ago - but never found time to go from "summary on serial" to "page in the UI". WLED-MM has extended pinmanager to ask for pin capabilities, maybe this would be a good for your PR, too. At that time my pin-info code was unwanted in upstream, due to "abuse of pinmanager" 😝. But times have changed :-) just for comparison: even on serial, the info below seems to help many users. So i think your PR is "on the right track" 👍 
|
|
Small improvement: instead of "System" you could display pins as "reserved", I think this is the usual wording in espressif docs. |
|
How easily can we extend this to create a reusable component to replace anywhere were we just have a text box for the user entering a pin to a drop-down incorporating this pin info? |
We (upstream WLED) have an annotated drop-down for LEDs settings already. You're right it should be possible to make a generic PIN drop-down and put it into |
the addition of "touch" to |
yes, from what I recall most of the code is already there, after all many pins already use dropdowns. good idea to move some of the json.cpp code into pinmanager. |
I had that initially but it felt a bit too "generic" as in "why is that pin reserved? and how to unreserve it?" stating it is used by the system and not available "no matter what" felt more appropriate and informative. |
yes, the serial output in MM is very helpful but inaccessible to most AC users - I do have that code for "extended error log" ready in a branch that adds a console of sorts to the info panel, maybe that can be extended as well as a general log (see #4959). |
|
@DedeHai , indeed, it was here : https://discord.com/channels/473448917040758787/795233398314237972/1444802942149591090 |
|
... this was pin manager related, or you refer to logging? WLED-MM has netdebug, sending serial logging over the network. |
|
only pin manager / pin info related - the logging I referred to is currently not in PR, I wanted to give that a checkup before I do but once it is up you are welcome to comment or share your inputs |
|
btw, in case that "pin conflicts" are still interesting. To get the conflicts,
This gives you the last conflict on each pin. Very helpful in case of "wtf I'm lost in pin probelms" ;-) |
|
double allocations can only happen if you force pins at compile time, the UI knows available pins and won't allow double allocations (if it does, that needs fixing). |
The UI does allow duplicates, dmx input is one place, but don't think the only place that doesn't use a drop-down for the pin selection |
|
LED pins also do not use dropdowns, they still do not allow conflicting pins. Anyway, moving towards dropdowns for all pins is definitely something we need and it should be pretty straight forward to implement. |
|
@coderabbitai please review latest changes and check if there are any newly introduced issues. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/pin_manager.cpp`:
- Around line 351-372: PinManager::isAnalogPin currently handles only specific
ESP32 targets and will return false for newer variants (C6, H2, etc.); add an
`#else` fallback under the ARDUINO_ARCH_ESP32 block that calls
digitalPinToAnalogChannel(gpio) and returns true if the result is >= 0 so
ADC-capable pins on unlisted ESP32 variants are detected; update only the
conditional branches in isAnalogPin to include this generic check referencing
digitalPinToAnalogChannel and keep existing per-target checks intact.
🧹 Nitpick comments (2)
wled00/json.cpp (2)
1120-1161: Consider the latency impact oftouchRead()during JSON serialization.
touchRead()can take several milliseconds per call on ESP32 (especially Touch V2 on S2/S3). SinceserializePins()runs while holding the JSON buffer lock, multiple touch-enabled buttons could add noticeable latency to the/json/pinsresponse. This is acceptable for a debug/info page but worth documenting if there's concern about the lock hold time.
1221-1238: URL matching for"pins"could collide with future endpoints.The
indexOf(F("pins"))check would match any URL containing "pins" (e.g., a hypothetical/json/mappins). Currently there's no conflict, but this is fragile compared to exact segment matching. The same pattern exists for other endpoints here, so this is consistent with the codebase style.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1642 🏁 Script executed: Length of output: 64 🏁 Script executed: Length of output: 3729 🏁 Script executed: Length of output: 2111 🏁 Script executed: Length of output: 4252 🏁 Script executed: Length of output: 5877 🏁 Script executed: Length of output: 2382 🏁 Script executed: Length of output: 6792 🏁 Script executed: Length of output: 4153 🏁 Script executed: Length of output: 3793 🏁 Script executed: Length of output: 564 🏁 Script executed: Length of output: 1195 🏁 Script executed: Length of output: 9020 🏁 Script executed: Length of output: 413
✅ Good ImprovementsThe latest commit "moved pin capabilities to pinmanager" is an excellent architectural improvement. The PIN_CAP constants are now properly defined in pin_manager.h, which centralizes pin capability handling where it belongs. 🔍 Code Quality ObservationsMemory Safety: All good here. Button array access is properly protected with Platform-Specific Code: The Magic Numbers: There are bit shift operations (
Security: The HTML page uses 📝 TODOs NotedESP8266 GPIO17/A0 has TODOs to "display as A0 pin" - this is documented and can be addressed in a follow-up. Usermod names have a TODO: "Get actual usermod name from UsermodManager" - currently shows generic "UM ✨ No Newly Introduced IssuesThe code is well-structured, uses proper error handling, includes appropriate platform-specific conditionals, and follows WLED coding patterns. The 250ms polling interval for live updates is reasonable and won't cause performance issues. Great work on this feature! The Pin Info page will be very helpful for users debugging their configurations. 🐰 🧠 Learnings used |
|
there are still things that can be improved / moved for pin dropdowns but I would like to tackle that in a seperate PR building upon this one. |
replaces #5122
I decided to replace the AI PR for proper attribution in history as only a few lines of the original AI code are left.
Features:
Screenshot for an ESP32:
Summary by CodeRabbit