Go Down

Topic: A fast PCD8544 library (Nokia 5110) (Read 28353 times) previous topic - next topic

bumsbert

Thanx alot for your reply! I am looking forward to this.

TheCoolest

@ bumsbert:
I've uploaded the new version, it's in the OP (Post #1). Please read the header file to see how to use different pins on your Arduino.

@ robtillaart:
Thanks again for you valuable feedback, I've taken into consideration most of your suggestions and implemented the optimizations you suggested to do.
Everything seems to works great.


1) No, it is the comparing with zero that is faster than comparing with nonzero const;  and % is just expensive.
I've made a comparison, and using an 'if' instead of % is indeed the reason for the huge difference in speed.

2) You're welcome,
There is little room to optimize (you can check this by commenting out the lowest level functions).
The line() is now a call to rectangle, there might be some gain making it dedicated.
I decided to leave line() as is for now, when I implement the ability to make diagonal lines, I may revisit the idea of adding dedicated code for straight lines.


The code looks quite good, good layered design, clear function and variable names and very little comments.

some remarks:
- Point of attention is that there is a begin() an init() and a clear(), sounds like one to many
  ==> merge begin() and init() into one. Some of the constants in init() could be parameters for begin();  // #define them.
I changed it, now you can use the 'simple' begin() which acts just as the begin in the previous version, but it also lets you select whether the display will be inverted or not.
The second begin() enables you to define invertion and custom Vop, Temperature coefficient and Bias values.


- swap could be inlined
Done, forgot about that.

- remove all the testing of x and y if (x >= LCD_X || y >= LCD_Y) return; or change signature and return FAIL/SUCCESS.
  now the user just don't know if a call did something when it returns.
Made this change as well, will return 1 (PCD8544_SUCCESS) when the function succeeds and 0 (PCD8544_ERROR) if it fails.
I have not applied the change to setPixel() because it slowed it down noticeably.


- from write()    if (data < 0x20 || data > 0x7F) return 0;   you could also map non printable data on space, might save some layout. (design choice)
Do you mean allow for user defined characters? I've left this out for now, I may implement it in a future version.

- clear()   this->m_Position = 0; is not needed as it is set in gotoXY()
Oops. Fixed.

- BufLen is a #define ==> BUFLEN should be used, more consistent style
Changed.

- rectangle code could use some explaining.
Done.

just my 2 cents ,


robtillaart

OK, motivated choices, I like that !

Can you post the new timings you get? (as my measurements might be biased)
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

TheCoolest

Sure, these are the results:

The time it took draw a rect and 3 lines: 1960
The time it took to print 84 chars is:    2316
The time it took to draw a 25x3 (25x18) bitmap is: 1560
The time it took to run setPixel on all 4032 pixels and render it:    18252

robtillaart

#19
Sep 14, 2013, 02:03 pm Last Edit: Sep 14, 2013, 02:06 pm by robtillaart Reason: 1
Thanks, even faster than my timings :)

assuming commands and data never exceeds 255 chars one could use 8 bit count  iso 16 bit. (or can it?)
Quote

void PCD8544_SPI_FB::writeLcd(uint8_t dataOrCommand, const uint8_t *data, uint8_t count)
{
    PORTB = (PORTB & ~0x05) | dataOrCommand;

    for (uint8_t i = count; i >0; i--)
        SPI.transfer(data[count-i]);

    PORTB |= 0x4;
}
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

TheCoolest

What do you think of the way I decided to manage pin mappings? Any interesting ideas how to improve on that and make it easier to use for less advanced users?

robtillaart

clear is inefficient as it makes 504 calls and set the PORT ditto times x 2,
try this:
Code: [Select]

void PCD8544_SPI::clear()
{
        PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_DATA;
for (uint16_t i = BUF_LEN; i >0; i--) SPI.transfer(0x00);
        PCD8544_PORT |= PIN_CE;
this->gotoXY(0, 0);
}

slightly more code but I expect quite some performance gain, please verify.

not dived into the pinmappings yet.
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart

#22
Sep 14, 2013, 02:27 pm Last Edit: Sep 14, 2013, 02:31 pm by robtillaart Reason: 1
similar for the bitmap...

Code: [Select]

uint8_t PCD8544_SPI::writeBitmap(const uint8_t *bitmap, uint8_t x, uint8_t y, uint8_t width, uint8_t height)
{
if (this->gotoXY(x, y) == PCD8544_ERROR) return PCD8544_ERROR;

const uint8_t *maxY = bitmap + height * width;

for (const uint8_t *y = bitmap; y < maxY;)
{
               PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_DATA;
for (uint8_t x = 0; x < width; x++, y++) SPI.transfer(*y);
               PCD8544_PORT |= PIN_CE;

this->gotoXY(this->m_Column, this->m_Line + 1);
}

this->advanceXY(width);
}


and this

Code: [Select]

uint8_t PCD8544_SPI::gotoXY(uint8_t x, uint8_t y)
{
if (x >= PCD8544_X_PIXELS || y >= PCD8544_ROWS) return PCD8544_ERROR;

this->m_Column = x;
this->m_Line = y;

       PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_COMMAND;
        SPI.transfer(80 | this->m_Column);
        SPI.transfer(40 | this->m_Line);
       PCD8544_PORT |= PIN_CE;

return PCD8544_SUCCESS;
}
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart

framebuffer variant has a hidden bug
Code: [Select]
size_t PCD8544_SPI_FB::write(uint8_t data)
{
// Non-ASCII characters are not supported.
if (data < 0x20 || data > 0x7F) return 0;

memcpy_P(this->m_Buffer + this->m_Position, ASCII[data - 0x20], 5);
this->m_Buffer[this->m_Position+5] = 0x00;   
this->m_Position += 6;
if (this->m_Position >= BUF_LEN) this->m_Position -= BUF_LEN;
//this->m_Position %= BUF_LEN;
return 1;
}

before the memcpy you need to check that  m_Buffer + this->m_Position  ... m_Buffer + this->m_Position +5  are valid positions ....
otherwise you copy beyond the buffer ...
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart

move gotoXY() as after any clear the cursor is expected at 0,0 ?
Code: [Select]

void PCD8544_SPI_FB::clear(bool render)
{
memset(this->m_Buffer, 0x00, sizeof(this->m_Buffer));
if (render)
{
this->renderAll();
}
this->gotoXY(0, 0);
}
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart

pinmapping
Code: [Select]

#define PIN_DC 0x01 // D8
#define PIN_RESET 0x02 // D9
#define PIN_CE 0x04 // D10
#define PINS_CE_DC (PIN_DC | PIN_CE)

// The DC pin tells the LCD if we are sending a command or data
#define PCD8544_COMMAND 0
#define PCD8544_DATA PIN_DC  <<<<<<<<<<<<<<<<<<<<<< this makes no sense, should be 1  as the semantics is different.
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

TheCoolest


similar for the bitmap...

Code: [Select]

uint8_t PCD8544_SPI::writeBitmap(const uint8_t *bitmap, uint8_t x, uint8_t y, uint8_t width, uint8_t height)
{
if (this->gotoXY(x, y) == PCD8544_ERROR) return PCD8544_ERROR;

const uint8_t *maxY = bitmap + height * width;

for (const uint8_t *y = bitmap; y < maxY;)
{
                PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_DATA;
for (uint8_t x = 0; x < width; x++, y++) SPI.transfer(*y);
                PCD8544_PORT |= PIN_CE;

this->gotoXY(this->m_Column, this->m_Line + 1);
}

this->advanceXY(width);
}


and this

Code: [Select]

uint8_t PCD8544_SPI::gotoXY(uint8_t x, uint8_t y)
{
if (x >= PCD8544_X_PIXELS || y >= PCD8544_ROWS) return PCD8544_ERROR;

this->m_Column = x;
this->m_Line = y;

        PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_COMMAND;
        SPI.transfer(80 | this->m_Column);
        SPI.transfer(40 | this->m_Line);
        PCD8544_PORT |= PIN_CE;

return PCD8544_SUCCESS;
}


clear is inefficient as it makes 504 calls and set the PORT ditto times x 2,
try this:
Code: [Select]

void PCD8544_SPI::clear()
{
        PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_DATA;
for (uint16_t i = BUF_LEN; i >0; i--) SPI.transfer(0x00);
        PCD8544_PORT |= PIN_CE;
this->gotoXY(0, 0);
}

slightly more code but I expect quite some performance gain, please verify.

not dived into the pinmappings yet.


That is true, it can make a considerable difference in speed. I actually just noticed that the writeBitmap() function doesn't need to do direct SPI writes, I changed it to this and I'm getting even better performance.
Code: [Select]
uint8_t PCD8544_SPI::writeBitmap(const uint8_t *bitmap, uint8_t x, uint8_t y, uint8_t width, uint8_t height)
{
if (this->gotoXY(x, y) == PCD8544_ERROR) return PCD8544_ERROR;
const uint8_t *maxY = bitmap + height * width;

for (const uint8_t *y = bitmap; y < maxY; y += width)
{
this->writeLcd(PCD8544_DATA, y , width);
this->gotoXY(this->m_Column, this->m_Line + 1);
}

this->advanceXY(width);
}


But strangely once I add the SPI code to either gotoXY() or clear() or both, the character write and bitmap drawing times increase dramatically from ~2600 to ~2900 and from ~270 to ~320 respectively. I can't explain it.


framebuffer variant has a hidden bug
Code: [Select]
size_t PCD8544_SPI_FB::write(uint8_t data)
{
// Non-ASCII characters are not supported.
if (data < 0x20 || data > 0x7F) return 0;

memcpy_P(this->m_Buffer + this->m_Position, ASCII[data - 0x20], 5);
this->m_Buffer[this->m_Position+5] = 0x00;   
this->m_Position += 6;
if (this->m_Position >= BUF_LEN) this->m_Position -= BUF_LEN;
//this->m_Position %= BUF_LEN;
return 1;
}

before the memcpy you need to check that  m_Buffer + this->m_Position  ... m_Buffer + this->m_Position +5  are valid positions ....
otherwise you copy beyond the buffer ...



Nice catch, fixed.


pinmapping
Code: [Select]

#define PIN_DC 0x01 // D8
#define PIN_RESET 0x02 // D9
#define PIN_CE 0x04 // D10
#define PINS_CE_DC (PIN_DC | PIN_CE)

// The DC pin tells the LCD if we are sending a command or data
#define PCD8544_COMMAND 0
#define PCD8544_DATA PIN_DC  <<<<<<<<<<<<<<<<<<<<<< this makes no sense, should be 1  as the semantics is different.



True, but then I'd add have to add conditions to every function which writes to the LCD. The semantics may be different, but it makes the rest of the code easier to understand.

robtillaart

1) your writeBitmap is even better! (WDITOT ;)  - do you have the numbers?



2) don't understand either why the clear() & gotoxy() is slower ....?
maybe caused as it uses the object private vars iso x, y

can you try this?
Code: [Select]
uint8_t PCD8544_SPI::gotoXY(uint8_t x, uint8_t y)
{
if (x >= PCD8544_X_PIXELS || y >= PCD8544_ROWS) return PCD8544_ERROR;

        PCD8544_PORT = (PCD8544_PORT & ~PINS_CE_DC) | PCD8544_COMMAND;
        SPI.transfer(80 | x);
        SPI.transfer(40 | y );
        PCD8544_PORT |= PIN_CE;

this->m_Column = x;
this->m_Line = y;

return PCD8544_SUCCESS;
}





Quote

Code: [Select]
// The DC pin tells the LCD if we are sending a command or data
#define PCD8544_COMMAND 0
#define PCD8544_DATA PIN_DC  <<<<<<<<<<<<<<<<<<<<<< this makes no sense, should be 1  as the semantics is different.

True, but then I'd add have to add conditions to every function which writes to the LCD. The semantics may be different, but it makes the rest of the code easier to understand.


Ah, now I understand the trick, if you change the pins the DATA line follows. Should explain this in the code e.g. something like:

Code: [Select]
// The DC pin tells the LCD if we are sending a command or data
#define PCD8544_COMMAND 0
#define PCD8544_DATA PIN_DC    // Follows the DataCommand pin definition !!




Q: any feedback on my comment on void PCD8544_SPI_FB::clear(bool render)
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

TheCoolest

1) Using the 'old' method I was getting around 324us for the bitmap in the sample sketch, using the new method it is just a bit over 270us.

2) I don't know either, the sketch isn't even taking 'clear()' into consideration when I print the string or bitmap... Just weirdness.

Q) Yes, forgot to mention that you were right, and I fixed it.

robtillaart

Quote
1) Using the 'old' method I was getting around 324us for the bitmap in the sample sketch, using the new method it is just a bit over 270us.

50 us off;   that is ~15% faster


how do these numbers relate to these original ones?
Quote
The time it took draw a rect and 3 lines: 1960
The time it took to print 84 chars is:    2316
The time it took to draw a 25x3 (25x18) bitmap is: 1560
The time it took to run setPixel on all 4032 pixels and render it:    18252

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Go Up