HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Writing an interface for networking in an embedded system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
networkingembeddedsystemwritinginterfacefor

Problem

I'm writing a driver in C for a WiFi module attached to a PIC18 microcontroller. I want to implement some functions that I'm familiar with in computer application level programming like Window's API recv function for WinSock.
The module itself already implements the WiFi communication protocol and the TCP/IP stack, and communicate with the PIC18 microcontroller through UART.

Is this the correct way to achieve this? Efficiency is very important here (It's an embedded system too).

#define RECEIVE_TIMEOUT 200
#define RECEIVE_RETRY_COUNT 4

UINT16 ReceiveData(void* _data, UINT16 length)
{
    UINT16 received = 0;
    UINT8 retry_count = 0;

    while(received < length)
    {
        if(UART1_Data_Ready())
        {
            ((BYTE*)_data)[received] = ReceiveByte();
            received++;
            continue;
        }

        if(retry_count == RECEIVE_RETRY_COUNT)
            break;

        retry_count++;
        Delay_ms(RECEIVE_TIMEOUT/RECEIVE_RETRY_COUNT);
    }

    return received;
}

Solution

Just a small note on sleeping when waiting for input. As others have said
your loop is inefficient in that it uses a simple busy loop. This has at
least two potential problems:

-
it prevent other activities from executing;

-
it uses power.

But these are not necessarily problems for your application. And if in
your application there really is nothing else to be done until bytes arrive
and if power consumption is not a problem, then a busy loop is by far the
simplest solution.

Using interrupts will inevitably complicate the rest of your code and should
be avoided unless really necessary (for example if there are other
computing tasks that need to run while waiting for input).

If 1 above is not important but 2 is (ie if you need to minimise power use)
you could put the core to sleep during waiting and have the byte-receive wake
it. This would retain the benefit of avoiding interrupt handling. You would
need to set a timer to wake the core too if you need a timeout.

On the details of your code, I dislike the use of Windows types in an embedded
system. What is wrong with int and char or their unsigned varieties?

Your leading underscore on _data is reserved by something or other and is anyway
pointless.

I don't object to passing in a void * (after all, C stdlib functions recv
and read do this), but I would assign it to a local variable of appropriate
type and avoid casts. This makes absolutely no difference when the buffer is
used just once, as here and is just a personal preference.

And using continue is usually frowned on and often unnecessary.

int ReceiveData(void *data, int length)
{
    unsigned char *p = data;
    int received = 0;
    int retry = 0;

    while (received < length)
    {
        if (UART1_Data_Ready())
        {
            p[received++] = ReceiveByte();
        }
        else if (++retry <= RECEIVE_RETRY_COUNT)
        {
            Delay_ms(RECEIVE_TIMEOUT/RECEIVE_RETRY_COUNT);
        }
        else break;
    }
    return received;
}

Code Snippets

int ReceiveData(void *data, int length)
{
    unsigned char *p = data;
    int received = 0;
    int retry = 0;

    while (received < length)
    {
        if (UART1_Data_Ready())
        {
            p[received++] = ReceiveByte();
        }
        else if (++retry <= RECEIVE_RETRY_COUNT)
        {
            Delay_ms(RECEIVE_TIMEOUT/RECEIVE_RETRY_COUNT);
        }
        else break;
    }
    return received;
}

Context

StackExchange Code Review Q#48325, answer score: 5

Revisions (0)

No revisions yet.