Moddable-OpenSource / moddable

Tools for developers to create truly open IoT products using standard JavaScript on low cost microcontrollers.
http://www.moddable.com
1.29k stars 235 forks source link

Incorrect rendering output with Poco using the ILI9341 Driver #1228

Closed kallistisoft closed 9 months ago

kallistisoft commented 9 months ago

Build environment: Linux - (Arch) 6.5.5-arch1-1 SMP PREEMPT_DYNAMIC x86_64 Moddable SDK version: 4.2.0 + ESP-IDF v5.1.1

Target device: ESP32-S3-WROOM-2 Devkit-C1 + HiLetgo ILI9341 2.8" SPI TFT

Description There appears to be an error in the Poco draw list compiler that is resulting in erroneous output when running the following program:

main.js:

debugger;
import Timer from "timer";

import Poco from "commodetto/Poco";
let render = new Poco(screen);
let backgroundColor = render.makeColor(0, 0, 0);
let squareColor = render.makeColor(49, 101, 173);

render.begin();
    render.fillRectangle(backgroundColor, 0, 0, render.width, render.height);
render.end();

render.begin();
    render.fillRectangle(squareColor, 0, 0, 1, 1);
    render.fillRectangle(squareColor, 32, 0, 2, 2);
    render.fillRectangle(squareColor, 64, 0, 3, 3);
    render.fillRectangle(squareColor, 96, 0, 4, 4);
    render.fillRectangle(squareColor, 128, 0, 5, 5);
    render.fillRectangle(squareColor, 160, 0, 6, 6);
    render.fillRectangle(squareColor, 192, 0, 7, 7);
    render.fillRectangle(squareColor, 224, 0, 8, 8);
render.end();

manifest.json:

{
    "build": {
        "ESP32_SUBCLASS": "esp32s3"
    },
    "include": [
        "$(MODDABLE)/examples/manifest_base.json",
        "$(MODDABLE)/examples/manifest_commodetto.json",
        "$(MODULES)/drivers/ili9341/manifest.json"
    ],
    "config": {
        "screen": "ili9341",
        "touch": ""
    },
    "defines": {
        "spi": {
            "mosi_pin": 11,         
            "miso_pin": 12,
            "sck_pin": 13
        },      
        "ili9341": {
            "hz": 2500000,
            "width": 240,
            "height": 320,
            "rst_pin": 2,
            "cs_pin": 10,
            "dc_pin": 14,
            "spi_port": 1
        }
    },
    "modules": {
        "*": [
            "./main"
        ]
    }
}

The application should draw 8 evenly spaced squares along the top of the screen, each square one pixel larger than the last 1x1 2x2 ... 8x8. It instead draws vertical columns the width of the request square, with the first column only being drawn every other line.

I have verified that the display and wiring harness are functional by testing the same setup with the TFT_eSPI library compiling with PlatformIO + Arduino.

Additionally, I wrote a simple draw_pixel_at(...) function that issues SPI commands directly to the display via the exposed xs_ILI9341_command function:

function draw_pixel_at( x, y, color ) {
  let startX0 = x >> 8;
  let startX1 = x & 0xFF;
  let endX0 = (x+1) >> 8;
  let endX1 = (x+1) & 0xFF;

  let startY0 = y >> 8;
  let startY1 = y & 0xFF;
  let endY0 = (y+1) >> 8;
  let endY1 = (y+1) & 0xFF;

  screen.command( 0x2A, Uint8Array.of(startX0, startX1, endX0, endX1).buffer );
  screen.command( 0x2B, Uint8Array.of(startY0, startY1, endY0, endY1).buffer );
  screen.command( 0x2C, Uint8Array.of(color >> 8, color & 0xFF).buffer );
}

This function worked properly -- so this isn't a hardware issue with the display or a base level communication issue with the moddable ILI9341 driver, but an issue with how Commodetto/Poco is generating the draw list from the render.fillRectangle(...) command.

This is confirmed by the logic analyzer traces image included below...

Steps to Reproduce

  1. Build and install the app using this build command: mcconfig -d -m -p esp32/esp32s3 -f rgb565le

Expected behavior Display 8 squares...

Images Screen Output: Image of output Logic Analyzer: This image shows the raw SPI output to the display, as you can see Poco is sending out the same raster line over again, with the first pixel of the line only being written to every other line!? Image of SPI Data

Other information Note that this bug also occurs on Moddable SDK 4.1 (idf_v5) The SPI speed is not causing the issue

Somewhere in the docs for Poco/Comoddetto I read that the render has to update a minimum of two scan lines -- this mechanism appears to be at the heart of this issue as the first two scan line are correct then appear to be repeated for the rest of the screen.

Hopefully someone with more knowledge of the internals of Poco can suggest a simple fix... I'm trying to convince my development team that we should use Moddable for our next product and am in the process of porting our display driver to the Moddable SDK along with a patch for QSPI support for the esp32 -- testing the internals using a known supported target display is the first step in this process :)

Thank You!

phoddie commented 9 months ago

Thank you for the very thorough report.

The problem is that the code is erasing the background in one begin/end block and then drawing the rectangles in another. If you merge those together, it works as expected.

import Poco from "commodetto/Poco";
let render = new Poco(screen);
let backgroundColor = render.makeColor(0, 0, 0);
let squareColor = render.makeColor(49, 101, 173);

render.begin();
    render.fillRectangle(backgroundColor, 0, 0, render.width, render.height);
    render.fillRectangle(squareColor, 0, 0, 1, 1);
    render.fillRectangle(squareColor, 32, 0, 2, 2);
    render.fillRectangle(squareColor, 64, 0, 3, 3);
    render.fillRectangle(squareColor, 96, 0, 4, 4);
    render.fillRectangle(squareColor, 128, 0, 5, 5);
    render.fillRectangle(squareColor, 160, 0, 6, 6);
    render.fillRectangle(squareColor, 192, 0, 7, 7);
    render.fillRectangle(squareColor, 224, 0, 8, 8);
render.end();

Here's the image rendered correctly in the simulator.

image

When using Poco, it is always necessary to draw to every pixel. That is because Poco does not keep a frame buffer on the microcontroller, so it does not have the results of the previous drawing operation to draw on top of. This approach allows for flicker-free rendering with minimal RAM use (as few as two scan lines, rather than 320!).

FWIW - your clever draw_pixel_at function can be done using Poco without having to go to the SPI driver. The following examples uses the clipping of render.begin() to limit the update area to a single pixel.

function draw_pixel_at( x, y, color ) {
    render.begin(x, y, 1, 1);
    render.fillRectangle(color, 0, 0, render.width, render.height);
    render.end();
}
kallistisoft commented 9 months ago

Thank you for your very prompt response!!!

Your statement about the draw_pixel_at(...) function made everything click for me. I'm usually used to buffered display surfaces and that was giving me a conceptual blind spot :)

The suggested code change works perfectly, but leaves me with a few more questions...

As you stated Poco redraws the entire screen every 'pass', I was under the impression that Poco used region invalidation to generate display fragments for update but that looks like that was wishful thinking on my part?? I was able to reduce the transmitted data by restraining the background fill to the image height + 2 pixels...

Which leads me to two architecture questions...

  1. When it comes to optimizing spi bus usage is it entirely up to the application to calculate the invalid area? Or is just expected to always redraw the entire screen?

  2. If I wanted to change the way the Poco renderer worked so that it used virtual frame buffers and fragmented updates would it be better to just start from scratch or could that be shoehorned into the existing code base? I know this both a deep+broad question just looking for an off-the-cuff first thought on the subject; if possible.

Thank you again for your response, it was extremely helpful!!

PS: very happy that the "problem" was totally a mistake on my end -- that's always the best outcome :)

phoddie commented 9 months ago

As you stated Poco redraws the entire screen every 'pass'...

This isn't quite correct. The begin method defines the area of the screen to update. Calling begin() with no arguments updates the entire screen; passing x, y, width, height limits the area of the screen that is updated, as shown above in draw_pixel_at.

..I was under the impression that Poco used region invalidation to generate display fragments for update but that looks like that was wishful thinking on my part?? I

Poco is just a renderer that executes drawing commands. It doesn't have a way to determine what has changed (there are no objects to track, just drawing commands). So, if you are using Poco directly, you are responsible for managing that. Our Piu user interface framework is object-based and uses Poco for rendering. Piu tracks invalid regions to optimize redraws. A good example of that is $MODDABLE/examples/piu/balls. It typically touches between 4500 and 5000 pixels to update a 240x320 frame, so it draws only about 6.5% of the pixels on each frame -- substantially reducing the SPI bandwidth used.

I wanted to change the way the Poco renderer worked so that it used virtual frame buffers and fragmented updates would it be better to just start from scratch or could that be shoehorned into the existing code base? I know this both a deep+broad question just looking for an off-the-cuff first thought on the subject; if possible.

By "virtual frame buffer" you mean a keeping a full frame buffer in memory on the ESP32? If so, that is possible. Poco supports a "frame buffer mode" which does just that. We use that on Moddable Four for the ls013b4dn0 display driver. It requires defining kPocoFrameBuffer in the build to enable frame buffer mode in Poco, and then updating the ili9341 driver to allocate the frame buffer. The details are, as you note, "deep+broad" so this is just a sketch, not a roadmap for implementation.

kallistisoft commented 9 months ago

Thank you for clarifying my confusion with how Poco and Piu work in concert!

By "virtual frame buffer" you mean a keeping a full frame buffer in memory on the ESP32?

We currently are allocating a double buffer that is larger than display to facilitate hardware "accelerated" scrolling in response to touch events. This may be a moot point depending on how Piu handles scrolling content - but it's good to have it in the back of my mind when designing our ported implementation...

Thank you very much for taking the time to respond to my second question -- you manged to answer both the obvious and all the implied questions :) The info on the "frame buffer mode" is going to provides me with an excellent place to start porting our current display driver (QSPI+DMA)!

Hopefully I won't be bugging you with any more questions until I'm ready to issue pull requests :)

Cheers!

phoddie commented 9 months ago

Glad you have the information you need for now. Your project sounds intruiging. I look forward to seeing what you do.