adafruit / Adafruit_CircuitPython_RGB_Display

Drivers for RGB displays for Adafruit CircuitPython.
MIT License
132 stars 52 forks source link

row start and column start offsets are not being applied #20

Closed BravoDelta151 closed 5 years ago

BravoDelta151 commented 6 years ago

Discovered corruption on two sides while testing the 1.44" TFT display with ST7735R. When I tested using Arduino, no issues were seen.

Looking at the Arduino Library code (Adafruit-ST7735), _rowstart and _colstart are being set in the function Adafruit_ST7735::initR for the different displays. These offsets are then used to adjust the start and end points when writing data in the function Adafruit_ST77xx::setAddrWindow. This is not being done in the CircuitPython code and is causing corruption along two edges of the screen.

I assume the offsets are the same, so it should be a simple matter of adding them to the rgb and st7735 modules.

deshipu commented 6 years ago

This is a little bit more complex. First of all, the CP driver doesn't set the window once and update the whole screen each time — instead it sets the window each time there is an update, only to the area that needs to be updated. So the offsets have to be added there in the window-setting functions. But the exact offsets that need to be added depend on the particular display module and on the rotation of the display. There are different modules on the market that differ in how the driver chip is connected to the physical display — which results in different offsets. I think the best would be to allow specifying the offsets in the init call, but then we get quite a lot of parameters in there. Right now I'm thinking about rewriting the whole library to get rid of inheritance, and instead use a factory function to create the display object with all the correct parameters.

riffautae commented 5 years ago

I ran in to this same issue and used this code to make it work.

class ST7735Rmini(ST7735R):
    def __init__(self, spi, dc, cs, rst=None, width=128, height=128, xoffset=2, yoffset=3):
        self.xoffset = xoffset
        self.yoffset = yoffset
        super().__init__(spi, dc, cs, rst, width, height)

    def _block(self, x0, y0, x1, y1, data=None):
        super()._block(x0+self.xoffset, y0+self.yoffset, x1+self.xoffset, y1+self.yoffset, data)

Until you refactor, would you mind if we added support for offsets?

I think there are two ways to add this.

The easy way is to just edit the ST7735R devices to support it. Since I don't know how the other displays work this might be preferred to keep this particular change cleaner. Here I also updated init since it sends _CASET/_RASET just like _block does, but it makes no difference on my TFT.

class ST7735R:
    def __init__(self, spi, dc, cs, rst=None, width=128, height=160, xoffset=0, yoffset=0):
        """
        Some variations of the ST7735R require an x,y offset to display correctly.
        For example, the type that comes with a 'green tab' sticker uses xoffset=2, yoffset=3
        """
        self.xoffset = xoffset
        self.yoffset = yoffset
        super().__init__(spi, dc, cs, rst, width, height)

    def _block(self, x0, y0, x1, y1, data=None):
        super()._block(x0+self.xoffset, y0+self.yoffset, x1+self.xoffset, y1+self.yoffset, data)

    def init(self):
        super().init()
        cols = struct.pack('>HH', self.xoffset, self.width - 1 + self.xoffset)
        rows = struct.pack('>HH', self.yoffset, self.height - 1 + self.yoffset)
        for command, data in (
                (_CASET, cols),
                (_RASET, rows),
                (_NORON, None),
                (_DISPON, None),
        ):
            self.write(command, data)

The other way is add xoffset/yoffset to Display and then anything that wants to be able to set an offset can pass it in. It would be similar to this:

class Display:
    def __init__(self, width, height, xoffset=0, yoffset=0):
        #save offsets

    # update _block

class DisplaySPI
    def __init__(self, spi, dc, cs, rst=None, width=1, height=1,
baudrate=12000000, polarity=0, phase=0, xoffset=0, yoffset=0):
        # pass them to super

class <anything based on Display that cares>
    def __init__(self, spi, dc, cs, rst=None, width=128, height=128, xoffset=0, yoffset=0):
        # classes that do *not* send the offset during init can just pass them to super

class ST7735R:
    # since it sends CASET/RASET in init we might as well use the offset
    # though it also uses it when sending pixels so i don't think it matters
    def init(self):
        super().init()
        cols = struct.pack('>HH', self.xoffset, self.width - 1+self.xoffset) # add offset
        rows = struct.pack('>HH', self.yoffset, self.height - 1+self.yoffset) # add offset
        #etc
siddacious commented 5 years ago

Hi @riffautae, your suggestions are both good however I believe this library is largely going to be replaced by individual drivers for each controller type using the new displayio Display object that will be available in Circuit Python 4 which is in beta right now. Now that I think of it however it may stick around to support Circuit Python 3.

I did essentially the same thing as you for a small ST7735R display however the Display object in displayio supports x and y offsets which should handle both our use cases once the ST7735 driver is complete.

Since it sounds like you already have code that works for you, you have several options:

I did some initial porting of the init code for the ST7735 however it's not working yet and needs some debugging. I'm more than happy to share it if you're interested.

riffautae commented 5 years ago

I will look into making a displayio driver for it. I'll also submit a pr for the shorter change for the ST7735R specifically. Since there is a new system I don't think its worth trying to adapt all the old code.

makermelissa commented 5 years ago

Fixed by #24.