Fix: SPI IO mapping for ESP32-S3 not be assigned #3201
Conversation
WalkthroughThe initialization of the SPI device pin mapping array was updated. The first SPI device (SPI1) now has specific GPIO pins assigned for MOSI, MISO, and Clock, while the second SPI device (SPI2) remains unassigned with all pins set to -1. No other changes were made. Changes
Estimated code review effort1 (<30 minutes) ✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:77-93
Timestamp: 2025-01-09T13:10:31.388Z
Learning: In nanoFramework SPI transactions, NULL buffers are valid cases when there is no data to send or receive. The code should handle NULL buffers by setting corresponding sizes to 0 instead of throwing exceptions.
🔇 Additional comments (1)
targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp (1)
15-17: Good implementation approach, but resolve GPIO conflicts first.The code structure and formatting look good, and this directly addresses the ArgumentException issue described in the PR. However, please resolve the GPIO pin conflicts with SDMMC1 before merging.
Once the GPIO conflicts are resolved, this will properly fix the SPI IO mapping issue for ESP32-S3.
| int8_t Esp32_SPI_DevicePinMap[MAX_SPI_DEVICES][Esp32SpiPin_Max] ={ | ||
| // SPI1 | ||
| {GPIO_NUM_11, GPIO_NUM_13, GPIO_NUM_12}, | ||
| // SPI2 - no pins assigned | ||
| {-1, -1, -1}}; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: GPIO pin conflict detected between SPI1 and SDMMC1 mappings.
The assigned SPI1 pins conflict with the SDMMC1 configuration:
- GPIO_NUM_12: Used as SPI1 Clock (line 16) and SDMMC1 D2 (line 82)
- GPIO_NUM_13: Used as SPI1 MISO (line 16) and SDMMC1 D3 (line 82)
This will cause hardware conflicts if both peripherals are used simultaneously.
Consider using alternative GPIO pins for SPI1 that don't conflict with other peripherals. Run this script to verify available GPIO options for ESP32-S3:
Please verify that these pin assignments don't create conflicts with existing SDMMC functionality and consider using alternative GPIO pins.
🏁 Script executed:
#!/bin/bash
# Description: Check ESP32-S3 GPIO usage and conflicts in device mappings
echo "=== Analyzing GPIO pin conflicts in ESP32-S3 device mappings ==="
# Search for GPIO_NUM_12 and GPIO_NUM_13 usage across all device mappings
echo "GPIO_NUM_12 usage:"
rg "GPIO_NUM_12" --type cpp targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp -n
echo -e "\nGPIO_NUM_13 usage:"
rg "GPIO_NUM_13" --type cpp targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp -n
# Look for ESP32-S3 GPIO definitions to suggest alternatives
echo -e "\n=== Searching for available GPIO definitions ==="
rg "GPIO_NUM_[0-9]+" --type cpp targets/ESP32/ -A 2 -B 2 | head -20Length of output: 2635
SPI1 pin mapping conflicts with SDMMC1 – update to non-overlapping GPIOs
The current SPI1 mapping in targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp (around line 16):
// SPI1
{ GPIO_NUM_11, GPIO_NUM_13, GPIO_NUM_12 },overlaps with the SDMMC1 D2/D3 pins at line 82:
{ GPIO_NUM_14, GPIO_NUM_15, GPIO_NUM_2, GPIO_NUM_4, GPIO_NUM_12, GPIO_NUM_13 },
… D2=GPIO_NUM_12, D3=GPIO_NUM_13If both SPI and SDMMC are ever used, this will cause hardware conflicts.
A quick fix is to use the module’s default SPI1 pins (as seen in cpu_spi.cpp):
- { GPIO_NUM_11, GPIO_NUM_13, GPIO_NUM_12 },
+ { GPIO_NUM_23, GPIO_NUM_25, GPIO_NUM_19 }, // MOSI, MISO, CLK• Verify that GPIO 23, 25, 19 are free on your ESP32-S3 board
• Update any documentation or examples to reflect the new mapping
• Run hardware tests to ensure both SPI and SDMMC operate correctly without overlap
🤖 Prompt for AI Agents
In targets/ESP32/_common/ESP32_S3_DeviceMapping.cpp around lines 14 to 18, the
SPI1 pin mapping uses GPIO_NUM_11, GPIO_NUM_13, and GPIO_NUM_12, which conflicts
with SDMMC1 pins GPIO_NUM_12 and GPIO_NUM_13. To fix this, update the SPI1 pins
to the module’s default non-overlapping pins such as GPIO_NUM_23, GPIO_NUM_25,
and GPIO_NUM_19 after verifying these pins are free on your ESP32-S3 board.
Also, update any related documentation or examples to reflect this change and
run hardware tests to confirm both SPI and SDMMC work without conflicts.
@dotnet-policy-service agree |
|
The default SPI gpio pins are purposely unassigned as every board uses different pins. There are no common pins used across different boards for the SPI unlike ESP32. The user just needs to assign the pins from the C# app before opening the SPI port. Use the SetPinFunction() method in the nanoframework.Hardware.Esp32 nuget https://docs.nanoframework.net/api/nanoFramework.Hardware.Esp32.Configuration.html |
|
I don't see any need to keep this open |
Description
On Esp32-S3, when the following code runs, an ArgumentException will be triggered because the SPI IO Ports are not assigned in ESP32_S3_DeviceMapping.cpp
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit