search

adafruit / Adafruit_CircuitPython_ST7735

CircuitPython display driver for ST7735
MIT License
5 stars 9 forks source link

Added the ST7735 Display Driver #2

Closed makermelissa closed 5 years ago

makermelissa commented 5 years ago

I have subclassed all of the ST7735 variations and this fixes #1. There will be a separate PR for the ST7789.

makermelissa commented 5 years ago

Oh yeah, I fixed all the stuff so it passes on Travis too.

makermelissa commented 5 years ago

I think you'll like how it is much better. I know I do.

jerryneedell commented 5 years ago

Should the example be updated to use TileGrid instead of Sprites and x,y instead of position? https://github.com/adafruit/Adafruit_CircuitPython_ST7735/blob/master/examples/st7735_simpletest.py#L16 Or should that be a separate PR

makermelissa commented 5 years ago

@jerryneedell, I updated the example in this PR to use TileGrid. I just went ahead and updated it in the Readme as well.

makermelissa commented 5 years ago

Since I just got a TFT Mini Breakout, I wrote an example for that too.

deshipu commented 5 years ago

Tested with a 160x128 display, works well.

tannewt commented 5 years ago

As we chatted about, let's split this into two repos because the ST7735 and ST7735R chips are separate ICs and datasheets. That way the driver structure for displays matches non-displays.

makermelissa commented 5 years ago

Hi @tannewt, should the new Repo be named Adafruit_CircuitPython_ST7735R then? I'm just wondering if this may be more confusing to users who aren't as familiar with the differences between the R and non-R. Before starting on this, I wouldn't have known the difference. I'm thinking we should get rid of the non-R code and just keep this repo with the R code (since that's what the displays use) so that it's closer to the Arduino setup. Also, if it sits better with you, we could either rename this repo to the R version or create a repo with the R on it and delete this one.

tannewt commented 5 years ago

I've created a new repo for the R variant: https://github.com/adafruit/Adafruit_CircuitPython_ST7735R

Since we may want to use this name in the future it's better to create a new repo. Renaming this one causes a redirect from here to the new name.

While I agree the naming is potentially confusing I think awareness of this detail is needed for all drivers named after the IC. I don't expect many customers to work at this level anyway.

We're moving to a product driver model where Adafruit customers will either grab example code from the product guide or use a library like FeatherWing that does it for them.

Thanks for sticking with me!

makermelissa commented 5 years ago

Thanks for making that. I’ll separate this out tonight and create an additional PR for the R version.

deshipu commented 5 years ago

The problem here is that there are really two components here — the IC, and the display itself. While the IC is the same in all those versions, the actual display used, and more importantly, the way they are wired differs. For most other displays this is not a problem, because you only have one "standard" wiring per display size (so you can infer options like RGB/BGR from the size), but with the ST7735 modules there is more variety for some reason.

I'm still unsure if we don't simply want to have a "variant" parameter for creating the display object.

tannewt commented 5 years ago

@deshipu I totally agree. I don't think this library should handle the wiring side except that it exposes constructor arguments to use. The modules themselves can have libraries or example code for those details. It doesn't belong in this library.

Enforcing this separation ensures we document the options well and make it easier to use the IC with other displays.

makermelissa commented 5 years ago

Ok, separation has been done. All of the R code is in the other Repo and is the preferred once since that's what the Adafruit Displays use. This is now the non-R ST7735 driver which is untested since I am not aware of any ST7735 (non-R) displays to test this on. In theory it should work. I'm not sure if we want to release this as is and if somebody does come across a display that this is for and it doesn't work, test it then?