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

Add multiple boards of the Waveshare ESP32-S3-Touch-LCD-1.46 type #10482

Merged
merged 11 commits into from
Oct 23, 2024

Conversation

Sail-211010
Copy link
Contributor

@Sail-211010 Sail-211010 commented Oct 18, 2024

Added several boards,Here are the boards I added this time:
Waveshare ESP32-S3-Touch-LCD-1.46
Waveshare ESP32-S3-LCD-1.46
Waveshare ESP32-S3-LCD-1.85
Waveshare ESP32-S3-Touch-LCD-1.85-Box
Waveshare ESP32-S3-LCD-1.47
Waveshare ESP32-S3-Touch-LCD-2.1
Waveshare ESP32-S3-Touch-LCD-2.8
Waveshare ESP32-S3-Relay-6CH

Delete old files
Add new board
Add new board
Copy link
Contributor

github-actions bot commented Oct 18, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Add files via upload":
    • summary looks empty
    • type/action looks empty
  • the commit message "Delete boards.txt":
    • summary looks empty
    • type/action looks empty
  • the commit message "Delete boards.txt":
    • 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 11 commits (simplifying branch history).
⚠️

The source branch "ADD-Boards" incorrect format:

  • contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS).
    Please rename your branch.
Messages
📖 This PR seems to be quite large (total lines of code: 2411), you might consider splitting it into smaller PRs

👋 Hello Sail-211010, 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.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- 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 ff5271d

@VojtechBartoska
Copy link
Contributor

VojtechBartoska commented Oct 18, 2024

Hello,

can you please change the name of the PR to more specific one?

Also please add more details into PR description.

Usually boards are added in seperate PRs.

Did you check all already included boards?

@Sail-211010 Sail-211010 changed the title Add boards Add multiple boards of the Waveshare ESP32-S3-Touch-LCD-1.46 type Oct 18, 2024
@Sail-211010
Copy link
Contributor Author

Sail-211010 commented Oct 18, 2024

@VojtechBartoska Hi,I have checked the uploaded file and found nothing wrong for the time being. Please contact me if you find anything wrong.
Because of the vacation. If there is any problem, I will revise it two days later

@VojtechBartoska
Copy link
Contributor

Thanks, we will review

@VojtechBartoska VojtechBartoska added Status: Review needed Issue or PR is awaiting review and removed Resolution: Awaiting response Waiting for response of author labels Oct 18, 2024
waveshare_esp32_s3_touch_lcd_185_box.build.mcu=esp32s3
waveshare_esp32_s3_touch_lcd_185_box.build.core=esp32
waveshare_esp32_s3_touch_lcd_185_box.build.variant=waveshare_esp32_s3_touch_lcd_185_box
waveshare_esp32_s3_touch_lcd_185_box.build.board=WAVESHARE_ESP32_S3_TOUCH_LCD_185_Box
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the board name, as its not fully uppercase

Suggested change
waveshare_esp32_s3_touch_lcd_185_box.build.board=WAVESHARE_ESP32_S3_TOUCH_LCD_185_Box
waveshare_esp32_s3_touch_lcd_185_box.build.board=WAVESHARE_ESP32_S3_TOUCH_LCD_185_BOX

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

@Sail-211010 I see you adding a lot of boards, but the pins_arduino.h file is for some boards common, and you only specify the SPI, I2C, UART and Analog+touch pins. It would be better for your users if you can add also more pins definitions.
For example for the boards with lcd, define the LDC pins, for the relay boards add the relay channel pins etc..

@Sail-211010
Copy link
Contributor Author

@P-R-O-C-H-Y I communicated with other engineers before and planned to define it in the example for developers to interpret the program. If there is any change in the relevant scheme in the future, I will modify the information again, and I may trouble you again. Thank you again

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

@P-R-O-C-H-Y I communicated with other engineers before and planned to define it in the example for developers to interpret the program. If there is any change in the relevant scheme in the future, I will modify the information again, and I may trouble you again. Thank you again

But if you define it there, you can already use the pins definition in the example and will be more useful for users. But ofc, it's your decision. Take a look for example on our esp32-s3-usbotg board pins definition.

Delete old files, modify the definition of the case error
Modify the definition of the case error
@Sail-211010
Copy link
Contributor Author

@P-R-O-C-H-Y Due to the large size of the file, I could not edit the file online, so I deleted the file and re-uploaded it, having modified the capitalization problem you raised(Updated boards.txt)

@Sail-211010
Copy link
Contributor Author

@P-R-O-C-H-Y I communicated with other engineers before and planned to define it in the example for developers to interpret the program. If there is any change in the relevant scheme in the future, I will modify the information again, and I may trouble you again. Thank you again

But if you define it there, you can already use the pins definition in the example and will be more useful for users. But ofc, it's your decision. Take a look for example on our esp32-s3-usbotg board pins definition.

yes,I know what you mean, I have designed this aspect on the board before, but I modified it to the current one before uploading. If necessary, I will launch a PR request next time, thank you for your trouble

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

@P-R-O-C-H-Y I communicated with other engineers before and planned to define it in the example for developers to interpret the program. If there is any change in the relevant scheme in the future, I will modify the information again, and I may trouble you again. Thank you again

But if you define it there, you can already use the pins definition in the example and will be more useful for users. But ofc, it's your decision. Take a look for example on our esp32-s3-usbotg board pins definition.

yes,I know what you mean, I have designed this aspect on the board before, but I modified it to the current one before uploading. If necessary, I will launch a PR request next time, thank you for your trouble

Would be nice to have that updated in future :) Thanks

@Sail-211010
Copy link
Contributor Author

@P-R-O-C-H-Y I communicated with other engineers before and planned to define it in the example for developers to interpret the program. If there is any change in the relevant scheme in the future, I will modify the information again, and I may trouble you again. Thank you again

But if you define it there, you can already use the pins definition in the example and will be more useful for users. But ofc, it's your decision. Take a look for example on our esp32-s3-usbotg board pins definition.

yes,I know what you mean, I have designed this aspect on the board before, but I modified it to the current one before uploading. If necessary, I will launch a PR request next time, thank you for your trouble

Would be nice to have that updated in future :) Thanks

Sure. I'll keep up with the situation

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Oct 23, 2024
@Sail-211010
Copy link
Contributor Author

Hello, is there any other file error in the current progress that I need to modify

@VojtechBartoska VojtechBartoska added this to the 3.1.0-RC2 milestone Oct 23, 2024
@me-no-dev me-no-dev merged commit 3502b9c into espressif:master Oct 23, 2024
16 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.

5 participants