ILI9341_t3 readRect() / writeRect() bug

pictographer

Well-known member
The readRect() and writeRect() functions produce a slight color shift. I would assume that reading a rectangular region and writing it back to the exact same location should not change the display. If you look closely at the display after running the program below, you'll see that the checkerboard pattern is subtly messed up. Some of the squares are split into a lighter part resulting from readRect/writeRect and a darker part with the correct color.

My first guess is that this is a result of the way the colors are packed into 16-bit integers. I don't know whether the problem occurs during reading or writing. Should be possible to figure it out by printing the color values before and after.

Here's the test case. I observed this on a T3.0 running Arduino 1.0.6 and Teensyduino 1.21-test4.

By the way, the copyright year on the Teensy Loader 'About' window probably ought to be updated to a range, e.g. "Copyright © 2013-2015, PJRC.COM, LLC".

Code:
#include "SPI.h"
#include "ILI9341_t3.h"

ILI9341_t3 d = ILI9341_t3(10, 9);

uint16_t buf[50 * 50]; 

void drawBackground() {
  d.fillScreen(ILI9341_BLACK);
  for (int x = 0; x < d.width(); x += 20) {
    for (int y = 0; y < d.height(); y += 20) {
      d.fillRect(x, y, 10, 10, d.color565(31, 31+x, 31+y));
      d.fillRect(x + 10, y + 10, 10, 10, d.color565(31+x, 31+y, 31));
    }
  }
  
  for (int x = 15; x < d.width(); x += 100) {
    d.readRect(x, 55, 50, 50, buf);
  
    // The following line should not change the display because it 
    // simply copies a region back from whence it came. The colors are 
    // altered slightly.
    d.writeRect(x, 55, 50, 50, buf);
    d.drawLine(2, 30, x, 54, ILI9341_WHITE);
  }
    
  for (int i = 0; i < 2; ++i) {
    if (i == 0) {
      d.setTextColor(ILI9341_BLACK, ILI9341_BLACK);
    } else {
      d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
    }
    d.setCursor(1 - i, 5 - i);
    d.print("Every square should be a single color\n"
            "but readRect/writeRect changes the colors,\n"
            "so some of the squares are split.");
  }
}

void setup() {
  d.begin();
  d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
  d.setRotation(1);
  drawBackground();
}

void loop() {}

ILI_rect_bug.jpg

The photo above shows the issue. Sorry the illustration is so subtle. The white lines point to the 50x50 squares that are color shifted but shouldn't be. The effect is easiest to see at the perimeter of these squares where the checkerboard pattern isn't quite right. All of the 10x10 squares should be a single color.
 
The readRect values are different than what was drawn. The example below shows the problem more clearly and in more detail.

Code:
#include "SPI.h"
#include "ILI9341_t3.h"


ILI9341_t3 d = ILI9341_t3(10, 9);


uint16_t buf[50 * 50]; 


void drawBackground() {
  d.fillScreen(ILI9341_BLACK);
  for (int x = 0; x < d.width() - 100; x += 100) {
    for (int y = 0; y < d.height() - 100; y += 100) {
      uint16_t c0 = d.color565(31, 31+x, 31+y);
      d.fillRect(x, y, 50, 50, c0);
      uint16_t c1 = d.color565(31+x, 31+y, 31);
      d.fillRect(x + 50, y + 50, 50, 50, c1);
      for (int k = 0; k < 51; k += 50) {
        d.setCursor(x + k + 1, y + k + 1);
        d.setTextColor(ILI9341_BLACK, ILI9341_BLACK);
        d.print(k ? c1 : c0, HEX);
        d.setCursor(x + k, y + k);
        d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
        d.print(k ? c1 : c0, HEX);
        d.readRect(x + k + 20, y + k + 20, 20, 20, buf);
        d.writeRect(x + k + 20, y + k + 20, 20, 20, buf);
        d.setCursor(x + k + 21, y + k + 21);
        d.setTextColor(ILI9341_BLACK, ILI9341_BLACK);
        d.print(buf[0], HEX);
        d.setCursor(x + k + 20, y + k + 20);
        d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
        d.print(buf[0], HEX);
      }
    }
  }
  
  for (int i = 0; i < 2; ++i) {
    if (i == 0) {
      d.setTextColor(ILI9341_BLACK, ILI9341_BLACK);
    } else {
      d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
    }
    d.setCursor(1 - i, 205 - i);
    d.print("Every square should be a single color\n"
            "but readRect/writeRect changes the colors,\n"
            "so some of the squares have little squares in them.");
  }


}


void setup() {
  d.begin();
  d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
  d.setRotation(1);
  drawBackground();
}


void loop() {}

ILI9341_write_bug_2.jpg
The photo above illustrates that certain colors are not read correctly by readRect(). For example a pixel with color 0x1C03 is read back as 1E03 and 0x18F0 is read as 0x18F8.
 
Interesting test, I just ordered a pair of these. I was wondering if you get the same results if you reorder what color goes where.
 
But you already know that when reading the colors are 18 bit (RGB 6-6-6)?

See following page 64 of the ILI9341 documentation:

reading.jpg
 
Last edited:
bkausbk, thank you for color space comment and the documentation reference.

I don't understand is why the color shifts when reading from the display and writing back to it. I would expect the shift to happen when the display was first written so copying a region to itself should not change the colors. Or if one rendered a ramp with more bands than the pixel color format, we would see the effects of color quantization.

When specifying a color, the color565() function takes RBG 5-6-5. It appears to be correct. It appears the colors read back come from a reduced color space that's usually lighter (brighter) than the color on the display.

I tried commenting out the gamma curves in init_commands, but there was no obvious effect. I'm wondering if there is a mistake in how the color is read over SPI and packed into 16 bits.

defragster, I tried a few quick variations on my test code. I didn't see any effect of changing the order of writing colors.

By the way, the reason I'm experimenting with readRect() and writeRect() is that I've implemented a joystick-like controller using the Teensy 3's touchRead() function. I am planning to share it after the cursor can be moved around on an arbitrary background without messing up the background.
 
Writing to the GRAM indeed goes through the LUT.

write.jpg

You can try to change the LUT with the Color Set (2dh) command. First 32 bytes are the corresponding 6 bit values of the given 5 bit red component, then following 64 bytes of 6 bit green component and then 32 bytes of 5 bit blue component.

Example (RED):

5 bit value6 bit value6 bit value (alternative)
00000000000000000
00001000010000010
......
11100111001111000
11101111011111010
11110111101111100
11111111111111111

So write command would be:

SPI.WRITE(0x2d) // COMMAND
SPI.WRITE(0x00) // 1st RED
SPI.WRITE(0x01) // 2nd RED
...
SPI.WRITE(0x3b) // 30th RED
SPI.WRITE(0x3d) // 31th RED
SPI.WRITE(0x3f) // 32th RED

SPI.WRITE(0x00) // 1st GREEN
...
SPI.WRITE(0x3e) // 63th GREEN
SPI.WRITE(0x3f) // 64th GREEN

SPI.WRITE(0x00) // 1st BLUE
...
SPI.WRITE(0x3d) // 31th BLUE
SPI.WRITE(0x3f) // 32th BLUE
 
Last edited:
Turns out that drawPixel()/readPixel() also don't work, but they fail differently. The code below should produce a grid, each cell of which is a single color. Again, if we draw a color on the display, the color is changed by reading it back and drawing what was read back.

Maybe both readRect() and readPixel() each has a different bug? Seems a bit unlikely, but could it be no one else has tried these?

Code:
#include "SPI.h"
#include "ILI9341_t3.h"


ILI9341_t3 d = ILI9341_t3(10, 9);


uint16_t buf[20 * 20]; 


void drawBackground() {
  d.fillScreen(ILI9341_BLACK);
  for (int x = 0; x < d.width(); x += 20) {
    for (int y = 0; y < d.height(); y += 20) {
      uint16_t c = d.color565(0, y, x/2);
      d.fillRect(x, y, 19, 19, c);
      d.readRect(x + 4, y, 9, 18, buf);
      d.writeRect(x + 4, y, 9, 18, buf);
      for (int i = 0; i < 3; ++i) {
        for (int j = 0; j < 18; ++j) {
          d.drawPixel(x + i, y + j, c);
          if (j & 1) {
            uint16_t k = d.readPixel(x + i, y + j);
            d.drawPixel(x + i, y + j, k);
          }
        }
      }
    }
  }
}


void setup() {
  d.begin();
  d.setRotation(1);
  drawBackground();
}


void loop() {
}
 
I tried the experiment of making a color using color565() and drawing it using drawFastVLine(), drawPixel(), and writeRect(). I get a reasonable ramp from drawFastVLine() and drawPixel(), but not writeRect(). No readback involved, so there's definitely an error in writeRect(). [I think I was wrong about this.]

Even though there's pronounced banding on the display, the readback values don't show the same banding. Also, my previous observation about the wrong values always being lighter isn't always true and in some cases there is a complete color shift from bright blue to bright green. This suggests to me that writeRect() isn't sending the bits in the correct order. [Wrong here, too.]

defragster, I'm keeping in the back of my mind the possibility of an order dependency I guess based on how the lookup table gets populated.
 
Last edited:
I am sort of confused here as fillRect and drawRect are more or less the same, except to one you only pass in 1 color value and on the other you pass in an array where each pixel is different. I have code that scrolls an area using readrect and writeRect and did not notice color changing, but maybe my eyes were just missing it.
 
New and improved bug example. Bottom third of the display should look like the top two thirds: a simple ramp from black to blue going from left to right. This points the finger squarely at readPixel().

I'm curious if others are seeing the same when they run this code. The output isn't consistent from run to run with vertical blue lines appearing at different places in the erroneously green field across the bottom. The blue lines are not the right shade of blue either.

Code:
#include "SPI.h"
#include "ILI9341_t3.h"


ILI9341_t3 d = ILI9341_t3(10, 9);


uint16_t buf[80];


uint16_t good[512];
uint16_t bad[512];


void setup() {
  Serial.begin(9600);
  while (!Serial);
  d.begin();
  d.setRotation(1);
  d.fillScreen(ILI9341_BLACK);
  for (int x = 0; x < 256; ++x) {
    // Black to blue ramp with drawFastVLine() - ok
    uint16_t c = d.color565(0, 0, x);
    d.drawFastVLine(x, 0, 80, c);
    
    // Same ramp below using writeRect() - ok
    for (int i = 0; i < 80; ++i) buf[i] = c;
    d.writeRect(x, 80, 1, 80, buf);
    
    // Attempting to copy from first row - readPixel() gets trash
    uint16_t r = d.readPixel(x, 0);
    for (int i = 0; i < 80; ++i) buf[i] = r;
    d.writeRect(x, 160, 1, 80, buf);
    
    // Save these to memory in case Serial interferes somehow.
    good[x] = c;
    bad[x] = r;
  }
  for (int x = 0; x < 256; ++x) {
    Serial.print(x, HEX);
    Serial.print(" ");
    Serial.print(good[x], HEX);
    Serial.print(" ");
    // These values should match the good column. They're not even deterministic!
    Serial.println(bad[x], HEX);
  }
}


void loop() {
}
 
I found the reason. It is a timing problem, clock rate is too fast. In my situation SPI clock of 20 MHz is still acceptable, 25 MHz for example causes data corruption on read. But why not on write?

Concerning "18.3.4 Display Serial Interface Timing Characteristics (4-line SPI system)" in reference documentation, even 20 MHz is already a kind of overclocking. Minimal timings for "SCL “H” pulse width (Write)" for example is specified as 40 ns (25 MHz), but for "SCL “H” pulse width (Read)" only 60 ns (16 MHz). The entire clock cycle is specified with 150 ns (read) (6.6 MHz) and 100 ns (write) (10 MHz).

So for a correct write and read operation without data corruption, SPI clock should be not faster than 16 MHz even if 20 MHz seems to be acceptable on most cases. However, in a noisy environment, even 16 MHz could be too much.
 
Last edited:
bkausbk, thank you! Lowering the SPI clock significantly reduced the problem.

I reduced SPICLOCK to 14Mhz in ILI9341_t3.cpp. At a CPU frequency of 48MHz and 96MHz, reading the display returns incorrect values about one out of every 500 reads, a big improvement. At 24MHz, reading the display almost always returns incorrect values. Even when I dropped the SPI frequency to 1.6MHz, there were still occasional glitches.

It does look like there is a timing issue here, however since dropping the SPI frequency didn't fix it entirely and since the problem only occurs exclusively when reading, I suspect the protocol the Teensy is following from ILI9341_t3 doesn't quite match what the driver chip expects.
 
Reading pixel is correct implemented concerning ILI9341 interface specification at least commands sent to the display are correct and commands for reading inclusive 18 bits to 16 bits conversion seems to be correct.

Image and comments removed, see update below.

This is how my test environment looks like. Could you test (oscilloscope) how the SPI clock, MISO and MOSI signals look like?

IMG_20150227_092005.jpg

In case of a noisy environment or long cables, you could take into consideration to use following Schmitt-Trigger buffers:

http://www.ti.com/product/sn74lvc3g17

Update: After some additional tests I found glitches with readRect(). First, there is indeed a bug in readRect(), the last pixel is lost on read. May be @Paul, could you take a look on it?

Second, there were data corruptions down to 6 MHz. For this purpose I created a test sketch to find out the highest possible SPI clock. ILI9341_t3 have to be modified. Replace define SPICLOCK with unsigned int SPICLOCK = F_BUS / 2; and add extern unsigned int SPICLOCK; in ILI9341_t3.h. After that, you are able to dynamically change the SPI clock. However as I said before, guaranteed maximum SPI clock in read situation is 6.6 MHz concerning ILI9341 interface specification.

My readRect() bug workaround is not comparing last pixel in buffer in method HasErrors().

Code:
#include "SPI.h"
#include "ILI9341_t3.h"

#define TFT_RESET 8
#define TFT_DC  9
#define TFT_CS 10

ILI9341_t3 d = ILI9341_t3(TFT_CS, TFT_DC, TFT_RESET);

#define FILL_COLOR 0xAAAA
#define FILL_BUFFER_SIZE 320 * 4

bool HasErrors(const uint16_t* Buffer) {
  for (unsigned int i = 0; i < FILL_BUFFER_SIZE - 1; i++) {
    if (Buffer[i] != FILL_COLOR) {
      return true;
    }
  }
  return false;
}
  
uint16_t Buffer[FILL_BUFFER_SIZE] = {0};
  
bool Draw() {
  bool Result = true; 
  
  int y = d.height() / 2;
  int w = d.width();
  int h = 4;
  
  d.fillRect(0, y - 2, w, h, FILL_COLOR);
  
  d.readRect(0, y - 2, w, h, Buffer);

  if (HasErrors(Buffer)) {
    Result = false;
  }

  d.writeRect(0, y - 2, w, h, Buffer);
  
  return Result;
}

void setup() {
  d.begin();
  d.setTextColor(ILI9341_WHITE, ILI9341_WHITE);
  d.setRotation(1);
  d.fillScreen(FILL_COLOR);
  SPICLOCK = F_BUS / 2;
}

const uint16_t ctar_div_table[23] = {
  2, 3, 4, 5, 6, 8, 10, 12, 16, 20, 24, 32, 40,
  56, 64, 96, 128, 192, 256, 384, 512, 640, 768
};

void loop() {
  d.setCursor(10, 10);
  d.print("Testing with SPI ");
  d.print(SPICLOCK);
  d.print(" Hz ");
  d.print("FBUS ");
  d.print(F_BUS);
  d.print(" Hz    ");

  if (Draw() == false) {
    d.setCursor(10, 30);
    d.print("SPI clock is too high, reducing clock...");

    for (unsigned int i = 0; i < 23; i++) {
      if (F_BUS / ctar_div_table[i] < SPICLOCK) {
        SPICLOCK = F_BUS / ctar_div_table[i];
        d.fillRect(10, 10, d.width(), 10, FILL_COLOR);
        break;
      }
    }

  } else {
    d.setCursor(10, 30);
    d.fillRect(10, 30, d.width(), 10, FILL_COLOR);
    d.print("SPI clock is perfect!                        ");
  }
}

Update 2: Big surprise! Setting Teensy 3.1 clock rate back to default 72 MHz don't produces any glitches even with the highest SPI clock of F_BUS / 2 (18 MHz). The bug in readRect() seems to be also not present. My guess, only with default cpu clock, a clean SPI clock can be generated. I'll take a further look on it with an oscilloscope.

Is the maximum SPI clock really as low as half the bus clock (18 MHz) (see SPI.h line 377 in SPISettings class)? If so only following SPI clock rates are available at default of Teensy 72 MHz clock rate.

18.00 MHz, 12.00 MHz, 9.00 MHz, 7.20 MHz, 6.00 MHz, 4.50 MHz, 3.60 MHz, 3.00 MHz, 2.25 MHz, 1.80 MHz, 1.50 MHz, 1.125 MHz, 900 KHz, 642.857 KHz, 562.5 KHz, 375 KHz, 281.250 KHz, 187.5 KHz, 140.625 KHz, 93.750 KHz, 70.312 KHz, 56.250 KHz, 46.875 KHz
 

Attachments

  • IMG_20150227_094549.jpg
    IMG_20150227_094549.jpg
    133.3 KB · Views: 174
Last edited:
Update: After some additional tests I found glitches with readRect(). First, there is indeed a bug in readRect(), the last pixel is lost on read. May be @Paul, could you take a look on it?
Actually probably my bug...

Probably not all bytes back yet. You might try updating the read rect function with:
Code:
void ILI9341_t3::readRect(int16_t x, int16_t y, int16_t w, int16_t h, uint16_t *pcolors) 
{
	uint8_t dummy,r,g,b;
    uint16_t c = w * h;
    uint8_t fFirst = 1;
	SPI.beginTransaction(SPISettings(SPICLOCK, MSBFIRST, SPI_MODE0));

	setAddr(x, y, x+w-1, y+h-1);
	writecommand_cont(ILI9341_RAMRD); // read from RAM
	waitTransmitComplete();
	SPI0.PUSHR = 0 | (pcs_data << 16) | SPI_PUSHR_CTAS(0)| SPI_PUSHR_CONT | SPI_PUSHR_EOQ;
    while ((SPI0.SR & SPI_SR_EOQF) == 0) ;
    SPI0.SR = SPI_SR_EOQF;  // make sure it is clear
    while ((SPI0.SR & 0xf0)) { 
        dummy = SPI0.POPR;	// Read a DUMMY byte but only once
    }
    c *= 3; // number of bytes we will transmit to the display
    while (c--) {
        if (c)
            SPI0.PUSHR = 0 | (pcs_data << 16) | SPI_PUSHR_CTAS(0)| SPI_PUSHR_CONT;
        else    
            SPI0.PUSHR = 0 | (pcs_data << 16) | SPI_PUSHR_CTAS(0)| SPI_PUSHR_EOQ;

        // If last byte wait until all have come in...
        if (c == 0) {
            while ((SPI0.SR & SPI_SR_EOQF) == 0) ;
            SPI0.SR = SPI_SR_EOQF;  // make sure it is clear
        
            // wait until we have all characters back
            while ((SPI0.SR & 0xf0) < 0x30) ;   
        }

        if ((SPI0.SR & 0xf0) >= 0x30) { // do we have at least 3 bytes in queue if so extract...
            r = SPI0.POPR;		// Read a RED byte of GRAM
            g = SPI0.POPR;		// Read a GREEN byte of GRAM
            b = SPI0.POPR;		// Read a BLUE byte of GRAM
           *pcolors++ = color565(r,g,b);
        }
        
        // like waitFiroNotFull but does not pop our return queue
		while ((SPI0.SR & (15 << 12)) > (3 << 12)) ;
    }

	SPI.endTransaction();
}
The change was on count = 0, that it waits until three characters have been returned... Let me know if this helps, and I will update my fork...
 
Hi KurtE thanks for looking at that. However after further investigation I found out that the problem does not exists when changing Teensy 3.1 clock rate back to default 72 MHz. The problem was 100% reproducable with 96 MHz clock rate. But of course I will test your changes as soon as I can.
 
KurtE, I just tried your change. Sorry to say, it doesn't fix it. Even if I drop the Teensy 3 clock to 48Mhz and the SPI clock to 8MHz, there are still display glitches. readPixel() is also glitchy.

At 72MHz on the T3 and the default SPICLOCK, there's a tiny bit of glitching. A few pixels here and there. I haven't tried it on a T3.1.
 
Last edited:
You might experiment with these two functions. In Particular try changing:
SPI.beginTransaction(SPISettings(SPICLOCK, MSBFIRST, SPI_MODE0));
To:
SPI.beginTransaction(SPISettings(6500000, MSBFIRST, SPI_MODE0));

See if that helps in the read. If so will change the two functions to maybe have SPICLOCK_READ
Which is defined at beginning of file where one can change it...
 
@pictographer: Please try default 72 MHz and SPI clock of 6-18 MHz. readPixel() seems to be ok if the clock is not too fast/noisy.
 
Looks like locally overriding the SPICLOCK fixes readRect() when the T3.0 clock is at 24MHz. Still major glitching at 96MHz, 72MHz and 48MHz.
 
@bkausbbk, readPixel() is behaving mostly correctly with T3 @ 72MHz and SPI at 6MHz, but readRect() is still a mess.
 
@KurtE: unfortunatelly there is still this one lost pixel. I added a counter variable and indeed one pixel is not read. But as I said, this only happens when Teensy 3.1 clock is set to 96 MHz.

Code:
        if ((SPI0.SR & 0xf0) >= 0x30) { // do we have at least 3 bytes in queue if so extract...
            r = SPI0.POPR;		// Read a RED byte of GRAM
            g = SPI0.POPR;		// Read a GREEN byte of GRAM
            b = SPI0.POPR;		// Read a BLUE byte of GRAM
            _readRectPixels++;
           *pcolors++ = color565(r,g,b);
        }
 
I made a version of readRect that uses two counters. One for bytes to write and the 2nd for bytes to return. Not sure if helps anything.

Anyway, I put it up in a new branch of my github fork: https://github.com/KurtE/ILI9341_t3/tree/ReadRect-Test

(New fork as to do it the way that Arduino and Intel (MRAA) do their pull requests, with each change in their own branch).

You might try the file and see if you think it helps.

At times I wonder if should put some form of timeouts in these functions, that if for example a byte was eaten, the system will wait forever....
 
Back
Top