• AVR Freaks

Hot!Volatile keyword with UART circular buffer and atomic safe code

Author
pic_guy
Starting Member
  • Total Posts : 39
  • Reward points : 0
  • Joined: 2007/01/25 13:15:09
  • Location: 0
  • Status: offline
2019/05/21 19:36:12 (permalink)
0

Volatile keyword with UART circular buffer and atomic safe code

Drawing from an example:
http://picforum.ric323.co...pic.php?f=76&t=204
volatile char comin_wptr; //write pointer. Is changed inside an interrupt
char comin_rptr; //read pointer
char comin_buf[16]; //input data buffer
#define BUFFER_MASK 0x0f //mask to force pointers to wrap after 16
 
void com_init(void)
{
    comin_wptr=0;
    comin_rptr=0;
    //code here to set baud rate, enable serial port, and enable receive interrupts
}
 
//call from USART RX interrupt (or embed inside the ISR)
void comin_put (char dat)
{
    if (((comin_rptr - comin_wptr) & BUFFER_MASK) != 1) //test if the write pointer is about to hit the read ppinter
    { //here if there is room in the buffer
        comin_buf[comin_wptr++]=dat; //add character to buffer, and bump write pointer
        comin_wptr &= BUFFER_MASK; //force pointer to wrap
    } else
    { //buffer is full
        //add code here to drop hardware handshaking, or set a flag saying "buffer overflow"
    }
}
 
//fetch the next character from the buffer. Return zero if buffer empty
char comin_get(void)
{
    char temp; //temp scratch
    if (comin_wptr == comin_rptr)
        return 0; //if we are called when there is nothing ready
    temp = comin_buf[comin_rptr++]; //fetch character from buffer, and bump read pointer
    comin_rptr &= BUFFER_MASK; //force pointer to wrap at 16
    return temp; //return the value we read from the buffer
}
 
//return 0 (false) if buffer empty, or 1 (true) if there is some data in the buffer
bit comin_ready(void)
{
    if ((comin_wptr == comin_rptr)
        return 0; //buffer is empty
    return 1; //buffer contains data
}

  1. I have a similar setup with unsigned integer "pointers" instead of char pointers (not sure if that's an important difference) and I was wondering if the comin_buf array also needs to be declared volatile since it would be modified in the ISR?
  2. Does anything read or modified in the ISR need to be declared as volatile, or just the global variables that are modified in the ISR?  I've seen a lot of language on the forums that say "accessed" and to me that means read and write, but want to clarify.
  3. I'm also worried about the idea that the interrupt could disturb the "pointers" because mine are multi-byte (unsigned int) and in this example they are chars (comin_wptr and comin_rptr).
  4. Do I have to worry about something else that I haven't asked?
I'm not sure if I'm making a mountain out of a mole hill or if I know just enough to be dangerous.
 
Thanks.
#1

7 Replies Related Threads

    qhb
    Superb Member
    • Total Posts : 9998
    • Reward points : 0
    • Joined: 2016/06/05 14:55:32
    • Location: One step ahead...
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/21 19:42:17 (permalink)
    +1 (1)
    pic_guy
    I have a similar setup with unsigned integer "pointers" instead of char pointers (not sure if that's an important difference) and I was wondering if the comin_buf array also needs to be declared volatile since it would be modified in the ISR?

    Certainly not in a PIC. There's no way the optimiser is going to cache the contents of an array somewhere.

    Does anything read or modified in the ISR need to be declared as volatile, or just the global variables that are modified in the ISR?  I've seen a lot of language on the forums that say "accessed" and to me that means read and write, but want to clarify.

    Only global variables that you write in the ISR, and read outside the ISR.
    Increment and decrement of a variable counts as a "read".

    I'm also worried about the idea that the interrupt could disturb the "pointers" because mine are multi-byte (unsigned int) and in this example they are chars (comin_wptr and comin_rptr).

    Yes, you can't guarantee atomic access of multi-byte variables without disabling interrupts around each access to them.

    Do I have to worry about something else that I haven't asked?

    Probably, but I don't know what... ;)
     

    Nearly there...
    #2
    mlp
    boots too small
    • Total Posts : 749
    • Reward points : 0
    • Joined: 2012/09/10 15:12:07
    • Location: previously Microchip XC8 team
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/21 20:30:15 (permalink)
    0
    qhb
    pic_guy
    I have a similar setup with unsigned integer "pointers" instead of char pointers (not sure if that's an important difference) and I was wondering if the comin_buf array also needs to be declared volatile since it would be modified in the ISR?

    Certainly not in a PIC. There's no way the optimiser is going to cache the contents of an array somewhere.

    It's not just caching - the optimizer is free to (for example) assume within the main code that the non-volatile-qualified array's contents never change from their initial value(s) if the array is only written to within the ISR.
     

    Does anything read or modified in the ISR need to be declared as volatile, or just the global variables that are modified in the ISR?  I've seen a lot of language on the forums that say "accessed" and to me that means read and write, but want to clarify.

    Only global variables that you write in the ISR, and read outside the ISR.

    or, that you read in the ISR and write outside the ISR.
     
     

    Mark (this opinion available for hire)
    #3
    LdB_ECM
    Junior Member
    • Total Posts : 56
    • Reward points : 0
    • Joined: 2019/04/16 22:01:25
    • Location: 0
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/21 20:55:13 (permalink)
    0
    You will need to disable interrupts for these lines or put a lock on it
    temp = comin_buf[comin_rptr++]; //fetch character from buffer, and bump read pointer
    comin_rptr &= BUFFER_MASK; //force pointer to wrap at 16

     
    Do you see the problem?
    If not lets setup the situation, so right between those two lines the  USART RX interrupt fires and calls comin_put. What value does the code in comin_put see on comin_rptr at that exact moment because the mask has not yet been applied?
     
    The interrupt can't be allowed to fire between those two lines because comin_rptr is unstable. You might get lucky with an optimizer and it do all those together in a register but on a micro with immediate byte access I suspect it will do it as written as two instructions. If you want to go that path I suggest you insert assembler code to be sure.
     
    So those two lines need protection comin_put can not run while those lines are happening.
    post edited by LdB_ECM - 2019/05/21 21:26:50
    #4
    qhb
    Superb Member
    • Total Posts : 9998
    • Reward points : 0
    • Joined: 2016/06/05 14:55:32
    • Location: One step ahead...
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/21 21:52:39 (permalink)
    +1 (1)
    That's one reason I like using 256 byte buffers, there's no need for a separate AND operation. :)
     

    Nearly there...
    #5
    pic_guy
    Starting Member
    • Total Posts : 39
    • Reward points : 0
    • Joined: 2007/01/25 13:15:09
    • Location: 0
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/22 20:08:17 (permalink)
    0
    Thanks for the feedback everyone, lots of info, I've been digging deeper the volatile tagging.  I marked all the accessed variables and it ran awhile without incident in optimized mode (optimizer set to 1), which was not the case before I tagged the variables.  More testing is needed, but that looks promising.
    post edited by pic_guy - 2019/05/22 20:32:57
    #6
    pic_guy
    Starting Member
    • Total Posts : 39
    • Reward points : 0
    • Joined: 2007/01/25 13:15:09
    • Location: 0
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/22 20:35:04 (permalink)
    0
    I also implemented a buffer of size 256 and made the position variable unsigned char so the incrementing operator will naturally wrap it to 0 without the masking as qhb suggested.  It's a clever maneuver qhb, is it always safe to rely on that mechanism?  It seems like it would be atomic and reliable, and hopefully I don't have to shut down the interrupts during my gets from the buffer now.
    #7
    LdB_ECM
    Junior Member
    • Total Posts : 56
    • Reward points : 0
    • Joined: 2019/04/16 22:01:25
    • Location: 0
    • Status: offline
    Re: Volatile keyword with UART circular buffer and atomic safe code 2019/05/22 21:32:09 (permalink)
    0
    It's always safe for 256 bytes because processors only have two choices
     
    1.) Single opcode that does a direct memory increment (Irq can't catch it)
    2.) Lift the value to a register, increment it and store it back. The irq can catch it but
    it reads the last value at the memory location which is still valid.
     
    I will also give you a way you could have fixed your code so it always go route 2, if you need to do this in future ... using volatile to a different end
     
    How this works is postfix is a char on the local stack and I have told it that it is volatile.
    So the first line must execute as is and be written to that position on the stack.
    Then after I have used the current value I simply transfer the postfix value.
    I have given the compiler and optimizer no wiggle room for what I want it to do.
    char comin_get(void)
    {
        volatile char postfix = (comin_rptr + 1) & BUFFER_MASK;
        char temp; //temp scratch
        if (comin_wptr == comin_rptr)
            return 0; //if we are called when there is nothing ready
        temp = comin_buf[comin_rptr]; //fetch character from buffer, and bump read pointer
        comin_rptr = postfix; // store the postfix value
        return temp; //return the value we read from the buffer
    }

    #8
    Jump to:
    © 2019 APG vNext Commercial Version 4.5