Pages: 1 [2] 3 4 ... 7   Go Down
Author Topic: A fast PCD8544 library (Nokia 5110)  (Read 13742 times)
0 Members and 2 Guests are viewing this topic.
Offline Offline
Newbie
*
Karma: 0
Posts: 15
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Israel
Offline Offline
Sr. Member
****
Karma: 4
Posts: 277
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

@ 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 ,

Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

OK, motivated choices, I like that !

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

Rob Tillaart

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

Israel
Offline Offline
Sr. Member
****
Karma: 4
Posts: 277
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks, even faster than my timings smiley

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;
}
« Last Edit: September 14, 2013, 07:06:54 am by robtillaart » Logged

Rob Tillaart

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

Israel
Offline Offline
Sr. Member
****
Karma: 4
Posts: 277
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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?
Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

clear is inefficient as it makes 504 calls and set the PORT ditto times x 2,
try this:
Code:
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.
Logged

Rob Tillaart

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

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

similar for the bitmap...

Code:
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:
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;
}
« Last Edit: September 14, 2013, 07:31:20 am by robtillaart » Logged

Rob Tillaart

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

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

framebuffer variant has a hidden bug
Code:
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 ...
Logged

Rob Tillaart

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

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

move gotoXY() as after any clear the cursor is expected at 0,0 ?
Code:
void PCD8544_SPI_FB::clear(bool render)
{
memset(this->m_Buffer, 0x00, sizeof(this->m_Buffer));
if (render)
{
this->renderAll();
}
this->gotoXY(0, 0);
}
Logged

Rob Tillaart

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

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

pinmapping
Code:
#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.
Logged

Rob Tillaart

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

Israel
Offline Offline
Sr. Member
****
Karma: 4
Posts: 277
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

similar for the bitmap...

Code:
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:
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:
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:
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:
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:
#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.
Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

1) your writeBitmap is even better! (WDITOT smiley-wink  - 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:
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:
// 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:
// 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)
Logged

Rob Tillaart

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

Israel
Offline Offline
Sr. Member
****
Karma: 4
Posts: 277
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 168
Posts: 12425
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

Rob Tillaart

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

Pages: 1 [2] 3 4 ... 7   Go Up
Jump to: