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

support for esp32p4 & updated to new released esp-idf v5.3 #226

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Vollbrecht
Copy link
Collaborator

No description provided.

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Thanks for adding support for P4 so early on! Just left a few question/suggestions.

@@ -72,17 +73,17 @@ jobs:
- uses: actions/checkout@v4
with:
path: /home/runner/work/esp-idf-template/esp-idf-template/github-esp-idf-template
- name: Generate v5.2
if: matrix.esp-idf.version == 'v5.2' || matrix.esp-idf.version == 'v5.1'
- name: Generate v5.3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Generate v5.3
- name: Generate Project

- name: Generate v5.2
if: matrix.esp-idf.version == 'v5.2' || matrix.esp-idf.version == 'v5.1'
- name: Generate v5.3
if: matrix.esp-idf.version == 'v5.3' || (matrix.esp-idf.version == 'v5.2' && matrix.target != 'esp32p4')
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this condition? Is it to avoid P4 and v5.2? We could add the esp-idf version to the target in the matrix so we can choose the esp-idf version per targets without having too much conditions (this could be helpful in the future where new targets will have the same issue as p4):

        target:
          - device: esp32
            esp-idf: v5.2
          - device: esp32
            esp-idf: v5.3
          - device: esp32c2
            esp-idf: v5.2
          - device: esp32c2
            esp-idf: v5.3                  
          - device: esp32c3
            esp-idf: v5.2
          - device: esp32c3
            esp-idf: v5.3          
          - device: esp32c6
            esp-idf: v5.2
          - device: esp32c6
            esp-idf: v5.3    
            ....
          - device: esp32p4
            esp-idf: v5.3                               

The matrix becomes significantly uglier though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm that somehow would nullify the complete concept of a matrix if i cramp all into a linear list of options right? That its merely a vector.

- version: v5.2
- version: v5.1 # only for compairson why no_std build fails on v5.2
# - version: v5.1 # only for compairson why no_std build fails on v5.2
Copy link
Member

Choose a reason for hiding this comment

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

Is this worth to keep it still? We removed the 5.1 option from the template

@Vollbrecht
Copy link
Collaborator Author

Thanks Sergio for looking into it!

I meanly started it to get CI feedback, and once i get more stuff working i also address all the Things!

I make a cleanup pass and ping you again when we addressed the necessary changes in the hal itself, merged and released them. E.g when we stopped with the patch.crates-io stuff.

@SergioGasquez SergioGasquez linked an issue Aug 9, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

esp-idf upgrade to v5.3?
2 participants