Electronics > Microcontrollers

Is this Circular Event Buffer thread safe? (STM32G431)

(1/3) > >>

syntax333:
I created a circular buffer structure for my MCU(STM32G431) where elements of structure are:


* EventBuffer[m]
* Head
* Tail
I use the events to drive a state machine. For my application ISRs are producers and my main is consumer. ISRs cannot preempt each other, each has same priority. ISRs push events to head index of event buffer and increment head variable while my main code pops events from tail index of event buffer and increment tail variable. If head or tail pointer reach end of the buffer they simply wrap around.

In main code I check the buffer like:

--- Code: ---CircBuffer.Head = 0;
CircBuffer.Tail = 0;
enable_ISRs();

while(1){
  if(CircBuffer.Head == CircBuffer.Tail)
  {
  //Low power mode
  }
  else
  {
    statemachine_dispatch(CircBuffer.EventBuff[CircBuffer.Tail])
    increment CircBuffer.Tail & BUFFLEN_MASK
  }
}

--- End code ---

In ISRs I add event to buffer like:


--- Code: ---void example_ISR1()
{
  clear flag
  CircBuffer.EventBuff[CircBuffer.Head] = example_EVENT1;
  increment CircBuffer.Head & BUFFLEN_MASK
}
void example_ISR2()
{
  clear flag
  CircBuffer.EventBuff[CircBuffer.Head] = example_EVENT2;
  increment CircBuffer.Head & BUFFLEN_MASK
}

--- End code ---

If I have to post an event to event buffer from main I disable and enable the interrupts.

I was wondering if this structure is thread safe or do I have to disable and enable the interrupts in my main loop while checking if buffer is empty (Head==Tail, No events to process) or dispatching the events or incrementing tail variable? Am I missing something?

capt bullshot:

--- Quote from: syntax333 on September 22, 2021, 05:24:09 am ---I created a circular buffer structure for my MCU(STM32G431) where elements of structure are:

... snip ...

I was wondering if this structure is thread safe or do I have to disable and enable the interrupts in my main loop while checking if buffer is empty (Head==Tail, No events to process) or dispatching the events or incrementing tail variable? Am I missing something?

--- End quote ---

Looks fine to me. The interrupts don't use the tail pointer, and CPU read access to head pointers should be atomic, so the main will read always a valid variable. Don't forget to declare the pointers modified by the interrupts "volatile", otherwise the compiler might optimise your (Head == Tail) comparison out and you'll get never an input.

newbrain:
Looks almost good to me.

One thing I think it's missing is checking for a full queue.
The way it's written, new events will overwrite old ones and this might be a design choice, but there's a latent problem.
If the processing of one event is slow, and enough events pile up while it's being processed, the case might arise when Head==Tail - so no further event dequeueing will happen, until a new one comes in (and a full queue of events would be lost).

Now this might or might not be a worry depending on many conditions, but in similar cases I've always used a separate atomic variable to keep count of the used (or free) places in the queue to be checked for empty (in main) or for full (in the ISRs). It's very little additional code and load, and allows to select the behaviour on a full queue (overwrite older events or throw new events away).

Another nitpick, but I know here opinions differ: if you mean modulo, say modulo (%). Using binary and (&) works only for 2^n sizes of the queue.
Any compiler worth its salt will change the % to a & even without optimizations and even when a modulo must be used, an efficient multiplication only algorithm is generally used (speaking for arm and similar, if you are on an 8bit machine, YMMV).

syntax333:


--- Quote from: newbrain on September 22, 2021, 09:03:22 am ---Looks almost good to me.

One thing I think it's missing is checking for a full queue.
The way it's written, new events will overwrite old ones and this might be a design choice, but there's a latent problem.
If the processing of one event is slow, and enough events pile up while it's being processed, the case might arise when Head==Tail - so no further event dequeueing will happen, until a new one comes in (and a full queue of events would be lost).

Now this might or might not be a worry depending on many conditions, but in similar cases I've always used a separate atomic variable to keep count of the used (or free) places in the queue to be checked for empty (in main) or for full (in the ISRs). It's very little additional code and load, and allows to select the behaviour on a full queue (overwrite older events or throw new events away).

Another nitpick, but I know here opinions differ: if you mean modulo, say modulo (%). Using binary and (&) works only for 2^n sizes of the queue.
Any compiler worth its salt will change the % to a & even without optimizations and even when a modulo must be used, an efficient multiplication only algorithm is generally used (speaking for arm and similar, if you are on an 8bit machine, YMMV).

--- End quote ---

I ensure that Head pointer never catches tail so checking full queue is not necessary.
I only use 2^n size queue which is not a problem for my application but thanks for the advice.


--- Quote from: capt bullshot on September 22, 2021, 07:45:25 am ---
--- Quote from: syntax333 on September 22, 2021, 05:24:09 am ---I created a circular buffer structure for my MCU(STM32G431) where elements of structure are:

... snip ...

I was wondering if this structure is thread safe or do I have to disable and enable the interrupts in my main loop while checking if buffer is empty (Head==Tail, No events to process) or dispatching the events or incrementing tail variable? Am I missing something?

--- End quote ---

Looks fine to me. The interrupts don't use the tail pointer, and CPU read access to head pointers should be atomic, so the main will read always a valid variable. Don't forget to declare the pointers modified by the interrupts "volatile", otherwise the compiler might optimise your (Head == Tail) comparison out and you'll get never an input.


--- End quote ---

I checked the if statement and comparison consists 4 ldr instructions 2 for head and 2 for tail and then 1 cmp instruction so read access to head pointer is not atomic. So I changed my code to:


--- Code: ---..
..

while(1)
{
  disable_interrupts();

  if(Head != Tail)
  {
     enable_interrupts();
     statemachine_dispatch(CircBuffer.EventBuff[CircBuffer.Tail])

     disable_interrupts();
     increment CircBuffer.Tail & BUFFLEN_MASK
     enable_interrupts();

  }
  else
  {
  //Low Power Mode
  enable_interrupts();
  }

}


--- End code ---

capt bullshot:

--- Quote from: syntax333 on September 22, 2021, 09:53:32 am ---
I checked the if statement and comparison consists 4 ldr instructions 2 for head and 2 for tail and then 1 cmp instruction so read access to head pointer is not atomic. So I changed my code to:

...


--- End quote ---

I don't think the disable / enable interrupts is necessary here. What I was thinking of when writing "atomic" is: Interrupt modifies Head pointer at any time. The CPU loads the value of Head pointer in one unbreakable memory access, this should be the case with almost all modern CPUs. Doesn't matter if it does load Head and Tail non-atomic, and check what the two ldr instructions do in detail - do they load e.g. 8 bits of a 16 bit word and combine that in a register (bad) or is the first ldr a pointer load and the second the pointer dereferencing, loading the register in one fetch (OK)?
From my (rather cursorily) knowledge of ARM CortexM assembly, the two ldr instructions would rather be one pointer register load and one indirect memory access to atomically fetch the Head pointer.

Navigation

[0] Message Index

[#] Next page

There was an error while thanking
Thanking...
Go to full version