Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (b7cfafc) to head (f8a9923).
⚠️ Report is 19 commits behind head on master.

@@            Coverage Diff             @@
##           master   #17951      +/-   ##
==========================================
- Coverage   98.38%   98.38%   -0.01%     
==========================================
  Files         171      171              
  Lines       22297    22296       -1     
==========================================
- Hits        21938    21937       -1     
  Misses        359      359              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

Everything is completed except the forced download start reset. @projectgus

But now there is a problem, that is, if I plug in the usb of CDC and the default JTAG at the same time, JTAG will not be able to load the storage space, and it is bound to CDC.

@projectgus Please review again.

I wanted to touch base on the I2C stuff. The reason why I am suggesting a separate PR for it is if it will work with the P4 the way it is done currently it is easier to revert the I2C changes if they are made in their own PR should there be an issue with it. When I went looking at the code in the ESP-IDF I didn't see any block put into place that stop the old style of I2C from working with the P4.

It does need to be updated without a doubt but a separate PR would be the better if it is not needed to make the P4 work.

@Vincent1-python Please check that review comments are actually resolved before pressing "Resolve conversation". If you're not sure, then it's better to leave it unresolved!

@Vincent1-python I see you pulled all the machine_i2c.c changes out of this PR, but didn't submit them anywhere else. Did you find that the ESP32-P4 I2C works correctly with the existing code?

There is nothing wrong with the test so far.

the I2C does need to be upgraded and I do suggest that @Vincent1-python submit a PR for those changes since it would bring the I2C code up to the current API in the ESP-IDF.

The LDO thing I think the best approach is to add in a new set of classes into the esp32 module.

the I2C does need to be upgraded and I do suggest that @Vincent1-python submit a PR for those changes since it would bring the I2C code up to the current API in the ESP-IDF.

OK.

the I2C does need to be upgraded and I do suggest that @Vincent1-python submit a PR for those changes since it would bring the I2C code up to the current API in the ESP-IDF.

#17971

Nice to see you made the PR. Just remember don't change anything other then the code that is added or removed that alters functionality. Don't reformat anything. I know that some IDE's will do that kind of a thing when you save the file, you have to go and turn those features off.

the only suggestion I have to make is this...

in ports/esp32/boards/ESP32_GENERIC_P4 add a file named mpconfigvariant_C6_WIFI.cmake

In that file add the following code....

set(IDF_TARGET esp32p4)

set(SDKCONFIG_DEFAULTS
    boards/sdkconfig.base
    boards/sdkconfig.p4
    boards/sdkconfig.p4usb
    boards/ESP32_GENERIC_P4/sdkconfig.board
    boards/ESP32_GENERIC_P4/sdkconfig.c6_wifi
)

In that same folder rename sdkconfig.board to sdkconfig.c_wifi and then make a new sdkconfig.board file and leave it empty. Doing that will add BOARD_VARIANT=C6_WIFI as a build command.

I don't remember what C series that is used as the wireless module so adjust as needed so the correct SOC is used. If more than one SOC is able to support the function of being a wireless module for the P4 then I would repeat this process for each of the SOC's that can be used and make any alterations to the sdkconfig file specific to that variant if changes need to be made. If they all use the same settings then remove the c6_ from the extension for the sdkconfig and point all of the mpconfigvariant_*_WIFI.cmake files to that common WiFi sdkconfig file.

IDK if this is something that @projectgus will agree on or not.

we can also flip flop how it works so as a default an external wireless module is enabled and add a board variant that disables it.

I just did a check and it appears that you would need to add a variant to turn it on and not off. You also have to create a variant explicitly for each of the following MCU's to enable it for and you would have to set a macro that is specific to the MCU being used..

Here are the config settings that are specific to the external MCU being used. These config flags need to be set according to what is being used as an external module. IDK if there is a way to automatically detect the module that is being used. I would imaging the external module and the P4 would chare the same USB so both devices should appear when the board gets plugged into a computer. If that is the case then that is able to be used to determine the external module and the build can be made to automatically detect it. However it would require that the MCU be plugged into the compiling machine for it to work.

Gonna have to make a board variant for each of the MCUs seen below and also an sdkconfig for each of the board variants to set the proper config flag in.

CONFIG_SLAVE_IDF_TARGET_ESP32
CONFIG_SLAVE_IDF_TARGET_ESP32C2
CONFIG_SLAVE_IDF_TARGET_ESP32C3
CONFIG_SLAVE_IDF_TARGET_ESP32C5
CONFIG_SLAVE_IDF_TARGET_ESP32C6
CONFIG_SLAVE_IDF_TARGET_ESP32S2
CONFIG_SLAVE_IDF_TARGET_ESP32S3
CONFIG_SLAVE_IDF_TARGET_ESP32H2
CONFIG_SLAVE_IDF_TARGET_ESP32P4
CONFIG_SLAVE_IDF_TARGET_ESP32C61

From what is said for the LDO seen here...

https://docs.espressif.com/projects/esp-idf/en/stable/esp32p4/api-reference/peripherals/ldo_regulator.html

the LDO is not something that is locked to only an SDCard reader. It is simply 4 outputs that the user is able to programmatically control. The LDO's are able to be used for anything that the user may want to turn on and off. The LDO falls more in line with being a high current GPIO pin that can only be used for output.

It looks like there are only 2 channels available for user use, 3 & 4 and those channels have a max current rating of 50mA.

From what is said for the LDO seen here...

https://docs.espressif.com/projects/esp-idf/en/stable/esp32p4/api-reference/peripherals/ldo_regulator.html

the LDO is not something that is locked to only an SDCard reader. It is simply 4 outputs that the user is able to programmatically control. The LDO's are able to be used for anything that the user may want to turn on and off. The LDO falls more in line with being a high current GPIO pin that can only be used for output.

It looks like there are only 2 channels available for user use, 3 & 4 and those channels have a max current rating of 50mA.

No, it needs to be initialized and transferred to the SD card drive.

CONFIG_SLAVE_IDF_TARGET_ESP32C2
CONFIG_SLAVE_IDF_TARGET_ESP32C3
CONFIG_SLAVE_IDF_TARGET_ESP32C5
CONFIG_SLAVE_IDF_TARGET_ESP32C6

It seems that there are only ESP32C2, ESP32C5 and ESP32C6.

CONFIG_SLAVE_IDF_TARGET_ESP32C2
CONFIG_SLAVE_IDF_TARGET_ESP32C3
CONFIG_SLAVE_IDF_TARGET_ESP32C5
CONFIG_SLAVE_IDF_TARGET_ESP32C6

It seems that there are only ESP32C2, ESP32C5 and ESP32C6.

choice ESP_HOSTED_P4_DEV_BOARD
	bool "Configure GPIOs for Development Board"
	depends on IDF_TARGET_ESP32P4
	default ESP_HOSTED_P4_DEV_BOARD_NONE
	help
		Sets GPIOs used for transport on these ESP32-P4 Development Boards
		- ESP32-P4 Function_EV_Board with on-board ESP32-C6
		- ESP32-P4 Function_EV_Board with ESP32-C5 on a EV board
		- ESP32-P4 Function_EV_Board with ESP32-C2 on a EV board
		- ESP32-P4-C5 Code Board with on-board ESP32-C5
		Select "No development board" if you want to modify GPIOs used

	config ESP_HOSTED_P4_DEV_BOARD_NONE
		bool "No development board"

	config ESP_HOSTED_P4_DEV_BOARD_FUNC_BOARD
		depends on SLAVE_IDF_TARGET_ESP32C6 || SLAVE_IDF_TARGET_ESP32C5 || SLAVE_IDF_TARGET_ESP32C2
		bool "ESP32-P4-Function-EV-Board with on-board C6 or C5/C2 on EV board"

	config ESP_HOSTED_P4_C5_CORE_BOARD
		depends on SLAVE_IDF_TARGET_ESP32C5
		bool "ESP32-P4-C5 Core Board with on-board C5"

No, it needs to be initialized and transferred to the SD card drive.

But this would be better done if what is passed to the SPI constructor is a type of Pin class for the LDO.

No, it needs to be initialized and transferred to the SD card drive.

But this would be better done if what is passed to the SPI constructor is a type of Pin class for the LDO.

I think this is still too difficult. I hope someone else will come to the PR function.

Take a look here...

https://github.com/espressif/esp-wifi-remote/blob/main/components/esp_wifi_remote/idf_tag_v5.4.2/include/esp_wifi_default_config.h

It's in the header files for the wifi remote component. You can see all of the config options that are listed.

The issue with the SD is disrupting the SPI API. I don't see why it would need to be passed to the SDCard driver,\ it's simply power on and power off. It's not like the voltages would be changing at all. This could be easily done by adding a couple of functions to the esp32 module.

That's now the cs pins are handled for SPI. It's left up to the user to set the CS state when they want to access an SPI device. The same thing could also be done with the LDO for the SDCard

Since the LDO is not going to have a variable voltage to it a reference doesn't need to be kept for the LDO channel handle. so it's easy to do.

The issue with the SD is disrupting the SPI API. I don't see why it would need to be passed to the SDCard driver,\ it's simply power on and power off. It's not like the voltages would be changing at all. This could be easily done by adding a couple of functions to the esp32 module.

https://github.com/espressif/esp-bsp/blob/master/bsp/esp32_p4_function_ev_board/esp32_p4_function_ev_board.c#L263
No, you need to bind and change it.

Take a look here...

https://github.com/espressif/esp-wifi-remote/blob/main/components/esp_wifi_remote/idf_tag_v5.4.2/include/esp_wifi_default_config.h

It's in the header files for the wifi remote component. You can see all of the config options that are listed.

No, its WIFI controller needs to be modified.

OK you are correct about the LDO with the SDCard. The driver needs to be able to shift the voltage between 1.8 and 3.3 volts in order to support UHS-I which is kind of funny because in the ESP-IDF docs it states that the ESP-IDF doesn't currently support UHS-I

https://docs.espressif.com/projects/esp-idf/en/release-v5.4/esp32p4/api-reference/peripherals/sdmmc_host.html#overview

    SDMMC_HOST_SLOT_1 is routed via GPIO Matrix. This means that any GPIO may be used for each of the SD card signals. It is for non UHS-I usage.

    SDMMC_HOST_SLOT_0 is dedicated to UHS-I mode, which is not yet supported in the driver.

last line there. It is not yet supported. So yes you can simply latch that LDO to 3.3v and the SD Card reader is going to function without any issues. UHS-II cards are able to work at 3.3 volts without needing the 1.8v.

Upon digging into the SD card code it does look like UHS-I is in fact supported and the documentation is not correct. I can see where the voltage for the SD is changed from the 3.3 that is needed during card initialization to the 1.8v that is used when sending data to and from the card.

The crappy thing is that with the LDO's a reference to the handle doesn't need to be held if using a static voltage out that doesn't change. for variable voltage a reference to the handle needs to be kept and that handle is what makes it harder to deal with without having to change the API. I supposed it could be set as a compile time option seeing as how it is not something that would need to be figured out at runtime.

OK you are correct about the LDO with the SDCard. The driver needs to be able to shift the voltage between 1.8 and 3.3 volts in order to support UHS-I which is kind of funny because in the ESP-IDF docs it states that the ESP-IDF doesn't currently support UHS-I

https://docs.espressif.com/projects/esp-idf/en/release-v5.4/esp32p4/api-reference/peripherals/sdmmc_host.html#overview

    SDMMC_HOST_SLOT_1 is routed via GPIO Matrix. This means that any GPIO may be used for each of the SD card signals. It is for non UHS-I usage.

    SDMMC_HOST_SLOT_0 is dedicated to UHS-I mode, which is not yet supported in the driver.

last line there. It is not yet supported. So yes you can simply latch that LCD to 3.3v and the SD Card reader is going to function without any issues. UHS-II cards are able to work at 3.3 volts without needing the 1.8v.

So basically there is no point to using the LDO for this purpose because the driver doesn't support UHS-I. Adding it doesn't appear like it needs to be done at this point in time.

OK.

OK you are correct about the LDO with the SDCard. The driver needs to be able to shift the voltage between 1.8 and 3.3 volts in order to support UHS-I which is kind of funny because in the ESP-IDF docs it states that the ESP-IDF doesn't currently support UHS-I

https://docs.espressif.com/projects/esp-idf/en/release-v5.4/esp32p4/api-reference/peripherals/sdmmc_host.html#overview

    SDMMC_HOST_SLOT_1 is routed via GPIO Matrix. This means that any GPIO may be used for each of the SD card signals. It is for non UHS-I usage.

    SDMMC_HOST_SLOT_0 is dedicated to UHS-I mode, which is not yet supported in the driver.

last line there. It is not yet supported. So yes you can simply latch that LCD to 3.3v and the SD Card reader is going to function without any issues. UHS-II cards are able to work at 3.3 volts without needing the 1.8v.

So basically there is no point to using the LDO for this purpose because the driver doesn't support UHS-I. Adding it doesn't appear like it needs to be done at this point in time.

I hope someone else will come to the PR function.

So now my PR can be merged? Still need to be revised. @projectgus @kdschlosser

my revisions don't mean a hill of beans in terms of being able to merge the PR. Mine is just for informational purposes I guess.

@projectgus needs to chime in on the whole LDO thing and how it should be handled because that is going to be something that causes problems with the P4 if the LDO is used to control the IO voltage for the SDReader. I think the best thing is going to be adding a macro for what LDO channel is to be used and if the macro has been defined then the LDO code is added and if it has not been defined then it's not added.

This way it's not something that is going to cause a change to the SPI API at all.