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

Implement i80 bus for ili9xxx displays #6537

Draft
wants to merge 30 commits into
base: dev
Choose a base branch
from

Conversation

clydebarrow
Copy link
Contributor

@clydebarrow clydebarrow commented Apr 15, 2024

What does this implement/fix?

NOTE

This PR has been changed to draft status, since it is going to be replaced by 3 separate PRs. In the meantime you can use this PR as an external component, e.g. for the W32-SC01-Plus display:

external_components:
 - source: github://pr#6537
   components: [ i80, io_bus, ili9xxx, spi ]

i80:
  dc_pin:
    ignore_strapping_warning: true
    number: 0
  data_pins:
    - 9
    - ignore_strapping_warning: true
      number: 46
    - ignore_strapping_warning: true
      number: 3
    - 8
    - 18
    - 17
    - 16
    - 15
  wr_pin: 47

display:
  - platform: ili9xxx
    bus_type: i80
    id: w32_disp
    model: st7796
    dimensions:
      height: 320
      width: 480
    transform:
      mirror_y: false
      mirror_x: false
      swap_xy: true
    reset_pin: 4
    data_rate: 4MHz
    color_order: bgr
    invert_colors: true

Types of changes

Add a new bus parallel type - i80 - used for interfacing LCD display controllers. This is used in a similar way to the spi component, and is utilised by the ili9xxx display driver, enabling support for the Seeed W32-SC01-Plus and the LilyGo T-Display S3.

The changes have been tested on the above as well as numerous SPI interfaced displays previously supported by ili9xxx to ensure that nothing has been broken in the process.

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3761

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# Example config.yaml
i80:
  dc_pin: 7
  wr_pin: 8
  rd_pin: 9
  data_pins:
    - 39
    - 40
    - 41
    - 42
    -
      ignore_strapping_warning: true
      number: 45
    -
      ignore_strapping_warning: true
      number: 46
    - 47
    - 48
    
display:
  - platform: ili9xxx
    bus_type: i80
    cs_pin: 6
    reset_pin: 5
    model: st7789v

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @nielsnl68, mind taking a look at this pull request as it has been labeled with an integration (ili9xxx) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (spi) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.90%. Comparing base (4d8b5ed) to head (af200fa).
Report is 720 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6537      +/-   ##
==========================================
+ Coverage   53.70%   53.90%   +0.19%     
==========================================
  Files          50       50              
  Lines        9408     9623     +215     
  Branches     1654     1698      +44     
==========================================
+ Hits         5053     5187     +134     
- Misses       4056     4112      +56     
- Partials      299      324      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Unfocused
Copy link
Contributor

In case this ends up being mentioned in documentation: Seeed is only a distributor for the WT32-SC01 and WT32-SC01 Plus boards. They're developed & made by Wireless-Tag, which makes a few things that'd be compatible with ESPHome.

@Unfocused
Copy link
Contributor

Unfocused commented Apr 17, 2024

Also on the topic of documentation, to avoid confusion the docs should probably mention that i80 is also commonly called 8080 (or more rarely Intel 8080, or Intel Parallel, or some combination of those words). And some specs call it DBI Type B, just for fun.

And worth mentioning this is only only supporting 8bit bus width for now. Someone will inevitably end up trying to use a display that only supports 16bit.

@clydebarrow
Copy link
Contributor Author

The name is discussed in the docs, and it does specifically say it requires 8 data lines, so one would hope that no-one will try it with 16 (which will fail config validation anyway.)

Copy link
Contributor

@nielsnl68 nielsnl68 left a comment

Choose a reason for hiding this comment

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

This is part of my suggestions.

Sorry need to clean the below. I did go something wrong.

esphome/components/byte_bus/byte_bus.h Outdated Show resolved Hide resolved
esphome/components/i80/__init__.py Show resolved Hide resolved
esphome/components/byte_bus/byte_bus.h Outdated Show resolved Hide resolved
esphome/components/byte_bus/byte_bus.h Outdated Show resolved Hide resolved
esphome/components/byte_bus/byte_bus.h Outdated Show resolved Hide resolved
@esphome esphome bot marked this pull request as draft April 18, 2024 10:38
@esphome
Copy link

esphome bot commented Apr 18, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@nielsnl68
Copy link
Contributor

The name is discussed in the docs, and it does specifically say it requires 8 data lines, so one would hope that no-one will try it with 16 (which will fail config validation anyway.)

We can always change the doc's so it could also be used with 16 bit data busses.

Copy link
Contributor

@nielsnl68 nielsnl68 left a comment

Choose a reason for hiding this comment

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

this is what i have so far

esphome/components/byte_bus/byte_bus.h Outdated Show resolved Hide resolved
esphome/components/byte_bus/byte_bus.h Outdated Show resolved Hide resolved
esphome/components/i80/__init__.py Show resolved Hide resolved
esphome/components/ili9xxx/ili9xxx_display.h Outdated Show resolved Hide resolved
esphome/components/ili9xxx/ili9xxx_display.h Outdated Show resolved Hide resolved
esphome/components/ili9xxx/ili9xxx_display.cpp Outdated Show resolved Hide resolved
esphome/components/ili9xxx/ili9xxx_display.cpp Outdated Show resolved Hide resolved
esphome/components/ili9xxx/ili9xxx_display.cpp Outdated Show resolved Hide resolved
esphome/components/ili9xxx/ili9xxx_display.cpp Outdated Show resolved Hide resolved
@clydebarrow clydebarrow marked this pull request as ready for review April 23, 2024 09:23
@esphome esphome bot requested a review from nielsnl68 April 23, 2024 09:23
@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (spi) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Please split up this PR into PRs for the separate component changes?

I know its all a big dependency chain, but its much easier to review and merge them individually. 🙏

@esphome esphome bot marked this pull request as draft May 21, 2024 01:07
@clydebarrow
Copy link
Contributor Author

Please split up this PR into PRs for the separate component changes?

Started - I have a PR for the i80 bus. Will have to wait for that to be merged before generating the others since they will fail against the dev branch until then. So have left the original PR in place as a draft, to support early adopters using it via external components, given this could be a drawn-out process.

@bearpawmaxim
Copy link
Contributor

bearpawmaxim commented May 26, 2024

Hello @clydebarrow ! Thank you for your work!
I finally got my WT32-SC01 Plus display working with esphome with the following config:

external_components:
  - source: github://pr#6537
    components: [ io_bus, i80, spi, ili9xxx ]

i80:
  - id: i80bus
    dc_pin: GPIO0
    wr_pin: GPIO47
    data_pins:
      - GPIO9
      - GPIO46
      - GPIO3
      - GPIO8
      - GPIO18
      - GPIO17
      - GPIO16
      - GPIO15

display:
  - platform: ili9xxx
    model: ST7789V
    bus_type: i80
    reset_pin: GPIO4
    data_rate: 20MHZ
    pixel_mode: 18bit
    color_order: bgr
    invert_colors: true
    dimensions:
      width: 480
      height: 320
    transform:
      swap_xy: true
    auto_clear_enabled: false

But I am a little confused about the fact that it can't work with 40mHz data rate (as with ArduinoGFX and LovyanGFX) and the 250ms full redraw time (when doing strftimewith auto_clear_enabled: true)..
Is there a way to somehow speed up the drawing?

@clydebarrow
Copy link
Contributor Author

Use LVGL for fast drawing: #6363

And I'm pretty sure LoyanGFX doesn't actually drive it at 40MHz, I believe the ESP32 maxes out at 20MHz I/O clock for parallel transfers. Just because someone wrote 40MHz in the setup doesn't mean it actually achieves that.

@bearpawmaxim
Copy link
Contributor

Use LVGL for fast drawing

Actually, I'm now drawing the UI with it. Thanks!

@nagyrobi
Copy link
Member

Wow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants