Writing safe UART interrupt buffer

Question

I'm aware of volatile keyboard and it doesn't ensure synchronization safety.

The following code is used inside an interrupt routine, and I'm using heavily the function

GetCharUART inside the main loop.

Is it safe to and stable to write code like that ? or I have to go to low level synchronization like a mutex, if that's true, how would I protect that rbuf ?

 volatile char rbuf[5][UART_BUFSIZE];
vu16 rin[5] = { 0, 0, 0, 0, 0 };
vu16 rout[5] = { 0, 0, 0, 0, 0 };
    void USART_IRQHandler(char Channel, USART_TypeDef *USARTx)
    {
        volatile unsigned int IIR;
     
        IIR = USARTx->SR;
        if (IIR & USART_FLAG_RXNE)
        {                  // read interrupt
          USARTx->SR &= ~USART_FLAG_RXNE;             // clear interrupt
     
          rbuf[Channel][rin[Channel]] = USART_ReceiveData(USARTx);
          rin[Channel]++;
          if(rin[Channel]>=UART_BUFSIZE) rin[Channel]=0;
        }
     
        if (IIR & USART_FLAG_TXE)
        {
          USARTx->SR &= ~USART_FLAG_TXE;              // clear interrupt
        }
    }   
     
     
    int GetCharUART (char Channel)
    {
      int result;
     
      if (rin[Channel]==rout[Channel]) {
            return EMPTY;
        }
     
      result=rbuf[Channel][rout[Channel]];
      rout[Channel]++;
      if(rout[Channel]>=UART_BUFSIZE) 
            rout[Channel]=0;
     
      return result;
    }

Modified code:

void USART_IRQHandler(char Channel, USART_TypeDef *USARTx)
{
    volatile unsigned int IIR;

    IIR = USARTx->SR;
    if (IIR & USART_FLAG_RXNE)
    {                  // read interrupt
      USARTx->SR &= ~USART_FLAG_RXNE;             // clear interrupt

      rbuf[Channel][rin[Channel]% UART_BUFSIZE] = USART_ReceiveData(USARTx);
      rin[Channel]++;
    }

    if (IIR & USART_FLAG_TXE)
    {
      USARTx->SR &= ~USART_FLAG_TXE;              // clear interrupt
    }
}
/******************************************************************************/
int GetCharUART (char Channel)
{
  int result;

  if (rin[Channel]==rout[Channel]) {
        return EMPTY;
    }

  result=rbuf[Channel][rout[Channel]% UART_BUFSIZE];
  rout[Channel]++;

  return result;
}

what is the purpose of volatility of `volatile unsigned int IIR` ? — tstanisl, Sep 29 '21 at 11:40
no, it is a *copy* of what `SR` register had at some moment in time — tstanisl, Sep 29 '21 at 11:43
While the user-side code is manipulating the *shared* variables it could just temporally disable/mask the interrupt. (a mutex is a bad idea, because it could cause the ISR to spin indefinitely) — wildplasser, Sep 29 '21 at 11:44
@tstanisl good point for another issue. How about the original question > — andre_lamothe, Sep 29 '21 at 11:44
@wildplasser Is it necessary ? I mean I can use a mutex at low level, that's my question. Would volatile help in the above code and case ? — andre_lamothe, Sep 29 '21 at 11:44
No. Volatile on the complete buffer(s) is nonsense. Only the access to the read/write pointers/counters (which you don't seem to have) needs to be shared/synchronised. — wildplasser, Sep 29 '21 at 11:49
@wildplasser so those counters and pointers should be volatile ? or should I place a mutex lock and unlock between the start of GetCharUart() and its end ? — andre_lamothe, Sep 29 '21 at 11:53
I think you better implement a FIFO queue to avoid any data corruption. Using FIFO queue you won't need to implement any lock or mutex if you have only one reader. — Kozmotronik, Sep 29 '21 at 11:55
@AhmedSaleh `volatile` does not help with synchronization. It only asks the compiler not to optimize access to the variables because the value may change outside the normal program flow or because accessing the variable may have a side effect. If you don't want to use synchronization mechanisms, atomic access to relevant variables is important. Think about what would happen if `GetCharUART` is interrupted by `USART_IRQHandler` for every possible point of execution. — Bodo, Sep 29 '21 at 11:56
@Bodo Is the above code safe ? If I'm using GetCharUART only in the main loop ? if its not, how would make it safe to use — andre_lamothe, Sep 29 '21 at 12:00
It's common for the IRQ to extract as many bytes as can be read from the UART fifo and load them into the buffer, incrementing/wrapping the volatile 'insert' index as it goes, and then signal a semaphore to make an rx thread ready. The rx thread then handles the data, incrementing/wrapping the 'extract' index until it reaches the 'insert' index. No other synchro required. — Martin James, Sep 29 '21 at 17:14
@Bodo: The Standard specifies that `volatile` accesses will have Implementation Defined semantics beyond an abstract-machine load-store, and implementations designed to be suitable for low-level programming used semantics that were strong enough to make things like interrupts and multi-threaded code work on systems with a strong memory model, without need for any non-standard syntax or "atomic" types. Compilers that are not designed around the needs of low-level programmers may not support such semantics unless optimization is disabled, but that merely means such compilers... — supercat, Nov 30 '21 at 17:49
...should be recognized as being of lower quality for such purposes than compilers which support the necessary semantics with `volatile`. — supercat, Nov 30 '21 at 17:50
@supercat I don't understand what's your point here. My main point is that `volatile` will solve *some* problems related with low-level programming but may *not* be *sufficient* (as the only means) to solve all problems related with concurrent access in programs using threads or interrupts. If/what other mechanisms might be necessary, depends on the specific algorithm. I did not specifically analyze the OP's code for possible problems. (https://en.cppreference.com/w/c/language/volatile) — Bodo, Nov 30 '21 at 18:07
@Bodo: For many years, commercial compilers intended for low-level programming would almost universally process `volatile` with semantics that would allow a mutex using `volatile` objects to guard storage that would be accessed using unqualified reads/writes. The authors of the Standard didn't feel a need to mandate that implementations intended for low-level programming should behave that way, because it seemed obvious that failure to do so would make an implementation less suitable for such purposes. — supercat, Nov 30 '21 at 19:11
@supercat I'm programming in C for >30 years and specifically low-level for >10 years. I think there might be some misunderstanding here, You just wrote that *a mutex can use `volatile` objects* This might mean the same what I am saying. `volatile` can solve *some* problems but is not sufficient for all concurrency problems, specifically if you access data structures, not only values with atomic. When you have some kind of ring buffer with read and write index and condition checking for full/empty buffer, and access from code parts that can interrupt each other you might need deeper analysis. — Bodo, Dec 01 '21 at 11:12
@Bodo: Compilers that were designed to be suitable for low-level programming had `volatile` semantics was sufficient to produce a mutex which could safely guard all kinds of objects (e.g. using Dekker's Algorithm) without need for any kind of atomic types beyond word-sized single-writer-multi-reader semantics naturally supplied by `volatile`. When using a ring buffer and keeping a count of items added and removed, if the size of the buffer is a power of two that's no greater than half the range of the type, and where one execution context only adds things and the other only removes things... — supercat, Dec 01 '21 at 16:06
...putting `volatile` on the read-write counters will be sufficient if accesses to the buffer don't get reordered across accesses to the counters. Such code may fail when processed with compilers that opt to use faster semantics *that are aren't designed to be suitable for tasks involving multiple execution contexts*, but the fact that code to accomplish a task won't work on compilers that aren't designed to be suitable for such a task hardly implies the code is defective. — supercat, Dec 01 '21 at 16:11
@Bodo: Suppose a ring buffer is of size 128, and the counters are eight bits. The number of bytes in the buffer at any moment in time will be `(stuffCount - fetchCount) & 0xFF`. If there are less than 128 bytes, an interrupt may add a character to the buffer by storing it into `buffer[stuffCount & 0x7F]` and incrementing `stuffCount`, in that order. On the main thread, if `stuffCount` and `fetchCount` are observed to be different, the main thread may read `buffer[fetchCount & 0x7F]` and then increment `fetchCount`, in that order. If the ISR and main thread are cache-consistent, ... — supercat, Dec 01 '21 at 18:10
@Bodo: ...the ISR will own all buffer slots when `stuffCount` and `fetchCount` are equal. It will hand ownership of `buffer[stuffCount & 0x7F]` to the main thread when it increments `stuffCount`, and the main thread will hand ownership of `buffer[fetchCount & 0x7F]` back when it increments `fetchCount`. — supercat, Dec 01 '21 at 18:13
@supercat You don't need to explain *to me* how it might be possible to implement a ring buffer based on some `volatile` variables. I'm not specifically intersted in this topic, and when I have to implement something like this, I think I'm able to do the analysis and to design a suitable algorithm. It might be more helpful if you would do an analysis of the code in the question and answer the actual questions: *Is it safe to and stable to write code like that ? or I have to go to low level synchronization like a mutex, if that's true, how would I protect that rbuf ?* — Bodo, Dec 02 '21 at 10:19
I'd suggest using `& UART_SIZE_MASK` rather than `% UART_BUFFER_SIZE`, putting the definition of the former immediately after the definition of the buffer size, to help amplify the fact that the size must be a power of two. — supercat, Dec 02 '21 at 15:41
@Bodo: With regard to safety, it will be safe on a single-core CPU if the size is a power of two if `vu16` means `volatile uint16`, and if the buffer is volatile (as is the case here). Traditional compilers wouldn't require that the buffer be volatile, but clang and gcc shouldn't be expected to work reliably if it isn't. — supercat, Dec 02 '21 at 15:43
@supercat **I don't care about this topic.** You might want to write an answer, but writing comments addressed to me does not make any sense. — Bodo, Dec 02 '21 at 15:49

score 1 · Accepted Answer · answered Sep 29 '21 at 12:17

1

Your code has a functional bug.

In the ISR you don't check for a "buffer full" condition. The code just increments rin[Channel] without looking at rout[Channel]. So a whole buffer of data can be lost.

Example: If rout[Channel] equals zero and rin[Channel] equals UART_BUFSIZE-1 then the ISR will set rin[Channel] to zero. In other words the buffer will now appear empty and data is lost.

So the first step is to fix the code.

answered Sep 29 '21 at 12:17

Support Ukraine

42,271
4
38
63

Can you show a fix to that code ? please the synchronization problem please ? plus 1 – andre_lamothe Sep 29 '21 at 12:21
@AhmedSaleh If UART_BUFSIZE is a power of 2, I would let `rin` and `rout` wrap when they reach MAX. In other words I would avoid code like: `if(rin[Channel]>=UART_BUFSIZE) rin[Channel]=0;` – Support Ukraine Sep 29 '21 at 12:27
Yes it's a power of two. so I would do rin[channel] = rin[channel]%UART_BUFSIZE and the same for rout ? but where should I write that – andre_lamothe Sep 29 '21 at 12:29
I have modified the code in the original post, would you check it please. – andre_lamothe Sep 29 '21 at 12:31
1

No, I wouldn't do that. Just keep incrementing. When it reach 65535 it will automaticcaly wrap to zero on the next increment. Then you use the `%` operator when accessing the buffer. – Support Ukraine Sep 29 '21 at 12:31
1

Like: `result=rbuf[Channel][rout[Channel] % UART_BUFSIZE];` – Support Ukraine Sep 29 '21 at 12:36
Can you check the modified code ? – andre_lamothe Sep 29 '21 at 12:37
You placed it wrong in you modified code – Support Ukraine Sep 29 '21 at 12:37
1

and you also need it here `rbuf[Channel][rin[Channel]]` – Support Ukraine Sep 29 '21 at 12:37
ok I fixed it in the modified code, is it right ? Now returning back to synchronization.. is it enough to use volatile . – andre_lamothe Sep 29 '21 at 12:41
@AhmedSaleh Writing an ISR requires a lot of low level knowledge about the processor. Depending on the processor `volatile` may not even be necessary. On another processor you may need it or something else. A general solution in C is not possible. BTW: A mutex in an ISR is not likely to be useful either. – Support Ukraine Sep 29 '21 at 13:02
I will protect the function GetCharUart by a mutex... is that feasible and right ? – andre_lamothe Sep 29 '21 at 13:18
No, a mutex is not the solution. Again, an ISR requires low level knowledge. You can't rely on standard C code. See https://stackoverflow.com/questions/35011322/why-not-to-use-mutex-inside-an-interrupt – Support Ukraine Sep 29 '21 at 13:23
@AhmedSaleh BTW: On some micro-processors a short interrupt disable is a good way to protect things. But - to repeat - it depends on your processor/system – Support Ukraine Sep 29 '21 at 13:27
A mutex in an ISR is not only unhelpful, it is a disaster:( – Martin James Sep 29 '21 at 13:46
its not in ISR it's in the function – andre_lamothe Sep 29 '21 at 14:01
@AhmedSaleh Are you saying that `USART_IRQHandler` isn't an ISR!? – Support Ukraine Sep 29 '21 at 14:07
it is an ISR, but why should I put mutex there.. the mutex will be inside the function GetCharUart() – andre_lamothe Sep 29 '21 at 14:22
@AhmedSaleh A mutex has two be two places. – Support Ukraine Sep 29 '21 at 14:30
@4386427 Why it should be in two places ? – andre_lamothe Sep 29 '21 at 19:36
@AhmedSaleh Because that is how a mutes works. But it's not important here. Just forget about mutex for this task – Support Ukraine Sep 29 '21 at 19:57
@4386427 disabling/enabling interrupt in the function GetCharUart would increase the latency a lot. I can't imagine that there is no solution – andre_lamothe Sep 29 '21 at 20:16
@AhmedSaleh You don't disale interrupt in the whole function. Only in the critical section, i.e. when reading/writing shared variables. In other words - only very shortly. – Support Ukraine Sep 30 '21 at 05:47
@AhmedSaleh Also see https://en.wikipedia.org/wiki/Compare-and-swap – Support Ukraine Sep 30 '21 at 05:59
@MartinJames: While mutexes are commonly used with spinlocks around the "attempt to acquire" operation, it's possible to have an ISR use an "attempt to acquire" operation on a mutex and, if it fails, defer an operation until some later interrupt. While it's common to both describe mutexes as being the simplest way of guarding resources, and as including spinlocks, those two definitions are contradictory, since the simplest ways of guarding resources don't use spinlocks. – supercat Nov 30 '21 at 17:56

score 0 · Answer 2 · answered Nov 30 '21 at 15:50

Rather than trying to make data structures that would work synchronously work with interrupts, it's best to use a different data structure that actually is safe for use in interrupts.

It is common for circular/ring buffers to be used for data structures that need to be shared between regular code and interrupt code.

https://en.wikipedia.org/wiki/Circular_buffer

A circular buffer is implemented with two indices, and each side is only modifying one of the indices, which has a sort of reader/writer style. This allows it to be pushed onto and popped off by concurrent executions.

Because of the dual indices and the fact the indices are always incremented in one direction, the worst case that could happen is that the interrupt dropped a byte when the buffer is almost full because it didn't pick up on a recent change to the other index, which is not a big deal.

If you make your own implementation, remember to test it, because it can be easy to mess up.

Writing safe UART interrupt buffer

2 Answers2