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

Added variant for Waveshare ESP32-S3-Touch-LCD-1.69 & ESP32-S3-LCD-1.69 board #10118

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

Y1hsiaochunnn
Copy link
Contributor

@Y1hsiaochunnn Y1hsiaochunnn commented Aug 5, 2024

Description of Change

Adding variant for Waveshare ESP32-S3-Touch-LCD-1.69 & ESP32-S3-LCD-1.69 board containing MCU, LCD, RTC, and IMU.

Tests scenarios

Untested, pin definition is based on information in Waveshare wiki.

Related links

Wiki For ESP32-S3-Touch-LCD-1.69
Wiki For ESP32-S3-LCD-1.69

Pending USB VID PR: espressif/usb-pids/pull/169

Copy link
Contributor

github-actions bot commented Aug 5, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Added Waveshare ESP32-S3-Touch-LCD-1.69 & ESP32-S3-LCD-1.69 board":
    • probably contains Jira ticket reference (LCD-1, LCD-1). Please remove Jira tickets from commit messages.
    • summary looks empty
    • type/action looks empty
  • the commit message "Added Waveshare ESP32-S3-Touch-LCD-1.69 & ESP32-S3-LCD-1.69 board":
    • probably contains Jira ticket reference (LCD-1, LCD-1). Please remove Jira tickets from commit messages.
    • summary looks empty
    • type/action looks empty
  • the commit message "Change the default PSRAM OPI mode to Enabled":
    • summary looks empty
    • type/action looks empty
  • the commit message "Change the default PSRAM OPI mode to disabled":
    • 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).

⚠️ Please consider squashing your 8 commits (simplifying branch history).

👋 Hello Y1hsiaochunnn, 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 af35ecc

@P-R-O-C-H-Y
Copy link
Member

@Y1hsiaochunnn CI is failing as the ESP32-S3-LCD-1.69 is missing SPI default pins. Can you please add those. Thanks

# Conflicts:
#	variants/waveshare_esp32_s3_lcd_169/pins_arduino.h
#	variants/waveshare_esp32_s3_touch_lcd_169/pins_arduino.h
@Y1hsiaochunnn
Copy link
Contributor Author

my bad, I'm a little confused about the configuration of the two boards,Now that I've reworked it, it should work just fine

Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

@Y1hsiaochunnn Both boards are using same chip -> ESP32S3 with 16MB flash and 8MB OPI PSRAM. I have requested changes only for 1 board to match the chip specifications + also added some recommended partition tables.

Please do the changes for both boards if you agree :)

boards.txt Show resolved Hide resolved
boards.txt Show resolved Hide resolved
boards.txt Outdated Show resolved Hide resolved
boards.txt Show resolved Hide resolved
boards.txt Show resolved Hide resolved
# Conflicts:
#	variants/waveshare_esp32_s3_lcd_169/pins_arduino.h
#	variants/waveshare_esp32_s3_touch_lcd_169/pins_arduino.h
@Y1hsiaochunnn
Copy link
Contributor Author

Hi, @P-R-O-C-H-Y According to the configuration parameters you mentioned, I have modified the configuration of two boards

Copy link
Contributor Author

@Y1hsiaochunnn Y1hsiaochunnn left a comment

Choose a reason for hiding this comment

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

I have made the requested changes. Please review the updates.

Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

Looks good now :)

@P-R-O-C-H-Y P-R-O-C-H-Y added the Status: Pending Merge Pull Request is ready to be merged label Aug 6, 2024
@P-R-O-C-H-Y
Copy link
Member

@Y1hsiaochunnn When this PR gets merged, it would be nice if you also update the Waveshare wiki, that instead of selecting "S3 dev module" you can select the board itself? :)

@Y1hsiaochunnn
Copy link
Contributor Author

Hi, @P-R-O-C-H-Y ,I will continue to monitor this PR merge status and update the wiki to select the boards as soon as possible after the merge

@Y1hsiaochunnn
Copy link
Contributor Author

Hi, @P-R-O-C-H-Y ,Actually, what I don't quite understand is that if I use arduino-esp32 version 2.0.11, do some adaptation of the codebase and it works. When I upgrade to the latest version like 3.0.3, it doesn't work. Is this related to the custom library? Or is it related to the configuration of the updated arduino-esp32? Can you provide relevant documentation for reference?

@P-R-O-C-H-Y
Copy link
Member

Hi, @P-R-O-C-H-Y ,Actually, what I don't quite understand is that if I use arduino-esp32 version 2.0.11, do some adaptation of the codebase and it works. When I upgrade to the latest version like 3.0.3, it doesn't work. Is this related to the custom library? Or is it related to the configuration of the updated arduino-esp32? Can you provide relevant documentation for reference?

It's hard to answer as I don't properly understand the issue you are having. Do you have compilation issues or run-time issues? For 3.X we introduced some breaking changes to peripheral APIs so we also created a Migration guide to help with migrating projects/libraries from 2.X to 3.X version. Also this Compatibility guide for libraries maintainers. It's explaining how your code base can support both 2.X and 3.X versions.

Feel free to open an issue with all informations, reproducible sketch and logs. So we don't spam this board addition as its not related :)

@Y1hsiaochunnn
Copy link
Contributor Author

Hi, @P-R-O-C-H-Y ,Actually, what I don't quite understand is that if I use arduino-esp32 version 2.0.11, do some adaptation of the codebase and it works. When I upgrade to the latest version like 3.0.3, it doesn't work. Is this related to the custom library? Or is it related to the configuration of the updated arduino-esp32? Can you provide relevant documentation for reference?

It's hard to answer as I don't properly understand the issue you are having. Do you have compilation issues or run-time issues? For 3.X we introduced some breaking changes to peripheral APIs so we also created a Migration guide to help with migrating projects/libraries from 2.X to 3.X version. Also this Compatibility guide for libraries maintainers. It's explaining how your code base can support both 2.X and 3.X versions.

Feel free to open an issue with all informations, reproducible sketch and logs. So we don't spam this board addition as its not related :)

Thank you for your answer. This is just a little confusion for me. I think I should follow what you said and check the Migration guide and Compatibility guide,If I have a new problem, I'll open a new issue

@Xylopyrographer
Copy link
Contributor

@Y1hsiaochunnn - If you are able to update the Waveshare Wiki pages, could you also add that the Waveshare ESP32-S3-Matrix now has direct support starting with core v3.0.4 and can be selected from the Arduino IDE Boards manager.

@Y1hsiaochunnn
Copy link
Contributor Author

Y1hsiaochunnn commented Aug 7, 2024

@Y1hsiaochunnn - If you are able to update the Waveshare Wiki pages, could you also add that the Waveshare ESP32-S3-Matrix now has direct support starting with core v3.0.4 and can be selected from the Arduino IDE Boards manager.

Hi, @Xylopyrographer I will plan it

@me-no-dev me-no-dev merged commit 8792145 into espressif:master Aug 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Pending Merge Pull Request is ready to be merged Type: 3rd party Boards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants