Does this code need interrupts disabled?

[peter-h]

I think it does but it's not obvious.

The code is self explanatory. It is a get_ipqcount() function which returns how many bytes are in a circular queue. The input pointer is rx_index and the output pointer is get_index.

Receive is under interrupt, and extraction from the queue is in an RTOS task so basically also under interrupt.

Code: [Select]

count = (int32_t)serial_receive_buffer_rx_index[port] - (int32_t)serial_receive_buffer_get_index[port];  // buffers of uint8_t

if (count < 0) {
// rx index has wrapped and is now before the get index
count = SERIAL_RX_BUFFER_SIZE + count;
}

if (count > (SERIAL_RX_BUFFER_SIZE-1)) {
// This should never be allowed to happen
count = 0;
serial_flush_rx(port);
}
and here is another similar one; this is get_opqspace() which returns how many bytes can be placed into a tx queue

Code: [Select]

count = (int32_t)serial_transmit_buffer_put_index[port] - (int32_t)serial_transmit_buffer_tx_index[port];

if (count < 0) {
// put index has wrapped and is now before the tx index
count = SERIAL_TX_BUFFER_SIZE + count;
}

if (count > (SERIAL_TX_BUFFER_SIZE-1)) {
// This should never be allowed to happen
count = 0;
serial_flush_tx(port);
}

// Space is buffer size - count -1 and the -1 is needed because full buffer = buffer_size-1
qsize = SERIAL_TX_BUFFER_SIZE - count -1;
I am also not sure of the evaluation order of the last line of code above.

(SERIAL_TX_BUFFER_SIZE - count) -1

is not same as

SERIAL_TX_BUFFER_SIZE - (count -1)

This is someone else's code. I always do circular buffers as inptr, outptr, size, disabling ints all around, and then a 256 byte buffer can actually hold 256 bytes, whereas the above can hold only 255.

[ejeffrey]

Yes, it's obvious:

Code: [Select]

count = (int32_t)serial_receive_buffer_rx_index[port] - (int32_t)serial_receive_buffer_get_index[port];
...
if (count > (SERIAL_RX_BUFFER_SIZE-1)) { // This should never be allowed to happen

Even if the access to the serial_receive_buffer_* arrays were atomic (which they are not as written, so it's 100% wrong), there is no guarantee that the access to the two different arrays is consistent.  Thus, your "this should never happen" definitely can happen.

[peter-h]

Let me clarify by posting the function which fills the buffer

Code: [Select]


/*
* Write a number of bytes to the transmit queue and start transmission if necessary
* Returns false if there is insufficient space for the given length
* Port is numbered 0 to 4, with 0 being the USB VCP
*
* The port 0 processing is different for port 0 because there is flow control on tx (to PC)
* which necessitates a circular buffer before it just like ports 1-4 have. The difference
* is that with serial ports the flow control is the baud rate whereas with port 0 is it
* how fast the PC application is reading the data out of the VCP.
*
* This function is NON blocking, if the data fits into the TX buffer, otherwise no action
*
*/

bool serial_transmit(uint8_t port, uint8_t *data, uint16_t length)
{
bool ok = false;

if (port > 0 && port <= SERIAL_MAX_PORT)
{
if (serial_get_opqspace(port) >= length)
{

portENTER_CRITICAL(); // prevent interrupts while updating the index

// Copy the data to the circular buffer
port--; // offset from 0
for (uint16_t i = 0; i < length; i++)
{
serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]++] = data[i];
serial_transmit_buffer_put_index[port] %= SERIAL_TX_BUFFER_SIZE;
}

if (!serial_transmit_active[port])
{

// Transmitter is not already running so load the first byte and enable the TXE and TC interrupts
// First clear TC. The read of SR (RMW actually) and a write to DR also clears pending TC int.
serial_port_handles[port]->Instance->SR &= ~(UART_FLAG_TC);
serial_port_handles[port]->Instance->DR = serial_transmit_buffer[port][serial_transmit_buffer_tx_index[port]++];
serial_transmit_buffer_tx_index[port] %= SERIAL_TX_BUFFER_SIZE;
serial_transmit_active[port] = true;

// Enable TX empty and TX complete interrupts
__HAL_UART_ENABLE_IT(serial_port_handles[port], UART_IT_TXE);
__HAL_UART_ENABLE_IT(serial_port_handles[port], UART_IT_TC);
}

portEXIT_CRITICAL();

ok = true;
}
}
else if (port == 0)
{
// USB CDC port. Same as port 1-4 above but no need to mess with interrupts etc
// The circular buffer gets copied to CDC in the USB RTOS task (in main.c)

CDC_serial_transmit(data, length);
ok=true;

}

return ok;
}
What made me suspicious is that the above calls serial_get_opqspace() with interrupts enabled. But the CDC transmit function does disable them:

Code: [Select]


// Same as serial_transmit().

void CDC_serial_transmit(uint8_t *data, uint16_t length)
{

portENTER_CRITICAL();

if (CDC_get_opqspace() >= length)
{
for (uint16_t i = 0; i < length; i++) {
cdc_transmit_buffer[cdc_transmit_buffer_put_index++] = data[i];
cdc_transmit_buffer_put_index %= CDC_TX_BUFFER_SIZE;
}
}

portEXIT_CRITICAL();

}
and cdc_get_opqspace does it correctly - I wrote that part a year or two ago and commented on it

Code: [Select]



// Return the space available in CDC circular transmit circular buffer
// This function should be called with ints disabled (with portENTER_CRITICAL())

uint16_t CDC_get_opqspace(void)
{

// Count is buffer size - space
uint16_t space = CDC_TX_BUFFER_SIZE - CDC_get_opqcount() -1;

return space;
}


My point is that often there is no need for atomic code. For example a function returning input queue size is safe even if the size increases since you checked the value but before you read the data out (obviously). Similarly with a function returning output queue space; if it returns 20 but actually by the time you write to it there is space for 21, that's ok.

[peter-h]

I guess this thread is a living example of: one should never post more than 20 lines of code if one wants a response

However, I don't understand this

there is no guarantee that the access to the two different arrays is consistent

I have four serial ports, and therefore four TX buffers and four RX buffers. The index is 1-4 (0 means something else - a USB VCP).

I cannot see a problem with one RTOS thread using say port 2 and another using port 3. What one might want to do is transmit on port 2 and receive on port 3

Anyway, this has led down another rabbit hole and the discovery that portENTER_CRITICAL() (which I have been using around various sections) does not disable interrupts! It merely sets the priority mask to 5 which disables everything needed to stop FreeRTOS switching tasks. This in turn led to the discovery that the serial port interrupts were all set to priority 0 which cannot be masked! That originated from Cube MX generated code from years ago. So I set them to priority 1, which is still fine (some timer ISRs are also 1 but those ISRs are all very short, and anyway the UARTs do have a buffer in them). Then I redefined configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY from the default value of 5 to 1 which as far as I can find out with extensive googling blocks interrupts of priority 0, and I have no such sources. I wonder if anyone knows about this stuff...

[peter-h]

Yes I think that is the basic idea to support not needing DI/EI.

The suspected problem turns out to be some interaction between the mail loop and the transmit ISR.

[Sauli]

My point is that often there is no need for atomic code. For example a function returning input queue size is safe even if the size increases since you checked the value but before you read the data out (obviously). Similarly with a function returning output queue space; if it returns 20 but actually by the time you write to it there is space for 21, that's ok.

True.

If the code doesn't use the buffer Count value at all, doesn't the interrupt issue disappear?  The ISR would only change the pointer to the head of the buffer as it adds entries to it, and the main loop would only change the tail pointer as it takes entries out of the buffer.  Instead of keeping Count, you just compare the head and tail pointers to see if they are equal.

The code is not keeping count but indexes into the buffer, which is no different from using pointers.

It is code like this that requires DI/EI:

Code: [Select]

serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]++] = data[i];
serial_transmit_buffer_put_index[port] %= SERIAL_TX_BUFFER_SIZE;


But you can avoid the index from even momentarily having an illegal value by using a local variable:

Code: [Select]

int index = serial_transmit_buffer_put_index[port];
serial_transmit_buffer[port][index++] = data[i];
serial_transmit_buffer_put_index[port] = index % SERIAL_TX_BUFFER_SIZE;


[tggzzz]

Anyway, this has led down another rabbit hole and the discovery that portENTER_CRITICAL() (which I have been using around various sections) does not disable interrupts! It merely sets the priority mask to 5 which disables everything needed to stop FreeRTOS switching tasks. This in turn led to the discovery that the serial port interrupts were all set to priority 0 which cannot be masked! That originated from Cube MX generated code from years ago. So I set them to priority 1, which is still fine (some timer ISRs are also 1 but those ISRs are all very short, and anyway the UARTs do have a buffer in them). Then I redefined configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY from the default value of 5 to 1 which as far as I can find out with extensive googling blocks interrupts of priority 0, and I have no such sources. I wonder if anyone knows about this stuff...

By and large priority levels cause at least as many problems as they address. My preference is for three levels:

• extreme prejudice: watchdog/powerfail
• normal interrupt: grab input, put it in a FIFO, then exit interrupt
• normal processing: everything else

Normal interrupt processing should be so short that they don't need to be interruptable. Run them all at the same priority. By definition, it doesn't matter if an extreme prejudice interrupt occurs.

FIFO put and get operations must be atomic.

That modus operandi avoids all sorts of subtle unrepeatable behaviour.

[NorthGuy]

I think it does but it's not obvious.

You don't need to disable interrupts for this code if few conditions are met:

- All writers are in the same thread (that is there never will be two different writers in concurrent threads)
- All readers are in the same thread (that is there never will be two different readers in concurrent threads)
- The "port" variable is not shared between threads (that is when you run the code, the "port" cannot be changed by any other thread)

The flushing code (not shown?) is most likely problematic, but it is not needed anyway. Better make sure you don't overflow/underflow the buffer. If you do allow overflow/underflow to happen, this is already a bug and must be fixed. Checking for overflow/underflow after it has already happened is idiotic, so I would remove such checks completely.

[NorthGuy]

Let me clarify by posting the function which fills the buffer

Code: [Select]

    serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]++] = data[i];


The concern here is that the increment should happen after the write. C doesn't guarantee this. Therefore

Code: [Select]

serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]] = data[i];
serial_transmit_buffer_put_index[port]++;
is better. It's still a small possibility that the compiler may alter the order of writes if it believes that this doesn't cause any effects. If you want to be paranoid about this, use some sort of barrier:

Code: [Select]

serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]] = data[i];
//some_sort_of_barrier here
serial_transmit_buffer_put_index[port]++;
which may be an asm's nop or even empty asm line in the simplest case. If you have multiple cores you barrier may be more complicated because CPU may reorder memory accesses by itself. Linux has special functions for that.

[peter-h]

This seems to work fine. Is there a potential issue?

Code: [Select]

int index = serial_transmit_buffer_put_index[port];
serial_transmit_buffer[port][index++] = data[i];
serial_transmit_buffer_put_index[port] = index % SERIAL_TX_BUFFER_SIZE;

In the old Z80 etc days one would just do a DI/EI around stuff but the ARM doesn't have such instructions and there is no simple way to do it (some previous threads on that). The portENTER_CRITICAL() macro (part of FreeRTOS) leads to quite a bit of code and merely elevates the priority threshold.

[Berni]

Just making the relevant arrays and counters as volatile should make sure the compiler doesn't swap things around.

In general circular buffers are the best idea because you have only the read side incrementing the read pointer and only the write side incrementing the write pointer. This solves the problem of having no atomic increment instruction on most modern CPUs.

You still get problems if you have multiple threads writing to the buffer. For that i think the only easy solution is a mutex.(Luckily a RTOS will tend to provide those out of the box). But i find myself mostly using these kind of buffers to move data between fast interrupt routines and main code(so 1 writer, 1 reader), in that case a circular buffer just simply works on its own.

[abyrvalg]

In the old Z80 etc days one would just do a DI/EI around stuff but the ARM doesn't have such instructions

ARM does have DI/EI equivalent: CPSID I/CPSIE I

[tggzzz]

Just making the relevant arrays and counters as volatile should make sure the compiler doesn't swap things around.

Are you sure? From https://en.cppreference.com/w/c/language/volatile

"Note that volatile variables are not suitable for communication between threads; they do not offer atomicity, synchronization, or memory ordering. A read from a volatile variable that is modified by another thread without synchronization or concurrent modification from two unsynchronized threads is undefined behavior due to a data race. "

The whole area is a (poorly marked) minefield.

[wek]

"doesn't swap things around" is explained in the first part of the article you've linked to, namely, that all volate accesses are finished and none started at the sequence points (e.g. at the semicolon delimiting recordings). That's all volatile gives you.

Now how many volatile accesses you have in between successive sequence points, and what exactly those accesses are, and whether that is or is not safe in context of interrupts (and multitasking is just a glorified interrupt) is up to you to judge, as that is  context-dependent.

There are no silver bullets in mcu programming.

JW

[tggzzz]

There are no silver bullets in mcu programming.

That's just as true in all programming.

Some tools help by reducing complexity. Some tools hinder by increasing complexity - sometimes due to outdated presumptions and/or requirements for backward compatibility.

[tggzzz]

"doesn't swap things around" is explained in the first part of the article you've linked to, namely, that all volate accesses are finished and none started at the sequence points (e.g. at the semicolon delimiting recordings). That's all volatile gives you.

Volatile has a long and inglorious history in C. Don't take my word for it; listen to Dennis Ritchie, Lunis Torvalds, John Mashey...
https://www.yarchive.net/comp/volatile.html

Most people don't understand C as well as they think they do. Zealots claim this is peoples' fault rather than the language's fault.

[Berni]

Are you sure? From https://en.cppreference.com/w/c/language/volatile

Yep i was using it in the premise that you can use volatile to make sure you write the data buffer before writing the new counter value. That link explains that volatile makes sure to finish all writes/reads before evaluating the next part of code. The use of that is to make sure the other thread is informed of there being new data(writing the counter) only after the data is there(in the buffer), thus avoiding a race condition.

The wrong use of the volatile keyword is expecting it to make the operation atomic. There is indeed no way for the compiler to magically make things atomic on a CPU that doesn't have appropriate atomic instructions for a certain thing. Hence why it doesn't magically make multithreaded access to a variable work. So it should indeed NOT be used for that.

But this is still enough to implement the circular buffer with separate read and write pointers to communicate between two threads. A lot of the time this is all you really need anyway.(Like an interrupt pumping data into a worker on the main thread)

[NorthGuy]

Yep i was using it in the premise that you can use volatile to make sure you write the data buffer before writing the new counter value. That link explains that volatile makes sure to finish all writes/reads before evaluating the next part of code. The use of that is to make sure the other thread is informed of there being new data(writing the counter) only after the data is there(in the buffer), thus avoiding a race condition.

Yes, that always worked. But now, on big out-of-order CPUs, the order of writes seen by one core may be different from the sequence seen by another core. Say, you may write data first then index, but the CPU re-arranges writes, so the other core executing at the same time sees the index change, while data are still not written. So, if you're writing a Linux driver for RPi, or communicating between RPi threads, you may get very rare data corruptions, hence the need for SMP barriers.

[wek]

There are no silver bullets in mcu programming.

That's just as true in all programming.

"Big computers" have way more silver at their disposal, and bullets are cast regardless of their individual cost, as it's easier to simply machine-gun  the zombies wholesale than to deal with them individually using all the avalable tools there are (including simply running away from them, which is the gist of the circular buffer solution above, with some cheap garlic thrown to avoid corner cases).

Volatile has a long and inglorious history in C. Don't take my word for it; listen to Dennis Ritchie, Linud Torvalds, John Mashey...

With all due respect to these gentlemen, their experience is rather limited to said big computers. Note liberal usage of "lock" (.EQ. silver bullet) since the very second post (together with strong opinions but we knew that).

Volatile is a tool very adequate to the PDP11 macroassembler C really is. Blunt, leaving all the heavy lifting to the user, hard to use without undestanding the roots (i e. compiler design) and ultimately nonportable. But also completely transparent, well controllable and lightweight, which are treats you want when your goals are minimal memory usage and hard real time execution.

You want something smarter? Don't use C then, go Rust or whatever is en vogue this week.

JW

[nctnico]

Are you sure? From https://en.cppreference.com/w/c/language/volatile

Yep i was using it in the premise that you can use volatile to make sure you write the data buffer before writing the new counter value. That link explains that volatile makes sure to finish all writes/reads before evaluating the next part of code. The use of that is to make sure the other thread is informed of there being new data(writing the counter) only after the data is there(in the buffer), thus avoiding a race condition.

Agreed. Back to the OP's questions: disabling IRQs is only needed if the code can be interrupt by another interrupt that uses the buffer indexes.

Assuming the UART buffer has the typical interrupt space / user space interaction, it depends on whether an RTOS task switch can happen while inside the UART interrupt. I don't see how an RTOS would work if a task switch (timer) interrupt can interrupt dealing with hardware interrupt. So my guess would be that the code doesn't need disabling the interrupts because the task switch timer interrupt has a much lower prority.

[Siwastaja]

* push only modifies write pointer
* pop only modifies read pointer
* when pushing, update write pointer after writing the data; so that concurrent pop does not detect availability of new item too early
* when popping, update read pointer before returning the pointer to the data; so that concurrent push detects the full condition before overwriting the returned data
* unless you can make it atomic with say 256-long FIFO, calculate the magical "+1 with wrap" with local, temporary variables and make sure assignment into the global pointer or index is atomic
* no volatile needed
* no disabling of interrupts needed, given that:
* make pushes from one thread (e.g.; if from ISRs, they have to be same priority level not to pre-empt each other)
* make pops from one thread (again, if from ISRs, same priority level).
* push and pop threads can be of course different from each other.

Feel free to correct me, probably got something wrong anyway. In any case, I think I have had zero problems with FIFOs, they are an excellent way to transfer data asynchronously between two threads, but one needs the fixed roles of one pusher thread and one popper thread.

[nctnico]

You'll need to use volatile in order to do something like (with read_index or write_index modified in an interrupt or different thread):
while (read_index == write_index) wait_for_characters();

Without volatile, the compiler will produce an endless loop that never exits.

[wek]

> * no volatile needed

Without volatile, compiler is free to assume nothing in the buffer changed (both data and indices) and optimize out related code entirely.

And without volatile, compiler is free to reorder accesses, e.g "write data first then increment index" may happen in the opposite order than written in source.

JW

[gf]

* when pushing, update write pointer after writing the data; so that concurrent pop does not detect availability of new item too early

But this requires to enforce memory ordering. Preferably and machine-independent via atomic_store_explicit(..., memory_order_release) in order to store the incremented index after the data, or alternative (for single-core machines only) by making both, the buffer and the index variable volatile.

[Siwastaja]

Use some memory ordering barrier and wrap the push / pop in functions.