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

bsp: Add display and touch support to gerenic BSP. #255

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Conversation

espzav
Copy link
Collaborator

@espzav espzav commented Dec 1, 2023

ESP-BSP Pull Request checklist

  • Version of modified component bumped
  • CI passing

Change description

  • Updated led_indicator and support classic RGB LED (e.g. ESP32C2)
  • Added LCD and touch support to generig BSP. I selected only some drivers
  • LCD is supported only via SPI
  • Touch is supported only via I2C

Issues

  • When some part not used (e.g. buttons, leds, LCD, touch), the component is still downloaded
  • Components for all touches and LCDs are downloaded

Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

thanks @espzav I left few comments

bsp/esp_bsp_generic/include/bsp/display.h Show resolved Hide resolved
bsp/esp_bsp_generic/include/bsp/esp_bsp_generic.h Outdated Show resolved Hide resolved
bsp/esp_bsp_generic/include/bsp/esp_bsp_generic.h Outdated Show resolved Hide resolved
bsp/esp_bsp_generic/src/esp_bsp_generic.c Outdated Show resolved Hide resolved
bsp/esp_bsp_generic/Kconfig Outdated Show resolved Hide resolved
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

  • There is a missing comma at esp_bsp_generic.c:77 and similarly below. Button ADC fails to compile
  • It is a little misleading that I can still set 'Number of LEDs' to 5, but I can define only 1 'Classic RGB LED'. Can we hide the 'number of LEDs' menu for classic RGB LEDs?

@espzav
Copy link
Collaborator Author

espzav commented Dec 8, 2023

  • There is a missing comma at esp_bsp_generic.c:77 and similarly below. Button ADC fails to compile
  • It is a little misleading that I can still set 'Number of LEDs' to 5, but I can define only 1 'Classic RGB LED'. Can we hide the 'number of LEDs' menu for classic RGB LEDs?

@tore-espressif Thank you for review! Fixed all mistakes and changed LEDs in KConfig - the first select type and then number of leds (zero still means no leds).

Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

@espzav Thanks for the updates, I left few comments worth considering.

I will test with a real custom board and LCD tomorrow

bsp/esp_bsp_generic/Kconfig Show resolved Hide resolved
bsp/esp_bsp_generic/Kconfig Outdated Show resolved Hide resolved
bsp/esp_bsp_generic/Kconfig Outdated Show resolved Hide resolved
bsp/esp_bsp_generic/Kconfig Outdated Show resolved Hide resolved
@espzav espzav force-pushed the bsp/generic_lcd branch 2 times, most recently from 2d9751e to 0be3141 Compare December 14, 2023 07:58
@espzav
Copy link
Collaborator Author

espzav commented Dec 14, 2023

@tore-espressif Thank you for the review. Fixed and one comment left.

Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

@espzav Thanks for the updates. I tested with ESP32-S3-EYE and found few things that did not work out of the box

bsp/esp_bsp_generic/src/esp_bsp_generic.c Outdated Show resolved Hide resolved
bsp/esp_bsp_generic/src/esp_bsp_generic.c Outdated Show resolved Hide resolved
@espzav
Copy link
Collaborator Author

espzav commented Dec 15, 2023

@tore-espressif Thank you for the review and HW test. Everything fixed/added.

@espzav espzav merged commit c629281 into master Jan 8, 2024
10 checks passed
@espzav espzav deleted the bsp/generic_lcd branch January 8, 2024 12:53
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.

2 participants