lexus2k / lcdgfx

Driver for LCD displays running on Arduino/Avr/ESP32/Linux (including Rasperry) platforms
MIT License
392 stars 56 forks source link

display.drawBitmap16 should use uint16_t #53

Closed AndreasExner closed 3 years ago

AndreasExner commented 3 years ago

Hi Aleksei

Thank your your work with this great library.

This is not a bug by definition but IMHO not the correct behavior.

Most of the 565 converters are using uint16 for output. The display.drawBitmap16 function expects uint8. This requires a conversation of the convertes output. It's not a big deal but somewhat annoing if you are working with a lot of icons and bitmaps.

Can you have a look on this topic?

Thank you,

Best regards,

Andy

lexus2k commented 3 years ago

Hi Andreas,

Can you provide simple code example describing the problem? Yes, drawBitmap16 accepts uint8_t * as input argument, but it expects 16-bit data. uint8_t * is just pointer type, maybe it would be better to use void * instead to make understanding more clean. So, the conversion is not required at all.

Best regards

AndreasExner commented 3 years ago

Hi Aleksei

test

The image2cpp tool, for example, generates uint16 hex codes for the attached sample (10x10 black square with red verticle line).

const uint8_t test[100] PROGMEM = { 0x0000, 0x0000, 0x0000, 0x1000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x2000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x1800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x1000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000 };

The code above cuts MSB and stores the LSB bits, only. The result is a broken image.

const uint16_t test[100] PROGMEM = { 0x0000, 0x0000, 0x0000, 0x1000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x2000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x1800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x1000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000 };

The next example causes a compiler error: no matching function for call to 'DisplaySSD1351_128x128x16_SPI::drawBitmap16(int, int, int, int, const uint16_t [100])'

const uint8_t test[100] PROGMEM = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x18, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF8, 0x00, 0xF8, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };

Splitting the 16 bit into 8bit works fine, but requires a conversation of the most (all?) 565 converter outputs.

const void test[100] PROGMEM = { 0x0000, 0x0000, 0x0000, 0x1000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x2000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x1800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0800, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x1000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0xf800, 0xf800, 0x0800, 0x0000, 0x0000, 0x0000 };

Unfortunatly, the datatype void seems not working with PROGMEM:

C:\Users\xxx\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.7.4/tools/sdk/libc/xtensa-lx106-elf/include/sys/pgmspace.h:25:130: error: declaration of 'logo' as array of void

define PROGMEM attribute((section( "\".irom.text." FILE "." STRINGIZE(LINE) "." STRINGIZE(COUNTER) "\"")))

sketch\test.h:2:30: note: in expansion of macro 'PROGMEM'

extern const void test[100] PROGMEM;

D:\\Dokumente\Arduino_F5\TubeAmpDisplay\TubeAmpDisplay.ino: In function 'void DisplayMainLeft()':

TubeAmpDisplay:161:41: error: 'test' was not declared in this scope

display.drawBitmap16(16, 10, 101, 19, test);

Thank you,

Best regards,

lexus2k commented 3 years ago
const uint16_t test[100] PROGMEM = [........];
...
display.drawBitmap16(16, 10, 101, 19, static_cast<const uint8_t *>(test));

That's just C++.

AndreasExner commented 3 years ago

unfortunately, static_cast don't work for this case:

const uint16_t logo [101*19] PROGMEM = { display.drawBitmap16(16, 10, 101, 19, static_cast<const uint8_t *>(logo)); invalid static_cast from type 'const uint16_t [] {aka const short unsigned int []}' to type 'const uint8_t* {aka const unsigned char*}'

I've tried different formats for the array declaration, but it always ends in the same error.

reinterpret_cast works without errors, but it drops informations and the image shows wrong colors

I've tried a lot of things with pointers and conversations, but my sketch throws (too) many exeptions during runtime. Currently I'm not sure if this is a plattform issue. I'll switch from a NodeMCU to Arduino nano board later this weekend.

lexus2k commented 3 years ago

@AndreasExner OK, reinterpret_cast is good.

The problem is not in the data type itself. Here are useful links to help to understand whats going on in your case: https://en.wikipedia.org/wiki/Endianness https://en.wikipedia.org/wiki/Subpixel_rendering https://www.displayfuture.com/Display/datasheet/controller/ST7735.pdf (sections 9.7.15, describing RGB format used by controllers, section 10.1.26 MADCTL (36h): Memory Data Access Control)

The result is affected by memory representation (MSB,LSB or LSB,MSB), controller mode (4-4-4, 5-6-5, 5-5-5, 6-6-6, 2-3-2, RGB, BGR)

The library uses little endian byte order, display is initialized in RGB color filter panel mode, using 5-6-5 format.

At least, you didn't provide any code for debugging, any description of your display.

AndreasExner commented 3 years ago

The byte order is exactly the problem with reinterpret_cast. I've uplodes a sample code which shows both, a byte by byte conversation and reinterpret_cast: https://github.com/AndreasExner/TubeAmpDisplay/blob/main/uint16-uint8.ino

Byte by byte works fine with very small bitmaps. But with larger ones it runs in an out of memory exception (of course). The byte re-order pice of the reintCast() function causes out of memory exception with large bitmaps as well.

I think I'll port the byte by byte code to the PC to build a converter for larger bitmaps. It looks like the easiest way for me (and the tiny devices) to avoid any conversation during runtime.

I'm using the WaveShare 1.5 inch OLED display: https://www.waveshare.com/wiki/1.5inch_RGB_OLED_Module and the NodeMCU / ES8266 board.

lexus2k commented 3 years ago

If you've read SSD1351 datasheet, you know that these displays when using 3-wire SPI require the following input byte order: 1st byte is RRRRRGGG, 2nd byte is GGGBBBBBB. Actually bits to SPI are sent in the following sequence RRRRRGGGGGGBBBBB. The image in, provided by you, has platform dependent memory layout because of uint16_t type. Depending on LE/BE system you will get different results. I would advise you to fix the source images, used in the project.

The only thing I can help you with, here, is to add setRgbMode() function to switch display hardware (please, see latest commit). But definitely using uint16_t to store data in memory is the wrong way, which has a lot of hidden compatibility and cross-compilation issues.

    display.begin();
    display.getInterface().setRgbMode( 0 );
    display.fill( 0x0000 );
    display.drawBitmap16(16, 10, 101, 19, (const uint8_t *)in);
AndreasExner commented 3 years ago

I agree with you. 16 Bit is not the way to go. I wrote a simple power shell script for the conversation. It works for my and I think it can be easily adopted for different plattforms. https://github.com/AndreasExner/TubeAmpDisplay/blob/main/BitmapConverter.ps1

Thank you for your fast support. I much appriciate! :)

lexus2k commented 3 years ago

You're welcome. Let me know if setRgbMode() helps.

AndreasExner commented 3 years ago

Unfortunately not.

DisplayTest:64:26: error: 'class InterfaceSSD1351<PlatformSpi>' has no member named 'setRgbMode' display.getInterface().setRgbMode( 0 ); ^ exit status 1 'class InterfaceSSD1351<PlatformSpi>' has no member named 'setRgbMode' You can find the code here: https://github.com/AndreasExner/TubeAmpDisplay/tree/main/displayTest.

I've extended my PS script to convert multiple bitmaps in one file. This works for me very well. https://github.com/AndreasExner/TubeAmpDisplay/tree/main/Tools

lexus2k commented 3 years ago

@AndreasExner

Did you download latest code from master branch? Because I can compile your example.

AndreasExner commented 3 years ago

Initially I've installed the library from within Arduino IDE. Now I've downloaded the master branch and overwritten the local copy of the library. But the error still remains.

For me it's ok since I'm understood that a uint16 format is not the best choice to gain the best performance from the code.

lexus2k commented 3 years ago

Arduino IDE stores all libraries in ZIP-packed format. So, to replace the library correctly, you need to find all installed lcdgfx library instances and removed them.

AndreasExner commented 3 years ago

Now I've removed everything before installing the ZIP from git. But the error still remains. IMHO, as long as I'm the only one with this issue you can close the case.

AndreasExner commented 3 years ago

Hi. From my point we can close the case. The conversation works perfect for me.