bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

UART FIFO, ring buffer, interrupts and Atomic

Sun Apr 18, 2021 3:04 pm

I would like to implement a simple ring buffer for the UART RX interrupt example.
So the Interrupt routine would add data to the ring and update the head index (with bounds checks) and the main program would read data from the ring and update the tail index.

Is is safe to increment the head index (say a unint8/16/32_t) in the interrupt and read it the in the main program. i.e. is the increment atomic so you cannot read an intermediate state in the main program? If it is then I don't need to disable the interrupt while retrieving data from the ring buffer.

Thanks and Regards
Robert.
Last edited by bobtrex on Tue Apr 20, 2021 9:38 am, edited 1 time in total.

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART ring buffer, interrupts and Atomic

Sun Apr 18, 2021 3:22 pm

So google searches on M0+ tend to say that aligned access to 16 and 32 bit writes are atomic and unaligned is not allowed on M0+ (no mention of 8 bit writes). So I am guessing that I am safe to increment a variable in the interrupt routine and read it in the main program. Can anyone from Raspberry confirm.

Also if this is that case does this hold true between cores , increment in one core and read in the other ?

Thanks

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART ring buffer, interrupts and Atomic

Sun Apr 18, 2021 3:30 pm

I do something similar and in limited testing haven't noticed a problem. I read from one end and write in the other.

Code: Select all

void my_uart_handler()
{
   uint32_t i, status, dummy, value, nextPos;

   for (i=0; i<2; i++)
   {
      if (g.uartInited[i])
      {
         while (uart_is_readable(UART[i]))
         {
            g.event_flag |= (1<<(EVT_UART_0_RX+i));

            value = uart_getc(UART[i]);

            bufPushValue(EVT_UART_0_RX+i, value);
         }
      }
   }
}
...
int bufPushValue(int id, uint value)
{
   int nextPos;

   nextPos = (pd_buf[id].wPos + 1) % pd_buf[id].size;

   if (nextPos != pd_buf[id].rPos)
   {
      pd_buf[id].buf[pd_buf[id].wPos] = value;

      pd_buf[id].wPos = nextPos;
   }
}
...
uint32_t bufPopValue(int id, uint32_t default_value)
{
   uint32_t value = default_value;
   int nextPos;

   if (pd_buf[id].rPos != pd_buf[id].wPos)
   {
      value = pd_buf[id].buf[pd_buf[id].rPos];

      nextPos = (pd_buf[id].rPos + 1) % pd_buf[id].size;

      pd_buf[id].rPos = nextPos;
   }
   return value;
}

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART ring buffer, interrupts and Atomic

Sun Apr 18, 2021 3:35 pm

I do the same between cores with GPIO interrupts on one core being stored in a ring buffer for later retrieval on the other. I have not noticed any problems.

See https://github.com/joan2937/picod/blob/ ... ON/picod.c

kilograham
Raspberry Pi Engineer & Forum Moderator
Raspberry Pi Engineer & Forum Moderator
Posts: 501
Joined: Fri Apr 12, 2019 11:00 am
Location: austin tx

Re: UART ring buffer, interrupts and Atomic

Sun Apr 18, 2021 7:04 pm

a single write or read is atomic so therefore not an increment. you can just about cheat in some cases (if you are really careful on M0+ because there is minimal pipeline and no cache and do without normally requisite memory barriers at least, but for anything non trivial, you are much better off performing the proper of locking.

If you want to ensure exclusive access to state shared by IRQ and app code, then use a critical_section from pico_sync or a raw spin lock from hardware_sync

That said of course the SDK already has what you want in pico/util/queue.h (a queue which is multicore/IRQ safe)

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART ring buffer, interrupts and Atomic

Mon Apr 19, 2021 10:50 am

Thanks Joan, Kilograham

"a single write or read is atomic so therefore not an increment. you can just about cheat in some cases (if you are really careful on M0+ because there is minimal pipeline and no cache and do without normally requisite memory barriers at least, but for anything non trivial, you are much better off performing the proper of locking." - So for a simple increment on M0+ you can get away with it , but its bad practice ?

If you want to ensure exclusive access to state shared by IRQ and app code, then use a critical_section from pico_sync or a raw spin lock from hardware_sync - do you just encase your code in the critical_section_enter_blocking and critical_section_exit

"That said of course the SDK already has what you want in pico/util/queue.h (a queue which is multicore/IRQ safe)" - I looked at the queues but was wondering if I wanted to be efficient as possible and use the UART FIFOs with functions like uart_read_blocking , rather than getting a byte at a time ? It looks to me that you would need to have an intermediate buffer and then load the queue from that.

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART ring buffer, interrupts and Atomic

Mon Apr 19, 2021 11:17 am

The code I use reads or writes the pointers in a single instruction so is quite safe. It not as if you are trying to set a semaphore which would need a combined read/write/increment.

kilograham
Raspberry Pi Engineer & Forum Moderator
Raspberry Pi Engineer & Forum Moderator
Posts: 501
Joined: Fri Apr 12, 2019 11:00 am
Location: austin tx

Re: UART ring buffer, interrupts and Atomic

Mon Apr 19, 2021 1:33 pm

Yeah, but in general you need memory barriers if you are going to rely on a memory write (even if it is atomic at the bus level) to signal data availability between two threads. That was my point, you can get away with this on Cortex M0+ without memory barriers (or worrying about out of order execution or caches), but you still need to worry about compiler instruction re-ordering

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART ring buffer, interrupts and Atomic

Mon Apr 19, 2021 2:56 pm

If there is a problem with compiler code generation I expect the effect will be obvious fairly quickly.

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART ring buffer, interrupts and Atomic

Tue Apr 20, 2021 9:38 am

Hi, thanks for the info and the code. I now have a simple implementation of a ring buffer working with the UART.
Could I ask about using the FIFO to reduce interrupts?

To keep things simple at first, can I use the FIFO without DMA ? Using uart_read_blocking , uart_write_blocking.
If this is the case i believe the default water mark level of the FIFOs is Receive FIFO becomes >= 1 / 2 full? I cant see a function to set these levels (direct register access, UARTIFLS ?).

To go further and use DMA for UART RX_FIFO to RX ring buffer (mainly) and TX ring buffer to UART TX FIFO.
For writing to the UART FIFO via DMA should I look at the example pico-examples/dma/control_blocks ?
Is there an example of reading from the UART FIFO via DMA

Mang thanks and regards
Robert.

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART FIFO, ring buffer, interrupts and Atomic

Tue Apr 20, 2021 10:12 am

I did use the FIFO but abandoned it because of the problem with the watermark. You end up with data in the FIFO but are not notified as the watermark isn't reached. DMA has similar problems. You are only notified when the transfer is complete, i.e. buffer full.

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART FIFO, ring buffer, interrupts and Atomic

Tue Apr 20, 2021 10:35 am

Thanks Joan

Yes I have been playing with FIFOs enabled this morning and getting strange results!
of the problem with the watermark. Is this a documented bug in the silicon ? If so there should be a note added to the FAQ section.

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART FIFO, ring buffer, interrupts and Atomic

Tue Apr 20, 2021 10:40 am

It's not a bug, just a feature. It would be nice if the feature was changed to make the FIFO more useful. What is needed is an interrupt on FIFO not empty.

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART FIFO, ring buffer, interrupts and Atomic

Tue Apr 20, 2021 11:04 am

What I see on FIFO enabled, with sending 4 bytes (puts) at 115200, TX linked to RX is that the interrupt fires below the water mark (is there a timeout so the interrupt is fired after a period if the watermark is not reached ?). However in the interrupt handler uart_is_readable only ever returns 1, so I cant determine the number of bytes to use with uart_read_blocking. Is this what you mean with problem with the watermark ?

The SDK doc for uart_read_blocking states 0 if no data available, otherwise the number of bytes, at least, that can be read. So is this a bug or am i likely doing something wrong in code (its return type is a bool, which seems strange.)

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART FIFO, ring buffer, interrupts and Atomic

Tue Apr 20, 2021 11:21 am

In my case I would only read the UART when interrupted. But you don't get an interrupt until the watermark is reached. So you don't get interrupted for short messages or the residue of long messages.

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART FIFO, ring buffer, interrupts and Atomic

Wed Apr 21, 2021 5:08 pm

Hi Joan

Yes you are correct, from my code below it appears that with the RX FIFO enabled, no interrupt is called below 4 bytes, does this match your tests).
So I assume the default watermark level set in register is for 4 bytes (do you know how to check the value of a hardware register ?)
Did you find a mechanism to set the watermark level ? (I tried "hw_write_masked", and "UARTIFLS" but probably got it wrong as it does not seem to change!)
I was thinking of using the FIFO but then have a polled check for any remaining bytes (based on time of last Interrupt, Baud and watermark level).

As "uart_read_blocking" always seems to returns 1 when there are one or more bytes in the FIFO (surely this cant be the intent of the function) I would still have to loop through the bytes using "uart_getc", rather than using "uart_read_blocking"

I was also going to ask would you be able to share your code for reading from the UART with DMA ?

Many thanks

Code: Select all

#include <stdio.h>
#include "pico/stdlib.h"
#include "pico/util/queue.h"
#include "pico/multicore.h"
#include <string.h>
#include "hardware/timer.h"
#include "hardware/uart.h"
#include "hardware/irq.h"




critical_section_t acrit;


uint32_t count = 0;

struct data_s
{
    uint8_t data[64];
}queue_entry_t;

queue_t rx_queue;
queue_t tx_queue;
queue_t rx_buf;
queue_t tx_buf;

uint8_t buff[256];

uint8_t t1[64];
uint8_t t2[64];
volatile uint8_t last = 0;

uint32_t int_calls = 0;

volatile absolute_time_t before_add;
volatile absolute_time_t after_add ;
volatile absolute_time_t int_called_first ;
volatile absolute_time_t int_fin_first ;
volatile absolute_time_t int_start_first ;

uint8_t write_data [64]; 
#define rx_buff_size 256

volatile uint32_t received = 0;
volatile uint32_t int_rentered = 0;
volatile uint8_t int_flag = 0;

struct ringbuff_t
{
    volatile uint8_t buff[rx_buff_size];
    volatile uint32_t head;
    volatile uint32_t tail;
    volatile uint32_t added;
    volatile uint32_t consumed;
    volatile uint32_t discared;
};

struct ringbuff_t rx_buff;

#define UART_ID uart0
#define BAUD_RATE 300
#define DATA_BITS 8
#define STOP_BITS 1
#define PARITY    UART_PARITY_NONE

// We are using pins 0 and 1, but see the GPIO function select table in the
// datasheet for information on which other pins can be used.
#define UART_TX_PIN 12
#define UART_RX_PIN 13

static int chars_rxed = 0;
static uint32_t chars_rx_last = 0;

// RX interrupt handler
void on_uart_rx() {
    if(int_calls==0){int_start_first =  get_absolute_time();}//record first time the interupt was called
    if(int_flag == 1){int_rentered++;}// The interupt routine was re-entered
    int_flag = 1;
    if(int_calls!=0){int_called_first =  get_absolute_time();}//record first time the interupt was called
    int_calls++; // update the number of times the interupt is called

    // received = 0;
    received  = uart_is_readable(UART_ID);
    //chars_rx_last = 0;
    printf("Bytes in FIFO = :%u\n", received);
    while (uart_is_readable(UART_ID)) 
    {
        uint8_t ch = uart_getc(UART_ID);
        // Can we send it back?
        
            // Change it slightly first!
         //   ch++;
         //   uart_putc(UART_ID, ch);
         //printf("%c\n",ch);
       
        chars_rx_last++;
        chars_rxed++;
        uint16_t space = 0;
        //absolute_time_t before =  get_absolute_time();
       // for (int i = 0; i<32; i++)
        //{
            if (rx_buff.head < (rx_buff.tail))
            {
                space = rx_buff.tail - rx_buff.head -1;
            }
            else{
                    space = (rx_buff_size - rx_buff.head) + rx_buff.tail -1;
            }
            if(space>0)
            {
                //add data
                rx_buff.buff[rx_buff.head] = ch;
                 
                 last = ch;
                 //printf("%c\n",last);
                  //printf("%c\n",rx_buff.buff[rx_buff.head] );
                 // printf("%c\n",rx_buff.buff[rx_buff.head] );
                 // printf("Head Counter :%u\n",rx_buff.head);
                rx_buff.head++;
                if (rx_buff.head == rx_buff_size) {rx_buff.head = 0;}
                rx_buff.added++;
                
            }
            else{
                rx_buff.discared++;
               // printf("Core1 discarded head =   %u\n", rx_buff.head);
               // printf("Core1 discarder tail =   %u\n", rx_buff.tail);
            }
       // }

    }
     if(int_calls==1){int_fin_first =  get_absolute_time();}//record first time the interupt was called
      int_flag = 0;
}


int main()
{
    
    stdio_init_all();    
    // Set up our UART with a basic baud rate
    uart_init(UART_ID, BAUD_RATE);

    // Set the TX and RX pins by using the function select on the GPIO
    // Set datasheet for more information on function select
    gpio_set_function(UART_TX_PIN, GPIO_FUNC_UART);
    gpio_set_function(UART_RX_PIN, GPIO_FUNC_UART);

    // Actually, we want a different speed
    // The call will return the actual baud rate selected, which will be as close as
    // possible to that requested
    int actual = uart_set_baudrate(UART_ID, BAUD_RATE);

    // Set UART flow control CTS/RTS, we don't want these, so turn them off
    uart_set_hw_flow(UART_ID, false, false);

    // Set our data format
    uart_set_format(UART_ID, DATA_BITS, STOP_BITS, PARITY);

    //Attemp to change RX watermark level, does not seem to change.
    //hw_set_bits(&uart_get_hw(UART_ID)->lcr_h, UART_UARTIFLS_RXIFLSEL_BITS); //bits 543
    hw_set_bits(&uart_get_hw(UART_ID)->lcr_h, _u(0x00000020)); //bits 5 = 1
    hw_clear_bits(&uart_get_hw(UART_ID)->lcr_h, _u(0x00000018));//clear bit 4,1

    //try to set the watermark level, does not seem to change
    uint32_t val_set = 0x20;
    hw_write_masked(&uart_get_hw(UART_ID)->lcr_h,val_set,
                   UART_UARTIFLS_RXIFLSEL_BITS);

    // Turn off FIFO's - we want to do this character by character
    //uart_set_fifo_enabled(UART_ID, false);
    uart_set_fifo_enabled(UART_ID, true);
    // Set up a RX interrupt
    // We need to set up the handler first
    // Select correct interrupt for the UART we are using
    int UART_IRQ = UART_ID == uart0 ? UART0_IRQ : UART1_IRQ;

    // And set up and enable the interrupt handlers
    irq_set_exclusive_handler(UART_IRQ, on_uart_rx);
    irq_set_enabled(UART_IRQ, true);

    // Now enable the UART to send interrupts - RX only
    uart_set_irq_enables(UART_ID, true, false);

    //load data to send on the UART
    for(int i =0; i<64; i++)
    {
        write_data[i]= i+47; //ascii values from "\""
    }
    critical_section_init(&acrit);
    

    sleep_ms(10000);

    printf("Start UART FIFO and Interupt Test\n");
   // printf("time 1  %lld\n", time_us_64() );
   // printf("time2  %lld\n", time_us_64() );
    before_add = get_absolute_time(); // record time we start to put data out on the UART
    sleep_us(100);
    after_add = get_absolute_time(); // record time we start to put data out on the UART
    int64_t time_diff = absolute_time_diff_us(before_add, after_add);
    printf("Absolute time diff test =   %lld\n", time_diff);

   //initialise ring buffer
    rx_buff.added = 0;
    rx_buff.consumed =0;
    rx_buff.head = 0;
    rx_buff.tail = 0;
    rx_buff.discared = 0;
    chars_rxed = 0;
    int_calls = 0;
    int32_t loop_count = 0;
    //sleep_ms(1000);
    while(1)
    {
        
        before_add =  get_absolute_time(); // record time we start to put data out on the UART
        sleep_us(100);
        //uart_puts(UART_ID, "Loop 123456789 123456789 "); // 
        //With the FIFO eneabled the interupt seem to be called after 4 bytes
        if (loop_count ==0){uart_write_blocking(UART_ID,write_data,16);}
        loop_count++;
        printf("UART Data Set, wait for 5 seconds\n");
 
        sleep_ms(5000);
       // for (int i = 0; i<4; i++)
       // {
       //     printf("Data: %u\n",rx_buff.buff[i]);
       // }
        uint32_t tmphead = 0;
        uint32_t num_data = 0;
        uint8_t data = 0;
        uint64_t time_diff = absolute_time_diff_us(before_add, int_called_first);
        critical_section_enter_blocking(&acrit);  //Seems to be ok without so increment atomic ?
        tmphead = rx_buff.head ;
        critical_section_exit(&acrit);

        printf("*******************************************************\n");
        printf("\n");
        printf("%c\n",last);
        printf("Interupt handler was called, number of times = %u\n", int_calls);// int_flag = 1;
        printf("Interupt handler was re-entered , number of times = %u\n", int_rentered);// int_flag = 1;
        printf("Approx micorsecond after data available interupt was called:%lld\n", time_diff); //int_called_first 
        time_diff = absolute_time_diff_us(int_start_first, int_fin_first);
        printf("Interupt runtime microsedocns: :%lld\n", time_diff); //int_called_first 
        printf("Bytes processec by interupt = %u\n", chars_rxed);//rx_buff.tail 
        printf("Ring buffer head = %u\n", tmphead);
        printf("Ring buffer tail = %u\n", rx_buff.tail);
        printf("Total bytes added to ring: %u\n",rx_buff.added);
        //printf("Rx bytes recievd last %u\n", chars_rx_last);//after_add 
        printf("*******************************************************\n");


        //Get waht data in on the buffer and print it.

        if (tmphead != rx_buff.tail)
        {
            
            if (tmphead < rx_buff.tail)
            {
                num_data = (rx_buff_size - rx_buff.tail) + tmphead ;
            }
            else
            {
                  num_data = tmphead - rx_buff.tail ;
            }
            printf("Number of bytes to process in ring buffer: %u\n", num_data);
            int32_t tmptail = rx_buff.tail;
            printf("Start to decode buffer, tail: %u\n", tmptail);
            for(int i = 0; i < num_data; i++)
            {
                //printf("ring buffer decode for loop\n");
                data = rx_buff.buff[tmptail];
                 printf("\ndecoding at position: %u", tmptail);
                printf("  Data:%u",data);
                tmptail++;
                rx_buff.consumed++;
                if(tmptail == rx_buff_size) {tmptail=0;}

            }
            critical_section_enter_blocking(&acrit);
                rx_buff.tail = tmptail;
            critical_section_exit(&acrit);
        }
        printf("\nTotal bytes read from ring: = %u\n", rx_buff.consumed);
        printf("Ring buffer processed head = %u\n", tmphead);
        printf("Ring buffer tail = %u\n", rx_buff.tail);
        printf("\n");
       // for (int i = 0; i<4; i++)
      //  {
       //     printf("Data: %u\n",rx_buff.buff[i]);
       // }
        sleep_ms(100000);
    }

    /// \end::setup_multicore[]
    return 0;
}
Output from Code

Code: Select all

Absolute time diff test =   135

UART Data Set, wait for 5 seconds

Bytes in FIFO = :1

Bytes in FIFO = :1

Bytes in FIFO = :1

Bytes in FIFO = :1

*******************************************************



>

Interupt handler was called, number of times = 4

Interupt handler was re-entered , number of times = 0

Approx micorsecond after data available interupt was called:532510

Interupt runtime microsedocns: :161

Bytes processec by interupt = 16

Ring buffer head = 16

Ring buffer tail = 0

Total bytes added to ring: 16

*******************************************************

Number of bytes to process in ring buffer: 16

Start to decode buffer, tail: 0



decoding at position: 0  Data:47

decoding at position: 1  Data:48

decoding at position: 2  Data:49

decoding at position: 3  Data:50

decoding at position: 4  Data:51

decoding at position: 5  Data:52

decoding at position: 6  Data:53

decoding at position: 7  Data:54

decoding at position: 8  Data:55

decoding at position: 9  Data:56

decoding at position: 10  Data:57

decoding at position: 11  Data:58

decoding at position: 12  Data:59

decoding at position: 13  Data:60

decoding at position: 14  Data:61

decoding at position: 15  Data:62

Total bytes read from ring: = 16

Ring buffer processed head = 16

Ring buffer tail = 16

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART FIFO, ring buffer, interrupts and Atomic

Wed Apr 21, 2021 5:48 pm

Yes, your results match mine.

I'm not sure I bothered writing any code for UART/DMA. I just thought about what would be needed and decided it was more effort than it was worth (at that time). I did write some for I2C but I couldn't get it to work properly and decided that interrupts were just as good for I2C anyhow.

I'm happy enough with interrupts for the UART for the time being. If high data rates do become a problem I would revisit using the FIFO.

bobtrex
Posts: 27
Joined: Mon Jan 25, 2021 6:24 pm

Re: UART FIFO, ring buffer, interrupts and Atomic

Wed Apr 21, 2021 9:27 pm

Thanks very much for your help

I will create a new thread to see if I can get help setting the watermark and understanding why "uart_read_blocking" always seems to returns 1.

User avatar
joan
Posts: 15564
Joined: Thu Jul 05, 2012 5:09 pm
Location: UK

Re: UART FIFO, ring buffer, interrupts and Atomic

Wed Apr 21, 2021 9:32 pm

Re the watermark, I looked at the time and there is no way of setting the level to one byte or more according to the RP2040 datasheet.

Return to “SDK”