Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing SPI interface to align interface with the default pins used … #755

Closed
wants to merge 1 commit into from

Conversation

MalteSchm
Copy link

…in this project

The GPIO pins used in this project for SPI communication (19, 23, 18, 5) are the default pins for SPI3 / VSPI. The code uses HSPI / SPI2 however.
See: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/spi_master.html

@markusdd
Copy link
Contributor

markusdd commented Mar 28, 2023

This change must not be made as it will kill compatibilty with S3/C3 chipsets (which are the new ones anyway), as they know no VSPI, they have actually 4 hardware SPI interfaces. 2 of them usable externally and freely configurable.
If you want to do something like this, you have to distinguish between those possible choices. See my PR for ahoy recently:

https://github.com/lumapu/ahoy/blob/29b7279288a2cc1a64dec1f7d4c2e56abc61a17f/src/hm/hmRadio.h#L104-L115

@tbnobody
Copy link
Owner

Currently I also don't see a need to change the SPI Bus interface from HSPI to VSPI as the pins are manually configured anyways.

The ESP32-C2 has 3 SPI peripherals, whereby only 1 is usable externally. The other 2 seem to be usable to access the attached flash only. This is not a problem when just using the NRF24 module. If you like to use the CMT2300A module in future in parallel this is not possible as this module is using half duplex communication with the ESP (MOSI/MISO via one pin) and therefor needs a special configuration for this SPI bus.

@markusdd
Copy link
Contributor

Yeah, also the naming of this bus is inconsistent across generations or it does not even exist.
So explicitly using VSPI would also always mean you have to 'ifdef' yourway around the different ESP32 variants.

@markusdd
Copy link
Contributor

markusdd commented Mar 28, 2023

Also, I just noticed this PR will basically supersede this one here, so I would recommend to close here based on: #664

@MalteSchm
Copy link
Author

o.k. I see that this has more side effects then I suspected.
I don't quite get the argument about the different ESP variants though. From the other PRs it seems that distinguishing is necessary anyhow even if you keep using HSPI. This change applies obviously only to ESPs that offer both HSPI/VSPI.

I can see that this would break on a ESP that has a single SPI controller called HSPI. The -C2 calls this SPI2 in the manual though...

@markusdd
Copy link
Contributor

markusdd commented Mar 28, 2023

No, it breaks on ESP32-S3/C3 (the new ones) because they do not do emulated Virtual SPI (aka VSPI) anymore. They have 2 hardware controllers exposed externally, 4 in total. (one Flash, 1 PSRAM, and 2 external ones for free use).

They are called HSPI and FSPI. There is no VSPI for these, so this PR would inhibit compilation for ESP32-S3 and ESP32-C3.

The current version works fine for all ESP32s. There is actually no need to change this. The only reason to change anything and be explicit about the PHY used is when a SPI Display shall be supported. This is usually on HSPI, then FSPI or VSPI (depending if S3/C3 or not) must explicitly be selected for the NRF24. Hence the construction in ahoyDTU I posted above as they support these display types.

@MalteSchm
Copy link
Author

Ok I understand. Then lets consider this closed and I will use whatever SPI remains for the other device. (which is part of the -onBattery fork)

@MalteSchm MalteSchm closed this Mar 28, 2023
@MalteSchm MalteSchm deleted the spi_interface_change branch March 28, 2023 17:22
Copy link

github-actions bot commented Apr 6, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants