C Message Encapsulation (SPI/nRF24 Driver): Any good practices?-CodePudding

I'm currently developing a radio (nRF24) module driver on top of a homemade SPI driver. The driver is pretty simple: to send a message via the radio module, you need to send the message via SPI, prefixed with a command:

I got it working perfectly, but I'm struggling with the encapsulation of the user payload (i.e adding the NRF24_CMD before the user payload).

As of today, I'm allocating a new char array for the SPI payload each time I'm sending a message. I'm writing the radio module command in the first byte of the array, and then copying the user payload as demonstrated here:

void nrf24_send(char * payload, size_t length){
    char spi_payload[MAX_DATA];
    spi_payload[0] = RF_SEND_CMD;
    for(int i=0; i<length; i  ){
        spi_payload[i 1] = payload[i];
    }
    spi_write(spi_payload, length   1);
}

This generates a lot of pointless memory accesses just to copy the user payload and I feel that there is room for improvement.

What would be an efficient way to encapsulate the user message ?

Is there a good practice in the driver development community for these kinds of problems ?

FYI:

I'm programming a microcontroller (MSP430FR5969) bare metal, so I don't have any dynamic allocation mechanisms implemented yet
The MCU I'm using is equipped with a DMA

Here are a few ideas I had:

using memcpy instead of the for loop -> Better, the copy of the array would be faster, but there's still the need to copy the payload
tell the user to allocate the buffer and left the first byte empty -> No copy needed, but the user needs to know about the hardware, and I don't want that
create a c struct to hide the complexity (cf below) -> I'm afraid that alignments issues may arise, and since my NRF24_CMD byte needs to be directly followed by the payload, I don't know if I can trust my compiler...

Hiding the complexity with a struct:

struct nrf24_message_t {
   char cmd;
   char payload;
}

Thanks for your help!

Hugo

CodePudding user response：

As the command byte and the payload array's underlying data type are all of type char you don't need to care for alignment issues – most important, there won't be any gaps in between if you define one after the other.

I'd now allow the user to allocate the memory appropriately and provide some kind of conversion function, e.g.:

// header:
// TODO: include guards, etc
struct nrf24_message;

struct nrf24_message* nrf24_createFromRaw(size_t size, char* raw);

// to allow the user to write to
size_t nrf24_bufferSize(struct nrf24_message* message);
char* nrf24_buffer(struct nrf24_message* message);

void nrf24_send(struct nrf24_message* message, size_t length);
// still a length parameter:
// maybe a message gets shorter than maximum the buffer size!

This is the visible user interface while the implementation sees the details of this struct:

//source:
struct nrf24_message
{
    size_t payloadSize;
    char cmd;
    char payload[]; // flexible array member...
};

struct nrf24_message* nrf24_createFromRaw(size_t size, char* raw)
{
    if(size <= sizeof(size_t)   1) // < if you want to allow empty message
    {
        // failure! buffer is too short
        return NULL;
    }
    struct message* m = (struct message*)raw;
    m->payloadSize = size - sizeof(size_t) - 1;
    return m;
}

size_t nrf24_bufferSize(struct message* nrf24_message)
{
    return message->payloadSize;
}
char* nrf24_buffer(struct nrf24_message* message)
{
    return message->payload;
}
void nrf24_send(struct nrf24_message* message, size_t length)
{
   spi_write(&message->cmd, length   1);
}

Et voilà: Now the user can provide memory while you still have space in front of the data.

Usage example:

char raw[256];
struct* nrf24_message = nrf24_createFromRaw(sizeof(raw), raw);
size_t length = snprintf(nrf24_buffer(message), nrf24_bufferSize(message), "...");
nrf24_send(message, length);

CodePudding user response：

Working at a really low level is definitely challenging, and this is a bit of mostly-C that I've used in the past that avoids this kind of copying by giving the caller a pointer directly into your buffer

struct nrf24_message_t {
   char buffer[MAX_SIZE];
};

static struct nrf24_message_t transmitBuffer; // assuming you have just one

// give the caller a pointer where it can stuff bytes, letting it
// know how much space is available. 
char *prepareForTransmit(int *nbytes)
{
    *nbytes = sizeof(transmitbuffer.buffer) - 1; // room for CMD

    transmitbuffer.buffer[0] = RF_SEND_CMD; // or whatever
    return transmitbuffer.buffer   1; // room for CMD:
}

void transmit(int nbytes)
{
    // do the SPI thing. be sure to  1 for the cmd byte.
}

Then in the caller it can request the buffer - noting the max size - and then transmit. I don't know the formatting of your packets but you should be able to get the idea.

{
    ....
    int bufsize;
    char *tbuf = prepareForTransmit(&bufsize);

    // put stuff in tbuf up to the # of bytes available. Keep 
    // track of how many bytes you actually used!

    transmit(nbytes);
}

EDIT: if you have multiple possible access, that makes it more interesting. Best is if you can use a small memory allocator so the caller owns the buffer, though it does have to protect the actual transmit function so they don't walk on top of each other.

The above is just a framework; it's not a complete example that even compiles.