Address issues with interrupts and the shared variables used to

manage the movebuffer (mb_head, mb_tail, movebuffer).

The approach taken is to leave all of the variables non-volatile
and use memory barriers to control reads/writes. read/modify/write
operations that can be done outside of the interrupt context are
protected by disabling interrupts.

Also, manually cache the pointer to the movebuffer of interest
as the compiler does a poor job of reusing the pointer even in
places that it could.

There are still some groups of accesses to the movebuffer data that need
to be protected by disabling interrupts, but these are all related
to sending back debug data. The error will cause occassional mismatched
values to be sent back via the serial connection, but they do not
affect the actual operation of the code, so they will be addressed
in a separate checkin.

Conflicts:

	dda_queue.c

dda_queue.c

@@ -15,33 +15,43 @@
 #include	"delay.h"
 #include	"sersendf.h"
 #include	"clock.h"
+#include	"memory_barrier.h"
 
 /// movebuffer head pointer. Points to the last move in the queue.
+/// this variable is used both in and out of interrupts, but is
+/// only written outside of interrupts.
 uint8_t	mb_head = 0;
 
 /// movebuffer tail pointer. Points to the currently executing move
+/// this variable is read/written both in and out of interrupts.
 uint8_t	mb_tail = 0;
 
 /// move buffer.
 /// holds move queue
+/// contents are read/written both in and out of interrupts, but
+/// once writing starts in interrupts on a specific slot, the
+/// slot will only be modified in interrupts until the slot is
+/// is no longer live.
 DDA movebuffer[MOVEBUFFER_SIZE] __attribute__ ((__section__ (".bss")));
 
 /// check if the queue is completely full
 uint8_t queue_full() {
-	uint8_t sreg = SREG, r;
+	MEMORY_BARRIER();
-	cli();
+	return (((mb_tail - mb_head - 1) & (MOVEBUFFER_SIZE - 1)) == 0)?255:0;
-	r = (((mb_tail - mb_head - 1) & (MOVEBUFFER_SIZE - 1)) == 0)?255:0;
-	SREG = sreg;
-	return r;
 }
 
 /// check if the queue is completely empty
 uint8_t queue_empty() {
-	uint8_t sreg = SREG, r;
+	uint8_t save_reg = SREG;
 	cli();
-	r = ((mb_tail == mb_head) && (movebuffer[mb_tail].live == 0))?255:0;
+	CLI_SEI_BUG_MEMORY_BARRIER();
-	SREG = sreg;
+	
-	return r;
+	uint8_t result = ((mb_tail == mb_head) && (movebuffer[mb_tail].live == 0))?255:0;
+
+	MEMORY_BARRIER();
+	SREG = save_reg;
+
+	return result;
 }
 
 // -------------------------------------------------------
@@ -51,11 +61,12 @@ uint8_t queue_empty() {
 /// Take a step or go to the next move.
 void queue_step() {
 	// do our next step
-	if (movebuffer[mb_tail].live) {
+	DDA* current_movebuffer = &movebuffer[mb_tail];
-		if (movebuffer[mb_tail].waitfor_temp) {
+	if (current_movebuffer->live) {
-			setTimer(movebuffer[mb_tail].c >> 8);
+		if (current_movebuffer->waitfor_temp) {
+			setTimer(current_movebuffer->c >> 8);
 			if (temp_achieved()) {
-				movebuffer[mb_tail].live = movebuffer[mb_tail].waitfor_temp = 0;
+				current_movebuffer->live = current_movebuffer->waitfor_temp = 0;
 				serial_writestr_P(PSTR("Temp achieved\n"));
 			}
 
@@ -65,18 +76,19 @@ void queue_step() {
 		}
 		else {
 			// NOTE: dda_step makes this interrupt interruptible after steps have been sent but before new speed is calculated.
-			dda_step(&(movebuffer[mb_tail]));
+			dda_step(current_movebuffer);
 		}
 	}
 
 	// fall directly into dda_start instead of waiting for another step
 	// the dda dies not directly after its last step, but when the timer fires and there's no steps to do
-	if (movebuffer[mb_tail].live == 0)
+	if (current_movebuffer->live == 0)
 		next_move();
 }
 
 /// add a move to the movebuffer
 /// \note this function waits for space to be available if necessary, check queue_full() first if waiting is a problem
+/// This is the only function that modifies mb_head and it always called from outside an interrupt.
 void enqueue(TARGET *t) {
 	// don't call this function when the queue is full, but just in case, wait for a move to complete and free up the space for the passed target
 	while (queue_full())
@@ -85,57 +97,75 @@ void enqueue(TARGET *t) {
 	uint8_t h = mb_head + 1;
 	h &= (MOVEBUFFER_SIZE - 1);
 
+	DDA* new_movebuffer = &(movebuffer[h]);
+	
 	if (t != NULL) {
-		dda_create(&movebuffer[h], t);
+		dda_create(new_movebuffer, t);
 	}
 	else {
 		// it's a wait for temp
-		movebuffer[h].waitfor_temp = 1;
+		new_movebuffer->waitfor_temp = 1;
-		movebuffer[h].nullmove = 0;
+		new_movebuffer->nullmove = 0;
 		#if (F_CPU & 0xFF000000) == 0
 			// set "step" timeout to 1 second
-			movebuffer[h].c = F_CPU << 8;
+			new_movebuffer->c = F_CPU << 8;
 		#else
 			// set "step" timeout to maximum
-			movebuffer[h].c = 0xFFFFFF00;
+			new_movebuffer->c = 0xFFFFFF00;
 		#endif
 	}
 
+	// make certain all writes to global memory
+	// are flushed before modifying mb_head.
+	MEMORY_BARRIER();
+	
 	mb_head = h;
-
+	
-	uint8_t sreg = SREG;
+	uint8_t save_reg = SREG;
 	cli();
-	// fire up in case we're not running yet
+	CLI_SEI_BUG_MEMORY_BARRIER();
-	if (movebuffer[mb_tail].live == 0) {
+
-		SREG = sreg;
+	uint8_t isdead = (movebuffer[mb_tail].live == 0);
+	
+	MEMORY_BARRIER();
+	SREG = save_reg;
+	
+	if (isdead)
 		next_move();
 	}
-	else
-		SREG = sreg;
 }
 
 /// go to the next move.
 /// be aware that this is sometimes called from interrupt context, sometimes not.
+/// Note that if it is called from outside an interrupt it must not/can not by
+/// be interrupted such that it can be re-entered from within an interrupt.
+/// The timer interrupt MUST be disabled on entry. This is ensured because
+/// the timer was disabled at the start of the ISR or else because the current
+/// move buffer was dead in the non-interrupt case (which indicates that the 
+/// timer interrupt is disabled).
 void next_move() {
-	if (queue_empty() == 0) {
+	while ((queue_empty() == 0) && (movebuffer[mb_tail].live == 0)) {
-		do {
+		// next item
-			// next item
+		uint8_t t = mb_tail + 1;
-			uint8_t t = mb_tail + 1;
+		t &= (MOVEBUFFER_SIZE - 1);
-			t &= (MOVEBUFFER_SIZE - 1);
+		DDA* current_movebuffer = &movebuffer[t];
-			if (movebuffer[t].waitfor_temp) {
+		// tail must be set before setTimer call as setTimer
-				#ifndef	REPRAP_HOST_COMPATIBILITY
+		// reenables the timer interrupt, potentially exposing
-					serial_writestr_P(PSTR("Waiting for target temp\n"));
+		// mb_tail to the timer interrupt routine. 
-				#endif
+		mb_tail = t;	
-				movebuffer[t].live = 1;
+		if (current_movebuffer->waitfor_temp) {
-				setTimer(movebuffer[t].c >> 8);
+			#ifndef	REPRAP_HOST_COMPATIBILITY
-			}
+				serial_writestr_P(PSTR("Waiting for target temp\n"));
-			else {
+			#endif
-				dda_start(&movebuffer[t]);
+			current_movebuffer->live = 1;
-			}
+			setTimer(current_movebuffer->c >> 8);
-			mb_tail = t;
+		}
-		} while ((queue_empty() == 0) && (movebuffer[mb_tail].live == 0));
+		else {
-	}
+			dda_start(current_movebuffer);
-	else
+		}
+		
+	} 
+	if (queue_empty())
 		setTimer(0);
 }
 
@@ -148,24 +178,25 @@ void print_queue() {
 /// dump queue for emergency stop.
 /// \todo effect on startpoint/current_position is undefined!
 void queue_flush() {
-	// save interrupt flag
+	// Since the timer interrupt is disabled before this function
-	uint8_t sreg = SREG;
+	// is called it is not strictly necessary to write the variables
-
+	// inside an interrupt disabled block...
-	// disable interrupts
+	uint8_t save_reg = SREG;
 	cli();
-
+	CLI_SEI_BUG_MEMORY_BARRIER();
+	
 	// flush queue
 	mb_tail = mb_head;
 	movebuffer[mb_head].live = 0;
 
 	// disable timer
 	setTimer(0);
-
+	
-	// restore interrupt flag
+	MEMORY_BARRIER();
-	SREG = sreg;
+	SREG = save_reg;
 }
 
-/// waits for a space in the queue to become available
+/// wait for queue to empty
 void queue_wait() {
 	for (;queue_empty() == 0;) {
 		ifclock(clock_flag_10ms) {