mspivak commented 2 months ago

I successfully used this driver with a SH1106 display by only changing the display offset. I'd like to make this change available to everyone. This is not a new BSP, nor I would create an entirely new component for this small change, so I'm submitting this pull request to know if you want to take if from here or should I continue my plan.

I propose:

Changing the name of the component to esp_lcd_sh110x
Extend sh110x_panel_t to add offset_param two-byte param.
Update ESP_LCD_IO_I2C_SH1107_CONFIG to add the new offset_param set to 0xd3 0x60.
Create ESP_LCD_IO_I2C_SH1106_CONFIG with offset_param set to 0xd3 0x00.
Update docs accordingly.

Please confirm my plans are OK and I'll make them and re-submit the pull request.

Thanks!

ESP-BSP Pull Request checklist

Note: For new BSPs create a PR with this link.

[ ] Version of modified component bumped
[ ] CI passing

Change description

Please describe your change here

CLAassistant commented 2 months ago

All committers have signed the CLA.

tore-espressif commented 2 months ago

Hi @mspivak thank you for your contribution!

We want to keep the API as flexible as possible; that means we are avoiding any hard-coded changes.

Could you please check whether function esp_lcd_panel_set_gap does what you want? See docs here https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/lcd.html#_CPPv421esp_lcd_panel_set_gap22esp_lcd_panel_handle_tii

mspivak commented 2 months ago

To clarify, this is not my final PR, just wanted to check the development workflow on my PR description before I really spend time on it. My proposal actually involves removing a hard-coded value in favour of a new, configurable offset_param parameter. Just OK me on this and I'll send a proper PR.

I tried esp_lcd_panel_set_gap, it's not what I want to do. My change involves a different startup sequence.

espressif / esp-bsp

SH1106 compatibility (BSP-473) #308

ESP-BSP Pull Request checklist

Change description