Atomicity locking.

[feilipu]

For AVR 8 bit you need to introduce some atomicity locking of the ring buffer. Otherwise subtle runtime nightmares...

Take a look at Dean Camera's solution in LUFA "ringBuffer.h"using the AVR libc tools in <util/atomic.h> as the right way to provide atomic protection.

Cheers, Phillip

[wizard97]

Thanks for reviewing my code, and pointing that out!

I forgot to account for conditions where multiple interrupts are modifying the buffer. Or conditions where you are in the middle modifying the buffer, and an interrupt fires that also modifies the buffer.

However, looking at the code you referenced (https://github.com/abcminiuser/lufa/blob/master/LUFA/Drivers/Misc/RingBuffer.h), it seems in the current implementation, any routine that is modifying the buffer in a non-thread safe way simply disables interrupts. I don't quite understand what you mean by using "AVR libc tools." I don't see any sort of mutex or or atomic protection. Can you explain what you mean a bit more?

Thanks, Aaron

[feilipu]

The AVR is a simple beast, and the only tool to provide atomicity is the "hammer" of a global interrupt disable. That said, it is important to minimise the use of the hammer so that important interrupts are not delayed.

All the implementations of providing atomicity in AVR simply store the SREG and disable the global interrupt with cli();, then recover the stored SREG value on exit to restore the former global interrupt state. The guys who write the AVR-libc have tried to do it a little more user friendly by writing macros using curly braces and indenting, to ensure that matching "disable - enable" pairs are inherent. Therefore, I like to use their standard implementation found by using #include <util/atomic.h>.

Simply put anything which needs to be atomic inside the macro...

uint16_t ctr, ctr_copy;
ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
{
 ctr_copy = ctr; // this code needs to be atomic in nature.
}

This is an older version of Dean's RingBuffer.h code (before he abstracted beyond AVR), that shows the use of the AVR-libc tools for atomicity protection. In his current LUFA code, he's changed his implementation around again. And, hence I've created confusion. Sorry.

[wizard97]

Thanks for your quick reply.

Basically, I just need to copy SREG, disable interrupts, handle the atomic part of the function call, then restore my earlier copy of SREG.

That is an incredible macro. I am not to great with macros, so I would love to see how the macro is implemented. Somehow it must paste in a block of code before and after those curly braces. I didn't know there was a way to do that.

Anyways, I was really trying to make my library as portable as possible. I would have to limit my library to AVR's if I used that macro. Do you know of any portable way to copy the state of the interrupt register? I know there is a portable way to disable interrupts with noInterrupts(). Looking in the Arduino.h for AVR's you can see noInterrupts() is just cli() #define noInterrupts() cli(). I would also need a portable way to restore the state of the register. Any ideas?

If it is to complicated, I just I can always just limit my library to AVR.

Thanks for your help, Aaron

[feilipu]

It is a joy to read an expert's code, and the avr-libc implementation is just that.

Here's a version of the macro. They pack everything into a for(;;); construction to enable the curly bracket indentation with no following code, and pack the SREG storage declaration into the first element.

The only important difference from the Arduino implementation is the use of __asm__ volatile ("" ::: "memory"); to ensure that there is a memory barrier, preventing the compiler (GCC both AVR & ARM) from re-ordering the instructions. But now there's no reason why you can't add this code into your implementation either.

Have another look at how Dean does it in 2015 LUFA. That is likely to be as portable as it comes.

Good luck.

[wizard97]

It is quite a hack of a for loop.

I don't quite understand what you mean in your third paragraph. The Arduino implementation of what?

One thing I still don't quite understand in any of these implementations for atomic protection, is the case where an interrupt fires in between where the program copies the status of SREG and disables interrupts. If this interrupt modifies SREG, later when SREG is restored from the copy, it will be incorrect. Doesn't this issue exist in every implementation?

Best, Aaron

[feilipu]

By Arduino implementation, I meant all of the normal implementations using an exposed uint8_t temp = SREG; cli(); at the start, and a SREG = temp; at the end. Generally they all leave the memory barrier out and that could mean that the compiler optimiser may place the cli(); before the SREG store.

The SREG register holds only one important bit, the Global Interrupt. All the other bits are just arithmetic flags and are irrelevant.

If the interrupt fires when the Global Interrupt bit is off, then it will be missed. Hence the need to keep this interrupt off period as short as possible, in the hope you don't miss the interrupt.

The exception is where an Interrupt Flag bit exists, separately from the Interrupt Enable bit for the event. Then, if the Flag is set during the Global Interrupt off period, I think (shaky ground here) that an interrupt will be triggered when the Global Interrupt bit is re-enabled, which leads only to a delayed interrupt. This might just mean a missed character on the USART for example.

The only outcome is to keep the period during which the cli(); is in effect as short as possible.

Cheers, Phillip

[wizard97]

I see. Thanks for explaining, I am just not too familiar with asm.

While I don't think you completely understood my question, you indirectly answered it. My question was in a case such as the following.

tmp = SREG;
// What if interrupt fires here and changes SREG?
cli();
SREG = tmp;

After doing a bit of research, and reading you answer I now understand. For an interrupt to fire in-between those two lines, the global interrupt bit must be set in SREG. Regardless of what you do to this bit in SREG (even if you call cli()), the compiler will always automatically call sei() right before the ISR returns. Therefore, tmp will never be incorrect.

Thanks again for your help, my library is now available on the Arduino library manager!

Best, Aaron