Does this code need interrupts disabled?

[NorthGuy]

* 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

If you update the index before reading data, the "pusher" will think that the data position is empty and will overwrite it. You should always update indices after you're done with data accesses - index update is a signal to the other side that the cell pointed to by the index is processed (read or written).

* no volatile needed

It is. For erxample, if you access an index which is modified only by the other side in a loop, the compiler may decide that the index never changes and won't re-load it at every pass. Volatile will force re-loading.

[gf]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

[Siwastaja]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

C programmers are slow to change their habits; they use what they learned to use, and it shows in the code. Many don't even use the useful C99 stuff such as stdint.h.

[peter-h]

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.

The scenario is a foreground program (which is an RTOS task, like everything in this product after main() completes) puts TX data into that buffer, and then a TXE interrupt takes data out of it. And similarly, on the RX side, I have an RXNE interrupt putting data into the buffer and an RTOS task taking it out.

Accompanying this I have functions like opqspace() and ipqcount() which do the obvious thing. Because my buffers don't have a "quesize" value (the difference between the pointers is the queue size) these functions do the pointer subtraction. This scheme wastes one byte compared to the one where you maintain a separate "quesize" value. These functions also need to do the job atomically, because the pointers chase each other round the buffer - example:

Code: [Select]


// Get the number of bytes available in the receive buffer for this port
// Port is numbered 0 to 4, with 0 being the USB VCP

uint16_t serial_get_ipqcount(uint8_t port)
{
int32_t count;

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

// Port number is 1 to 4, so -1 to offset from 0
count = (int32_t)serial_receive_buffer_rx_index[port-1] - (int32_t)serial_receive_buffer_get_index[port-1];

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_receive_buffer_rx_index[port-1] = 0;
serial_receive_buffer_get_index[port-1] = 0;
}

portEXIT_CRITICAL();
}

return (uint16_t) count;
}

I am not calling any bit of FreeRTOS from an ISR, even though this is supposedly possible.

I have changed the portENTER_CRITICAL() and portEXIT_CRITICAL() to block ints with a priority of >0 (was >5 previously as default on FreeRTOS, because the intention of these macros is only to stop an RTOS task switch, while allowing most/all interrupts). This in effect does a DI/EI (because I have no priority 0 interrupts) but will look into:

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

These appear to need

Code: [Select]

__asm("CPSID i");
// ...some code which needs to be protected from interrupts...
__asm("CPSIE i");
Does that look right? I have seen other and much longer variations of the asm constructs. I don't think the above work on GCC v10. Like this

Code: [Select]

/**
  \brief   Enable IRQ Interrupts
  \details Enables IRQ interrupts by clearing the I-bit in the CPSR.
           Can only be executed in Privileged modes.
 */
__attribute__( ( always_inline ) ) __STATIC_INLINE void __enable_irq(void)
{
  __ASM volatile ("cpsie i" : : : "memory");
}


/**
  \brief   Disable IRQ Interrupts
  \details Disables IRQ interrupts by setting the I-bit in the CPSR.
  Can only be executed in Privileged modes.
 */
__attribute__( ( always_inline ) ) __STATIC_INLINE void __disable_irq(void)
{
  __ASM volatile ("cpsid i" : : : "memory");
}

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.

We did this in that long "is compiler smart enough" thread, etc, but the reality is that practically nobody uses "volatile" on buffers I guess that's because most are declared "static", or "extern" at some place. But even where I have a buffer as a local variable in a function, it is still not getting removed.

Makes me wonder how anybody's code ever worked... My suspicion is that compiler writers don't just want to break everybody's code.

[tggzzz]

Makes me wonder how anybody's code ever worked... My suspicion is that compiler writers don't just want to break everybody's code.

Many choose not to use aggressive optimisation because of such issues with their code, or interaction with libraries, or because they are worried that the next compiler version will "improve" the optimisation.

Then there's the definition of "worked". I have heard programmers say in all seriousness "it works because the unit tests are showing a green light". It is just as well that there wasn't a cluebat within range!

Then² there's the issue of how you test concurrent programs to prove constraints aren't violated. Ask someone that has too much faith in unit testing to provide a test that reliably fails as a result of incorrect use of a database's ACID properties.

[peter-h]

OK, but if somebody has written

char fred [256];

and the compiler "somehow" determines that only first 10 bytes actually get accessed, e.g. because the only reference to fred that it finds is

Code: [Select]

for (i=0;i<10;i++)
  fred[i]=0;
would it not be monumentally dumb to remove the other 246 bytes of fred and silently replace that definition with

char fred[10];

I know nothing about the C language definition (which seems to be a virtual machine) but that would be a totally perverted thing to do.

Apart from anything else, it would prevent you declaring a buffer bigger than you need, because the compiler might spot the excess.

[Berni]

Yep compilers can be impressively smart and these sort of things are the reason why sometimes peoples code breaks when the optimizations are all turned on.

At one point i even had the compiler optimize out a whole CRC checksum because it figured out that if i call the crc calculation function with these certain arguments, then the function ends up just computing something purely from flash memory and so the result will be the same every time. Seeing an excellent optimization opportunity the compiler went and calculated what the result of that function would be and the whole function was replaced by "return 0x75657678;" Obviously this defeats the whole point of a CRC check. Easy fix is to just define the target array as volatile in the function.

There are many ways to skin a cat here. The best way just depends on what kind of cat you are skinning. Some of the particularly simple examples of letting an interrupt pump data to a main thread is as easy as a circular buffer and a few volatile keywords (as soon as you directly write interrupt service routines your code is machine specific anyway). Other examples of a OS running on a x86 machine with quad socket Xeon CPUs and you want threads running on cores located on two physically separate CPUs to play together you got a whole new kettle of fish, since here you now need architecture specific memory barriers to inform the CPU to not only write the new value out to memory but also do the whole cache invalidation dance across multiple CPU chips, so the special x86 instructions for doing that have to be placed by the compiler.

Just use whatever fits the job. Don't over-complicate things for when the simple solution works fine.

[tggzzz]

OK, but if somebody has written

char fred [256];

and the compiler "somehow" determines that only first 10 bytes actually get accessed, e.g. because the only reference to fred that it finds is

Code: [Select]

for (i=0;i<10;i++)
  fred[i]=0;
would it not be monumentally dumb to remove the other 246 bytes of fred and silently replace that definition with

char fred[10];

I know nothing about the C language definition (which seems to be a virtual machine) but that would be a totally perverted thing to do.

Apart from anything else, it would prevent you declaring a buffer bigger than you need, because the compiler might spot the excess.

That's an amusing analogy

Welcome to the wonderful world of undefined behaviour in C. Google C "nasal demons" for the start of an interesting trip.

If there is undefined behaviour in a C program, then the entire program has no meaning. That's not completely unreasonable given the effects that can occur with incorrect pointer arithmetic. If the program has no meaning, it can do anything, and there is nothing to prevent that being "reformat the disc" or "cause dæmons to fly out of your nose". But there are many many things that are undefined behaviour in C, e.g. division by zero or adding 1 to the largest representable integer. Some compilers are better at detecting undefined behaviour than programmers, and take full advantage of that to, for example, remove entire blocks of code. Others just let the program execute the exploit code that has been "placed just beyond the end of the buffer"

https://blog.regehr.org/archives/213

https://markshroyer.com/2012/06/c-both-true-and-false/

The language lawyers tend to claim undefined behaviour is a strength, and that programmers are faulty when they invoke it. That's right from one point of view , but is it beneficial?

[wek]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

That's because "abstraction" also means "less control" (as in micro*control*ler). It's almost exactly the "silver bullet" from my earlier posts.

In the particular case we are talking about here, i.e. ring buffer on a CM4, stdatomic probably stores/disables/restores interrupts globally (didn't try but how else?). That introduces a couple of extra instructions, but also pipeline flushes and dozens of cycles of latency for all interrupts, versus streamline execution and no extra latency.

This is always the game, comfort versus control. Not all mcu applications are control-oriented, though, so in many cases you can get away with handwavings.

JW

[tggzzz]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

That's because "abstraction" also means "less control" (as in micro*control*ler). It's almost exactly the "silver bullet" from my earlier posts.

In the particular case we are talking about here, i.e. ring buffer on a CM4, stdatomic probably stores/disables/restores interrupts globally (didn't try but how else?). That introduces a couple of extra instructions, but also pipeline flushes and dozens of cycles of latency for all interrupts, versus streamline execution and no extra latency.

This is always the game, comfort versus control. Not all mcu applications are control-oriented, though, so in many cases you can get away with handwavings.

JW

I am aware that you are unlikely to be a native English speaker.

"Abstraction" does not nean "less control". It means something like "more general" and "wider applicability".

The game is not usefully characterised as "comfort vs control". Better charactisations are "predictable vs chaotic", or "right/correct vs wrong/incorrect".

As someone (who?) once noted something to the effect that "if I had realised that it was acceptable for me to create a program that didn't work, then I could have given you the program much sooner and it would run much faster".

[wek]

OK, but if somebody has written

char fred [256];

and the compiler "somehow" determines that only first 10 bytes actually get accessed, e.g. because the only reference to fred that it finds is

Code: [Select]

for (i=0;i<10;i++)
  fred[i]=0;
would it not be monumentally dumb to remove the other 246 bytes of fred and silently replace that definition with

char fred[10];

I know nothing about the C language definition (which seems to be a virtual machine) but that would be a totally perverted thing to do.

Apart from anything else, it would prevent you declaring a buffer bigger than you need, because the compiler might spot the excess.

And why would you declare a buffer bigger than you need? If you do that, surely there's at least one occasion where you access the highermost element too?

But rest assured, this type of optimization is highly unlikely to happen in C. The reason may be somewhat surprising - it's because there are no arrays in C.

More precisely, there *are* array *declarators* (i.e. you can tell the compiler "allocate me memory for 256 chars and give me it's address, let's call it fred") but there are no array-member-specific operations. All fred[ii] (and, if you want to hear about being perverse, equally ii[fred]) occurences are immediately converted into *(fred + ii) and then used as such. And any pointer into the array shall be dereferenced correctly (provided correct target type and alignment), regardless of whether fred was used in its value's calculation or not. In other words, array members are accessed through pointers, so learning which part of array is redundant means the compiler needs to compute *all* pointers' values across the whole lifetime of the program, which is hard, unrewarding and it's unlikely generally available compilers will ever attempt to do this  (although maybe the "omniscient" Hitech compiler (targeting originally 8-bit PIC and 8051, maybe still surviving as XC-8?) did it).

JW

[NorthGuy]

"Abstraction" does not nean "less control". It means something like "more general" and "wider applicability".

"Abstract" means "Detached" (from reality). For example, C standard is specified for an abstract machine which doesn't exist anywhere. This allows you program in C without knowing where your code is going to be executed.

[wek]

I am aware that you are unlikely to be a native English speaker.

 

That's because "abstraction" also means "less control" (as in micro*control*ler).

"Abstraction" does not nean "less control". It means something like "more general" and "wider applicability".

So let me try again:

"Abstraction" almost always implies "less control" (as in micro*control*ler).

More better? ;-)

JW

[SiliconWizard]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

Yeah. I've used that to implement a message queue library. You also get C threads in C11.

But there is no surprise there. Many people writing C do not even use C99 features yet. Maybe in 2100 will C11 become popular.

[tggzzz]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

Yeah. I've used that to implement a message queue library. You also get C threads in C11.

But there is no surprise there. Many people writing C do not even use C99 features yet. Maybe in 2100 will C11 become popular.

Just like COBOL66   

[peter-h]

Can I just ask some specific questions:

Is either of these methods ok? I know the 2nd option is, but I haven't yet sorted out a "DI/EI"

Code: [Select]

// Copy the data to the circular buffer
port--; // offset from 0
for (uint16_t i = 0; i < length; i++)
{
#if 1
uint32_t 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;
#else
DI();
serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]++] = data[i];
serial_transmit_buffer_put_index[port] %= SERIAL_TX_BUFFER_SIZE;
EI();
#endif
}

Re disabling all ints, I see this in cmsis_gcc.h in my project

Code: [Select]


/**
  \brief   Enable IRQ Interrupts
  \details Enables IRQ interrupts by clearing the I-bit in the CPSR.
           Can only be executed in Privileged modes.
 */
__STATIC_FORCEINLINE void __enable_irq(void)
{
  __ASM volatile ("cpsie i" : : : "memory");
}


/**
  \brief   Disable IRQ Interrupts
  \details Disables IRQ interrupts by setting the I-bit in the CPSR.
           Can only be executed in Privileged modes.
 */
__STATIC_FORCEINLINE void __disable_irq(void)
{
  __ASM volatile ("cpsid i" : : : "memory");
}


and it obviously compiles. Is there any downside to using these, within code where interrupts are normally enabled?

I have given up trying to construct DI/EI with portENTER_CRITICAL() and portEXIT_CRITICAL() by setting configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY=1. It ought to work if none of your interrupt sources have a priority of 0 but it broke a lot of stuff in a way so bizzare I could not even step through it, and it wasted a couple of days of my time.

[SiliconWizard]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

Yeah. I've used that to implement a message queue library. You also get C threads in C11.

But there is no surprise there. Many people writing C do not even use C99 features yet. Maybe in 2100 will C11 become popular.

Just like COBOL66   

COBOL666

[tggzzz]

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

Sure. I'm just surprised that people rather use volatile or proprietary barrier functions, although C11 provides an abstract (machine-independent) memory ordering model via the functions in stdatomic.h.

Yeah. I've used that to implement a message queue library. You also get C threads in C11.

But there is no surprise there. Many people writing C do not even use C99 features yet. Maybe in 2100 will C11 become popular.

Just like COBOL66   

COBOL666

OK, OK so the autocorrupt on this fondleslab may not have Coral66 in its dictionary.

Or maybe it does, and this is a long winded circuitous way of me reminding myself to understand how that language supported fixed point arithmetic and real-time behaviour.

Or maybe C++666 really is the answer to the question.

[uer166]

By and large priority levels cause at least as many problems as they address.

Same can be said about interrupts. I normally use NVIC to its' full extent and keep the inter-ISR comms atomic flags, but if you really don't want to have any issue with data consistency, locks, inversions, etc etc etc, use a cooperative scheduler without any interrupts. Of course that front-loads the scheduling and re-synchronization efforts, but hey life's hard.

[SiliconWizard]

Or maybe C++666 really is the answer to the question.

C++ is already evil.

[tggzzz]

By and large priority levels cause at least as many problems as they address.

Same can be said about interrupts. I normally use NVIC to its' full extent and keep the inter-ISR comms atomic flags, but if you really don't want to have any issue with data consistency, locks, inversions, etc etc etc, use a cooperative scheduler without any interrupts. Of course that front-loads the scheduling and re-synchronization efforts, but hey life's hard.

I don't disagree, but then I'm a fan of xC on xCORE

[tggzzz]

Or maybe C++666 really is the answer to the question.

C++ is already evil.

If C++ is the answer then you need to "revisit" the question

[SiliconWizard]

I would rather have questions that can’t be answered than answers that can’t be questioned.

- Richard Feynman

[tggzzz]

I would rather have questions that can’t be answered than answers that can’t be questioned.

- Richard Feynman

In that vein, the C++ Frequently Questioned Answers [sic] seems relevant http://yosefk.com/c++fqa/

[Sauli]

Is either of these methods ok? I know the 2nd option is, but I haven't yet sorted out a "DI/EI"

Code: [Select]

// Copy the data to the circular buffer
port--; // offset from 0
for (uint16_t i = 0; i < length; i++)
{
#if 1
uint32_t 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;
#else
DI();
serial_transmit_buffer[port][serial_transmit_buffer_put_index[port]++] = data[i];
serial_transmit_buffer_put_index[port] %= SERIAL_TX_BUFFER_SIZE;
EI();
#endif
}


In your case the following code will work just fine:

Code: [Select]

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

The code reading the data from the buffer does not need to know that there is new data in the buffer until it is all there.