Sending instructions to an LCD display controlled by HD44780

My current embedded project uses a 16x2 LCD controlled by the HD44780 standard controller. My PIC18 speaks to the LCD via the Adafruit LCD serial backpack (schematic link). I chose the SPI interface.

The HD44780 is controlled through various instruction writes to 8 data pins (DB0-7), a read/write pin (R/W), a register select pin (RS), and an enable pin (E). Link to the instruction set.

The instructions are composed of bits that indicate certain settings... configuration parameters for lack of a better term.

Everything is working as expected, but I am uncertain about my design. In order to best organize my code for readability and flexibility I have tried to follow the following approach:

• Assign each HD44780 configuration parameter via #define directive in the header file

• Build an array of char type for each instruction and load it with the appropriate configuration parameters

• Execute instructions by passing the instruction array via pointer and using shift operations to construct the SPI output in the order expected by serial backpack

This is all well and good, however, I have then nagging feeling that storing the instructions as arrays is not the best approach. I would appreciate any advice on how you would approach this issue with a bent towards clarity and efficiency.
My header and .c files are below, main.c is not included but calls LCD_send via the instruction aliases.

HEADER FILE

```
/*
* File: LCD_SPI_16x2.h
* Author: rbs
* Comments:
* Revision history:
*/

/*Hardware configuration:
*
* Adafruit LCD serial backpack bit configuration:
* [DB4 | DB5 | DB6 | DB7 | E | RS | RW | LITE]
*
* HD44780 instruction set bit configuration (4-bit mode):
* [RS | RW | DB7 | DB6 | DB5 | DB4]
*
* HD44780 instruction set bit configuration (8-bit mode):
* [RS | RW | DB7 | DB6 | DB5 | DB4 | DB3 | DB2 | DB1 | DB0]
*
* PIC18 MSSP1 SPI peripheral:
* LSB first, clock = FOSC/4, SCK idle = low, TX on SCK low -> high, SDI not used
*/

// Thi

I see a number of things that may help you improve your code.

Don't put definitions in header files

There are declarations, such void LCD_spi_out(char data); and there are definitions which would be the actual body of the function. The header file should only have declarations. The code has proper separation for functions, but not for data. To fix that move lines like these

char Clear_display[10]      = {0,0,0,0,0,0,0,0,0,1};
char Home[10]               = {0,0,0,0,0,0,0,0,1,0};

into the .c file and then declare them like this in the .h file:

char Clear_display[10];
char Home[10];

Be careful with signed vs. unsigned

Your functions take char * and char as arguments, but it's actually implementation dependent whether a char is signed or unsigned. I'd recommend being specific and using uint8_t types (from `) instead.

Use more efficient storage

Using 10 bytes for the storage of 10 bits is not very efficient. Instead, I'd recommend storing things as bits. In particular, I'd suggest that each of the 8-bit commands should be stored as a uint8_t. For example:

uint8_t Home = 0x01u;

The RS and RW bits are intrinsic to the operation and don't need to be stored at all. That is, every command will have the RS bit equal to 0 (specifying "command" rather than "data") and RW will always be 0 (specifying "write" rather than "read").

Minimize work

Note that the low four bits of Upper and Lower are identical, so it would make sense to calculate them only once. Further, it would probably make sense to define them as bitmasks so they could easily be added in via bitwise-or operations.

Use const where practical

As @TobySpeight correctly pointed out in a comment, things that don't change should be declared const. In this code, that would be both the data arrays and the argument to the LCD_send` function since that function doesn't and shouldn't alter the passed array.

Consider assembly language

In embedded systems, both memory and CPU cycles are often both in short supply, so it's much more common for embedded system programmers to use assembly language for some parts of the code. The drawback is that the code is inherently less portable and probably harder to maintain. Bit manipulation is one thing that is often done more directly in assembly languages than in C, but we can still make improvements just in C. One way to do it might look like this:

void LCD_send_cmd(uint8_t cmd) {
    uint8_t hi = _BL;
    uint8_t lo = _BL;
    lo |= (cmd & 0x01) ? 0x80 : 0;
    lo |= (cmd & 0x02) ? 0x40 : 0;
    lo |= (cmd & 0x04) ? 0x20 : 0;
    lo |= (cmd & 0x08) ? 0x10 : 0;
    hi |= (cmd & 0x10) ? 0x80 : 0;
    hi |= (cmd & 0x20) ? 0x40 : 0;
    hi |= (cmd & 0x40) ? 0x20 : 0;
    hi |= (cmd & 0x80) ? 0x10 : 0;
    LCD_spi_out(hi);
    LCD_spi_out(lo);
}

Alternatively, it might be worth factoring out the bit manipulations so that you can send both commands and data:

void LCD_send(uint8_t cmd, uint8_t flags) {
    uint8_t hi = flags;
    uint8_t lo = flags;
    lo |= (cmd & 0x01) ? 0x80 : 0;
    lo |= (cmd & 0x02) ? 0x40 : 0;
    lo |= (cmd & 0x04) ? 0x20 : 0;
    lo |= (cmd & 0x08) ? 0x10 : 0;
    hi |= (cmd & 0x10) ? 0x80 : 0;
    hi |= (cmd & 0x20) ? 0x40 : 0;
    hi |= (cmd & 0x40) ? 0x20 : 0;
    hi |= (cmd & 0x80) ? 0x10 : 0;
    LCD_spi_out(hi);
    LCD_spi_out(lo);
}

void LCD_send_cmd(uint8_t cmd) {
    LCD_send(cmd, _BL);
}

void LCD_send_data(uint8_t cmd) {
    LCD_send(cmd, RS | _BL);
}

StackExchange Code Review Q#160332, answer score: 3