Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
    
Could you give further details about ReceiveByte()? –  black Apr 27 at 13:39
    
@no1 The ReceiveByte() part is just a simple UART read. UART1_Read(). I don't know the specific of this function because it's from a library. –  UnTraDe Apr 27 at 13:49
1  
How is Delay_ms implemented? nop loop, or asynchronous? –  rolfl Apr 27 at 14:11
    
@rolfl Delay_ms() Is implemented as loop. –  UnTraDe Apr 27 at 14:13

4 Answers 4

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;
}
share|improve this answer

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.

share|improve this answer
    
This is exactly the problem I though could happen in this code, but I don't know what is the best solution to fix that. Any ideas? As for the other things you mentioned, for the defines there is a comment above them which explains what are they doing and what physical quantities they are using, but if you say it's not enough i'll take your advice :). As for the parameter type, I have a question: Where should I really use void* as parameter? (Except function pointer and parameterless function) –  UnTraDe Apr 27 at 14:16
    
Look into using the UART's "data ready" interrupt for detecting incoming data, and potentially using a Timer/Counter for the delay function as well. The explanatory comments on the #defines are great, but in the code, the definition can be far away from the point of use, so I think the extra information in the macro name is helpful. IMO, void * is only useful where you truly don't know or care about the type of something, e.g. malloc(), memset(), etc., or if you're defining a standard API that takes an unspecified user-defined type that they take responsibility for. –  Niall C. Apr 27 at 14:44

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;
}
share|improve this answer

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.