Writing an interface for networking in an embedded system

Question

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;
}

Answer 1

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:

1. it prevent other activities from executing;

2. 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;
}

Answer 2

Your code is running synchronously (the Delay_ms is a spin loop). As a result, no other code can be running when you are waiting.

Ideally, you should be using an interrupt handler (PIR1.RCIF) to wait on the message, and when the USART indicates there is data available, you can append it to a message buffer, and signal the main-loop code when the data buffer is complete.

Consider reading up on some code examples ( Link to zip file ), and the USART data sheet (pdf) for details on the interrupt handler.

Using an interrupt would be a much better solution.

On the other hand, your code is not spinning on the USART very efficiently anyway.

You are waiting a full Delay_ms loop when the data may be available much sooner.

Consider spinning the following way:

#define RECEIVE_TIMEOUT 200
#define RECEIVE_RETRY_COUNT 4

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

    while(received < length)
    {
        timeout = TIMER1TICK; // pick a timer that cycles.... in your system
        timeout += TIMEALLOWED; // add the allowed timeout to the current time.
        while(TIMER1TICK != timeout && !UART1_Data_Ready())
        {
            // spin until data comes available on the USART

        }

        if(!UART1_Data_Ready())
            break; // timeout ....

        ((BYTE*)_data)[received] = ReceiveByte();
        received++;

    }

    return received;
}

Answer 3

The only thing I see wrong with the algorithm is that if you don't have any data coming in from the WiFi module, you could potentially sit and wait for 200ms (the value of RECEIVE_TIMEOUT) before returning to the caller to say "no data". Is there other work that your main program could be doing during that time? If so, it might be worthwhile to split the check for incoming data and the reading of that data into separate functions.

---

• When setting up constants that measure physical quantities, I like to attach the units of measurement to the symbolic name. That way, it becomes obvious if you pass a millisecond quantity into a function that expects seconds or microseconds (or kilograms or... you get the idea).

  #define RECEIVE_TIMEOUT_ms 200
  [... code elided ...]
  
    Delay_ms(RECEIVE_TIMEOUT_ms/RECEIVE_RETRY_COUNT);
  

• The first parameter to the function is declared as void *, but its only use is as a BYTE *:

  UINT16 ReceiveData(void* _data, UINT16 length)
  {
      [... code elided ...]
      ((BYTE*)_data)[received] = ReceiveByte();
      [... code elided ...]
  }
  

  This is a strong hint that it should be declared as a BYTE * in the first place.

Answer 4

One more thing missed in other comments. The UART is capable of detecting errors. They do happen, and must be reported upstairs. You must read and analyze RCSTA before reading RCREG