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

SPI (fix): Adds SPI 3 to the ESP32-S2 and adds comments about it #9216

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Feb 5, 2024

Description of Change

ESP32-S2 has 3 SPI buses available.
It fixes names and definitons in order to add the 3rd SPI to the list.
It also adds some commentaries in order to help users.

Tests scenarios

CI Only

Related links

Closes #9210

@SuGlider SuGlider added this to the 3.0.0-RC1 milestone Feb 5, 2024
@SuGlider SuGlider self-assigned this Feb 5, 2024
Copy link
Contributor

github-actions bot commented Feb 5, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "SPI (fix): Adds SPI 3 to the ESP32-S2 and adds comments about it":
    • summary looks empty
    • type/action looks empty
  • the commit message "SPI (fix): added a space - typo error, simple fix.":
    • summary should not end with a period (full stop)
    • summary looks empty
    • type/action looks empty

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️

The source branch "ESP32-S2_SPI_1_2_3" incorrect format:

  • contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS).
    Please rename your branch.

👋 Hello SuGlider, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 6d38707

@TD-er
Copy link
Contributor

TD-er commented Feb 6, 2024

While you're at it, can you perhaps also add (or link to) some info about how the naming schema for all FSPI/VSPI/HSPI is intended?
I didn't yet find any logic to it.

If FSPI is always connected to the flash, then it would make sense to me.
And if VSPI vs HSPI is about max speed (or bus width), then it also makes sense.

However it seems to be a bit random how it is used among ESP32-variants.

@me-no-dev
Copy link
Member

I'm kinda leaning towards switching to a different way to identify the buses. Maybe by a custom ID (because otherwise they will have to start from 2), so 0 is the first available port. No names.

@TD-er
Copy link
Contributor

TD-er commented Feb 6, 2024

I'm kinda leaning towards switching to a different way to identify the buses. Maybe by a custom ID (because otherwise they will have to start from 2), so 0 is the first available port. No names.

Maybe it can even be extended to the peripheral manager to ask for a bus used for peripheral type X
This also eliminates the need for code to start the SPI bus or checking if it has been started.
And when you want to "join" a SPI bus, you can hand over some identifier which you later can use to get the SPI bus.

This removes the need to know about all platforms how they all use the SPI bus.
And you can still get some access to busses you shouldn't use to get some info from them like pins used, frequency, etc.

@me-no-dev
Copy link
Member

Maybe it can even be extended to the peripheral manager to ask for a bus used for peripheral type X

This can't really work in Arduino. Such buses use multiple pins and so on. As sketch developer, it's your job to know which bus is used for which device. Sometimes this also needs to be defined on board level, when you have screen on a separate bus. Question is rather semantics. in ESP the SPI buses that are available to the users do not start from 0, which is confusing. It worked somewhat using names, but with more chips added, this also proves not perfect. Having all start from 0 would make it more intuitive and easier to handle. We could add name definitions for backward compatibility.

@TD-er
Copy link
Contributor

TD-er commented Feb 6, 2024

I know asking for used pins isn't really in the scope of Arduino.
The reason I mentioned it was that it would be nice to know at runtime if a bus is using QIO, OPI, etc.
Maybe slightly confusing example in this scope.

@me-no-dev
Copy link
Member

I know asking for used pins isn't really in the scope of Arduino.

We do provide the used pins from peripheral manager

GPIO Info:
------------------------------------------
  GPIO : BUS_TYPE[bus/unit][chan]
  --------------------------------------  
     1 : UART_TX[0]
     3 : UART_RX[0]
     4 : ETHERNET_SPI
     5 : ETHERNET_SPI
    12 : SPI_MASTER_MISO[3]
    13 : SPI_MASTER_MOSI[3]
    14 : SPI_MASTER_SCK[3]
    15 : ETH_CS

Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@me-no-dev me-no-dev merged commit 7d59554 into master Feb 7, 2024
39 checks passed
@me-no-dev me-no-dev deleted the ESP32-S2_SPI_1_2_3 branch February 7, 2024 12:43
@SuGlider
Copy link
Collaborator Author

SuGlider commented Feb 7, 2024

While you're at it, can you perhaps also add (or link to) some info about how the naming schema for all FSPI/VSPI/HSPI is intended? I didn't yet find any logic to it.

If FSPI is always connected to the flash, then it would make sense to me. And if VSPI vs HSPI is about max speed (or bus width), then it also makes sense.

However it seems to be a bit random how it is used among ESP32-variants.

ESP32 has introduced the SPI names FSPI/VSPI/HSPI in the Technical Reference Manual at chapter 7 (page 125) about SPI.

ESP32-S2 doesn't use these names. Just uses SPI0, SPI1, SPI2 and SPI3. ESP32-S2 TRM - Chapter 24
Is uses the prefix GP (General Purpose) for GP-SPI2 and GP-SPI3

ESP32-S3 does the same as ESP32-S2 with SPI0, SPI1, GP-SPI2 and GP-SPI3.

ESP32-H2, ESP32-C3 and ESP32-C6 have 3 SPI peripherals, but only GP-SPI2 is available for the users.

Therefore the use of FSPI/VSPI/HSPI names is ESP32 legacy.

@TD-er
Copy link
Contributor

TD-er commented Feb 7, 2024

@SuGlider Thanks, I will have another look at this list and try to find a proper way to represent the available SPI options in the ESPEasy UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

FSPI and ESP32-S2
4 participants