Update to IDF 5.4.1 and add basic ESP32_P4 support #3152
Conversation
WalkthroughThis set of changes introduces comprehensive support for the ESP32-P4 target, including new device mapping, build system configuration, toolchain files, and platform-specific source files. It upgrades the Espressif ESP-IDF version from v5.2.3 to v5.4.1 across Dockerfiles, CI pipelines, and build scripts, and adjusts environment variables and paths accordingly. The build system is enhanced with new CMake modules, toolchain presets, and configuration files for RISC-V-based ESP32 variants. Device mapping, filesystem, networking, and peripheral support are updated or extended for ESP32-P4 and other ESP32 variants. Conditional compilation and feature flags are refined to support new hardware capabilities and configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildSystem
participant ESP32_P4_Firmware
participant ESP-IDF
participant DeviceMapping
participant FileSystem
participant Network
User->>BuildSystem: Initiate build for ESP32-P4
BuildSystem->>ESP-IDF: Use v5.4.1 toolchain and SDK
BuildSystem->>ESP32_P4_Firmware: Configure build (CMake, toolchain, config)
ESP32_P4_Firmware->>DeviceMapping: Initialize device pin mappings
ESP32_P4_Firmware->>FileSystem: Set up internal storage (LittleFS)
ESP32_P4_Firmware->>Network: Initialize Ethernet/Wi-Fi (if supported)
ESP32_P4_Firmware->>ESP-IDF: Start main tasks (app_main)
ESP32_P4_Firmware->>User: Firmware runs with configured peripherals and storage
Assessment against linked issues
✨ 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:
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: 4
🔭 Outside diff range comments (2)
src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp (1)
48-193: 💡 Verification agent🧩 Analysis chain
Verify this significant SSL function removal.
The code has commented out several critical network functions including
net_would_block,mbedtls_net_recv,mbedtls_net_send,mbedtls_net_recv_timeout, andmbedtls_net_free. This is a substantial change that could impact SSL/TLS functionality.
🏁 Script executed:
#!/bin/bash # Check if these MbedTLS functions are reimplemented elsewhere or provided by the updated ESP-IDF # Search for any replacement implementations echo "Searching for replacement implementations..." rg "mbedtls_net_recv|mbedtls_net_send|mbedtls_net_recv_timeout|mbedtls_net_free" -A 5 --type cpp # Check SSL tests or implementations that might be affected echo "Checking for affected SSL implementations..." rg "net_would_block" -A 5 --type cppLength of output: 7712
Critical: Missing MbedTLS Network Function Implementations
The functions
net_would_block,mbedtls_net_recv,mbedtls_net_send,mbedtls_net_recv_timeout, andmbedtls_net_freehave been commented out insrc/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp. However, they are still declared insrc/PAL/COM/sockets/ssl/MbedTLS/mbedtls.hand are being referenced in SSL connection code (e.g., inssl_connect_internal.cppandssl_accept_internal.cppviambedtls_ssl_set_bio). This discrepancy could lead to linking failures or unexpected runtime behavior unless an alternative implementation—such as one provided by an updated ESP-IDF—is in place.
- Action Required:
- Verify whether these removals are intentional and that replacement implementations are available (e.g., from the updated ESP-IDF).
- If replacements exist, update both the declarations and the invocation points accordingly.
- Otherwise, reintroduce the missing network function implementations to ensure proper SSL/TLS functionality.
targets/ESP32/_nanoCLR/System.IO.Ports/sys_io_ser_native_System_IO_Ports_SerialPort.cpp (1)
1461-1462:⚠️ Potential issueDevice selector string needs updating for UART_NUM_4
The device selector string includes COM4 but is missing COM5 for UART_NUM_4.
Update the device selector string to include COM5 for the new UART_NUM_4 support:
#if SOC_UART_HP_NUM > 3 "COM4," #endif +#if SOC_UART_HP_NUM > 4 + "COM5," +#endif ;
🧹 Nitpick comments (14)
targets/ESP32/_IDF/sdkconfig.default.esp32s2 (2)
8-8: Confirm deprecated-driver warning disabling.
Be sure to monitor any runtime warnings to avoid missing crucial deprecation details.
25-25: Duplicate UART ISR setting.
CONFIG_UART_ISR_IN_IRAMappears twice (once at line 11, now again at line 25). This duplication can be confusing or overridden. Consider removing one.CMake/Modules/ESP32_P4_sources.cmake (1)
1-5: Initial ESP32_P4 Sources Module FileThis file currently only contains header comments with the appropriate copyright notice. If this file is intended to later define or list source files for the ESP32_P4 target, consider adding a TODO comment or placeholder section to clarify its future purpose.
targets/ESP32/ESP32_P4/target_system_device_spi_config.cpp (1)
1-9: SPI Configuration File PlaceholderThis file is properly set up as an intentional placeholder with the necessary licensing and explanation comments, indicating that no SPI configuration is required for the ESP32_P4 target at this time. If future SPI configurations become necessary, consider adding a TODO note.
targets/ESP32/ESP32_P4/common/CMakeLists.txt (1)
1-5: Minimal CMakeLists.txt for the ESP32_P4 Common DirectoryThis file currently contains only header and copyright information. If this directory is supposed to eventually include common build configuration commands for the ESP32_P4 target, consider adding a TODO or placeholder comment to indicate future enhancements.
targets/ESP32/ESP32_P4/target_system_device_dac_config.cpp (1)
6-9: Observation: Duplicate Placeholder Content for DAC Configuration.
This file is nearly identical to the one intarget_system_devices_dac_config.cpp. If both files are required by the build system for different configuration hooks, then this is fine; otherwise, consider consolidating to avoid redundancy.targets/ESP32/ESP32_P4/target_BlockStorage.h (1)
9-10: Minor Grammar Suggestion
The comment on line 9 reads "this device has 1 block storage devices." It would be clearer to use the singular form: "this device has 1 block storage device."targets/ESP32/ESP32_P4/README.md (1)
5-11: Documentation Clarity and Additional Resources
The provided links for build instructions and managed code getting-started guides are helpful. Consider adding a brief section on any known limitations or future roadmap items (such as planned display support) to further guide users.targets/ESP32/ESP32_P4/target_BlockStorage.c (1)
1-22: Implementation looks good but consider adding error handlingThe implementation follows the standard pattern for block storage initialization. It properly imports necessary headers and correctly calls the BlockStorageList_AddDevice function.
Consider adding error handling to check the return value of BlockStorageList_AddDevice and handle potential failures, which would make the code more robust:
void BlockStorage_AddDevices() { // add device - BlockStorageList_AddDevice( + if (BlockStorageList_AddDevice( (BlockStorageDevice *)&Device_BlockStorage, &ESP32Flash_BlockStorageInterface, &Device_BlockStorageConfig, - false); + false) != TRUE) + { + // Handle error or log failure + } }targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp (1)
6-7: Consider replacing UNUSED macro with standard approachThe UNUSED macro for suppressing compiler warnings is defined within this file.
Consider using a standard approach like
__attribute__((unused))for GCC/Clang or moving this macro definition to a common header file if it's used across multiple files:-#define UNUSED(x) (void)xtargets/ESP32/ESP32_P4/target_FileSystem.cpp (1)
58-61: Placeholder for removable volumes.The FS_MountRemovableVolumes function is implemented as a placeholder, which is appropriate since the current implementation focuses on internal storage only.
Consider adding a comment explaining future plans for removable storage support if applicable, to make it clear whether this is intended to be expanded later.
CMake/toolchain.riscv32-esp32c6-elf.cmake (1)
57-58: Confirm the chosen C/C++ standards meet project needs.You are currently setting C and C++ to version 11. If you need more recent language features (e.g., structured bindings, constexpr enhancements), consider bumping these to C++17 (or beyond), if toolchain support and project requirements allow.
targets/ESP32/CMakePresets.json (1)
354-377: ESP32_P4 USB configuration.You have enabled USB CDC and Ethernet at the same time, which can impact available memory on smaller devices. Confirm the memory footprint is acceptable for the intended use case, or consider providing a minimal configuration preset that omits Ethernet if it’s not strictly needed.
targets/ESP32/_nanoCLR/nanoFramework.Hardware.ESP32/nanoFramework_hardware_esp32_native_Hardware_Esp32_Sleep.cpp (1)
104-237: Consider documenting touchpad wakeup implementation plans for ESP32_P4The current implementation correctly disables touchpad wakeup support for ESP32_P4, but it would be helpful to expand the TODO comment to briefly outline future implementation plans or link to a related issue tracking this feature for ESP32_P4.
#elif defined(CONFIG_IDF_TARGET_ESP32P4) - //TODO + //TODO: Implement touchpad wakeup support for ESP32_P4 when hardware support is confirmed NANOCLR_SET_AND_LEAVE(CLR_E_NOT_SUPPORTED);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
targets/ESP32/_IDF/esp32/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32/partitions_nanoclr_2mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32/partitions_nanoclr_8mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32c6/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32c6/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32c6/partitions_nanoclr_8mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_32mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_8mb.csvis excluded by!**/*.csv
📒 Files selected for processing (82)
.devcontainer/All/Dockerfile.All.SRC(1 hunks).devcontainer/ESP32/Dockerfile.ESP32.SRC(1 hunks)CMake/Modules/ESP32_P4_GCC_options.cmake(1 hunks)CMake/Modules/ESP32_P4_sources.cmake(1 hunks)CMake/Modules/FindESP32_IDF.cmake(4 hunks)CMake/binutils.ESP32.cmake(8 hunks)CMake/riscv-esp32c3.json(1 hunks)CMake/riscv-esp32c6.json(2 hunks)CMake/riscv-esp32h2.json(2 hunks)CMake/riscv-esp32p4.json(1 hunks)CMake/toolchain.riscv32-esp-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32c3-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32c6-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32h2-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32p4-elf.cmake(1 hunks)CMake/toolchain.xtensa-esp32-elf.cmake(1 hunks)CMakePresets.json(1 hunks)azure-pipelines-nightly.yml(2 hunks)azure-pipelines.yml(2 hunks)src/DeviceInterfaces/System.Net/sys_net_native_System_Net_NetworkInformation_WirelessAPConfiguration.cpp(2 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp(4 hunks)targets/ESP32/CMakeLists.txt(1 hunks)targets/ESP32/CMakePresets.json(7 hunks)targets/ESP32/ESP32_C6/target_common.c(1 hunks)targets/ESP32/ESP32_H2/target_common.c(1 hunks)targets/ESP32/ESP32_P4/CMakeLists.txt(1 hunks)targets/ESP32/ESP32_P4/README.md(1 hunks)targets/ESP32/ESP32_P4/common/CMakeLists.txt(1 hunks)targets/ESP32/ESP32_P4/ffconf.h(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoHAL.cpp(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/target_board.h.in(1 hunks)targets/ESP32/ESP32_P4/target_BlockStorage.c(1 hunks)targets/ESP32/ESP32_P4/target_BlockStorage.h(1 hunks)targets/ESP32/ESP32_P4/target_FileSystem.cpp(1 hunks)targets/ESP32/ESP32_P4/target_common.c(1 hunks)targets/ESP32/ESP32_P4/target_common.h.in(1 hunks)targets/ESP32/ESP32_P4/target_lwip_sntp_opts.h(1 hunks)targets/ESP32/ESP32_P4/target_lwipopts.h(1 hunks)targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.h(1 hunks)targets/ESP32/ESP32_P4/target_system_device_adc_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_dac_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_i2c_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_i2c_slave_config.h(1 hunks)targets/ESP32/ESP32_P4/target_system_device_i2s_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_pwm_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_spi_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_devices_dac_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_io_ports_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_windows_storage_config.h(1 hunks)targets/ESP32/_IDF/esp32/app_main.c(1 hunks)targets/ESP32/_IDF/esp32p4/app_main.c(1 hunks)targets/ESP32/_IDF/project_elf_src_esp32p4.c(1 hunks)targets/ESP32/_IDF/sdkconfig.default(0 hunks)targets/ESP32/_IDF/sdkconfig.default.esp32p4(1 hunks)targets/ESP32/_IDF/sdkconfig.default.esp32s2(1 hunks)targets/ESP32/_Network/NF_ESP32_Ethernet.cpp(4 hunks)targets/ESP32/_Network/NF_ESP32_SmartConfig.cpp(2 hunks)targets/ESP32/_Network/NF_ESP32_Wireless.cpp(5 hunks)targets/ESP32/_common/ESP32_C3_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/Target_Network.cpp(4 hunks)targets/ESP32/_common/Target_System_IO_FileSystem.c(1 hunks)targets/ESP32/_common/WireProtocol_HAL_Interface.c(1 hunks)targets/ESP32/_common/targetHAL_ConfigurationManager.cpp(5 hunks)targets/ESP32/_common/targetHAL_Network.cpp(2 hunks)targets/ESP32/_include/Esp32_DeviceMapping.h(1 hunks)targets/ESP32/_include/lwipopts.h(2 hunks)targets/ESP32/_lwIP/nf_sys_arch.c(4 hunks)targets/ESP32/_lwIP_old/nf_api_msg.c(1 hunks)targets/ESP32/_lwIP_old/nf_sys_arch.c(1 hunks)targets/ESP32/_lwip/nf_sockets_priv.h(1 hunks)targets/ESP32/_nanoCLR/System.Device.I2c.Slave/sys_dev_i2c_slave_native_System_Device_I2c_I2cSlaveDevice.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.I2s/sys_dev_i2s_native_System_Device_I2s_I2sDevice.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.Pwm/sys_dev_pwm_native_System_Device_Pwm_PwmChannel.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.Spi/cpu_spi.cpp(7 hunks)targets/ESP32/_nanoCLR/System.IO.Ports/sys_io_ser_native_System_IO_Ports_SerialPort.cpp(2 hunks)targets/ESP32/_nanoCLR/nanoFramework.Hardware.ESP32/nanoFramework_hardware_esp32_native_Hardware_Esp32_Sleep.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- targets/ESP32/_IDF/sdkconfig.default
🧰 Additional context used
🧬 Code Graph Analysis (5)
targets/ESP32/_IDF/esp32/app_main.c (1)
targets/ESP32/_IDF/esp32p4/app_main.c (1)
receiver_task(16-23)
targets/ESP32/_IDF/esp32p4/app_main.c (2)
targets/ESP32/_include/CLR_Startup_Thread.h (1)
CLRStartupThread(10-10)targets/ESP32/_IDF/esp32/app_main.c (3)
receiver_task(15-22)main_task(25-40)app_main(52-68)
targets/ESP32/ESP32_P4/ffconf.h (1)
targets/FreeRTOS/NXP/_FatFs/diskio.c (5)
disk_initialize(43-51)disk_status(26-35)disk_read(59-70)disk_write(80-91)disk_ioctl(100-110)
targets/ESP32/_lwIP_old/nf_api_msg.c (3)
targets/ESP32/_lwIP/nf_api_msg.c (10)
lwip_netconn_do_writemore(1729-1893)netconn_drain(873-944)lwip_netconn_is_deallocated_msg(105-112)lwip_netconn_err_to_msg(121-135)lwip_netconn_is_err_msg(137-153)recv_tcp(312-363)poll_tcp(376-408)accept_function(552-622)lwip_netconn_do_write(1902-1948)lwip_netconn_do_dns_found(2206-2232)targets/ESP32/_lwIP/nf_sys_arch.c (2)
sys_mbox_trypost(249-262)sys_signal_sock_event(587-593)targets/ESP32/_lwIP_old/nf_sys_arch.c (2)
sys_mbox_trypost(250-263)sys_signal_sock_event(597-603)
targets/ESP32/_lwIP_old/nf_sys_arch.c (2)
targets/ESP32/_lwIP/nf_sys_arch.c (2)
set_signal_sock_function(582-585)sys_signal_sock_event(587-593)targets/ESP32/_common/targetHAL_Network.cpp (2)
sys_signal_sock_event(33-36)sys_signal_sock_event(33-33)
🪛 Cppcheck (2.10-2)
targets/ESP32/_lwIP_old/nf_api_msg.c
[error] 1604-1604: Syntax Error
(internalAstError)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
🔇 Additional comments (170)
targets/ESP32/_common/targetHAL_ConfigurationManager.cpp (5)
19-19: Good logging constant additionAdding a logging tag constant follows ESP-IDF best practices for consistent logging identification.
156-156: Helpful debug information addedThis logging statement provides valuable information about the total count of network configurations, which helps with debugging configuration initialization issues.
167-167: Good platform support extensionThe modified preprocessor directive correctly extends support for both Wi-Fi and the new wireless host capability required by ESP32_P4, which aligns with the PR's objective of supporting hosted Wi-Fi on a second chip.
181-181: Useful network interfaces loggingThis logging statement provides visibility into the number of network interfaces being configured, which is helpful for troubleshooting multi-interface setup issues.
210-210: Good validation log for network configurationThis log provides confirmation that the expected number of network configurations have been allocated and enumerated, which helps verify that the initialization process completed correctly.
targets/ESP32/_IDF/sdkconfig.default.esp32s2 (10)
1-2: No concerns with boilerplate.
The header comments are standard output fromidf.py save-defconfig.
16-17: Flash size configuration looks correct.
Allocating 4MB flash is common for ESP32S2 modules.
19-20: Verify partition table filename.
The config points to an S3 partition file (esp32s3/partitions_nanoclr_4mb.csv) despite being for the ESP32S2 target. Confirm this is intentional or update it.
22-23: Compiler optimizations and disabled assertions.
Disabling runtime assertions may complicate debugging. Confirm that the code is stable without them.
32-33: SPIRAM mode settings.
Enabling auto-detection of PSRAM (quad mode) is typically fine for the ESP32S2.
38-38: Watchdog disabled.
Confirm that disablingCONFIG_ESP_TASK_WDT_INITis acceptable, as it can remove an important safety net.
51-51: All logging disabled.
CONFIG_LOG_DEFAULT_LEVEL_NONE=yprevents debugging output. Ensure this is desired, especially for production vs. development builds.
58-59: IPv6 and DHCP server disabled.
This is fine if not needed, but confirm if it matches network requirements.
65-66: Peer certificate retained, no bundle.
Please confirm certificate handling is implemented through another means, or if direct chain-of-trust checks are used.
69-71: Security concern with DES enabled.
DES is considered cryptographically weak. Suggest verifying if it’s truly needed.targets/ESP32/_lwIP_old/nf_sys_arch.c (25)
8-22: Includes look standard.
All required FreeRTOS and lwIP headers are included. No issues noted.
32-94: Mutex implementation follows FreeRTOS patterns.
The usage ofxSemaphoreCreateMutex,xSemaphoreTake,xSemaphoreGive, andvSemaphoreDeleteis correct for a mutex.
104-123: Semaphore creation logic is correct.
The binary semaphore logic with an optional initial give matches lwIP expectations.
130-139: sys_sem_signal implementation is fine.
ReturningerrQUEUE_FULLis handled by an assertion and is typical for a signaling-only scenario.
142-149: sys_sem_signal_isr is correct.
The use ofxSemaphoreGiveFromISRwithwokenappropriately handles ISR context.
157-183: sys_arch_sem_wait code is standard.
Properly handles infinite vs. timed wait with correct rounding of FreeRTOS ticks.
190-195: sys_sem_free usage is correct.
Frees the semaphore reliably and resets the pointer to NULL.
235-241: sys_mbox_post logic is correct.
Posts a message with infinite block time.
250-263: sys_mbox_trypost logic is correct.
ReturnsERR_MEMif the queue is full.
274-291: sys_mbox_trypost_fromisr is standard.
Behavior for high-priority task wake is properly returned asERR_NEED_SCHED.
300-326: sys_arch_mbox_fetch is correct.
Proper logic for infinite vs. timed wait, returningSYS_ARCH_TIMEOUTon failure.
335-352: sys_arch_mbox_tryfetch is correct.
ReturnsSYS_MBOX_EMPTYif no message is present.
354-361: sys_mbox_set_owner is straightforward.
Sets an owner pointer for the mailbox if needed.
394-413: Thread creation logic is correct.
xTaskCreatePinnedToCoreusage is valid, with correct priority and stack size.
419-432: sys_init checks for global mutex and registers lwIP sockets.
Initialization approach is aligned with lwIP on FreeRTOS.
439-443: sys_jiffies retrieves FreeRTOS TickCount.
Implementation is standard for lwIP.
450-454: sys_now returns ms based on tick count.
A typical lwIP usage.
463-471: sys_arch_protect uses a mutex.
Locking the global lwIP-protect mutex ensures concurrency safety.
478-483: sys_arch_unprotect unlocks the global mutex.
Matches the lwIP protect/unprotect pattern.
488-498: sys_thread_sem_get uses lazy initialization.
Retrieves the per-thread semaphore or initializes it if absent.
516-535: Potential issue with sizeof(sys_sem_t).*
Allocatingsizeof(sys_sem_t*)may only reserve space for a pointer, yet usage implies a pointer to aSemaphoreHandle_t. Verify that this matches the actual type size.
537-545: sys_thread_sem_deinit logic is correct.
Frees the semaphore if it was allocated, then resets thread-specific data.
547-551: sys_delay_ms is straightforward.
UsesvTaskDelayfor a blocking delay in milliseconds.
553-584: sys_thread_tcpip logic is valid.
Handles marking the LWIP task and optional core-lock holder checks.
586-604: nanoFramework socket signaling.
Allows hooking a callback without referencing the CLR directly. Good decoupling approach.targets/ESP32/ESP32_P4/target_windows_storage_config.h (1)
1-5: New file: minimal boilerplate.
No code beyond licensing. This is fine for a placeholder header.targets/ESP32/ESP32_P4/target_lwipopts.h (1)
1-9: Clear Placeholder for lwIP OptionsThe file is intentionally left blank, with a detailed comment explaining that the ESP32_P4 target does not override any lwIP options. This clear documentation makes it obvious that no custom lwIP configuration is needed.
targets/ESP32/ESP32_P4/target_system_device_adc_config.cpp (1)
1-9: ADC Configuration File PlaceholderThe file is correctly created as an empty configuration file for ADC, accompanied by clear comments that explain no specific ADC configuration is needed for this target. This is acceptable as a minimal placeholder.
targets/ESP32/ESP32_P4/target_lwip_sntp_opts.h (1)
1-9: Placeholder File for lwIP SNTP OptionsThis new file correctly includes the licensing information and a clear comment indicating that no lwIP SNTP options are overridden for the ESP32_P4 target. The file follows the project’s established placeholder pattern.
targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.cpp (1)
1-9: Clear Placeholder for 1-Wire ConfigurationThe file contains all the necessary licensing headers and a comment explicitly stating that no specific configuration is required for the 1-Wire interface on this target. It is consistent with similar placeholder implementations in the project.
targets/ESP32/ESP32_P4/target_system_device_pwm_config.cpp (1)
1-10: PWM Configuration Placeholder is CorrectThis file serves as an intentional placeholder for PWM configuration for the ESP32_P4 target. It includes the standard licensing headers and a prominent comment noting that no PWM-specific configuration is required.
targets/ESP32/ESP32_P4/target_system_device_i2s_config.cpp (1)
1-9: I2S Configuration File is Intentionally BlankThe file is correctly set up as a placeholder. It includes the proper licensing information and a comment clearly stating that no I2S configuration is needed for the ESP32_P4 target, aligning with the project’s conventions for such placeholders.
targets/ESP32/ESP32_P4/target_system_io_ports_config.cpp (1)
1-9: I/O Ports Configuration Placeholder VerifiedThis new file follows the same established pattern as other configuration placeholder files in the project. It includes the standard header information and a comment indicating that no specific system I/O ports configuration is required for the ESP32_P4 target.
targets/ESP32/ESP32_P4/target_system_devices_dac_config.cpp (2)
1-4: Approved: Standard File Header Usage.
The file header correctly provides licensing information and attribution. No issues detected.
6-9: Note: Intentionally Blank File with Purposeful Comment.
The large banner clearly states that this configuration file is not required for the ESP32_P4 target. This approach helps maintain a modular structure.targets/ESP32/ESP32_P4/target_system_device_dac_config.cpp (1)
1-4: Approved: Consistent Header Information.
The header information is properly maintained with licensing details.targets/ESP32/ESP32_P4/target_system_device_i2c_config.cpp (1)
1-9: Approved: I2C Configuration Placeholder.
The file intentionally remains blank with a clear comment indicating that no specific I2C configuration is required for the ESP32_P4 target. This aligns with the modular configuration strategy.targets/ESP32/_IDF/project_elf_src_esp32p4.c (1)
1-9: Approved: ELF Source Placeholder File.
This file is correctly set up as a placeholder (intentionally left blank) with proper header and banner messaging, which is typical for targets requiring minimal configuration..devcontainer/All/Dockerfile.All.SRC (3)
84-84: Update ESP-IDF Version for Consistency.
The clone command now references--branch v5.4.1for the ESP-IDF repository, which aligns with the PR objectives. Please ensure that any downstream dependencies are compatible with this updated version.
130-131: Verify ESP_PATCH_VER Consistency.
The environment variableESP_PATCH_VERis still set toesp-13.2.0_20230928even though the ESP-IDF branch is updated to v5.4.1. Please verify if this patch version remains appropriate or requires an update to match the new framework version.
135-142: Ensure PATH Environment Consistency with ESP-IDF Version.
The PATH environment variable references/root/.espressif/python_env/idf5.2_py3.11_env/bin, which may be inconsistent with the new ESP-IDF version. Confirm whether the installation script or environment setup should be updated to reflect a v5.4.1 environment.targets/ESP32/ESP32_P4/CMakeLists.txt (3)
1-5: File Header and License Information
The header and license notice are correctly included. This lays a solid groundwork for compliance.
6-7: Module Inclusion Verification
The inclusion of thebinutils.ESP32module is appropriate. Please ensure that this module is available in the build system and correctly configures the necessary binary utilities for the ESP32 platform.
10-10: Target Build Setup Call
The call tonf_setup_target_build()appears to centralize the target’s build configuration. Verify that all required build settings specific to the ESP32-P4 are encapsulated in this function..devcontainer/ESP32/Dockerfile.ESP32.SRC (2)
51-51: ESP-IDF Branch Update
The Dockerfile now clones the ESP-IDF repository from branch v5.4.1 which aligns with the PR objectives. Please verify that the updated branch is fully compatible with the subsequent build and installation steps.
69-76: Environment PATH Consistency Check
The ENV PATH block (lines 69–76) still references the Python environment directory asidf5.2_py3.11_envand includes paths for both xtensa and riscv tools. Ensure that the tooling and environment paths remain valid and consistent with the move to ESP-IDF v5.4.1. Consider updating the environment name if a new one is provided by the updated IDF version.targets/ESP32/ESP32_P4/target_BlockStorage.h (1)
1-5: Header Information and Guard
The header file correctly includes the license information and uses an appropriate header guard. This ensures safe inclusion in multiple files.CMakePresets.json (1)
3-6: Preset Entry Update Verification
The change in line 6 from an xtensa-based preset to"CMake/riscv-esp32c3.json"reflects the migration to RISC‑V for the ESP32-C3 target. Please verify that similar adjustments (e.g., for ESP32-C6, ESP32-H2, and ESP32-P4) are made consistently throughout the presets to align with the updated architecture configurations.targets/ESP32/ESP32_P4/README.md (1)
1-4: ESP32-P4 README Overview
The README provides a clear introduction to the ESP32-P4 target and contains relevant links to documentation. This helps users quickly locate build instructions and additional resources.targets/ESP32/ESP32_H2/target_common.c (1)
11-11: Updated architecture-specific include path.The include path has been properly updated to reference
esp32h2/rom/spi_flash.hinstead of what was likelyesp32c3/rom/spi_flash.h. This change aligns with the PR objectives of correcting incorrectly addressed headers for ESP32_H2.targets/ESP32/ESP32_P4/nanoCLR/nanoHAL.cpp (1)
1-8: Basic ESP32_P4 support initiated with debugger port control flag.This new file introduces a global flag that appears to control debugger port uninitialization behavior. The implementation is straightforward and follows the expected pattern for hardware abstraction layer code.
targets/ESP32/ESP32_P4/target_system_device_i2c_slave_config.h (1)
6-9: Appropriate I2C slave buffer configuration.The buffer sizes (16 bytes) are reasonable for basic I2C slave communication. The constants are well-named and properly documented.
targets/ESP32/_IDF/esp32/app_main.c (1)
63-67:❓ Verification inconclusive
Task core assignments have been swapped for improved stability.
The receiver task has been moved from core 1 to core 0, while the main CLR task has been moved from core 0 to core 1. This change likely addresses the panic issues mentioned in the PR objectives for certain firmware versions on ESP32.
🏁 Script executed:
#!/bin/bash # Check if other ESP32 variants have similar core assignments echo "Checking core assignments in other ESP32 variant app_main files..." grep -r "xTaskCreatePinnedToCore" --include="app_main.c" targets/ESP32/_IDF/ | grep "ReceiverThread\|main_task"Length of output: 988
Core Assignment Update Verified for ESP32
The changes in targets/ESP32/_IDF/esp32/app_main.c (lines 63–67) correctly swap the core assignments by pinning the receiver task to core 0 and the main CLR task to core 1, which should help address the panic issues described in the PR objectives. Verification via grep confirms that:
- In the ESP32 and ESP32p4 variants, the receiver task is created on core 0 and the main task on core 1.
- In the ESP32s3 variant, however, while the receiver task is pinned to core 0 as expected, the main task remains on core 0.
Please confirm whether the differing assignment in the ESP32s3 variant is intentional. This discrepancy might warrant additional review if it was not by design.
targets/ESP32/ESP32_C6/target_common.c (1)
11-11: Good catch - fixing the incorrect header pathThe change correctly updates the include path to use ESP32C6-specific headers instead of ESP32C3 headers, which is appropriate for this target. This aligns with the PR's objective of fixing incorrectly addressed headers.
targets/ESP32/_nanoCLR/System.Device.I2s/sys_dev_i2s_native_System_Device_I2s_I2sDevice.cpp (1)
326-326: Correctly extending I2S workaround to ESP32P4This change appropriately extends the left/right channel swap workaround to include the new ESP32P4 target. Since both ESP32S3 and ESP32P4 need the same register bits set to correct the swapping issue, this ensures consistent I2S audio behavior across platforms.
targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp (1)
15-15:✅ Verification successful
SPI pin mapping updated to use MSPI constants
The change from
SPI_IOMUX_PIN_NUM_*toMSPI_IOMUX_PIN_NUM_*constants is likely necessary for compatibility with IDF 5.4.1, but warrants verification.
🏁 Script executed:
#!/bin/bash # Check if MSPI constants are defined in the ESP-IDF 5.4.1 headers echo "Searching for MSPI pin definitions in ESP-IDF headers..." rg -A 5 "MSPI_IOMUX_PIN_NUM_" --no-headingLength of output: 1649
MSPI Constants Verified for ESP-IDF 5.4.1 Compatibility
The verification confirms that the MSPI constants (
MSPI_IOMUX_PIN_NUM_MOSI,MSPI_IOMUX_PIN_NUM_MISO,MSPI_IOMUX_PIN_NUM_CLK) are correctly defined and used across device mapping files (includingESP32_H2_DeviceMapping.cpp). The change fromSPI_IOMUX_PIN_NUM_*toMSPI_IOMUX_PIN_NUM_*is appropriate for ensuring compatibility with ESP-IDF 5.4.1.targets/ESP32/CMakeLists.txt (1)
67-67:✅ Verification successful
ESP-IDF version updated to 5.4.1
This change directly implements the main objective of the PR by updating the ESP-IDF version tag. However, the comment block above (lines 60-65) indicates this version needs to be synchronized across multiple files.
🏁 Script executed:
#!/bin/bash # Check for consistency of ESP-IDF version references across the codebase echo "Checking for ESP-IDF version references:" rg "5\.4\.1|5\.2\.3" --no-headingLength of output: 854
ESP-IDF version synchronization verified
- The ESP-IDF version in targets/ESP32/CMakeLists.txt is now updated to
"5.4.1".- Similar updates in both azure-pipelines.yml and azure-pipelines-nightly.yml consistently reference version
"v5.4.1", which aligns with the intended update despite the minor formatting difference.- The occurrences of
"5.2.3"in theusbcfg.cfiles are part of CDC section comments and are unrelated to the ESP-IDF version tag update.targets/ESP32/_nanoCLR/System.Device.Pwm/sys_dev_pwm_native_System_Device_Pwm_PwmChannel.cpp (1)
175-175: Update to LEDC channel configuration structureThe code has been updated to accommodate changes in the
ledc_channel_config_tstructure in ESP-IDF 5.4.1, which now includes a sleep mode parameter. The update properly sets the sleep mode toLEDC_SLEEP_MODE_NO_ALIVE_NO_PD, which means the LEDC peripheral won't be kept alive during light sleep, and power-down will be enabled.targets/ESP32/ESP32_P4/nanoCLR/target_board.h.in (1)
1-19: LGTM: New header template for ESP32_P4 supportThis new file adds proper configuration support for the ESP32_P4 target board, including the necessary system information string macros. The file follows project conventions with appropriate copyright notices and includes clear auto-generation warnings to prevent manual modifications.
targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp (1)
14-15: Update to use MSPI pin definitionsThe pin mapping constants have been appropriately updated from
SPI_IOMUX_PIN_NUM_*toMSPI_IOMUX_PIN_NUM_*to align with ESP-IDF 5.4.1 changes. The "MSPI" prefix likely refers to "Memory SPI" interface, which is used in newer ESP32 variants for flash and PSRAM connections.targets/ESP32/_common/WireProtocol_HAL_Interface.c (2)
105-116: Added ESP32_P4 support for Wire ProtocolThe code properly adds ESP32_P4 support for the Wire Protocol using UART0 with the correct pin definitions. This follows the same pattern used for other ESP32 variants.
118-118: Extended USB CDC support for ESP32_P4The conditional compilation check for USB CDC support has been correctly updated to include the ESP32_P4 target, ensuring proper Wire Protocol communication over USB for this new platform.
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_NetworkInformation_WirelessAPConfiguration.cpp (2)
168-168: Improved feature-based conditional compilationThe change from platform-specific checks to feature-based checks is a better approach, making the code more adaptable to different ESP32 variants including the new ESP32_P4. This allows the wireless AP functionality to work on any platform that supports the required features.
266-266: Consistent feature-detection approach for wireless functionalityGood consistency with the earlier change. Using feature-detection macros instead of platform-specific checks ensures that the wireless station deauthentication will work properly across all ESP32 variants that support wireless features.
azure-pipelines-nightly.yml (2)
20-20: Updated ESP-IDF version referenceThe reference tag for the ESP-IDF repository has been updated from v5.2.3 to v5.4.1, which aligns with the PR objective to update to IDF 5.4.1 for latest device support and bug fixes.
589-589: Updated IDF_TAG variableThe IDF_TAG variable has been updated to "v5.4.1" to match the repository reference, ensuring consistency throughout the build pipeline.
targets/ESP32/_include/Esp32_DeviceMapping.h (1)
88-90: Added support for ESP32_P4 targetThe configuration for ESP32_P4 matches that of ESP32_S3, which aligns with the PR objective to add basic ESP32_P4 support. The ADC and LED pin definitions enable proper hardware mapping for the new chip.
targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.h (1)
1-11: New 1-wire configuration for ESP32_P4This new file properly configures the 1-wire interface for ESP32_P4, using UART 2 with appropriate GPIO pins (16 for RX, 17 for TX). The configuration follows the established pattern used for other ESP32 variants.
targets/ESP32/ESP32_P4/target_common.c (1)
1-30: Good implementation of HAL configuration for ESP32_P4.The implementation correctly sets up the HAL system configuration with appropriate hardware parameters and includes a mechanism to update the flash memory size. This follows the same pattern used for other ESP32 targets.
A couple of observations:
- The proper headers specific to ESP32_P4 are included
- The memory configuration parameters are well defined
- The
FixUpHalSystemConfigfunction appropriately updates flash size at runtimeCMake/riscv-esp32c3.json (1)
8-8: Correctly migrated ESP32-C3 preset from Xtensa to RISC-V architecture.The preset name and toolchain file path have been appropriately updated to reflect the RISC-V architecture used by the ESP32-C3. This aligns with the PR objective of renaming config files from "xtensa?" to "riscv?".
Also applies to: 15-15
targets/ESP32/_nanoCLR/System.Device.I2c.Slave/sys_dev_i2c_slave_native_System_Device_I2c_I2cSlaveDevice.cpp (1)
34-34:❓ Verification inconclusive
Updated I2C slave bus check to use high-performance I2C counter.
The change from
SOC_I2C_NUMtoSOC_HP_I2C_NUMis appropriate for the ESP32_P4 support, which likely distinguishes between standard I2C interfaces and high-performance I2C interfaces. This ensures the second I2C slave is only used when multiple high-performance I2C interfaces are available.
🏁 Script executed:
#!/bin/bash # Verify ESP-IDF definitions for I2C interfaces grep -r "SOC_HP_I2C_NUM" --include="*.h" .Length of output: 42
I'm generating an additional verification script before finalizing the review comment. Let's run a broader search for the definition of
SOC_HP_I2C_NUMacross common C/C++ source files:If this still produces no results, it's likely that the
SOC_HP_I2C_NUMmacro is provided externally by the ESP-IDF framework. In that case, please manually verify that your ESP-IDF version defines this macro correctly and that using it in the condition (#if SOC_HP_I2C_NUM > 1) behaves as expected on the ESP32_P4.
I2C Slave Bus Check Macro Update – Manual Verification Recommended
The update replacing
SOC_I2C_NUMwithSOC_HP_I2C_NUMis intended to ensure that the second I2C slave is activated only when multiple high-performance I2C interfaces are available—supporting ESP32_P4's distinct hardware capabilities. However, our initial lookup in header files did not locate a definition forSOC_HP_I2C_NUM, which suggests that this macro is likely provided by the external ESP-IDF library.
- Action Required: Please manually verify that:
- The ESP-IDF version in use defines
SOC_HP_I2C_NUMas expected.- The condition
#if SOC_HP_I2C_NUM > 1correctly differentiates between standard and high-performance I2C interfaces.File to check:
targets/ESP32/_nanoCLR/System.Device.I2c.Slave/sys_dev_i2c_slave_native_System_Device_I2c_I2cSlaveDevice.cpp(line 34)
🏁 Script executed:
#!/bin/bash echo "Searching for the definition of SOC_HP_I2C_NUM in all source files (.c, .cpp, .h, .hpp):" for ext in c cpp h hpp; do echo "Searching in *.$ext files:" grep -R "define[[:space:]]\+SOC_HP_I2C_NUM" --include="*.$ext" . doneLength of output: 782
I2C Slave Bus Macro Update – Manual Verification Required
The code change replacing
SOC_I2C_NUMwithSOC_HP_I2C_NUMis intended to ensure that the second I2C slave is enabled only when multiple high-performance I2C interfaces are available (supporting ESP32_P4). However, our searches did not locate a local definition forSOC_HP_I2C_NUM, which suggests it is likely defined externally by the ESP-IDF framework.Action Items:
- Manual Verification: Confirm that the ESP-IDF version being used defines
SOC_HP_I2C_NUMwith the expected value.- Behavior Check: Ensure that the condition
#if SOC_HP_I2C_NUM > 1correctly differentiates when to enable the second I2C slave based on the available high-performance interfaces.File to check:
targets/ESP32/_nanoCLR/System.Device.I2c.Slave/sys_dev_i2c_slave_native_System_Device_I2c_I2cSlaveDevice.cpp(line 34)azure-pipelines.yml (1)
51-51: Updated ESP-IDF version to v5.4.1.The ESP-IDF version references have been updated from v5.2.3 to v5.4.1 as specified in the PR objectives. This update brings in the latest device support and bug fixes, including those needed for the ESP32_P4.
Also applies to: 535-535
targets/ESP32/_common/Target_System_IO_FileSystem.c (1)
213-214: Good addition of ESP32P4 to the SPI host mapping.The code correctly includes the new ESP32P4 target in the conditional compilation, ensuring it uses SPI2_HOST as its first available bus, consistent with the other modern ESP32 variants. The comment has also been appropriately updated.
targets/ESP32/_common/ESP32_C3_DeviceMapping.cpp (1)
15-15: Updated to MSPI pin definitions for ESP-IDF 5.4.1 compatibility.The code correctly updates the SPI pin definitions from
SPI_IOMUX_PIN_NUM_*toMSPI_IOMUX_PIN_NUM_*to align with the new naming conventions in ESP-IDF 5.4.1.targets/ESP32/_Network/NF_ESP32_SmartConfig.cpp (2)
6-8: Appropriate conditional compilation for ESP32-P4.The added conditional prevents compiling SmartConfig functionality for ESP32-P4, which doesn't support it natively. This aligns with the PR objective to implement hosted Wi-Fi support on a second chip for ESP32-P4.
108-108: Correct closure of conditional compilation block.The endif correctly closes the conditional block that excludes SmartConfig functionality when wireless host support is not available.
targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp (2)
18-23: Incomplete implementation of TouchInterface::Initialize()The Initialize method currently has no implementation details.
This is likely a placeholder for future implementation, but consider adding a comment indicating that this is intentional basic support and will be expanded in the future:
bool TouchInterface::Initialize() { // Setup SPI configuration + // TODO: Implement SPI configuration for ESP32_P4 touch panel return true; }
25-36: Incomplete implementation of TouchInterface::Write_Read()This method marks parameters as unused but returns NULL without actual implementation.
Add comments to clarify this is intentional stub implementation that will be completed in future updates:
CLR_UINT8 *TouchInterface::Write_Read( CLR_UINT8 *valuesToSend, CLR_UINT16 numberOfValuesToSend, CLR_UINT16 numberValuesExpected) { UNUSED(valuesToSend); UNUSED(numberOfValuesToSend); UNUSED(numberValuesExpected); + // TODO: Implement SPI communication for ESP32_P4 touch panel return 0; }CMake/riscv-esp32p4.json (1)
1-47: Configuration looks good for basic ESP32_P4 supportThe configuration preset for ESP32_P4 is well-structured and enables the appropriate features and APIs for this new chip. The RISC-V toolchain is correctly referenced for this architecture.
targets/ESP32/_nanoCLR/System.IO.Ports/sys_io_ser_native_System_IO_Ports_SerialPort.cpp (3)
29-31: Addition of UART4 support looks goodThe addition of the Uart4_PAL variable with proper conditional compilation is implemented correctly.
52-55: Case statement for UART_NUM_3 update looks goodThe modification to handle UART_NUM_3 is implemented correctly.
57-61: Addition of UART_NUM_4 case looks goodThe implementation for the additional UART4 case follows the established pattern and is properly guarded with conditional compilation.
CMake/toolchain.riscv32-esp-elf.cmake (1)
47-48: Architecture enhancement to support broader instruction setThe compiler flags have been updated to include additional instruction set extensions:
- Added atomic instructions (a)
- Added single-precision floating-point support (f)
- Added ESP-specific position-independent code extension (xesppie)
These changes align with the IDF 5.4.1 requirements and enable better performance for floating-point operations.
CMake/toolchain.riscv32-esp32p4-elf.cmake (1)
1-62: New toolchain file for ESP32-P4 support looks goodThis new toolchain file properly configures the build environment for the ESP32-P4 chip with appropriate RISC-V settings. The addition of:
- Proper floating-point ABI specification (
-mabi=ilp32f)- ASM flags configuration
- Comprehensive compiler settings
The configuration correctly supports the ESP32-P4's RISC-V architecture requirements as outlined in the PR objectives.
targets/ESP32/_common/targetHAL_Network.cpp (1)
155-155: Enhanced network event handling for hosted wireless solutionsThe conditional compilation has been expanded to include both native WiFi and hosted wireless solutions. This change enables proper event handling for the ESP32-P4's hosted WiFi configuration, where WiFi functionality is provided by a secondary chip rather than built into the SoC.
Also applies to: 517-520
CMake/riscv-esp32c6.json (2)
8-8: Correctly renamed preset to match RISC-V architectureThe preset has been properly renamed from "xtensa-esp32c6-preset" to "riscv-esp32c6-preset" and the toolchain file path has been updated to use the ESP32-C6 specific toolchain. This aligns with the architectural migration mentioned in the PR objectives.
Also applies to: 15-15
28-28: ADC disabled for ESP32-C6 to address boot hang issueThe ADC API has been correctly disabled to mitigate the boot hang issue mentioned in the PR objectives, which occurs due to ADC calibration loops on ESP32-C6 and ESP32-H2.
CMake/toolchain.riscv32-esp32c3-elf.cmake (1)
1-62: New toolchain file for RISC-V ESP32-C3 looks well-structuredThis new toolchain file correctly sets up the cross-compilation environment for the RISC-V 32-bit ESP32-C3 target. It properly configures compiler paths, flags, and standards.
targets/ESP32/ESP32_P4/target_common.h.in (1)
1-49: Well-structured header template for ESP32_P4 targetThis template header file is well-organized with proper include guards and clear documentation about the memory configuration. The RAM and FLASH values will be populated at runtime by the referenced functions.
CMake/riscv-esp32h2.json (3)
8-8: Preset name updated from Xtensa to RISC-V architectureThe preset name change from "xtensa-esp32h2-preset" to "riscv-esp32h2-preset" correctly reflects the architecture shift mentioned in the PR objectives.
14-16: Toolchain file path updated to use ESP32-H2 specific toolchainThe path has been changed to use a dedicated ESP32-H2 toolchain file instead of the generic RISC-V one, which aligns with the architecture-specific approach in this PR.
28-28: ADC support disabled for ESP32-H2Disabling the ADC API aligns with the PR objectives that mentioned temporarily disabling ADC for ESP32_C6 and ESP32_H2 due to boot hang issues caused by ADC calibration loops.
CMake/toolchain.xtensa-esp32-elf.cmake (1)
47-48: Disabled built-in memory functions to improve stabilityAdding compiler flags to disable built-in memory functions (
-fno-builtin-memcpy,-fno-builtin-memset, etc.) is a good approach to fix the panic issues on ESP32 rev0 with PSRAM mentioned in the PR objectives. This forces the compiler to use the implementations provided by the ESP-IDF instead of compiler builtins.CMake/toolchain.riscv32-esp32h2-elf.cmake (1)
1-62: LGTM! Appropriate toolchain configuration for ESP32-H2The toolchain file correctly sets up the build environment for ESP32-H2 with the RISC-V architecture. The compiler flags, toolchain paths, and standards are properly configured.
targets/ESP32/_IDF/esp32p4/app_main.c (1)
1-57: Standard implementation for ESP32_P4 app_main setupThe file correctly sets up the dual-core task structure for the ESP32_P4 target. The implementation follows the established pattern used for other ESP32 targets, with the receiver task pinned to core 0 and the main CLR task pinned to core 1.
targets/ESP32/ESP32_P4/ffconf.h (1)
1-349: Good implementation of FatFs configuration file for ESP32-P4This configuration file is well-structured with appropriate settings for the FatFs module on ESP32-P4. The configuration includes proper disk I/O function name redefinitions to prevent collisions, memory allocation functions, and filesystem settings optimized for the target platform.
A few observations:
- The file includes comprehensive LFN (Long File Name) configuration based on build options
- Appropriate memory management through
ff_memallocandff_memfreefunctions- Correct handling of sector sizes for both SD card and wear-levelling library
- Thread safety enabled with proper timeout configuration
targets/ESP32/_IDF/sdkconfig.default.esp32p4 (1)
1-84:❓ Verification inconclusive
Well-configured default settings for ESP32-P4 target
The configuration file includes appropriate settings for the ESP32-P4 target with sensible defaults for memory, networking, and peripherals.
Some key points:
- Flash configuration is properly set for 4MB with QIO mode
- Bluetooth is conditionally disabled with a clear comment explaining why
- Power management and sleep configurations are enabled for better power efficiency
- Network settings are appropriate with reasonable socket limits and hostname configuration
- Security settings via mbedTLS are properly configured
Consider verifying the ESP-IDF version compatibility by checking if any new configuration options should be included:
🏁 Script executed:
#!/bin/bash # Check for any missing or deprecated configuration options in sdkconfig.default.esp32p4 # Get the list of all possible ESP32-P4 configuration options from ESP-IDF grep -r "CONFIG_.*=.*" --include="Kconfig*" $(find . -path "*esp-idf*/components" -type d 2>/dev/null) | \ grep -i "depends on.*esp32p4" | \ grep -v "# CONFIG_" | \ sort | uniqLength of output: 160
ESP32-P4 Configuration Looks Solid – Please Manually Confirm ESP-IDF Compatibility
The configuration file is well set up for the ESP32-P4 target with appropriate defaults for flash size/mode, Bluetooth, power management, networking, and security via mbedTLS. Automated checks for new or deprecated ESP-IDF options produced no output, which suggests that no additional configuration options are evidently required. However, since the verification output was inconclusive, please manually verify against the latest ESP-IDF release notes to ensure no new options should be included.
- The flash settings (4MB size and QIO mode) are appropriately configured.
- Bluetooth is clearly disabled with relevant comments, aligning with current support limitations.
- Key power management, network, and security options appear correctly set.
- No new/deprecated options were automatically detected; however, manual confirmation is recommended.
targets/ESP32/_Network/NF_ESP32_Wireless.cpp (4)
11-11: Good extension of WiFi support for ESP32-P4The preprocessor condition has been expanded to include support for both traditional WiFi and hosted wireless configurations, which is necessary for the ESP32-P4.
131-132: External function declaration for hosted wireless initializationAppropriate addition of the external C function for initializing the hosted wireless functionality.
157-159: Initialization of hosted wireless when supportedThe code correctly initializes the hosted wireless functionality when the appropriate configuration is enabled, which is essential for ESP32-P4 wireless support.
348-359: Appropriate conditional handling for smartconfigThe code correctly disables smartconfig support for ESP32-P4 since it's not currently supported in the wireless host configuration, preventing potential runtime issues.
targets/ESP32/_common/Target_Network.cpp (2)
9-9: Added necessary WiFi types headerThe inclusion of the ESP WiFi types header is necessary for the wireless host support functionality.
48-48: Consistent extension of wireless supportThe preprocessor conditions have been consistently updated throughout the file to include both traditional WiFi and hosted wireless configurations, maintaining code coherence with the other network-related files.
Also applies to: 93-94, 119-119
targets/ESP32/_Network/NF_ESP32_Ethernet.cpp (5)
42-102: ESP32_P4 Ethernet configuration added with proper pin assignmentsThis change adds robust support for Ethernet on the new ESP32_P4 chip alongside the existing ESP32 configuration. The new
NANO_ETH_ESP32_EMAC_DEFAULT_CONFIGmacro defines target-specific settings including different GPIO pins for the SMI interface (31/27 for P4 vs 23/18 for ESP32) and RMII data interface settings.
121-121: Good replacement of old macro with new target-aware configurationReplacing
ETH_ESP32_EMAC_DEFAULT_CONFIGwithNANO_ETH_ESP32_EMAC_DEFAULT_CONFIGensures the proper target-specific Ethernet settings are used during initialization.
153-157: Updated logging to use dynamic pin configurationAccessing pin numbers from the configuration structure rather than constants makes the logging more accurate for different target platforms.
159-160: Fixed assignment of SMI pins from configuration constantsThe GPIO pin assignments for MDC and MDIO are correctly mapped from the defined constants to the configuration structure.
176-176: Improved conditional compilation with defined()Changing from
#ifdefto#if defined()is a more robust pattern that works better with complex conditional expressions and is consistent with other code in the file.targets/ESP32/_nanoCLR/System.Device.Spi/cpu_spi.cpp (6)
173-174: Default level for SPI data pins addedAdding the
data_io_default_levelfield with a value of 0 configures the default state of data IO pins when no transaction is in progress, which is necessary for the updated ESP-IDF 5.4.1.
184-186: Extended target support to include ESP32_P4Added ESP32_P4 to the target check conditional to ensure the SPI functionality works correctly on the new chip with its specific SPI host numbering.
195-196: Error logging updated for ESP32_P4 supportError message consistently includes the new target in the comment for better debugging and clarity.
217-220: Updated bus release logic for ESP32_P4The bus uninitialize function correctly handles the ESP32_P4 target with appropriate SPI host mapping.
229-231: Error logging comments updated for all supported targetsError message comments comprehensively list all supported ESP32 variants including the new P4 target.
367-370: SPI device addition supports ESP32_P4Device addition logic now correctly handles the ESP32_P4 target with proper SPI host selection.
targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp (3)
1-14: Well-structured header with appropriate includes for graphics memory managementThe file includes necessary headers for nanoFramework integration and ESP32-specific functionality, with proper header guards and licensing information.
15-29: Good documentation of SPI RAM integration approachThe comments explain how the external SPI RAM is mapped in the ESP32 memory space and the approach for managing this memory region.
30-79: Robust implementation of graphics memory allocation with fallback strategyThe
GraphicsHeapLocationfunction provides:
- Memory reuse for efficiency
- Allocation from SPIRAM when available
- Fallback to regular RAM when SPIRAM is unavailable
- Size limitation to prevent excessive allocation
- Proper memory tracking for future reuse
This ensures that graphics operations have access to sufficient memory while adapting to different hardware configurations.
targets/ESP32/_lwIP/nf_sys_arch.c (4)
1-8: Updated licensing headers with SPDX formatThe licensing information has been updated to use SPDX-style comments, clearly indicating copyright holders and license type (BSD-3-Clause).
395-396: Updated format specifier for better compatibilityChanged from
%xto%U32_Ffor the thread task handle, which ensures proper formatting across different platforms and sizes.
414-415: Improved error logging formatRemoved trailing newline from error log message, which is more consistent with ESP-IDF logging patterns where the logger automatically appends newlines.
436-437: Documentation correctionFixed a minor typo in the function description, changing "miliseconds" to "milliseconds" for better accuracy.
targets/ESP32/_lwip/nf_sockets_priv.h (8)
1-4: Clear file description comment block.The file description accurately explains that this contains internal implementations for the Sockets API that shouldn't be used in application code.
6-36: Appropriate licensing and attribution.The file includes proper licensing information and attribution to the original author, which is important for open source compliance.
37-42: Proper include guards and dependencies.The file correctly uses include guards and includes the necessary lwIP headers while being conditionally compiled based on lwIP configuration.
61-65: Well-structured socket data union.The union for socket data is properly defined to handle both netbuf and pbuf types efficiently.
66-99: Socket structure with NF-specific extension.The lwip_sock structure is well-designed and includes a nanoFramework-specific extension (error field) that is clearly marked with comments indicating the custom change.
101-134: Robust sockopt data structure.The sockopt data structure provides a comprehensive mechanism for handling socket options with proper memory alignment considerations for both MPU compatible and non-compatible configurations.
141-141: Debug function for socket access.The debug function declaration will be helpful for troubleshooting socket-related issues.
143-178: Complete select callback structure.The select callback structure is well-defined with support for both standard select and poll operations, with proper semaphore handling for thread synchronization.
CMake/binutils.ESP32.cmake (6)
187-191: Added ESP32_P4 to CPU type condition.The CPU type is correctly set to "riscv" for ESP32_P4, which is consistent with other RISC-V based ESP32 variants.
470-473: Added ESP32_P4 to partition table generation conditions.ESP32_P4 is now included in the list of targets that need 4MB partition tables, ensuring proper partition allocation for this new target.
485-488: Added ESP32_P4 to 8MB partition table generation.ESP32_P4 is now included in the targets that support 8MB flash partition tables, consistent with other ESP32 variants with similar capabilities.
506-517: Added ESP32_P4 support for 32MB flash.32MB partition table generation now includes ESP32_P4, which indicates this chip supports larger flash sizes similar to ESP32_S3.
606-714: ESP32_P4 specific WiFi implementation.The implementation correctly uses ESP32_P4-specific WiFi components (esp_wifi_remote and esp_hosted) instead of the standard esp_wifi, which suggests this chip has a different WiFi architecture that requires hosted support on a second chip.
937-941: Enabled Smartconfig for ESP32_P4.Smartconfig is explicitly enabled for ESP32_P4, which is important for WiFi provisioning on this new target.
targets/ESP32/_include/lwipopts.h (2)
369-369: Updated DHCP request backoff sequence macro.The function signature has been updated to include the state parameter, which provides more context for calculating backoff times based on the DHCP state.
381-387: Improved DHCP timeout macros with safer implementation.The DHCP timeout calculation macros have been replaced with safer implementations using do-while(0) pattern, which is a best practice for C macros to avoid unexpected behavior in compound statements.
targets/ESP32/ESP32_P4/target_FileSystem.cpp (4)
7-25: Proper inclusion of necessary headers and extern references.The file correctly includes all required headers and external references for file system and stream driver interfaces, with conditional compilation for SDC support.
26-34: Well-structured file system interfaces array.The available file system interfaces array is properly initialized with LittleFS and conditionally FATFS, with a constant indicating the installed count.
35-38: Global variables for file system management.The necessary global variables for managing file system volumes and driver details are properly declared.
39-57: Basic FS_AddVolumes implementation.The function correctly initializes a single internal storage volume using LittleFS, with appropriate memory allocation and configuration.
CMake/toolchain.riscv32-esp32c6-elf.cmake (2)
12-18: Macro definition is well-structured.The
nf_set_compiler_varmacro is clear and follows conventional best practices for setting compiler variables in a cross-compiling context.
47-48: Embedded architecture flags appear correct.Specifying
-march=rv32imac_zicsr_zifenceialong with-Wno-frame-addressaligns well with ESP32-C6’s RISC-V compiler requirements for most embedded development scenarios.targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp (2)
48-60: Array size vs. comment mismatch.The comment indicates “16 channels LED1 to LED16,” yet
Esp32_LED_DevicePinMaphas a length of 8. Please verify whether additional channels are defined elsewhere or if the comment needs updating.
83-85: SDMMC pin definitions differ from the comment’s pin count.The comment enumerates more signals (Clock, Command, D0, D1, D2, D3, D4, D5, D6, D7, D8) than the 6-element array actually provides. Confirm if the device only requires these 6 pins or if the comment is outdated.
targets/ESP32/CMakePresets.json (2)
5-8: New RISC-V includes appear consistent.References to
riscv-esp32c3.json,riscv-esp32c6.json,riscv-esp32h2.json, andriscv-esp32p4.jsonshould be verified against the rest of the repository to confirm that all RISC-V toolchain files are consistently referenced and present.
327-351: Initial support for ESP32_P4_UART.The newly introduced preset inherits from the
riscv-esp32p4-presetand sets relevant cache variables (Ethernet pins, Wi-Fi). This is well-aligned with typical ESP-IDF configurations for UART-based provisioning.targets/ESP32/_lwIP_old/nf_api_msg.c (1)
1601-1604: Static analysis “syntax error” is likely a false positive.This usage of a nested
do { … } while(0)block insideLWIP_ERROR()is standard lwIP practice. The static analysis complaint can be safely disregarded unless your compiler genuinely fails to parse it.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 1604-1604: Syntax Error
(internalAstError)
CMake/Modules/FindESP32_IDF.cmake (5)
12-13: Driver components reorganization noted.These commented-out include paths suggest a deliberate reorganization of the driver component structure in ESP-IDF 5.4.1. This aligns with the ESP-IDF's ongoing modularization efforts to improve component independence and reduce dependencies.
Also applies to: 29-29, 34-34, 40-40
30-33: Updated to use new esp_driver_ components.*The migration from generic driver includes to specific esp_driver_* components reflects ESP-IDF 5.4.1's architecture changes. This improves modularity and allows for more focused dependency management.
Also applies to: 36-39, 41-42
24-24: Additional support directories added.The inclusion of register directories, DMA support, and WiFi local components provides necessary headers for the new ESP32_P4 chip support as outlined in the PR objectives.
Also applies to: 26-26, 65-65
94-94: USB Serial JTAG driver support added.Adding support for the USB serial JTAG driver enhances the debugging capabilities for the new ESP32_P4 target.
98-99: Enhanced ROM support for target series.These additional include paths provide access to ROM-specific functions for each target series, which is essential for proper operation of the ESP32_P4 and other chips with the updated IDF version.
targets/ESP32/_nanoCLR/nanoFramework.Hardware.ESP32/nanoFramework_hardware_esp32_native_Hardware_Esp32_Sleep.cpp (2)
167-169: New ESP32_P4 target support added with appropriate placeholderThe code correctly adds conditional handling for the new ESP32_P4 target in the touchpad wakeup functionality. Currently, it's marked with a TODO and returns CLR_E_NOT_SUPPORTED, which aligns with the PR objective of adding basic ESP32_P4 support without implementing all features yet.
114-115: LGTM: Improved variable scopingThe reorganization of the
pad1andcoefficientvariable declarations moves them into their respective target-specific conditional blocks, improving code organization by ensuring variables are only declared in the scope where they're needed.Also applies to: 171-172
AdrianSoundy
added
the
Platform: ESP32
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp (1)
161-161: Consistent comment style changes, but within commented-out codeComment style has been changed from block comments (
/* */) to single-line comments (//). While this is a minor formatting update, it's only relevant if the functions were to be uncommented later.If the plan is to eventually restore these functions, consider applying consistent comment styles throughout the file. However, the more important question is whether these functions are meant to be permanently commented out.
Also applies to: 178-178
targets/ESP32/_IDF/sdkconfig.default.esp32s2 (4)
8-13: Review the suppression of deprecation warnings.
Enabling these flags will hide important warnings on deprecated drivers (ADC, PCNT, RMT, I2S). Although it declutters builds, it can also mask future migration or compatibility issues. If any of these drivers are actively used, consider addressing the deprecations rather than suppressing the notices.
37-37: Consider re-enabling Task Watchdog Timer.
Disabling the Task WDT might complicate debugging of process stalls or deadlocks. If stability is a concern, re-enabling WDT is recommended.
50-50: No default logs may hinder troubleshooting.
Suppressing logs withCONFIG_LOG_DEFAULT_LEVEL_NONE=yhelps reduce overhead but can make diagnosing runtime issues difficult. Consider a minimal log level (e.g., Error or Warn).
68-70: DES is considered obsolete.
EnablingCONFIG_MBEDTLS_DES_C=yintroduces a weak cipher. If not strictly required, disable DES for better security.Below is a sample diff to remove DES support:
-CONFIG_MBEDTLS_DES_C=y +CONFIG_MBEDTLS_DES_C=n
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMake/Modules/ESP32_P4_GCC_options.cmake(1 hunks)azure-pipelines.yml(3 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp(4 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp(1 hunks)targets/ESP32/_IDF/sdkconfig.default.esp32s2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp
- CMake/Modules/ESP32_P4_GCC_options.cmake
⏰ Context from checks skipped due to timeout of 90000ms (27)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, TI, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- GitHub Check: build-target (ESP32_C6_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
🔇 Additional comments (9)
azure-pipelines.yml (3)
51-51: Update ESP-IDF Repository Reference
The reference in theesp32_idfrepository has been updated from an older tag (likely v5.2.3) torefs/tags/v5.4.1. This change is in line with the PR objective to upgrade the IDF to version 5.4.1 and should help fetch the latest bug fixes and device support.
519-527: Add New ESP32_P4_UART Target Configuration
A new build matrix entry for the ESP32_P4 target (ESP32_P4_UART) has been added with the following properties:
- TargetBoard: ESP32_P4
- TargetSeries: "esp32p4"
- IDF_Target: esp32p4
- TargetName: ESP32_P4_UART
- PackageName: ESP32_P4_UART
- CMakePreset: ESP32_P4_UART
This addition introduces basic support for the new ESP32_P4 chip, aligning with the PR objectives. Please verify that the (currently empty)
BuildOptionsfield suffices for the minimal support; you may need to extend it as the support is iteratively improved.
544-544: Synchronize IDF_TAG Variable Update
The variableIDF_TAGin the ESP32 job has been updated to"v5.4.1", ensuring consistency with the updated ESP-IDF repository reference. This alignment is essential for the build pipeline to use the correct version across all related configurations.src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp (1)
48-193:✅ Verification successful
Critical: Multiple key MbedTLS networking functions have been commented out
Significant portions of code including
net_would_block,mbedtls_net_recv,mbedtls_net_send, andmbedtls_net_recv_timeoutfunctions have been commented out. These functions are essential for SSL/TLS functionality.This change will break SSL connections unless replacements are provided by the updated ESP-IDF v5.4.1. Please verify that:
- These functions are now provided by the ESP-IDF v5.4.1
- Alternative implementations exist elsewhere in the codebase
- The SSL functionality is intentionally being disabled or modified
🏁 Script executed:
#!/bin/bash # Check if these MbedTLS functions are implemented elsewhere in the codebase echo "Searching for alternative implementations of the commented-out MbedTLS functions..." echo "Checking for net_would_block implementations:" rg -p "net_would_block\s*\(" --type cpp echo "Checking for mbedtls_net_recv implementations:" rg -p "mbedtls_net_recv\s*\(" --type cpp echo "Checking for mbedtls_net_send implementations:" rg -p "mbedtls_net_send\s*\(" --type cpp echo "Checking for mbedtls_net_recv_timeout implementations:" rg -p "mbedtls_net_recv_timeout\s*\(" --type cpp echo "Checking for references to these functions (to see what might break):" rg -p -A 2 -B 2 "mbedtls_net_(recv|send|free)" --type cppLength of output: 4356
SSL Functions Verified: Active Implementations in Use
- The legacy block comment in
src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp(lines 48–193) contains deprecated implementations that are no longer active.- Active implementations of
net_would_block,mbedtls_net_recv,mbedtls_net_send, andmbedtls_net_recv_timeoutare available as declared insrc/PAL/COM/sockets/ssl/MbedTLS/mbedtls.hand defined inssl_generic.cpp(e.g., functions at lines 73, 108, and 143).- References in files like
ssl_connect_internal.cppcorrectly use these active implementations, and the ESP-IDF v5.4.1 requirements are satisfied.- If the legacy commented code is not needed for historical reference, consider removing it to improve code clarity.
targets/ESP32/_IDF/sdkconfig.default.esp32s2 (5)
1-2: No concerns for auto-generated header.
The initial comments reflecting that the file was generated withidf.py save-defconfiglook consistent and pose no direct issues.
19-19: Verify custom partition table for correct target.
Using anesp32s3partition table file in anesp32s2config may be unintentional or cause mismatched memory layouts. Confirm this is correct for the S2 target.
31-32: Confirm SPIRAM mode settings for S2 hardware.
EnablingCONFIG_SPIRAM_MODE_QUADand auto-detection is suitable only if external PSRAM is actually present on the target. Verify that your physical board or device includes supported SPIRAM.
57-58: Check implications of disabling DHCP server and IPv6.
Preventing DHCP server (CONFIG_LWIP_DHCPS=n) and IPv6 (CONFIG_LWIP_IPV6=n) might limit network versatility. Confirm that these features are unwanted before dropping them.
64-65: Review memory impact of keeping peer certificates.
WithCONFIG_MBEDTLS_SSL_KEEP_PEER_CERTIFICATE=yand the certificate bundle disabled, ensure your application or handshake logic needs retained certificates in memory, as this may affect RAM usage.
|
Not sure what's going on the build here. Is it a cmake issue ? Builds locally ok Looking at code it like the environment variable ESP_ROM_ELF_DIR is missing |
|
For the smoke tests, it looks like there is something wrong with installing openocd: |
|
Similarly Azure-pipelines is struggling to find GDB (perhaps this can be switched off with a flag)...
|
|
Perhaps this needs some love in the docker container(s): Notably: and changes to the patch version (from https://github.com/espressif/crosstool-NG/releases)? |
|
Speculative changes here: f9c6fe8 |
Missing ESP_ROM_ELF_DIR path |
Automated fixes for code style.
Mark them _nkweak so they will be ignored if methods are available from mbedtls port files.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
targets/ESP32/_Network/NF_ESP32_Ethernet.cpp (1)
166-174: 🛠️ Refactor suggestionPin reservation list is still ESP32‑specific
The run‑time pin reservations (TX/RX, CRS_DV…) are hard‑coded for the original ESP32 RMII mapping.
ESP32‑P4 uses{49,34,35,28,29,30}(as declared in the new macro) so reservations and logs will be wrong, potentially allowing another task to repurpose the real Ethernet pins.Please wrap the reservation block in target‑specific
#ifguards or derive the pin list fromesp32_emac_config.emac_dataif_gpio.
🧹 Nitpick comments (9)
CMake/toolchain.riscv32-esp32h2-elf.cmake (2)
6-7: Consider replacing deprecated CMakeForceCompilerThe
CMakeForceCompilermodule is deprecated in modern CMake. While it's included here, it doesn't appear to be actively used in the rest of the file. Consider removing this include or replacing it with the recommended approach of directly setting compiler variables.-include(CMakeForceCompiler)
56-58: Consider future updates to newer language standardsWhile C11 and C++11 standards are appropriate for current use, consider evaluating the benefits of newer standards (C17/C++17) for future updates, if they're supported by the toolchain and relevant for your embedded application needs.
targets/ESP32/ESP32_P4/ffconf.h (2)
9-9: Duplicate inclusion of<sys/param.h>The header is included at lines 9 and 329. One copy is enough and keeps compile times down.
Also applies to: 329-329
313-327: Timeout conversion may truncate to zero
FF_FS_TIMEOUTis defined as#define FF_FS_TIMEOUT (CONFIG_FATFS_TIMEOUT_MS / portTICK_PERIOD_MS)If
CONFIG_FATFS_TIMEOUT_MSis smaller than one RTOS tick, the integer division yields 0, disabling the timeout unintentionally. Consider rounding up:-#define FF_FS_TIMEOUT (CONFIG_FATFS_TIMEOUT_MS / portTICK_PERIOD_MS) +#define FF_FS_TIMEOUT ((CONFIG_FATFS_TIMEOUT_MS + portTICK_PERIOD_MS - 1) / portTICK_PERIOD_MS)This keeps the timeout ≥ 1 tick whenever the millisecond value is non‑zero.
targets/ESP32/_lwip/nf_sockets_priv.h (1)
137-142: Missingextern "C"wrapper for diagnostic helper
lwip_socket_dbg_get_socket()is declared outside the earlierextern "C"block, so C++ compilation will mangle the symbol and callers written in C will not link.Move the declaration inside the
extern "C"section or add its own wrapper:-struct lwip_sock* lwip_socket_dbg_get_socket(int fd); +#ifdef __cplusplus +extern "C" { +#endif +struct lwip_sock* lwip_socket_dbg_get_socket(int fd); +#ifdef __cplusplus +} +#endiftargets/ESP32/_IDF/sdkconfig.default.esp32s2 (2)
37-37: Task watchdog timer disabled by default.Disabling the task watchdog timer initialization may help prevent spurious resets during debugging, but could mask deadlocks or hangs in production. Consider if this should be conditionally enabled in release builds.
50-50: Default logging level set to NONE.Setting the default logging level to NONE will completely disable logging, which may make debugging difficult. Consider using at least ERROR level for critical issues while keeping minimal resource usage.
targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp (2)
27-29: Consider adding memory cleanup mechanism.Static pointers track allocated memory, but there's no mechanism to free this memory if the graphics subsystem is reinitialized or reconfigured during runtime.
Consider adding a companion method to release the allocated memory when needed:
+ bool GraphicsMemory::ReleaseGraphicsHeap() + { + if (heapStartingAddress != 0) + { + heap_caps_free(heapStartingAddress); + heapStartingAddress = 0; + heapEndingAddress = 0; + return true; + } + return false; + }
36-36: Consider making memory size configurable.The 2MB graphics memory block size is hard-coded without any configuration option. Consider making this configurable based on screen resolution or available in a header/config file.
- CLR_INT32 graphicsMemoryBlockSize = 2 * 1024 * 1024; + // Default to 2MB but allow configuration + #ifndef ESP32_P4_GRAPHICS_MEMORY_SIZE + #define ESP32_P4_GRAPHICS_MEMORY_SIZE (2 * 1024 * 1024) + #endif + CLR_INT32 graphicsMemoryBlockSize = ESP32_P4_GRAPHICS_MEMORY_SIZE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
targets/ESP32/_IDF/esp32/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32/partitions_nanoclr_2mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32/partitions_nanoclr_8mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32c6/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32c6/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32c6/partitions_nanoclr_8mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_32mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_8mb.csvis excluded by!**/*.csv
📒 Files selected for processing (79)
.devcontainer/All/Dockerfile.All.SRC(2 hunks).devcontainer/ESP32/Dockerfile.ESP32.SRC(2 hunks)CMake/Modules/ESP32_P4_GCC_options.cmake(1 hunks)CMake/Modules/ESP32_P4_sources.cmake(1 hunks)CMake/Modules/FindESP32_IDF.cmake(4 hunks)CMake/binutils.ESP32.cmake(8 hunks)CMake/riscv-esp32c3.json(1 hunks)CMake/riscv-esp32c6.json(2 hunks)CMake/riscv-esp32h2.json(2 hunks)CMake/riscv-esp32p4.json(1 hunks)CMake/toolchain.riscv32-esp-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32c3-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32c6-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32h2-elf.cmake(1 hunks)CMake/toolchain.riscv32-esp32p4-elf.cmake(1 hunks)CMake/toolchain.xtensa-esp32-elf.cmake(1 hunks)CMakePresets.json(1 hunks)azure-pipelines-nightly.yml(2 hunks)src/DeviceInterfaces/System.Net/sys_net_native_System_Net_NetworkInformation_WirelessAPConfiguration.cpp(2 hunks)src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp(6 hunks)targets/ESP32/CMakeLists.txt(1 hunks)targets/ESP32/CMakePresets.json(7 hunks)targets/ESP32/ESP32_C6/target_common.c(1 hunks)targets/ESP32/ESP32_H2/target_common.c(1 hunks)targets/ESP32/ESP32_P4/CMakeLists.txt(1 hunks)targets/ESP32/ESP32_P4/README.md(1 hunks)targets/ESP32/ESP32_P4/common/CMakeLists.txt(1 hunks)targets/ESP32/ESP32_P4/ffconf.h(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/nanoHAL.cpp(1 hunks)targets/ESP32/ESP32_P4/nanoCLR/target_board.h.in(1 hunks)targets/ESP32/ESP32_P4/target_BlockStorage.c(1 hunks)targets/ESP32/ESP32_P4/target_BlockStorage.h(1 hunks)targets/ESP32/ESP32_P4/target_FileSystem.cpp(1 hunks)targets/ESP32/ESP32_P4/target_common.c(1 hunks)targets/ESP32/ESP32_P4/target_common.h.in(1 hunks)targets/ESP32/ESP32_P4/target_lwip_sntp_opts.h(1 hunks)targets/ESP32/ESP32_P4/target_lwipopts.h(1 hunks)targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.h(1 hunks)targets/ESP32/ESP32_P4/target_system_device_adc_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_dac_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_i2c_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_i2c_slave_config.h(1 hunks)targets/ESP32/ESP32_P4/target_system_device_i2s_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_pwm_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_device_spi_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_devices_dac_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_system_io_ports_config.cpp(1 hunks)targets/ESP32/ESP32_P4/target_windows_storage_config.h(1 hunks)targets/ESP32/_IDF/esp32/app_main.c(1 hunks)targets/ESP32/_IDF/esp32p4/app_main.c(1 hunks)targets/ESP32/_IDF/project_elf_src_esp32p4.c(1 hunks)targets/ESP32/_IDF/sdkconfig.default(0 hunks)targets/ESP32/_IDF/sdkconfig.default.esp32p4(1 hunks)targets/ESP32/_IDF/sdkconfig.default.esp32s2(1 hunks)targets/ESP32/_Network/NF_ESP32_Ethernet.cpp(4 hunks)targets/ESP32/_Network/NF_ESP32_SmartConfig.cpp(2 hunks)targets/ESP32/_Network/NF_ESP32_Wireless.cpp(5 hunks)targets/ESP32/_common/ESP32_C3_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp(1 hunks)targets/ESP32/_common/Target_Network.cpp(4 hunks)targets/ESP32/_common/Target_System_IO_FileSystem.c(1 hunks)targets/ESP32/_common/WireProtocol_HAL_Interface.c(1 hunks)targets/ESP32/_common/targetHAL_ConfigurationManager.cpp(5 hunks)targets/ESP32/_common/targetHAL_Network.cpp(2 hunks)targets/ESP32/_include/Esp32_DeviceMapping.h(1 hunks)targets/ESP32/_include/lwipopts.h(2 hunks)targets/ESP32/_lwIP/nf_sys_arch.c(4 hunks)targets/ESP32/_lwip/nf_sockets_priv.h(1 hunks)targets/ESP32/_nanoCLR/System.Device.I2c.Slave/sys_dev_i2c_slave_native_System_Device_I2c_I2cSlaveDevice.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.I2s/sys_dev_i2s_native_System_Device_I2s_I2sDevice.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.Pwm/sys_dev_pwm_native_System_Device_Pwm_PwmChannel.cpp(1 hunks)targets/ESP32/_nanoCLR/System.Device.Spi/cpu_spi.cpp(7 hunks)targets/ESP32/_nanoCLR/System.IO.Ports/sys_io_ser_native_System_IO_Ports_SerialPort.cpp(2 hunks)targets/ESP32/_nanoCLR/nanoFramework.Hardware.ESP32/nanoFramework_hardware_esp32_native_Hardware_Esp32_Sleep.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- targets/ESP32/_IDF/sdkconfig.default
✅ Files skipped from review due to trivial changes (10)
- targets/ESP32/ESP32_P4/target_BlockStorage.h
- CMakePresets.json
- targets/ESP32/_include/Esp32_DeviceMapping.h
- CMake/toolchain.riscv32-esp-elf.cmake
- targets/ESP32/CMakeLists.txt
- targets/ESP32/ESP32_H2/target_common.c
- azure-pipelines-nightly.yml
- targets/ESP32/_common/ESP32_C3_DeviceMapping.cpp
- targets/ESP32/ESP32_P4/target_FileSystem.cpp
- src/PAL/COM/sockets/ssl/MbedTLS/ssl_generic.cpp
🚧 Files skipped from review as they are similar to previous changes (60)
- targets/ESP32/ESP32_P4/target_windows_storage_config.h
- targets/ESP32/ESP32_P4/target_system_device_pwm_config.cpp
- targets/ESP32/ESP32_P4/target_system_device_dac_config.cpp
- targets/ESP32/ESP32_P4/target_system_device_adc_config.cpp
- targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.cpp
- targets/ESP32/_IDF/project_elf_src_esp32p4.c
- CMake/Modules/ESP32_P4_sources.cmake
- targets/ESP32/ESP32_P4/target_system_device_i2c_config.cpp
- targets/ESP32/ESP32_P4/target_system_device_spi_config.cpp
- targets/ESP32/ESP32_P4/target_lwipopts.h
- targets/ESP32/ESP32_P4/nanoCLR/nanoHAL.cpp
- targets/ESP32/ESP32_P4/target_system_devices_dac_config.cpp
- targets/ESP32/_common/ESP32_H2_DeviceMapping.cpp
- targets/ESP32/ESP32_P4/target_system_io_ports_config.cpp
- targets/ESP32/ESP32_P4/README.md
- targets/ESP32/_Network/NF_ESP32_SmartConfig.cpp
- targets/ESP32/ESP32_P4/target_lwip_sntp_opts.h
- targets/ESP32/ESP32_P4/common/CMakeLists.txt
- targets/ESP32/_common/targetHAL_Network.cpp
- targets/ESP32/_nanoCLR/System.Device.I2s/sys_dev_i2s_native_System_Device_I2s_I2sDevice.cpp
- .devcontainer/All/Dockerfile.All.SRC
- targets/ESP32/ESP32_P4/CMakeLists.txt
- targets/ESP32/_nanoCLR/nanoFramework.Hardware.ESP32/nanoFramework_hardware_esp32_native_Hardware_Esp32_Sleep.cpp
- targets/ESP32/ESP32_P4/target_system_device_i2s_config.cpp
- targets/ESP32/_common/Target_System_IO_FileSystem.c
- targets/ESP32/_lwIP/nf_sys_arch.c
- targets/ESP32/_common/ESP32_C6_DeviceMapping.cpp
- src/DeviceInterfaces/System.Net/sys_net_native_System_Net_NetworkInformation_WirelessAPConfiguration.cpp
- targets/ESP32/ESP32_C6/target_common.c
- targets/ESP32/ESP32_P4/target_system_device_i2c_slave_config.h
- .devcontainer/ESP32/Dockerfile.ESP32.SRC
- targets/ESP32/_common/Target_Network.cpp
- targets/ESP32/ESP32_P4/target_common.c
- targets/ESP32/_Network/NF_ESP32_Wireless.cpp
- targets/ESP32/ESP32_P4/target_BlockStorage.c
- targets/ESP32/_nanoCLR/System.Device.I2c.Slave/sys_dev_i2c_slave_native_System_Device_I2c_I2cSlaveDevice.cpp
- targets/ESP32/ESP32_P4/nanoCLR/target_board.h.in
- targets/ESP32/_IDF/esp32/app_main.c
- targets/ESP32/_common/targetHAL_ConfigurationManager.cpp
- targets/ESP32/_common/WireProtocol_HAL_Interface.c
- targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Spi_To_TouchPanel.cpp
- CMake/Modules/FindESP32_IDF.cmake
- CMake/riscv-esp32p4.json
- targets/ESP32/ESP32_P4/target_common.h.in
- CMake/Modules/ESP32_P4_GCC_options.cmake
- CMake/riscv-esp32h2.json
- CMake/toolchain.xtensa-esp32-elf.cmake
- CMake/riscv-esp32c6.json
- targets/ESP32/_nanoCLR/System.Device.Pwm/sys_dev_pwm_native_System_Device_Pwm_PwmChannel.cpp
- targets/ESP32/_IDF/sdkconfig.default.esp32p4
- targets/ESP32/_IDF/esp32p4/app_main.c
- targets/ESP32/_nanoCLR/System.IO.Ports/sys_io_ser_native_System_IO_Ports_SerialPort.cpp
- targets/ESP32/_nanoCLR/System.Device.Spi/cpu_spi.cpp
- CMake/riscv-esp32c3.json
- CMake/binutils.ESP32.cmake
- targets/ESP32/_include/lwipopts.h
- targets/ESP32/ESP32_P4/target_nf_dev_onewire_config.h
- targets/ESP32/_common/ESP32_P4_DeviceMapping.cpp
- CMake/toolchain.riscv32-esp32p4-elf.cmake
- CMake/toolchain.riscv32-esp32c6-elf.cmake
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- GitHub Check: build-target (ESP32_H2_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C6_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, ChibiOS)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
🔇 Additional comments (21)
CMake/toolchain.riscv32-esp32h2-elf.cmake (3)
11-19: Well-structured macro for compiler setupThe
nf_set_compiler_varmacro provides a clean, consistent way to set up compiler variables, making the toolchain file more maintainable.
47-48: Correctly configured RISC-V architecture flagsThe compiler flags properly specify the RISC-V architecture variant (
rv32imac_zicsr_zifencei) and ABI (ilp32) required for the ESP32-H2 chip. The warning suppressions are also appropriate for this target.
50-54: Properly configured cross-compilation pathsThe root path setup correctly configures the toolchain for cross-compilation, ensuring libraries and includes are only searched within the toolchain directories while allowing programs to be found from the host system.
targets/ESP32/_IDF/sdkconfig.default.esp32s2 (5)
1-2: Clear file header explanation of minimal configuration approach.The updated file header clearly indicates this is a manually editable minimal configuration file, which aligns with the project's goal of simplifying ESP32-S2 configuration.
8-13: Good approach to handle deprecated drivers.Explicitly suppressing deprecation warnings for legacy drivers (ADC, ADC calibration, PCNT, RMT, I2S, DAC) is appropriate during the transition to newer ESP-IDF 5.4.1, allowing the codebase to continue using these components while planning for future migration.
31-32: Explicit SPIRAM configuration is appropriate.The explicit configuration of SPIRAM mode (quad) and type (auto) ensures consistent memory initialization across different ESP32-S2 boards.
57-58: Disabling DHCP server and IPv6 support.Disabling DHCP server and IPv6 support helps reduce memory footprint and aligns with the minimal configuration approach. This is appropriate for resource-constrained environments.
19-19:✅ Verification successful
Verify partition table compatibility.
The configuration points to an ESP32S3 partition CSV file for an ESP32S2 device. While this might work if partitions are compatible, it could lead to unexpected issues if the partition layouts differ between chip variants.
🏁 Script executed:
#!/bin/bash # Check if ESP32S2 has its own partition file or if ESP32S3 partition is intentionally shared # Look for ESP32S2-specific partition files echo "Checking for ESP32S2-specific partition files:" fd -e csv -p "partitions.*esp32s2" targets/ESP32/ # Compare with the referenced ESP32S3 partition file echo -e "\nDisplaying the referenced ESP32S3 partition file:" cat targets/ESP32/_IDF/esp32s3/partitions_nanoclr_4mb.csvLength of output: 1336
Partition table usage confirmed – no changes required
We searched undertargets/ESP32/and found no ESP32S2‐specific partition CSV. Thepartitions_nanoclr_4mb.csvfile defines the 4 MB nanoCLR layout used across both ESP32S2 and ESP32S3 variants, so pointing S2 at the S3 path is intentional and safe.CMake/toolchain.riscv32-esp32c3-elf.cmake (5)
1-10: Well-structured toolchain file for RISC-V ESP32-C3.The toolchain file correctly sets up the cross-compilation environment for the RISC-V ESP32-C3 target with proper copyright notice and system name configuration.
11-19: Clean macro implementation for compiler tools.The
nf_set_compiler_varmacro provides a clean, reusable way to locate compiler tools, making the toolchain file more maintainable.
22-46: Comprehensive compiler and tools setup.All necessary tools for the RISC-V toolchain (gcc, g++, assembler, objcopy, objdump, size) are correctly configured with appropriate search paths.
47-49: Architecture-specific compiler flags are appropriate.The compiler flags correctly specify the RISC-V architecture variant (
rv32imc_zicsr_zifencei) and necessary warning suppressions for both C and C++ compilation.
56-61: Standards and compiler test configuration.Setting C11 and C++11 standards as the baseline and configuring the compiler test type as a static library is appropriate for this embedded platform.
targets/ESP32/ESP32_P4/nanoCLR/nanoFramework.Graphics/Graphics_Memory.cpp (3)
1-14: Well-documented graphics memory implementation for ESP32-P4.The header includes and introductory comments provide good context for the graphics memory allocation strategy, particularly regarding SPI RAM integration.
17-26: Informative comments on ESP-IDF memory architecture.The comments clearly explain how external SPI RAM is integrated into the ESP32 memory map and the responsibilities of applications using this memory.
54-69: Good fallback mechanism for devices without SPIRAM.The code appropriately checks for available SPIRAM and falls back to normal RAM if unavailable, with adaptive size adjustment based on available memory. This ensures graphics can still work on boards with limited memory.
targets/ESP32/CMakePresets.json (5)
4-8: Updated architecture includes for RISC-V based ESP32 variants.The file correctly replaces Xtensa architecture presets with RISC-V presets for ESP32-C3, ESP32-C6, and ESP32-H2, and adds support for ESP32-P4, reflecting the architectural transition in these chip variants.
244-244: Consistent update of inheritance to RISC-V presets.All ESP32-C3, ESP32-C6, ESP32-H2, and XIAO_ESP32C3 presets now correctly inherit from RISC-V architecture presets instead of Xtensa, ensuring proper toolchain configuration.
Also applies to: 262-262, 281-281, 308-308, 460-460
327-378: Comprehensive configuration for new ESP32-P4 variants.The new ESP32_P4_UART and ESP32_P4_USB presets include detailed configurations for:
- Debugging support
- Serial/USB communication
- Hardware peripherals (RTC, SD card)
- Network support (Wi-Fi, Ethernet)
- Ethernet pin assignments
The configuration is thorough and provides two connectivity options (UART and USB CDC) for flexibility.
346-350: Ethernet configuration for ESP32-P4.The Ethernet configuration for ESP32-P4 is set up with the IP101 PHY interface and specific GPIO pins for MDC (31), MDIO (52), PHY reset (51), and PHY address (1). The PR objectives confirm this has been tested and is functional.
Also applies to: 372-376
343-343:❓ Verification inconclusive
Verify Wi-Fi implementation for ESP32-P4.
The PR objectives mention that ESP32-P4 lacks native Wi-Fi support and requires hosted Wi-Fi support on a second chip. Verify that the System.Device.Wifi API is properly implemented to work with this hosted configuration.
Also applies to: 369-369
🏁 Script executed:
#!/bin/bash # Check implementation of Wi-Fi support for ESP32-P4 # Look for ESP32-P4 specific Wi-Fi implementation echo "Checking for ESP32-P4 specific Wi-Fi implementation:" rg -A 10 "CONFIG_SOC_WIRELESS_HOST_SUPPORTED" targets/ESP32/_Network/ # Check for any ESP32-P4 specific wireless host code echo -e "\nChecking for ESP32-P4 specific wireless host code:" rg -A 10 "ESP32P4" --glob "*.{c,cpp,h}" targets/ESP32/_Network/Length of output: 5783
Wi‑Fi Hosted Implementation Verified
- In
targets/ESP32/_Network/NF_ESP32_Wireless.cpp, the Wi‑Fi stack is conditionally compiled underCONFIG_SOC_WIRELESS_HOST_SUPPORTEDand callsesp_hosted_init().
- No ESP32‑P4–specific Wi‑Fi code is required beyond the generic hosted path.
- Please confirm that the ESP32‑P4 CMake preset (intargets/ESP32/CMakePresets.json) enablesCONFIG_SOC_WIRELESS_HOST_SUPPORTEDso the hosted‑Wi‑Fi code path is activated.
| #if CONFIG_IDF_TARGET_ESP32 | ||
| #define NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG() \ | ||
| { \ | ||
| .smi_gpio = \ | ||
| { \ | ||
| .mdc_num = 23, \ | ||
| .mdio_num = 18 \ | ||
| }, \ | ||
| .interface = EMAC_DATA_INTERFACE_RMII, \ | ||
| .clock_config = \ | ||
| { \ | ||
| .rmii = \ | ||
| { \ | ||
| .clock_mode = DEFAULT_RMII_CLK_MODE, \ | ||
| .clock_gpio = (emac_rmii_clock_gpio_t)DEFAULT_RMII_CLK_GPIO \ | ||
| } \ | ||
| }, \ | ||
| .dma_burst_len = ETH_DMA_BURST_LEN_32, \ | ||
| .intr_priority = 0, \ | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use configurable pin macros instead of hard‑coding GPIOs in the default‑config macros
Both NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG variants hard‑code MDC/MDIO and (for ESP32) the RMII clock GPIO.
Because you later allow users to override these pins via ETH_MDC_GPIO / ETH_MDIO_GPIO / ETH_RMII_CLK_OUT_GPIO, the initialiser values should be driven by the same macros; otherwise the struct is filled twice and the first values are misleading in traces and harder to maintain.
- .mdc_num = 23,
- .mdio_num = 18
+ .mdc_num = ETH_MDC_GPIO,
+ .mdio_num = ETH_MDIO_GPIO
...
- .clock_mode = DEFAULT_RMII_CLK_MODE,
- .clock_gpio = (emac_rmii_clock_gpio_t)DEFAULT_RMII_CLK_GPIO
+ .clock_mode = DEFAULT_RMII_CLK_MODE,
+ .clock_gpio = (emac_rmii_clock_gpio_t)ETH_RMII_CLK_OUT_GPIODo the same substitution in the ESP32‑P4 block (lines 67/68 and 75‑77).
This keeps behaviour identical while removing duplication and the risk of the two values drifting apart.
Also applies to: 63-101
I’ll verify the existence and usage of the configurable‐pin macros and inspect both the ESP32 and ESP32‑P4 blocks.
🏁 Script executed:
#!/bin/bash
# Locate the Ethernet config file
file=$(rg -l "NF_ESP32_Ethernet.cpp")
echo "Found file: $file"
# Show the first 120 lines to cover both ESP32 and ESP32‑P4 default‑config macros
echo "--- File snippet (lines 1–120) ---"
sed -n '1,120p' "$file"
# Search the repository for the pin‐override macros and the default RMII clock GPIO macro
echo "--- Macro occurrences in repo ---"
rg -n "ETH_MDC_GPIO|ETH_MDIO_GPIO|ETH_RMII_CLK_OUT_GPIO|DEFAULT_RMII_CLK_GPIO"Length of output: 4459
I’ll retrieve the actual NF_ESP32_Ethernet.cpp to inspect both the ESP32 and ESP32‑P4 default‑config macros.
🏁 Script executed:
#!/bin/bash
# Find the Ethernet C++ source
file=$(fd -t f NF_ESP32_Ethernet.cpp)
echo "Inspecting: $file"
# Show the first 200 lines to cover defaults and ESP32‑P4 block
sed -n '1,200p' "$file"Length of output: 8376
Use configurable pin macros in NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG
Both ESP32 and ESP32‑P4 variants currently hard‑code MDC/MDIO and RMII‑CLK pins. Because you expose ETH_MDC_GPIO, ETH_MDIO_GPIO, and ETH_RMII_CLK_OUT_GPIO for board‑specific overrides, the defaults should come from those same macros to avoid duplication and drift.
File: targets/ESP32/_Network/NF_ESP32_Ethernet.cpp
Change in the ESP32 block (around lines 42–61):
#if CONFIG_IDF_TARGET_ESP32
#define NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG() \
{ \
.smi_gpio = \
{ \
- .mdc_num = 23, \
- .mdio_num = 18 \
+ .mdc_num = ETH_MDC_GPIO, \
+ .mdio_num = ETH_MDIO_GPIO \
}, \
.interface = EMAC_DATA_INTERFACE_RMII, \
.clock_config = \
{ \
.rmii = \
{ \
.clock_mode = DEFAULT_RMII_CLK_MODE, \
- .clock_gpio = (emac_rmii_clock_gpio_t)DEFAULT_RMII_CLK_GPIO \
+ .clock_gpio = (emac_rmii_clock_gpio_t)ETH_RMII_CLK_OUT_GPIO \
} \
}, \And in the ESP32‑P4 block (around lines 67–77):
#elif CONFIG_IDF_TARGET_ESP32P4
#define NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG() \
{ \
.smi_gpio = \
{ \
- .mdc_num = 31, \
- .mdio_num = 27 \
+ .mdc_num = ETH_MDC_GPIO, \
+ .mdio_num = ETH_MDIO_GPIO \
}, \
.interface = EMAC_DATA_INTERFACE_RMII, \
.clock_config = \
{ \
.rmii = \
{ \
.clock_mode = EMAC_CLK_EXT_IN, \
- .clock_gpio = 50 \
+ .clock_gpio = (emac_rmii_clock_gpio_t)ETH_RMII_CLK_OUT_GPIO \
} \
}, \This ensures the defaults always match the user‑configurable macros.
There was a problem hiding this comment.
@AdrianSoundy is the comment from the 🐰 accurate?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@josesimoes - I'll review this code. Although it's working I think it could be better
| graphicsStartingAddress = (CLR_UINT8 *)heap_caps_malloc(graphicsMemoryBlockSize, memoryCaps); | ||
| ASSERT(graphicsStartingAddress != NULL); | ||
| graphicsEndingAddress = (CLR_UINT8 *)(graphicsStartingAddress + graphicsMemoryBlockSize); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for allocation failure.
The code uses an assertion to verify allocation success but doesn't handle failure gracefully. Consider adding proper error handling to prevent crashes if memory allocation fails.
- graphicsStartingAddress = (CLR_UINT8 *)heap_caps_malloc(graphicsMemoryBlockSize, memoryCaps);
- ASSERT(graphicsStartingAddress != NULL);
- graphicsEndingAddress = (CLR_UINT8 *)(graphicsStartingAddress + graphicsMemoryBlockSize);
+ graphicsStartingAddress = (CLR_UINT8 *)heap_caps_malloc(graphicsMemoryBlockSize, memoryCaps);
+ if (graphicsStartingAddress == NULL)
+ {
+ return false;
+ }
+ graphicsEndingAddress = (CLR_UINT8 *)(graphicsStartingAddress + graphicsMemoryBlockSize);
missing v5.4.1 ref.
| #if CONFIG_IDF_TARGET_ESP32 | ||
| #define NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG() \ | ||
| { \ | ||
| .smi_gpio = \ | ||
| { \ | ||
| .mdc_num = 23, \ | ||
| .mdio_num = 18 \ | ||
| }, \ | ||
| .interface = EMAC_DATA_INTERFACE_RMII, \ | ||
| .clock_config = \ | ||
| { \ | ||
| .rmii = \ | ||
| { \ | ||
| .clock_mode = DEFAULT_RMII_CLK_MODE, \ | ||
| .clock_gpio = (emac_rmii_clock_gpio_t)DEFAULT_RMII_CLK_GPIO \ | ||
| } \ | ||
| }, \ | ||
| .dma_burst_len = ETH_DMA_BURST_LEN_32, \ | ||
| .intr_priority = 0, \ | ||
| } |
There was a problem hiding this comment.
@AdrianSoundy is the comment from the 🐰 accurate?
|
By adding an apt -get for Libusb, the smoketest is no longer failing on the openocd install. Now just fails at the same place as other builds with the missing ESP_ROM_ELF_DIR path. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.devcontainer/All/Dockerfile.All.SRC (4)
58-59: Ensure correct libusb package and optimize apt install
The Dockerfile currently installslibusb-1.0, which on Debian/Ubuntu is typically provided aslibusb-1.0-0(runtime) orlibusb-1.0-0-dev(headers). Please verify which one IDF needs—likely the-devpackage—and update accordingly. Also consider adding--no-install-recommendsto this apt-get invocation to keep the image slim.Suggested diff:
RUN apt-get install -y \ nodejs \ - libffi-dev \ - libusb-1.0 + libffi-dev \ + libusb-1.0-0-dev + --no-install-recommends
96-96: Normalize git clone flags for ESP‑IDF
Thegit cloneoptions (--branch,--depth,--recursive) are mixed after the repo URL, which can work but is less conventional. For clarity and to avoid subtle parsing issues, reorder the flags before the URL and explicitly pass the destination path last.Example:
- RUN git clone --branch v5.4.1 https://github.com/espressif/esp-idf --depth 1 --recursive ./sources/esp-idf + RUN git clone --branch v5.4.1 --depth 1 --recursive \ + https://github.com/espressif/esp-idf ./sources/esp-idf
131-131: Parametrize ESP_PATCH_VER for flexibility
CurrentlyESP_PATCH_VERis hard-coded as anENV. Consider defining it first as anARG(with a default) so that downstream users can override it at build time. Also double‑check that this value matches the actual directory name created byinstall.sh.Example adjustment:
- ENV ESP_PATCH_VER=esp-14.2.0_20241119 + ARG ESP_PATCH_VER=esp-14.2.0_20241119 + ENV ESP_PATCH_VER=${ESP_PATCH_VER}
136-136: Clean up PATH entries and remove trailing whitespace
The multi-lineENV PATH=…has a stray space at the end of line 138, which can introduce empty entries in$PATH. It may also be clearer to wrap the value in quotes or split into multipleENVinstructions for maintainability.For example:
ENV PATH="/root/.espressif/python_env/idf5.4_py3.11_env/bin:${PATH}:/root/.espressif/tools/xtensa-esp-elf/${ESP_PATCH_VER}/xtensa-esp-elf/bin:..."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
targets/ESP32/_IDF/esp32p4/partitions_nanoclr_16mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_32mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_4mb.csvis excluded by!**/*.csvtargets/ESP32/_IDF/esp32p4/partitions_nanoclr_8mb.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
.devcontainer/All/Dockerfile.All.SRC(4 hunks).devcontainer/ESP32/Dockerfile.ESP32.SRC(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .devcontainer/ESP32/Dockerfile.ESP32.SRC
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_ETHERNET_KIT_1.2)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_S3_ALL)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_H2_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C6_THREAD)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_C3)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_BLE_REV0)
- GitHub Check: nf-interpreter (Build_TI_SimpleLink_targets TI_CC1352R1_LAUNCHXL_915)
- GitHub Check: nf-interpreter (Build_ESP32_targets ESP32_PSRAM_REV0)
- GitHub Check: nf-interpreter (Build_NXP_targets NXP_MIMXRT1060_EVK)
- GitHub Check: nf-interpreter (Build_WIN32_nanoCLR)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F769I_DISCOVERY)
- GitHub Check: nf-interpreter (Build_Azure_RTOS_targets SL_STK3701A)
- GitHub Check: nf-interpreter (Build_STM32_targets ST_STM32F429I_DISCOVERY)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (ESP32_H2_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C6_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, ChibiOS)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.devcontainer/All/Dockerfile.All.SRC(4 hunks).devcontainer/ESP32/Dockerfile.ESP32.SRC(4 hunks)targets/ESP32/_include/nf_sockets_priv.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .devcontainer/ESP32/Dockerfile.ESP32.SRC
- .devcontainer/All/Dockerfile.All.SRC
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, TI, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, FreeRTOS-NXP)
- GitHub Check: build-target (ESP32_H2_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C6_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, ChibiOS)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
🔇 Additional comments (1)
targets/ESP32/_include/nf_sockets_priv.h (1)
141-142: Verify implementation availability oflwip_socket_dbg_get_socketThe prototype is declared here without
staticorinline, implying a definition exists in some translation unit.
If it is only compiled for debug builds, guard the declaration with#ifdefto avoid missing‑symbol linker errors in release builds.#ifdef LWIP_DEBUG struct lwip_sock* lwip_socket_dbg_get_socket(int fd); #endif
Ethernet changes - MDC, MDIO tidy up - IPV6 local address not allocated on ethernet adapters Add P4 to builds Other fixes as per review
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
targets/ESP32/_Network/NF_ESP32_Ethernet.cpp (1)
32-51: 🛠️ Refactor suggestionPrefer configurable pin macros over hard-coded GPIO numbers
NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG()still embeds literals for MDC, MDIO and RMII-CLK.
Because the same pins can already be overridden later viaETH_MDC_GPIO,ETH_MDIO_GPIO, andETH_RMII_CLK_OUT_GPIO, keeping two sources of truth risks drift and complicates maintenance.- .mdc_num = 23, - .mdio_num = 18 + .mdc_num = ETH_MDC_GPIO, + .mdio_num = ETH_MDIO_GPIO ... - .clock_gpio = (emac_rmii_clock_gpio_t)DEFAULT_RMII_CLK_GPIO + .clock_gpio = (emac_rmii_clock_gpio_t)ETH_RMII_CLK_OUT_GPIOAdopting the macros makes the defaults self-documenting and removes the need for the later override block.
(The same applies to the ESP32-P4 block below.)targets/ESP32/_include/nf_sockets_priv.h (1)
84-88:⚠️ Potential issue
errfield still truncates negative lwIP error codesPrevious review explained that storing
err_t(signed) intou8_tcorrupts values like-1→255.
Please change the field type toerr_t(or at leasts8_t) so callers receive the correct error.Apply:
- u8_t err; + err_t err;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
azure-pipelines-nightly.yml(2 hunks)azure-pipelines.yml(2 hunks)targets/ESP32/_Network/NF_ESP32_Ethernet.cpp(3 hunks)targets/ESP32/_common/targetHAL_Network.cpp(3 hunks)targets/ESP32/_include/nf_sockets_priv.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- azure-pipelines.yml
- targets/ESP32/_common/targetHAL_Network.cpp
- azure-pipelines-nightly.yml
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, All, 915)
- GitHub Check: build-target (TI_CC1352R1_LAUNCHXL, Debug, TI, 915)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, All)
- GitHub Check: build-target (NXP_MIMXRT1060_EVK, Debug, FreeRTOS-NXP)
- GitHub Check: build-target (ESP32_H2_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C6_THREAD, Debug, ESP32)
- GitHub Check: build-target (ESP32_C3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S3, Debug, ESP32)
- GitHub Check: build-target (ESP32_S2_USB, Debug, ESP32)
- GitHub Check: build-target (ESP_WROVER_KIT, Debug, ESP32)
- GitHub Check: build-target (M5Core2, Debug, ESP32)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, All)
- GitHub Check: build-target (ST_STM32F769I_DISCOVERY, Debug, ChibiOS)
- GitHub Check: build-target (SL_STK3701A, Debug, All)
- GitHub Check: build-target (SL_STK3701A, Debug, AzureRTOS)
🔇 Additional comments (2)
targets/ESP32/_Network/NF_ESP32_Ethernet.cpp (1)
140-147:CPU_GPIO_ReservePin()should match the actual clock pinFor clock-IN mode you reserve a constant
EMAC_CLK_EXT_INand whatever happens to be stored inclock_gpio.
If the constant differs fromclock_gpio(or is undefined for ESP32-P4) you may reserve the wrong pin or fail to compile.Verify that:
EMAC_CLK_EXT_INis defined for all supported targets and equalsclock_gpio; or- Only the resolved value in
clock_gpiois reserved.Even better, drop the extra reservation and rely on the value inside the config struct.
targets/ESP32/_include/nf_sockets_priv.h (1)
37-40: Header guard renamed – good catchUsing
NF_LWIP_HDR_SOCKETS_PRIV_Havoids a collision with upstreamsockets_priv.h.
No further action required.
| esp_eth_mac_t *mac = esp_eth_mac_new_esp32(&esp32_emac_config, &mac_config); | ||
|
|
||
| ESP_LOGI(TAG, "Ethernet mdio %d mdc %d\n", ETH_MDIO_GPIO, ETH_MDC_GPIO); | ||
|
|
There was a problem hiding this comment.
Potential compile failure when ETH_MDC_GPIO/ETH_MDIO_GPIO are undefined
ESP_LOGI(TAG, "Ethernet mdio %d mdc %d\n", ETH_MDIO_GPIO, ETH_MDC_GPIO);
These macros are optional.
If a user builds without defining them (and relies on the defaults you hard-coded earlier) the compiler will choke on the undefined identifiers.
Two easy wins:
-ESP_LOGI(TAG, "Ethernet mdio %d mdc %d\n", ETH_MDIO_GPIO, ETH_MDC_GPIO);
+ESP_LOGI(TAG, "Ethernet mdio %d mdc %d\n",
+ esp32_emac_config.smi_gpio.mdio_num,
+ esp32_emac_config.smi_gpio.mdc_num);or guard the log with #ifdef.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| esp_eth_mac_t *mac = esp_eth_mac_new_esp32(&esp32_emac_config, &mac_config); | |
| ESP_LOGI(TAG, "Ethernet mdio %d mdc %d\n", ETH_MDIO_GPIO, ETH_MDC_GPIO); | |
| esp_eth_mac_t *mac = esp_eth_mac_new_esp32(&esp32_emac_config, &mac_config); | |
| ESP_LOGI(TAG, "Ethernet mdio %d mdc %d\n", | |
| esp32_emac_config.smi_gpio.mdio_num, | |
| esp32_emac_config.smi_gpio.mdc_num); |
| #define NANO_ETH_ESP32_EMAC_DEFAULT_CONFIG() \ | ||
| { \ | ||
| .smi_gpio = \ | ||
| { \ | ||
| .mdc_num = 31, \ | ||
| .mdio_num = 27 \ | ||
| }, \ | ||
| .interface = EMAC_DATA_INTERFACE_RMII, \ | ||
| .clock_config = \ | ||
| { \ | ||
| .rmii = \ | ||
| { \ | ||
| .clock_mode = EMAC_CLK_EXT_IN, \ | ||
| .clock_gpio = 50 \ | ||
| } \ | ||
| }, \ | ||
| .dma_burst_len = ETH_DMA_BURST_LEN_32, \ | ||
| .intr_priority = 0, \ | ||
| .emac_dataif_gpio = \ | ||
| { \ | ||
| .rmii = \ | ||
| { \ | ||
| .tx_en_num = 49, \ | ||
| .txd0_num = 34, \ | ||
| .txd1_num = 35, \ | ||
| .crs_dv_num = 28, \ | ||
| .rxd0_num = 29, \ | ||
| .rxd1_num = 30 \ | ||
| } \ | ||
| }, \ | ||
| .clock_config_out_in = \ | ||
| { \ | ||
| .rmii = \ | ||
| { \ | ||
| .clock_mode = EMAC_CLK_EXT_IN, \ | ||
| .clock_gpio = -1 \ | ||
| } \ | ||
| }, \ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ESP32-P4 default config duplicates pin information & gets out-of-sync
Hard-coding {31,27} for MDC/MDIO and the full RMII pin map here means:
- The later runtime overrides (
#ifdef ETH_MDC_GPIO …) silently discard these values, defeating the purpose of setting them here. - Any board that tweaks
ETH_*macros must remember to update both places.
Consider rewriting this block to reference the existing ETH_* macros, e.g.
- .mdc_num = 31,
- .mdio_num = 27
+ .mdc_num = ETH_MDC_GPIO,
+ .mdio_num = ETH_MDIO_GPIO
...
- .tx_en_num = 49,
- .txd0_num = 34,
- .txd1_num = 35,
- .crs_dv_num = 28,
- .rxd0_num = 29,
- .rxd1_num = 30
+ .tx_en_num = ETH_RMII_TX_EN_GPIO,
+ .txd0_num = ETH_RMII_TXD0_GPIO,
+ .txd1_num = ETH_RMII_TXD1_GPIO,
+ .crs_dv_num = ETH_RMII_CRS_DV_GPIO,
+ .rxd0_num = ETH_RMII_RXD0_GPIO,
+ .rxd1_num = ETH_RMII_RXD1_GPIO(where the extra macros mirror IDF’s esp_eth_phy_rtl8201_config_t pattern).
This removes duplication and instantly brings P4 in line with the macro-override mechanism.
Committable suggestion skipped: line range outside the PR's diff.
| CPU_GPIO_ReservePin(esp32_emac_config.smi_gpio.mdio_num, true); // MDIO | ||
| CPU_GPIO_ReservePin(19, true); // TXD0 | ||
| CPU_GPIO_ReservePin(21, true); // TX_EN | ||
| CPU_GPIO_ReservePin(22, true); // TXD1 | ||
| CPU_GPIO_ReservePin(ETH_MDC_GPIO, true); // MDC (23) | ||
| CPU_GPIO_ReservePin(esp32_emac_config.smi_gpio.mdc_num, true); // MDC | ||
| CPU_GPIO_ReservePin(25, true); // RXD0 | ||
| CPU_GPIO_ReservePin(26, true); // RXD1 | ||
| CPU_GPIO_ReservePin(27, true); // CRS_DV | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded pin reservations ignore ESP32-P4’s custom RMII map
Pins 19/21/22/25/26/27 are correct for the classic ESP32 but not for ESP32-P4 (which uses 49/34/35/28/29/30).
Attempting to reserve the wrong pins breaks Ethernet on P4 and may clash with other peripherals.
Reserve the pins held in esp32_emac_config.emac_dataif_gpio.rmii instead, e.g.
-CPU_GPIO_ReservePin(19, true); // TXD0
-CPU_GPIO_ReservePin(21, true); // TX_EN
-CPU_GPIO_ReservePin(22, true); // TXD1
-...
-CPU_GPIO_ReservePin(25, true); // RXD0
-CPU_GPIO_ReservePin(26, true); // RXD1
-CPU_GPIO_ReservePin(27, true); // CRS_DV
+const emac_rmii_gpio_t &rmii = esp32_emac_config.emac_dataif_gpio.rmii;
+CPU_GPIO_ReservePin(rmii.txd0_num, true);
+CPU_GPIO_ReservePin(rmii.tx_en_num, true);
+CPU_GPIO_ReservePin(rmii.txd1_num, true);
+CPU_GPIO_ReservePin(rmii.rxd0_num, true);
+CPU_GPIO_ReservePin(rmii.rxd1_num, true);
+CPU_GPIO_ReservePin(rmii.crs_dv_num,true);This keeps the reservation list in sync with whichever target (and overrides) the build selects.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CPU_GPIO_ReservePin(esp32_emac_config.smi_gpio.mdio_num, true); // MDIO | |
| CPU_GPIO_ReservePin(19, true); // TXD0 | |
| CPU_GPIO_ReservePin(21, true); // TX_EN | |
| CPU_GPIO_ReservePin(22, true); // TXD1 | |
| CPU_GPIO_ReservePin(ETH_MDC_GPIO, true); // MDC (23) | |
| CPU_GPIO_ReservePin(esp32_emac_config.smi_gpio.mdc_num, true); // MDC | |
| CPU_GPIO_ReservePin(25, true); // RXD0 | |
| CPU_GPIO_ReservePin(26, true); // RXD1 | |
| CPU_GPIO_ReservePin(27, true); // CRS_DV | |
| CPU_GPIO_ReservePin(esp32_emac_config.smi_gpio.mdio_num, true); // MDIO | |
| const emac_rmii_gpio_t &rmii = esp32_emac_config.emac_dataif_gpio.rmii; | |
| CPU_GPIO_ReservePin(rmii.txd0_num, true); | |
| CPU_GPIO_ReservePin(rmii.tx_en_num, true); | |
| CPU_GPIO_ReservePin(rmii.txd1_num, true); | |
| CPU_GPIO_ReservePin(rmii.rxd0_num, true); | |
| CPU_GPIO_ReservePin(rmii.rxd1_num, true); | |
| CPU_GPIO_ReservePin(rmii.crs_dv_num,true); | |
| CPU_GPIO_ReservePin(esp32_emac_config.smi_gpio.mdc_num, true); // MDC |
Description
This PR is an update to the latest IDF version v5.4.1 which includes latest device support and latest bug fixes. The PR also includes support for the new ESP32_P4.
This includes following changes as well as numerous IDF CONFIG_? changes.
- Serial port driver now supports up to 4 com ports (P4)
Outstanding issues/work.
Some the devices with no psram may not run existing code as reduced memory available due to increased memory usage by IDF. Some work needs to be done optimize memory use. Rduce network buffers etc.
There was a problem with the ESP32_C6 & ESP32_H2 hanging on boot which was being caused by a loop in the ADC calibration. For the moment the ADC has been disabled for these devices. We are still using legacy ADC driver so going to the latest ADC driver will probably resolve this issue.
Commented out Mbedtls methods as now duplicates. maybe needed for other targets(ssl_generic.cpp), Test with build system. Yes these were required by some other builds. Changed methods to _nfweak (fixed problem)ESP32_P4 support
Motivation and Context
Needed to resolve some outstanding issues and to support new devices coming. ESP32_C5, ESP32_C61
How Has This Been Tested?
The following targets were built and tested. Networking(Listen/accept). I2C scan, GPIO
More testing required
ESP32_REV0, ESP32_PSRAM_REV0, ESP32_REV3, ESP32_PSRAM_REV3, ESP32_REV3_IPV6
ESP32_WROVER_KIT
ESP32_C3
ESP32_C6_THREAD
ESP32_H2_THREAD
ESP32_S2_UART
ESP32_S3, ESP32_S3_ALL
ESP32_P4
The firmware versions: ESP32_PSRAM_REV0 & ESP32_WROVER_KIT which panic on start are now working.
Types of changes
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores