landonr / lilygo-tdisplays3-esphome

tdisplay s3 170x320 running esphome using patched tft_espi
90 stars 31 forks source link

Update touchscreen to ESPHome 2023.12.0 #50

Closed guillempages closed 10 months ago

guillempages commented 11 months ago

The touchscreen base component had some breaking changes that required adaptations.

Also the yaml configuration is slightly different, so updated the example-touch.yaml file as well

Fixes: #48

guillempages commented 11 months ago

@landonr we have a problem with CI 😞 The current example-with-touch.yaml cannot build with the latest ESPHome version (2023.12.x), since some changes to the touchscreen (and display) code are needed. But the yaml file is referencing this repo in the external_components part (as it should, since that's what is needed to use the repo at all), and then it just uses main instead of actually checking out the code of this PR that should be checked.

I've tested the changes locally and they work for me, if I set

external_components:
  - source: github://landonr/lilygo-tdisplays3-esphome@new_touchscreen
    components: [tdisplays3]

in the yaml file.

But of course this is not what should get into the file.

We have three options (sorted by my opinion from "best" to "worse", although the "best" one would require some work on your side):

a) Configure CI, so that it actually uses the PR being checked in the external_components part b) Force-merge this PR bypassing CI c) Adapt this PR to use the branch mentioned above in the yaml file, and create a following PR to set the branch back to the "right" value.

landonr commented 11 months ago

@landonr we have a problem with CI 😞 The current example-with-touch.yaml cannot build with the latest ESPHome version (2023.12.x), since some changes to the touchscreen (and display) code are needed. But the yaml file is referencing this repo in the external_components part (as it should, since that's what is needed to use the repo at all), and then it just uses main instead of actually checking out the code of this PR that should be checked.

I've tested the changes locally and they work for me, if I set

external_components:
  - source: github://landonr/lilygo-tdisplays3-esphome@new_touchscreen
    components: [tdisplays3]

in the yaml file.

But of course this is not what should get into the file.

We have three options (sorted by my opinion from "best" to "worse", although the "best" one would require some work on your side):

a) Configure CI, so that it actually uses the PR being checked in the external_components part b) Force-merge this PR bypassing CI c) Adapt this PR to use the branch mentioned above in the yaml file, and create a following PR to set the branch back to the "right" value.

I usually make a commit with "ref: branch_name" as a parameter, get it to pass, and then reverse that commit and merge.

Have you ever seen anything like solution A? It would be nice to implement, but my only idea on how to implement it right now would be to look for the string in the yaml and change it before building

guillempages commented 10 months ago

@landonr I went for solution A. Took a little bit of trial-and-error, but it seems to work now :-)

I've added the change to the github workflow to this PR, because nothing can pass CI until this is merged. :-(

Can you approve and merge, so that we have a working setup again? (I cannot self-approve)