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 support for the Waveshare ESP32-S3-Touch-LCD1.28 board #67311

Conversation

joelguittet
Copy link
Contributor

@joelguittet joelguittet commented Jan 8, 2024

The purpose of this Pull Request is to add support for the Waveshare ESP32-S3-Touch-LCD1.28 board.

@zephyrbot zephyrbot added area: Samples Samples area: Display area: ADC Analog-to-Digital Converter (ADC) area: Input Input Subsystem and Drivers labels Jan 8, 2024
@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from 2cd93a6 to 0d93a65 Compare January 8, 2024 13:30
Copy link
Member

@fabiobaltieri fabiobaltieri left a comment

Choose a reason for hiding this comment

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

cool board

@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch 2 times, most recently from 5128043 to 2b24650 Compare January 8, 2024 20:47
@fabiobaltieri
Copy link
Member

@joelguittet one more thing, you named the board waveshare_esp32s3_touch_lcd but when I see that Waveshare has two "waveshare esp32s3 touch lcd" boards, a 1.28'' and a 4.3'', could you rename the board to something like waveshare_esp32s3_touch_lcd_1_28? (We have a buydisplay_2_8_tft_touch_arduino shield already)

@kartben
Copy link
Collaborator

kartben commented Jan 8, 2024

@joelguittet one more thing, you named the board waveshare_esp32s3_touch_lcd but when I see that Waveshare has two "waveshare esp32s3 touch lcd" boards, a 1.28'' and a 4.3'', could you rename the board to something like waveshare_esp32s3_touch_lcd_1_28? (We have a buydisplay_2_8_tft_touch_arduino shield already)

It might make sense to keep the generic folder name though so that it can become the home for waveshare_esp32s3_touch_lcd_4_3, should it be added someday?

@fabiobaltieri
Copy link
Member

It might make sense to keep the generic folder name though so that it can become the home for waveshare_esp32s3_touch_lcd_4_3, should it be added someday?

They really don't have much in common https://files.waveshare.com/wiki/ESP32-S3-Touch-LCD-1.28/ESP32-S3-Touch-LCD-1.28-Sch.pdf https://files.waveshare.com/wiki/ESP32-S3-Touch-LCD-4.3/ESP32-S3-Touch-LCD-4.3-Sch.pdf, normally the board directory is for different revisions or variation with common pinout or stuff like that, but these are really two different boards, they barely use the same chip (one uses a module).

@kartben
Copy link
Collaborator

kartben commented Jan 8, 2024

Ah! Sorry then :) Product naming is always funny. There I was, thinking only display size would be different based on the names 😅

@joelguittet
Copy link
Contributor Author

Good point, yes I just checked too and they are very different. Will rename this one now.

@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch 2 times, most recently from bd14ef5 to 751fbcf Compare January 8, 2024 22:36
Copy link
Member

@fabiobaltieri fabiobaltieri 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, guess we have to wait on the display pr

@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from 751fbcf to 90c647c Compare January 9, 2024 20:24
@@ -0,0 +1 @@
CONFIG_MAIN_STACK_SIZE=4096
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh, our minimal LVGL sample is not minimal anymore? What is the display resolution? Would you need same one-line-overlay for another LVGL sample?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the default stack size is not enough. Display is 240x240. I have seen at least one other board with the 4096 stack size.
What do you mean by one line overlay for another lvgl sample ?

@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from 90c647c to d613574 Compare January 20, 2024 21:42
@zephyrbot zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Jan 20, 2024
@zephyrbot zephyrbot requested a review from galak January 20, 2024 21:43
@joelguittet
Copy link
Contributor Author

@sylvioalves true! I was waiting other esp32 board to be migrated to have an example and the common soc files I need here. It's now updated here!

The board esp32s3_touch_lcd_1_28/esp32s3/procpu has been tested with the following samples:

  • samples/hello_world => works fine
  • samples/drivers/adc/ to read the battery voltage (overlay is provided) => works fine
  • samples/subsys/display/lvgl/ to check the display and the touchscreen controller => works fine

The board esp32s3_touch_lcd_1_28/esp32s3/appcpu has been tested compiled with the samples/hello_world but it's not booting. esp32s3_devkitm has the same behavior. I guess this is not due to this new waveshare board, but to be honest, I don't know the behavior of the appcpu board / how to test it... if somebody can guide me on this part ?

@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from cbb48bb to ad44654 Compare March 20, 2024 22:31
@nordicjm
Copy link
Collaborator

This needs to be targeted to main not the collab-hwm branch

@fabiobaltieri
Copy link
Member

@joelguittet you can change the base of the existing PR, just click edit on the top and the base should become a dropbox and you can pick main there

@joelguittet
Copy link
Contributor Author

@nordicjm @fabiobaltieri are you sure of your request ? If I target main then the PR will show all the commits of the collab hwm branch..... And if my PR is merged then it will merge everything in main including all the collab hwm commits.
Maybe you want me to target main, review only the last 3 commits and wait until collab hwm is merge before merging my PR ? If collab branch is merge and deleted GitHub should automatically retarget this PR to main so should not be blocking? (It s working like that on bitbucket but I m not sure on GitHub to be honest)

Note : I will fix the doc issue reported by the pipeline asap of course.

@nordicjm
Copy link
Collaborator

nordicjm commented Mar 21, 2024

You need to rebase it properly so that your changes apply to main, not the collab-hwm branch, so this PR needs to contain just your commits that would clearly apply. collab-hwm was merged weeks ago

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Mar 21, 2024

yeah well if you don't change the base it's going to be merged on the branch which is certainly not what you want either :-)

after you change the base the PR may become a giant mess, then you can fix it up locally and force push it again, all normal

@joelguittet
Copy link
Contributor Author

Oh my bad 😞 seems I m tired, just realized the collab hwm branch feature have merged to main branch too...
I was thinking it was not done yet and still a rework in progress
For sure now I will rebase on main !

@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from ad44654 to 7cc6fc2 Compare March 22, 2024 11:16
@joelguittet joelguittet changed the base branch from collab-hwm to main March 22, 2024 11:17
@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from 7cc6fc2 to 765fd96 Compare March 22, 2024 11:18
@joelguittet
Copy link
Contributor Author

Updated :)

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Comments are optional

Comment on lines +51 to +54
scratch_partition: partition@210000 {
label = "image-scratch";
reg = <0x00210000 0x00040000>;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would remove and give to slots instead since swap using scratch is not recommended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree but it's retrieved from the esp32s3_devkitm board from Espressif. I guess it's better to keep consistency across similar boards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nordicjm what is your opinion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sylvioalves ? Your opinion about this ? What should be the default in zephyr for this kind of board ?

Copy link
Member

Choose a reason for hiding this comment

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

@nordicjm hey since we are on the topic I'll add a question: do you think it would make sense to have few templates for in-tree board partitions maps? We have a lot of boards based on esp32, nrf52840 etc... and the flash partitioning is pretty much always the same, maybe we can have some dtsi and #include it in the board file? As much as I dislike mid-structure includes, it may be better than repeating this to infinity.

Comment on lines +93 to +96
scratch_partition: partition@210000 {
label = "image-scratch";
reg = <0x00210000 0x00040000>;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above

@nordicjm nordicjm dismissed their stale review March 22, 2024 12:35

addressed

@joelguittet
Copy link
Contributor Author

@nordicjm gentle ping for the above question, thanks!

fabiobaltieri
fabiobaltieri previously approved these changes Apr 23, 2024
Add support for the Waveshare ESP32-S3-Touch-LCD-1.28 board, including
support the LCD and touchscreen controllers. Tested with samples
already available.

Signed-off-by: Joel Guittet <[email protected]>
Add configuration for the Waveshare ESP32-S3-Touch-LCD-1.28 board.

Signed-off-by: Joel Guittet <[email protected]>
Add configuration for the Waveshare ESP32-S3-Touch-LCD-1.28 board.

Signed-off-by: Joel Guittet <[email protected]>
@joelguittet joelguittet force-pushed the boards/waveshare-esp32s3-touch-lcd branch from 3e30de3 to 36850dc Compare April 24, 2024 19:35
@carlescufi carlescufi merged commit d9e5aa4 into zephyrproject-rtos:main Apr 25, 2024
22 checks passed
@joelguittet joelguittet deleted the boards/waveshare-esp32s3-touch-lcd branch April 25, 2024 15:13
@joelguittet
Copy link
Contributor Author

Thanks for the reviews here ! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ADC Analog-to-Digital Converter (ADC) area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Display area: Input Input Subsystem and Drivers area: Samples Samples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants