search

limpkin / mooltipass

Github repository dedicated to the mooltipass project
https://www.themooltipass.com
521 stars 113 forks source link

[Mini Firmware] Ghosting Problems #261

Closed NicoHood closed 8 years ago

NicoHood commented 8 years ago

Expected behavior

No Ghosting happens and all chars a shown correct.

Actual behavior

The " character overwrites some other characters sometimes

Edit: This also seem to be the fact for the chars: ^ and ` and - and '

Step by step guide to reproduce the problem

aaaaaa
bbbbbb
cc^^ccc

Note: This might be fixed, as I am using an old image bundle and only updated the firmware. Edit: Updated to the latest imagebundle, still existent.

v0.7 (current master) but self compiled for MP Mini

NicoHood commented 8 years ago

Checking the font output gives me no error for the .img file itself:

000003b0: ac01 bec0 00c0 2828 fe28 fe28 2848 54fe  ......((.(.((HT.

To compare with the .h file output:

const uint8_t test_0x21[] __attribute__((__progmem__)) = {   /* '!' width: 2 */
      0xbe,                          /* [. .....] */
};

const uint8_t test_0x22[] __attribute__((__progmem__)) = {   /* '"' width: 4 */
      0xc0,                          /* [..] */
      0x00,                          /* [  ] */
      0xc0,                          /* [..] */
};

const uint8_t test_0x23[] __attribute__((__progmem__)) = {   /* '#' width: 8 */
      0x28,                          /* [  . .  ] */
      0x28,                          /* [  . .  ] */
      0xfe,                          /* [.......] */
      0x28,                          /* [  . .  ] */
      0xfe,                          /* [.......] */
      0x28,                          /* [  . .  ] */
      0x28,                          /* [  . .  ] */
};

It makes me wonder why "0" or "d" does not overlay other chars, but " and ^ do. They are the same height. Investigating further...

NicoHood commented 8 years ago

I think that I've shorten the bug down to this:

    // Initialize bitstream & draw the character
    bitstream_mini_t bs;
    // reference ^
    miniBistreamInit(&bs, 3, 5, 6232); // ^
    miniOledBitmapDrawRaw(0,0, &bs);

    // shows that ^ overwrites 3 px above
    miniBistreamInit(&bs, 3, 5, 6232);
    miniOledBitmapDrawRaw(6,0, &bs);
    miniBistreamInit(&bs, 3, 5, 6232);
    miniOledBitmapDrawRaw(9,3, &bs);

    // ^ must be min 3 px below another char to not overwrite. gets overwriteen by 0 below.
    miniBistreamInit(&bs, 3, 5, 6232);
    miniOledBitmapDrawRaw(0,6, &bs);

    // Draw zero at the bottom
    miniBistreamInit(&bs, 7, 5, 6019); // 0
    miniOledBitmapDrawRaw(0,6+3+7, &bs);

    // zero above will not overwrite the zero, but 1px of the ^
    miniBistreamInit(&bs, 7, 5, 6019); // 0
    miniOledBitmapDrawRaw(0,6+3, &bs);

    // This tests if the ^ also overwrites data below. but it does not
    miniBistreamInit(&bs, 3, 5, 6232);
    miniOledBitmapDrawRaw(3,6, &bs);

You can clearly see that a "^" (add address 6232) overlaps 3 pixels above its actual space. This seem to also happen for other smaller characters like ". It must be a bug of the miniOledBitmapDrawRaw() function if I am not wrong. It does not overwrite chars below.

If you look at the "0" character you can see that this one also hides the other character, but only with a border of 1px, not 3. But only above, not below.

NicoHood commented 8 years ago

I've analyzes the miniOledBitmapDrawRaw() function (for the ^ character) and it seems that the calculations are theorethically correct. I can only think of two possible errors:

Edit: the input 0,6 seems to work fine. However 0,3 (and possibly others) seem to cause errors. I will add another analyze of 0,3 later.

Beside there are a few improvements for flash usage and readablitiy that can be made, but fixing the issue is more important now. I've noted those ideas in TODO.

0,6 analyze (works correct)

void miniOledBitmapDrawRaw(int8_t x, uint8_t y, bitstream_mini_t* bs) // 0, 6 + (5, 3)
{
    // Computing bitshifts, start/end pages...
    uint8_t end_ypixel = (miniOledBufferYOffset + y + bs->height - 1); // 0 + 6 + 3 - 1 = 8
    uint8_t end_page = end_ypixel >> SSD1305_PAGE_HEIGHT_BIT_SHIFT; // 8 >> 3 = 1 -> end_ypixel / 8
    uint8_t start_page = (miniOledBufferYOffset + y) >> SSD1305_PAGE_HEIGHT_BIT_SHIFT; // 6 >> 3 = 0 -> 6 / 8 = 0
    uint8_t data_rbitshift = 7 - (end_ypixel & 0x07); // 7 - (8 & 7 (=0)) = 7 -> 7 - (8 % 8) // TODO %8
    uint8_t data_lbitshift = (8 - data_rbitshift) & 0x07; // (8-7 (=1)) & 7 = 1 // TODO remove & 0x07
    uint8_t cur_pixels = 0, prev_pixels = 0;
    uint8_t end_x = x + bs->width - 1; // 0 + 5 -1 = 4 // TODO remove -1 and correct for loop
    uint8_t start_x; // 0 below

    // Check if x is < 0
    if (x < 0)
    {
        // Are we actually drawing in screen?
        if ((uint8_t)(-x) > bs->width)
        {
            return;
        }

        // Remove the unused pixels
        uint16_t nb_bytes_to_remove = (uint16_t)(-x) * (((uint16_t)bs->height + 7) >> 3);
        while (nb_bytes_to_remove--)
        {
            miniBistreamGetNextByte(bs);
        }

        start_x = 0;
    }
    else
    {
        start_x = x; // 0
    }

    // glyph data offsets are from the end of the glyph header array
    OLEDDEBUGPRINTF_P(PSTR("Draw raw: xs %d xe %d ps %d pe %d rbits %d lbits %d"), start_x, end_x, start_page, end_page, data_rbitshift, data_lbitshift);

    // Bitmasks
    uint8_t rbitmask[] = {0x00, 0x80, 0xC0, 0xE0, 0xF0, 0xF8, 0xFC, 0xFE};
    //uint8_t lbitmask[] = {0xFF, 0x01, 0x03, 0x07, 0x0F, 0x1F, 0x3F, 0x7F};

    // x = 0; x <=4 && x < 128; x++ -> 5 iterations for each column
    for (uint8_t x = start_x; (x <= end_x) && (x < SSD1305_OLED_WIDTH); x++)
    {
        int16_t buffer_shift = (((uint16_t)end_page % SSD1305_OLED_BUFFER_PAGE_HEIGHT)
                                  << SSD1305_WIDTH_BIT_SHIFT); // (1 % 5) << 7 = 128 or 0x80
        uint8_t pixels_to_be_displayed = bs->height; // 3
        // p = 1; p >= 0; p-- // TODO dont use signed here and count to zero or to end_page instead
        for (int8_t page = end_page; page >= start_page; page--)
        {
            if (page == end_page) // page = 1
            {
                cur_pixels = miniBistreamGetNextByte(bs); // 0x80 for first pixel (0x80, 0x40, 0x20, 0x40, 0x80)
                miniOledFrameBuffer[buffer_shift+x] &= rbitmask[data_rbitshift]; // [7] = 0xFE -> destroy LSB
                miniOledFrameBuffer[buffer_shift+x] |= cur_pixels >> data_rbitshift; // write first bit
                pixels_to_be_displayed -= (8 - data_rbitshift); // 3 - (8 - 7 = 1) = 2
            }
            else if (page == start_page) // page = 0
            {
                if(data_rbitshift == 0) // 7 -> false
                {
                    // Data is aligned with our data storage system :D
                    cur_pixels = miniBistreamGetNextByte(bs);
                    miniOledFrameBuffer[buffer_shift+x] = cur_pixels;
                }
                else // true
                {
                    miniOledFrameBuffer[buffer_shift+x] &= ~rbitmask[pixels_to_be_displayed]; // [2] = 0xC0 -> ~0xC0 = 0x3F -> destroy 2 MSB

                    if (pixels_to_be_displayed > (8 - data_lbitshift)) // 2 > (8 - 1 (=7)) -> false
                    {
                        cur_pixels = miniBistreamGetNextByte(bs);
                        miniOledFrameBuffer[buffer_shift+x] |= cur_pixels >> data_rbitshift;
                    }

                    miniOledFrameBuffer[buffer_shift+x] |= prev_pixels << data_lbitshift; // 0x80 << 1
                }
            }
            else if (page != end_page && page != start_page) // TODO remove if as else is clear
            {
                cur_pixels = miniBistreamGetNextByte(bs);
                if(data_rbitshift == 0)
                {
                    // Data is aligned with our data storage system :D
                    miniOledFrameBuffer[buffer_shift+x] = cur_pixels;
                }
                else
                {
                    miniOledFrameBuffer[buffer_shift+x] = prev_pixels << data_lbitshift;
                    miniOledFrameBuffer[buffer_shift+x] |= cur_pixels >> data_rbitshift;
                }
                pixels_to_be_displayed -= 8;
            }
            prev_pixels = cur_pixels; // 0x80 for first pixel (0x80, 0x40, 0x20, 0x40, 0x80)
            buffer_shift -= ((uint16_t)1 << SSD1305_WIDTH_BIT_SHIFT); // - 0x80 = 0

            // Check if the buffer shift isn't negative because of the buffer y offset
            if (buffer_shift < 0)
            {
                buffer_shift = ((uint16_t)(SSD1305_OLED_BUFFER_PAGE_HEIGHT-1) << SSD1305_WIDTH_BIT_SHIFT);
            }
        }
    }
}
NicoHood commented 8 years ago

Yup the errors is in this line: https://github.com/limpkin/mooltipass/blob/79046e9cc9784df8716ff43f98d0ed01d57edd84/source_code/src/OLEDMINI/oledmini.c#L717

It does not take into account that sometimes you are only writing a single page, so it only keeps the left side of the byte, but trashes the upper(LSB/left) bits. Thatswhy only pixels above get trashed and only in special rows where a single page is used.

I am going to prepare a PR...