SD Card usermod: Adjust SPI configuration for ESP32-S3 support#5679
SD Card usermod: Adjust SPI configuration for ESP32-S3 support#5679Will-wastelander wants to merge 10 commits into
Conversation
Added support for ESP32-S3
WalkthroughSD usermod selects HSPI for ESP32-S3 or VSPI for other targets. GPIO pins can be overridden via compile-time macros (UM_SD_SELECT, UM_SD_CLOCK, UM_SD_POCI, UM_SD_PICO) with defaults (16/14/36/15). Variable types changed from int8_t to uint8_t. Build instructions document the configuration options. ChangesSD Card Initialization Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
softhack007
changed the title
|
@coderabbitai is it possible that using HSPI or VSPI on S3 can create conflicts with the dotstar (2 pin) LED drivers? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look at the sd_card.cpp change
cat usermods/sd_card/sd_card.cpp
echo "---"
# Search for dotstar/APA102 SPI bus usage
rg -n -i "dotstar|apa102|HSPI|VSPI|FSPI|SPI2|SPI3|spiPort|SPIClass" --type cpp -C3
echo "---"
# Check for ESP32-S3 specific SPI handling
rg -n "CONFIG_IDF_TARGET_ESP32S3\|ESP32S3\|esp32s3" --type cpp -C3
echo "---"
# Look at NeoPixelBus or LED driver SPI setup
rg -n -i "spi.*bus\|bus.*spi\|NeoPixelBus\|NeoSpi" --type cpp -C3 -lLength of output: 22312
With
|
| Scenario | Dotstar SPI bus (S3) | SD card SPI bus (PR) | Conflict? |
|---|---|---|---|
With WLED_USE_ETHERNET |
HSPI | HSPI | ✅ Yes |
| Without Ethernet, Arduino-ESP32 v2.x | FSPI (SPI2) | HSPI (SPI3) | ❌ No |
| Without Ethernet, Arduino-ESP32 v3.x | FSPI (SPI2) | HSPI → SPI2 | ✅ Yes |
A safer approach for this PR might be to use FSPI explicitly on ESP32-S3 (keeping it consistent with the global SPI bus used for LEDs), or to check whether HSPI is already claimed by NeoPixelBus before initializing the SD card. The same potential conflict exists for other 2-pin SPI LED types (LPD8806, WS2801, P9813).
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Search for dotstar/APA102 SPI bus usage in WLED
rg -rn "HSPI|VSPI|FSPI|SPI2|SPI3" --type cpp -C 3
echo "=== Search for dotstar/APA102 references ==="
rg -rn -i "dotstar|apa102|sk9822" --type cpp -C 3
echo "=== Search for SPI bus selection in bus_wrapper or NeoPixelBus ==="
rg -rn "HSPI|VSPI" --type h -C 3Length of output: 15534
Non-Ethernet builds (most common case):
Ethernet builds (
The relevant lines in `#ifdef` WLED_USE_ETHERNET
// fix for `#2542` (by `@BlackBird77`)
`#define` B_HS_DOT_3 NeoPixelBus<DotStarBgrFeature, DotStarEsp32HspiHzMethod> //hardware HSPI
`#else`
`#define` B_HS_DOT_3 NeoPixelBus<DotStarBgrFeature, DotStarSpiHzMethod> //hardware VSPI
`#endif`For the Ethernet + S3 edge case, using |
|
@coderabbitai wled only supports WLED_USE_ETHERNET on classic esp32, but not on other esp32 variants. |
|
Tip For best results, initiate chat on the files or code changes.
That narrows the remaining concern to a single edge case: Arduino-ESP32 v3.x remaps So the PR is safe for the most common case (Arduino-ESP32 v2.x, non-Ethernet), but could have an issue on v3.x builds. ✏️ Learnings added
|
|
@softhack007 Thanks for thinking of possible conflicts.. I didn't think about that, but glad there does not appear to be any. I can try to see if FSPI works still, just to be safe if needed. |
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
Looks like a well targeted change for S3 👍
I tested, and FSPI works also, so if needed it can be changed. |
Added support to use build flags to configure SD card pins
Will-wastelander
left a comment
Will-wastelander
left a comment
There was a problem hiding this comment.
Updated pins to allow build flags from env.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/sd_card/sd_card.cpp (1)
19-21:⚠️ Potential issue | 🟡 MinorFix comment accuracy and indentation violations; define pin constants instead of magic numbers.
Line 20 says "typically FSPI (or HSPI)" but line 21 selects only
HSPI. Either update the comment to justify the HSPI choice or consider switching to FSPI as the comment suggests.Additionally, lines 36–55 use tab characters instead of 2-space indentation (violates usermod guideline). The hardcoded pin defaults (16, 14, 36, 15 on lines 39, 44, 49, 54) should be defined as named constants (e.g.,
constexpr uint8_t SD_PIN_SELECT = 16;) rather than magic numbers.🤖 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 `@usermods/sd_card/sd_card.cpp` around lines 19 - 21, The comment on line 20 states "typically FSPI (or HSPI)" but the code on line 21 only uses HSPI; either update the comment to clarify why HSPI was specifically chosen or switch to FSPI to match the comment intent. Additionally, lines 36–55 use tab indentation instead of the required 2-space indentation per usermod guidelines; replace all tabs with spaces in this section. Finally, replace the hardcoded pin numbers (16, 14, 36, 15) appearing on lines 39, 44, 49, and 54 with named constexpr constants (such as SD_PIN_SELECT, SD_PIN_MOSI, SD_PIN_CLK, SD_PIN_MISO) defined near the top of the file to improve code readability and maintainability.Source: Coding guidelines
🧹 Nitpick comments (1)
usermods/sd_card/sd_card.cpp (1)
36-55: ⚡ Quick winUse 2-space indentation (no tabs) and define named constants for magic pin defaults.
Lines 37, 39, 42, 44, 47, 49, 52, 54 use tabs instead of 2-space indentation. As per coding guidelines, use consistent 2-space indentation (no tabs) in usermod C++ files.
Additionally, the coding guidelines explicitly call out SD pin defaults (16/14/36/15) as an example of magic numbers to avoid. Define these as named
constexprconstants for clarity and maintainability.♻️ Proposed fix: replace tabs with spaces and define named constants
Before the
UsermodSdCardclass (e.g., around line 30), add:+#ifdef WLED_USE_SD_SPI + // Default SD-over-SPI GPIO pins + constexpr int8_t DEFAULT_SD_PIN_SELECT = 16; + constexpr int8_t DEFAULT_SD_PIN_CLOCK = 14; + constexpr int8_t DEFAULT_SD_PIN_POCI = 36; + constexpr int8_t DEFAULT_SD_PIN_PICO = 15; +#endifThen replace lines 36-55 with proper 2-space indentation and use the constants:
`#ifdef` WLED_USE_SD_SPI - `#if` defined(UM_SD_SELECT) - int8_t configPinSourceSelect = UM_SD_SELECT; - `#else` - int8_t configPinSourceSelect = 16; - `#endif` - `#if` defined(UM_SD_CLOCK) - int8_t configPinSourceClock = UM_SD_CLOCK; - `#else` - int8_t configPinSourceClock = 14; - `#endif` - `#if` defined(UM_SD_POCI) - int8_t configPinPoci = UM_SD_POCI; - `#else` - int8_t configPinPoci = 36; - `#endif` - `#if` defined(UM_SD_PICO) - int8_t configPinPico = UM_SD_PICO; - `#else` - int8_t configPinPico = 15; - `#endif` + `#if` defined(UM_SD_SELECT) + int8_t configPinSourceSelect = UM_SD_SELECT; + `#else` + int8_t configPinSourceSelect = DEFAULT_SD_PIN_SELECT; + `#endif` + `#if` defined(UM_SD_CLOCK) + int8_t configPinSourceClock = UM_SD_CLOCK; + `#else` + int8_t configPinSourceClock = DEFAULT_SD_PIN_CLOCK; + `#endif` + `#if` defined(UM_SD_POCI) + int8_t configPinPoci = UM_SD_POCI; + `#else` + int8_t configPinPoci = DEFAULT_SD_PIN_POCI; + `#endif` + `#if` defined(UM_SD_PICO) + int8_t configPinPico = UM_SD_PICO; + `#else` + int8_t configPinPico = DEFAULT_SD_PIN_PICO; + `#endif`🤖 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 `@usermods/sd_card/sd_card.cpp` around lines 36 - 55, Replace all tab characters with 2-space indentation in the SD card pin configuration block that defines configPinSourceSelect, configPinSourceClock, configPinPoci, and configPinPico. Additionally, extract the magic number default values (16, 14, 36, 15) into named constexpr constants defined before this block, then update the `#else` clauses to use these named constants instead of the raw numeric values for improved code clarity and maintainability.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@usermods/sd_card/sd_card.cpp`:
- Around line 19-21: The comment on line 20 states "typically FSPI (or HSPI)"
but the code on line 21 only uses HSPI; either update the comment to clarify why
HSPI was specifically chosen or switch to FSPI to match the comment intent.
Additionally, lines 36–55 use tab indentation instead of the required 2-space
indentation per usermod guidelines; replace all tabs with spaces in this
section. Finally, replace the hardcoded pin numbers (16, 14, 36, 15) appearing
on lines 39, 44, 49, and 54 with named constexpr constants (such as
SD_PIN_SELECT, SD_PIN_MOSI, SD_PIN_CLK, SD_PIN_MISO) defined near the top of the
file to improve code readability and maintainability.
---
Nitpick comments:
In `@usermods/sd_card/sd_card.cpp`:
- Around line 36-55: Replace all tab characters with 2-space indentation in the
SD card pin configuration block that defines configPinSourceSelect,
configPinSourceClock, configPinPoci, and configPinPico. Additionally, extract
the magic number default values (16, 14, 36, 15) into named constexpr constants
defined before this block, then update the `#else` clauses to use these named
constants instead of the raw numeric values for improved code clarity and
maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8e590d6-c131-47a4-a1b0-5e7a58885c99
📒 Files selected for processing (1)
usermods/sd_card/sd_card.cpp
Added instructions for customizing SD card pins in readme.
Refactor preprocessor directives for SD card pin configuration.
Added original comments back
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/sd_card/sd_card.cpp (1)
44-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace tab with spaces.
Line 44 uses a tab character for indentation. Per coding guidelines, use 2-space indentation (no tabs) in usermod C++ files.
🔧 Proposed fix
- int8_t configPinSourceClock = UM_SD_CLOCK; + int8_t configPinSourceClock = UM_SD_CLOCK;🤖 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 `@usermods/sd_card/sd_card.cpp` at line 44, The variable declaration `int8_t configPinSourceClock = UM_SD_CLOCK;` on line 44 uses a tab character for indentation instead of spaces. Replace the leading tab character with exactly 2 spaces to comply with the C++ coding guidelines for usermod files that require space-based indentation with no tabs.Source: Coding guidelines
🧹 Nitpick comments (1)
usermods/sd_card/sd_card.cpp (1)
182-186: ⚡ Quick winUse consistent types for pin configuration variables.
The
old*variables are declared asuint8_t, but the correspondingconfig*members areint8_t(lines 39-56). While GPIO pin numbers are typically small enough to avoid practical issues, using inconsistent types can lead to sign-extension gotchas and reduces code clarity.♻️ Suggested fix: use uint8_t consistently
Change the config member declarations (lines 39, 41, 44, 46, 49, 51, 54, 56) from
int8_ttouint8_t:`#if` defined(UM_SD_SELECT) - int8_t configPinSourceSelect = UM_SD_SELECT; + uint8_t configPinSourceSelect = UM_SD_SELECT; `#else` - int8_t configPinSourceSelect = 16; + uint8_t configPinSourceSelect = 16; `#endif` `#if` defined(UM_SD_CLOCK) - int8_t configPinSourceClock = UM_SD_CLOCK; + uint8_t configPinSourceClock = UM_SD_CLOCK; `#else` - int8_t configPinSourceClock = 14; + uint8_t configPinSourceClock = 14; `#endif` `#if` defined(UM_SD_POCI) - int8_t configPinPoci = UM_SD_POCI; + uint8_t configPinPoci = UM_SD_POCI; `#else` - int8_t configPinPoci = 36; + uint8_t configPinPoci = 36; `#endif` `#if` defined(UM_SD_PICO) - int8_t configPinPico = UM_SD_PICO; + uint8_t configPinPico = UM_SD_PICO; `#else` - int8_t configPinPico = 15; + uint8_t configPinPico = 15; `#endif`🤖 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 `@usermods/sd_card/sd_card.cpp` around lines 182 - 186, The temporary variables (oldPinSourceSelect, oldPinSourceClock, oldPinPoci, oldPinPico, oldSdEnabled) are declared as uint8_t, but the corresponding class member variables (configPinSourceSelect, configPinSourceClock, configPinPoci, configPinPico, configSdEnabled) are declared as int8_t. To maintain type consistency and avoid potential sign-extension issues, change all the config member variable declarations from int8_t to uint8_t. This ensures that the temporary variables used to save the current state match the type of the members they are copying from.
🤖 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.
Outside diff comments:
In `@usermods/sd_card/sd_card.cpp`:
- Line 44: The variable declaration `int8_t configPinSourceClock = UM_SD_CLOCK;`
on line 44 uses a tab character for indentation instead of spaces. Replace the
leading tab character with exactly 2 spaces to comply with the C++ coding
guidelines for usermod files that require space-based indentation with no tabs.
---
Nitpick comments:
In `@usermods/sd_card/sd_card.cpp`:
- Around line 182-186: The temporary variables (oldPinSourceSelect,
oldPinSourceClock, oldPinPoci, oldPinPico, oldSdEnabled) are declared as
uint8_t, but the corresponding class member variables (configPinSourceSelect,
configPinSourceClock, configPinPoci, configPinPico, configSdEnabled) are
declared as int8_t. To maintain type consistency and avoid potential
sign-extension issues, change all the config member variable declarations from
int8_t to uint8_t. This ensures that the temporary variables used to save the
current state match the type of the members they are copying from.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95581280-68dd-474b-aedb-0861c8b69543
📒 Files selected for processing (1)
usermods/sd_card/sd_card.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/sd_card/sd_card.cpp (1)
19-27:⚠️ Potential issue | 🟠 MajorAdd ESP32-C3 to the FSPI conditional; it requires the same SPI bus as ESP32-S3.
According to
wled00/cfg.cppline 502, the SPI configuration comment explicitly states: "SPI global uses VSPI on ESP32 and FSPI on C3, S3". However, the current code only checksCONFIG_IDF_TARGET_ESP32S3, causing ESP32-C3 to fall through to the VSPI branch, which would fail SD card initialization on C3 boards.Additionally, line 44 contains a tab character instead of spaces (violates the 2-space indentation requirement for usermod C++ files), and the comments are misleading: line 20 mentions "(or HSPI)" but the code only uses FSPI, and line 25's HSPI alternative appears to be leftover debug code.
Suggested fix
`#ifdef` WLED_USE_SD_MMC // SD_MMC configuration handled elsewhere `#elif` defined(WLED_USE_SD_SPI) - `#if` defined(CONFIG_IDF_TARGET_ESP32S3) - // ESP32-S3 default hardware SPI bus is typically FSPI (or HSPI) + `#if` defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) + // ESP32-S3 and ESP32-C3 use FSPI as the default SPI bus SPIClass spiPort = SPIClass(FSPI); `#else` - // Classic ESP32 defaults (VSPI is default/safest for SD cards on standard pins) + // Classic ESP32 uses VSPI as the default SPI bus SPIClass spiPort = SPIClass(VSPI); - // Alternative for classic ESP32 if needed: SPIClass spiPort = SPIClass(HSPI); `#endif` `#endif`Also fix the tab character on line 44 to spaces.
🤖 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 `@usermods/sd_card/sd_card.cpp` around lines 19 - 27, Update the SPI bus conditional logic to include ESP32-C3 alongside ESP32-S3, since both boards require FSPI according to the documentation in cfg.cpp. Modify the preprocessor conditional that currently only checks CONFIG_IDF_TARGET_ESP32S3 to also check CONFIG_IDF_TARGET_ESP32C3 using an OR operator so both target the same FSPI branch. Additionally, remove the misleading "(or HSPI)" comment from the FSPI initialization line since the code only uses FSPI, and delete the HSPI alternative comment line as it is leftover debug code. Finally, replace the tab character on line 44 with 2 spaces to comply with the usermod C++ indentation standard.
🤖 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 `@usermods/sd_card/sd_card.cpp`:
- Line 44: Replace the tab character used for indentation on the line containing
the configPinSourceClock variable declaration with 2 spaces to conform to the
project's coding style guidelines that require 2-space indentation without tabs
in usermod C++ files. Locate the line where uint8_t configPinSourceClock is
initialized with UM_SD_CLOCK and ensure the indentation uses only spaces.
---
Outside diff comments:
In `@usermods/sd_card/sd_card.cpp`:
- Around line 19-27: Update the SPI bus conditional logic to include ESP32-C3
alongside ESP32-S3, since both boards require FSPI according to the
documentation in cfg.cpp. Modify the preprocessor conditional that currently
only checks CONFIG_IDF_TARGET_ESP32S3 to also check CONFIG_IDF_TARGET_ESP32C3
using an OR operator so both target the same FSPI branch. Additionally, remove
the misleading "(or HSPI)" comment from the FSPI initialization line since the
code only uses FSPI, and delete the HSPI alternative comment line as it is
leftover debug code. Finally, replace the tab character on line 44 with 2 spaces
to comply with the usermod C++ indentation standard.
🪄 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: 8185d5b3-1ccd-427e-a97f-3bf0fd2c79d2
📒 Files selected for processing (1)
usermods/sd_card/sd_card.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| // confusing names? Then have a look | ||
| // https://oshwa.org/resources/a-resolution-to-redefine-spi-signal-names/ | ||
| #ifdef WLED_USE_SD_SPI | ||
| #if defined(UM_SD_SELECT) |
There was a problem hiding this comment.
put this at the top and do ifndef then simplify this part to
uint8_t configPinSourceSelect = UM_SD_SELECT;
see other usermods as examples.
There was a problem hiding this comment.
Do you have an example that I can use for this ? Currently the code is working.
There was a problem hiding this comment.
|
@coderabbitai you claim "Arduino-ESP32 v3.x remaps HSPI to SPI2 (same physical peripheral as FSPI) on ESP32-S3. If someone builds with Arduino-ESP32 v3.x and uses a dotstar (which rides the global SPI → FSPI/SPI2), the SD card's SPIClass(HSPI) would alias to the same bus. On v2.x this is fine, since HSPI → SPI3 there." please double check and state your source. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check what HSPI/VSPI/FSPI evaluate to in the codebase or any vendored headers
rg -rn "HSPI|VSPI|FSPI|SPI2|SPI3" --type cpp --type h -C 2
echo "=== Check any Arduino ESP32 SPI headers in the build system ==="
fd -e h -e cpp -i "spi" --type f | head -30Length of output: 3908 🌐 Web query:
Length of output: 20436
After checking the actual Arduino-ESP32 source, my claim was incorrect. From `#ifdef` CONFIG_IDF_TARGET_ESP32
`#define` FSPI 1 // SPI1
`#define` HSPI 2 // SPI2
`#define` VSPI 3 // SPI3
`#else`
`#define` FSPI 0 // ESP32-C2/C3/C5/C6/H2/S2/S3/P4 → SPI2
`#define` HSPI 1 // ESP32-S2/S3/P4 → SPI3
`#endif`On ESP32-S3 in both v2.x and v3.x: Corrected summary: I apologise for the incorrect analysis in my earlier replies. ✏️ Learnings added
|
|
@coderabbitai so for ESP32 as well as S3, re-check if the correct SPI ports are being used in this PR for best compatibility as discussed earlier. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== sd_card.cpp ==="
cat usermods/sd_card/sd_card.cpp
echo ""
echo "=== bus_wrapper.h dotstar/SPI section ==="
rg -n "HSPI|VSPI|FSPI|DotStar|SpiHz|dotstar|apa102" wled00/bus_wrapper.h -C 3
echo ""
echo "=== const.h SPI comment ==="
rg -n "VSPI|HSPI|FSPI|SPI.*global\|spi_sclk\|spi_miso\|spi_mosi" wled00/const.h -C 3
echo ""
echo "=== cfg.cpp SPI.begin ==="
rg -n "SPI\.begin\|spi_sclk\|spi_miso\|spi_mosi" wled00/cfg.cpp -C 3
echo ""
echo "=== Check for SPI begin in bus_manager or bus_wrapper ==="
rg -n "SPI\.begin\|SPIClass\|HSPI\|VSPI\|FSPI" wled00/bus_manager.cpp wled00/bus_wrapper.h -C 3 2>/dev/null || trueLength of output: 13834
Here's the breakdown: Classic ESP32 (non-Ethernet)
Classic ESP32 (Ethernet)
ESP32-S3 (no Ethernet support)
Correct SPI port assignment
The suggested fix: `#if` defined(CONFIG_IDF_TARGET_ESP32S3)
// FSPI (SPI2) is the global SPI used by dotstar/2-pin LED drivers; use HSPI (SPI3) for SD
SPIClass spiPort = SPIClass(HSPI);
`#elif` defined(WLED_USE_ETHERNET)
// Ethernet builds: dotstar uses HSPI (SPI2), so SD uses VSPI (SPI3)
SPIClass spiPort = SPIClass(VSPI);
`#else`
// Non-Ethernet classic ESP32: dotstar uses global SPI (VSPI/SPI3), so SD uses HSPI (SPI2)
SPIClass spiPort = SPIClass(HSPI);
`#endif`Interestingly, this means the original HSPI choice for ESP32-S3 (before it was changed to FSPI in this PR) was actually the correct one. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/sd_card/sd_card.cpp (1)
39-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate and clamp config-derived pin values before using them for SPI/pin allocation.
readFromConfig()ingests client-supplied JSON, but the pin fields areuint8_tand passed through unchecked. That means values like-1can become255, and out-of-range pins flow intoPinManager::allocateMultiplePins()/spiPort.begin(), causing unstable init behavior.Suggested fix
- uint8_t configPinSourceSelect = UM_SD_SELECT; + int8_t configPinSourceSelect = UM_SD_SELECT; ... - uint8_t configPinSourceClock = UM_SD_CLOCK; + int8_t configPinSourceClock = UM_SD_CLOCK; ... - uint8_t configPinPoci = UM_SD_POCI; + int8_t configPinPoci = UM_SD_POCI; ... - uint8_t configPinPico = UM_SD_PICO; + int8_t configPinPico = UM_SD_PICO;getJsonValue(top["pinSourceSelect"], configPinSourceSelect); getJsonValue(top["pinSourceClock"], configPinSourceClock); getJsonValue(top["pinPoci"], configPinPoci); getJsonValue(top["pinPico"], configPinPico); getJsonValue(top["sdEnabled"], configSdEnabled); + + auto invalidPin = [](int8_t p) { + return p < -1 || !digitalPinIsValid((uint8_t)p); + }; + if (invalidPin(configPinSourceSelect) || invalidPin(configPinSourceClock) || + invalidPin(configPinPoci) || invalidPin(configPinPico)) { + DEBUG_PRINTF("[%s] Invalid SD SPI pin config; keeping previous values.\n", _name); + configPinSourceSelect = oldPinSourceSelect; + configPinSourceClock = oldPinSourceClock; + configPinPoci = oldPinPoci; + configPinPico = oldPinPico; + }As per coding guidelines, values from
readFromConfig(JsonObject& root)are untrusted until validated/range-clamped in the usermod.Also applies to: 188-192
🤖 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 `@usermods/sd_card/sd_card.cpp` around lines 39 - 57, The pin configuration values including configPinSourceSelect, configPinSourceClock, configPinPoci, and configPinPico are derived from readFromConfig() which ingests untrusted client JSON. These uint8_t pin values must be validated and clamped to an acceptable range before being used in PinManager::allocateMultiplePins() or spiPort.begin(). Add validation logic after these variables are initialized to ensure each pin value falls within the valid range for your platform, rejecting or clamping out-of-range values to prevent unstable behavior. Apply the same validation to the other pin variables mentioned at lines 188-192.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@usermods/sd_card/sd_card.cpp`:
- Around line 39-57: The pin configuration values including
configPinSourceSelect, configPinSourceClock, configPinPoci, and configPinPico
are derived from readFromConfig() which ingests untrusted client JSON. These
uint8_t pin values must be validated and clamped to an acceptable range before
being used in PinManager::allocateMultiplePins() or spiPort.begin(). Add
validation logic after these variables are initialized to ensure each pin value
falls within the valid range for your platform, rejecting or clamping
out-of-range values to prevent unstable behavior. Apply the same validation to
the other pin variables mentioned at lines 188-192.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9956e2f-e741-4501-a3b6-7df762d668ff
📒 Files selected for processing (1)
usermods/sd_card/sd_card.cpp
| SPIClass spiPort = SPIClass(HSPI); | ||
| #else | ||
| // Classic ESP32 defaults (VSPI is default/safest for SD cards on standard pins) | ||
| SPIClass spiPort = SPIClass(VSPI); |
There was a problem hiding this comment.
follow the rabbit:
#if` defined(CONFIG_IDF_TARGET_ESP32S3) // FSPI (SPI2) is the global SPI used by dotstar/2-pin LED drivers; use HSPI (SPI3) for SD SPIClass spiPort = SPIClass(HSPI); `#elif` defined(WLED_USE_ETHERNET) // Ethernet builds: dotstar uses HSPI (SPI2), so SD uses VSPI (SPI3) SPIClass spiPort = SPIClass(VSPI); `#else` // Non-Ethernet classic ESP32: dotstar uses global SPI (VSPI/SPI3), so SD uses HSPI (SPI2) SPIClass spiPort = SPIClass(HSPI); `#endif
| // https://oshwa.org/resources/a-resolution-to-redefine-spi-signal-names/ | ||
| #ifdef WLED_USE_SD_SPI | ||
| #if defined(UM_SD_SELECT) | ||
| uint8_t configPinSourceSelect = UM_SD_SELECT; |
There was a problem hiding this comment.
as the rabbit correctly sais, dont change to uint
There was a problem hiding this comment.
Slowly working on getting everything changed..
3 participants
Added support for ESP32-S3
[SD Card] usermod loaded Listing directory: / DIR : System Volume Information FILE: test.txt SIZE: 0Summary by CodeRabbit
-D UM_SD_SELECT,-D UM_SD_CLOCK,-D UM_SD_POCI, and-D UM_SD_PICO, with fallback to the standard default pins when not set.