Atomic access of a variable in PIC18

[sangar]

Hello all,


I developed the code to receive a character through UART in interrupt mode and use that character in main.c. I shared the Received character between ISR and main something like below. It's working fine. But I want to know the written code is safe or not.

Here is the code

variables:

Code: [Select]

volatile uint8_t Receive_byte;   //should it be volatile?
volatile bool Usart_Receive=false;
uint8_t Test_command;
UART ISR:

Code: [Select]

void interrupt Usart_ISR(void)
{

 if (PIR1bits.RCIF) //poll receive interrupt flag from USART
 {
        Receive_byte=RCREG;
        Usart_Receive=true;
 }
 if (RCSTAbits.OERR == 1)
 {
        RCSTAbits.CREN = 0; //handle overrun error bit by clearing it
        RCSTAbits.CREN = 1;
 }

}
main.c

Code: [Select]

void main(void)
{
PIR1bits.RCIF = 0; //USART interrupt
PIE1bits.RCIE = 1; //USART interrupt
IPR1bits.RCIP = 0; //USART interrupt
RCONbits.IPEN = 1; //enable interrupt priority
INTCONbits.GIEH = 1; //enable all global interrupts
INTCONbits.GIEL = 1; //enable all global interrupts
INTCONbits.PEIE = 1; //enable peripheral
while(1)
{
      if(Usart_Receive)
      {
        di();
        Test_command=Receive_byte;   //Atomic access  //assigning volatile to
        ei();
        switch(Test_command)
        {
            case TEST_ONE:
            {
                Test_command=test_task1();  //A
            }
            break;
            case TEST_TWO:
            {
                Test_command=test_task2();             //B
            }
            break;
            case TEST_THREE:
            {
               Test_command=test_task3();             //C
            }
            break;
             case TEST_FOUR:
            {
               Test_command=test_task4();             //D
            }
            break;
           
            default:
            Test_command=0;
            break;
        }
        Test_command=0;
        Usart_Receive=false;


}

}

Can I use the  Receive_byte without volatile because I access it atomically?
 

[newbrain]

Disclaimer: I don't do PICs in any of their incarnations.

Still this can be addressed from quite general C principles.
Until C11 standard, whether a variable is accessed atomically is defined by the implementation and the HW platform.
In this case, though, being it a single byte, I'm hard pressed to see how it could not be an atomic access, unless PICs are very strange.
This means the the di() ei() couple should not be necessary.

With atomicity out of the way, let's consider the volatile qualifier: its function is to inform the compiler that the variable can be modified in ways it does not know, from C99:

An  object  that  has  volatile-qualified  type  may  be  modified  in  ways  unknown  to  the implementation  or  have  other  unknown  side  effects.

If volatile is removed, the compiler could decide that (since the value is never changed in the main function as far as it can see) it does not need to re-read from storage every time, and omit the copy to Test_Command altogether.

So, di()/ei() is not really needed, while volatile is.

[Mechatrommer]

volatile is necessary otherwise the compiler will make 2 copies of Receive_byte and Usart_Receive. both are independent of each other each one for interrupt and one for main. otoh di() ei() is a good practice for mutex, but if you can ensure the generated machine/assembly code for "Test_command=Receive_byte;" is atomic you can safely remove them, but its a nasty hack (not good programming practice), you should stick the di() ei() mutex imho. but... you have another problem at your whole "if(Usart_Receive)" condition, that rely on another volatile parm "Usart_Receive"... this condition assumes interrupt will not get called in there, or this condition will run faster than the next usart transmission... otherwise if interrupt get called before the "Usart_Receive=false;" line at the end of the condition, you will miss one received byte, you should put ei() after the "Usart_Receive=false;", not after the "Test_command=Receive_byte;" but i'm not sure whats gonna happened to received byte in case of di(), ymmv... btw how do you managed to compile that code? its lack one closing brace "}" for the condition.

[sangar]

Hi ,
There are A,B,C and D command from PC to do Test1 ....Test 4. I f I received 'A', I will do Test-1.Once Test-1 get finished and sent back its result to PC, I make "Usart_Receive" to false to wait for next test command .As per project, PC will not send any command until current test get finished.Thats why I am using Usart_Receive condition.Sorry, I missed end brace while uploading code.



Thanks,
Sangar

[Mechatrommer]

PC will not send any command until current test get finished.

this is sequential communication then no need di() ei() entirely the code will work fine.

[NorthGuy]

So, you want to ensure atomic access here:

Code: [Select]

di();
Test_command=Receive_byte;   //Atomic access  //assigning volatile to
ei();
So that your code wouldn't be interrupted by ISR. But why? What happens if the interrupt happened before you call "di()"? Right. It will overwrite Received_byte, so that the previous content of Received_byte is lost. Does "di()" (or atimicity in general) helps against that? Of course not.

Another problem is if the interrupt happens after "ei()". It'll set "Usart_receive", but your main code will clear it just few lines later, so the new character will be lost. Probably not what you want.

Atomicity of the operation is irrelevant to the well-being of your code. What is relevant, is to make sure that your "if (Usart_Receive)" routine is called quickly enough - before the next character arrives (causing next interrupt and destruction of the pevious character stored in Recceived_byte). Does your interrupt helps to solve this task? Not at all. You could remove your interrupt, your "Usart_Receive" and "Received_byte" variables and re-write this:

Code: [Select]

if(Usart_Receive)  {
  di();
  Test_command=Receive_byte;   //Atomic access  //assigning volatile to
  ei();
}
as this:

Code: [Select]

if (PIR1bits.RCIF) {
  Test_command=RCREG;

This would produce the same effect as your code except that there wouldn't be any overhead of the ISR, so it would actually work better.

I guess you wanted to accomplish something different with the interrupt, did you?