Ringbuffer - how do I identify "crititcal" sections

[leomar01 8]

Hello,

 

the last two days I've implemented a portable ringbuffer for use with UART, SPI or even different microcontrollers (minor changes required). 

I grabbed some ideas from here and there and mixed it all together in my own solution. 

I tested the functionality with some code that writes in unregular time distances into the buffer while the SPI TX interrupt is reading from it.

It _seems_ to work well, but a friend of mine told me I definately have to deactivate the interrupt in "critical sections" of my code.

Now I'm a little bit stuck. How do I identify what a critical section is? I don't want you guys to review my code. I just wanted to ask if there's a general approach on identifying such possible problems. My friend also said I should draw a timing diagram on what's going on, but again, I have no idea how you do that.

Any advice?

 

Best regards,

Leo

[greeeg 460]

Without seeing exactly how it is implemented it is a little hard to comment on exactly where.

 

But you need to be careful of situations like this:

AddItemToBuffer(int c)
{
   increment index
   <------- interrupt happens now ------->
   Add data to index position
}

If the interrupt then takes data out and alters the index you have a problem.

 

 

Also be wary of things like this

*index++ = newDataForBuffer

Because this is actually compiled as more than 1 instruction. and an interrupt might happen during its execution.

 

I don't know if you've already tried reading up on the topic. Wikipedia has a decent article on it. (http://en.wikipedia.org/wiki/Critical_section)

[leomar01 8]

I read that wikipedia article - I just can't make the transition to my actual case

 

I'm now adding my current code (please excuse the german comments).

 

First I have a layer that abstracts the ringbuffer logic:

ringbuffer.h:

#ifndef RINGBUFFER_H_#define RINGBUFFER_H_#define BUFFERLENGTH 64 // Must be 2^n#define BUFFER_MASK (BUFFERLENGTH-1) // Klammern auf keinen Fall vergessentypedef struct ringbuffer{	unsigned char ringbuffer[BUFFERLENGTH];	unsigned int index_reading; // zeigt auf das Feld das zuletzt gelesen wurde	unsigned int index_writing; // zeigt auf das Feld in welches das n

[igor 163]

Have you checked your code carefully for how it handles buffer empty and buffer full conditions?

(e.g. what happens if you initialize it and then call BufferOut) - looks like the code may fail.

Also looks like you may be reserving more unused bytes than you need in the buffer almost full condition.

 

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

 

Sorry, not an answer to your question.

[leomar01 8]

Hello @@igor,

 

up until now I only used it to send data over a peripheral like SPI or UART. In this case only the ISR triggers BufferOut(). If the buffer is empty no new value gets written in the output buffer register of the peripheral. Thereby no ISR will fire anymore and the two indices will remain the same. Thats the reason why BufferIn() checks if the two indices were the same and if so, writes the current char directly to the register of the peripheral. If only one char has been sent to the buffer the ISR again will do nothing except increasing the reading index the thereby leading to same indices. 

 

If on the other hand the buffer is full, BufferIn() will discard the char. At this point actually on char of the buffer gets wasted if I'm not mistaken

 

Thanks for the wiki link, I've read it in the beginning, but maybe it's worth a reread with my in the meantime gained knowledge

 

Leo

[igor 163]

You might consider implementing it in a more encapsulated fashion, providing the usual access functions (including test for buffer empty and buffer full), and documenting the limitations (e.g. that must check that buffer is not empty before calling BufferOut, as

well as whatever the intended handling of the first byte written (looks like it gets thrown away?)).

[leomar01 8]

Hello @@igor,

 

I had another night of trial and error

As you correctly pointed out, the first byte got discarded by BufferOut. That was my solution to overcome the problem caused by directly transmitting the first byte in buffered_send_SPI.

 

I have corrected this behaviour and again, had the problem of first byte getting sent double. Then I discovered the UCBUSY bit in UCxSTAT. Instead of directly sending the first byte to get the peripheral running...

	else if(in == send_toSPI_buffer.index_reading)
	{
		//IFG2 |= UCB0TXIFG;
		UCB0TXBUF = ch;
	}

... I fired an artificial interrupt to get things going:

	if(!(UCA0STAT & UCBUSY))
	{
		// ...initiate an artificial interrupt to get things going
		IFG2 |= UCB0TXIFG;
	}

This seemed to work well until I tested this ISR:

#pragma vector=USCIAB0RX_VECTOR
__interrupt void USCI0RX_ISR_HOOK(void)
{
    /* USER CODE START (section: USCI0RX_ISR_HOOK) */
	DEBUGPIN1_HIGH
	unsigned char receivedByte;
		// UART RX Interrupt
	    if (IFG2 & UCA0RXIFG) {
	    	receivedByte = UCA0RXBUF;
	    	buffered_send_byte_over_UART(receivedByte);
	        buffered_send_byte_over_SPI(receivedByte);
	        IFG2 &= ~UCA0RXIFG;
	    }
}

Main thing in this example is: The through UART received char gets directly transmitted through that whole ringbuffer logic.

 

The result looked like this:

 

 

Sometimes it even looked more weird:

 

 

This happens because UCBUSY is 1 while the peripheral is TXing OR RXing .... 

 

This led me to add another variable to the RINGBUFFER struct to indicate if currently a transmission is running:

typedef struct ringbuffer{
	unsigned char ringbuffer[BUFFERLENGTH];
	unsigned int index_reading; // indicates the field to be read next
	unsigned int index_writing; // indicates field to be written to next
	bool ringbuffer_running;    // indicates if output peripheral is allready running
}RINGBUFFER; 

The corresponding part in buffered_send_SPI changed to this (here I think is the first time I clearly have to disable the interrupts):

	if(!send_toSPI_buffer.ringbuffer_running)
	{
		// ...initiate an artificial interrupt to get things going
		IFG2 |= UCB0TXIFG;
		send_toSPI_buffer.ringbuffer_running = true;
	}

In BufferOut it now looks like this:

	if(buffer->index_reading == buffer->index_writing)
	{
		buffer->ringbuffer_running = false;
		// buffer is empty, nothing to read
		return 1;
	}

Now I get the intended behaviour:

 

 

Now the ringbuffer logic is working with a interrupt triggered peripheral and also "standalone" in code.

 

As you mentioned I've also added a few accompanying functions:

unsigned char CountFreeBytesInBuffer(RINGBUFFER *buffer);
bool IsBufferEmpty(RINGBUFFER *buffer);
int printstrBuffer(unsigned char *str, RINGBUFFER *buffer);
int ReadBuffer(unsigned char *dest, RINGBUFFER *buffer);

I'm getting towards a more portable solution ... as I intended in first place

 

Comments and suggestions welcome

 

Regards

Leo

 

PS: I've attached source files

ringbuffer.c

ringbuffer.h

types.h

SPI.c

SPI.h

UART.c

InterruptVectors_init_clean.c

[zlalanne 37]

Just as another datapoint if you want to look at some code, Dr. Valvano from UT Austin (who ran the online Stellaris course) has all his code online and has a FIFO implementation worth looking at. Just search for "FIFO" on that page and you will see the files.

 

http://users.ece.utexas.edu/~valvano/arm/

 

It should be easily adaptable to MSP430.

[leomar01 8]

Thanks @@zlalanne, actually he has a lot of fifo implementations, most of them are hard to understand to me, but here's one that's pretty close to mine: http://users.ece.utexas.edu/~valvano/arm/FIFO.c

 

I think I'll leave my implementation like it is ... until I discover a new problem ;-)

 

Leo

[RobG 1,892]

I want to add one thing, you should disable interrupts when manipulating buffer and updating it's state.

[leomar01 8]

Thanks @@RobG, I added that throughout my library. 

 

Up until now I'm happy with my ringbuffer implementation. Didn't encounter any issues by now

 

Leo