Merge pull request #507 from juhdanad/hd44780-opengl-mutex

Fix data race when rendering an LCD display

Author: gatk555

examples/board_hd44780/Makefile

@@ -33,9 +33,10 @@ LDFLAGS += -lpthread
 
 include ../Makefile.opengl
 
-all: obj atmega48_charlcd.axf ${target} 
+all: obj atmega48_charlcd.axf atmega48_fps_test.axf ${target} 
 
 atmega48_charlcd.axf: atmega48_charlcd.c
+atmega48_fps_test.axf: atmega48_fps_test.c
 
 include ${simavr}/Makefile.common
 

examples/board_hd44780/atmega48_fps_test.c

@@ -0,0 +1,47 @@
+#undef F_CPU
+#define F_CPU 16000000
+
+#include <avr/io.h>
+
+#include "avr_mcu_section.h"
+AVR_MCU(F_CPU, "atmega48");
+
+#include "avr_hd44780.c"
+
+int main()
+{
+	hd44780_init();
+	/*
+	 * Clear the display.
+	 */
+	hd44780_outcmd(HD44780_CLR);
+	hd44780_wait_ready(1); // long wait
+
+	/*
+	 * Entry mode: auto-increment address counter, no display shift in
+	 * effect.
+	 */
+	hd44780_outcmd(HD44780_ENTMODE(1, 0));
+	hd44780_wait_ready(0);
+
+	/*
+	 * Enable display, activate non-blinking cursor.
+	 */
+	hd44780_outcmd(HD44780_DISPCTL(1, 1, 0));
+	hd44780_wait_ready(0);
+
+	uint16_t count = 0;
+	while (1)
+	{
+		uint16_t temp = count;
+		for (uint8_t i = 5; i > 0; i--)
+		{
+			hd44780_outcmd(HD44780_DDADDR(i - 1));
+			hd44780_wait_ready(0);
+			hd44780_outdata(temp % 10 + 48);
+			temp /= 10;
+			hd44780_wait_ready(0);
+		}
+		count++;
+	}
+}

examples/board_hd44780/charlcd.c

@@ -146,11 +146,15 @@ main(
 		char *argv[])
 {
 	elf_firmware_t f = {{0}};
-	const char * fname = "atmega48_charlcd.axf";
+	const char *fname = argc > 1 ? argv[1] : "atmega48_charlcd.axf";
 //	char path[256];
 //	sprintf(path, "%s/%s", dirname(argv[0]), fname);
 //	printf("Firmware pathname is %s\n", path);
-	elf_read_firmware(fname, &f);
+	if (elf_read_firmware(fname, &f) == -1)
+	{
+		fprintf(stderr, "Unable to load firmware from file %s\n", fname);
+		exit(EXIT_FAILURE);
+	};
 
 	printf("firmware %s f=%d mmcu=%s\n", fname, (int) f.frequency, f.mmcu);
 
@@ -168,6 +172,8 @@ main(
 
 	hd44780_init(avr, &hd44780, 20, 4);
 
+	hd44780_setup_mutex_for_gl(&hd44780);
+
 	/* Connect Data Lines to Port B, 0-3 */
 	/* These are bidirectional too */
 	for (int i = 0; i < 4; i++) {

examples/parts/hd44780.c

@@ -76,7 +76,7 @@ _hd44780_busy_timer(
 {
 	hd44780_t *b = (hd44780_t *) param;
 //	printf("%s called\n", __FUNCTION__);
-	hd44780_set_flag(b, HD44780_FLAG_BUSY, 0);
+	hd44780_set_private_flag(b, HD44780_PRIV_FLAG_BUSY, 0);
 	avr_raise_irq(b->irq + IRQ_HD44780_BUSY, 0);
 	return 0;
 }
@@ -189,7 +189,7 @@ hd44780_write_command(
 			hd44780_set_flag(b, HD44780_FLAG_F, b->datapins & 4);
 			if (!four && !hd44780_get_flag(b, HD44780_FLAG_D_L)) {
 				printf("%s activating 4 bits mode\n", __FUNCTION__);
-				hd44780_set_flag(b, HD44780_FLAG_LOWNIBBLE, 0);
+				hd44780_set_private_flag(b, HD44780_PRIV_FLAG_LOWNIBBLE, 0);
 			}
 		}	break;
 		// Cursor display shift
@@ -231,7 +231,7 @@ hd44780_process_write(
 {
 	uint32_t delay = 0; // uS
 	int four = !hd44780_get_flag(b, HD44780_FLAG_D_L);
-	int comp = four && hd44780_get_flag(b, HD44780_FLAG_LOWNIBBLE);
+	int comp = four && hd44780_get_private_flag(b, HD44780_PRIV_FLAG_LOWNIBBLE);
 	int write = 0;
 
 	if (four) { // 4 bits !
@@ -240,7 +240,7 @@ hd44780_process_write(
 		else
 			b->datapins = (b->datapins & 0xf) | ((b->pinstate >>  (IRQ_HD44780_D4-4)) & 0xf0);
 		write = comp;
-		b->flags ^= (1 << HD44780_FLAG_LOWNIBBLE);
+		b->private_flags ^= (1 << HD44780_PRIV_FLAG_LOWNIBBLE);
 	} else {	// 8 bits
 		b->datapins = (b->pinstate >>  IRQ_HD44780_D0) & 0xff;
 		write++;
@@ -249,13 +249,15 @@ hd44780_process_write(
 
 	// write has 8 bits to process
 	if (write) {
-		if (hd44780_get_flag(b, HD44780_FLAG_BUSY)) {
+		if (hd44780_get_private_flag(b, HD44780_PRIV_FLAG_BUSY)) {
 			printf("%s command %02x write when still BUSY\n", __FUNCTION__, b->datapins);
 		}
+		hd44780_lock_state(b);
 		if (b->pinstate & (1 << IRQ_HD44780_RS))	// write data
 			delay = hd44780_write_data(b);
 		else										// write command
 			delay = hd44780_write_command(b);
+		hd44780_unlock_state(b);
 	}
 	return delay;
 }
@@ -266,42 +268,44 @@ hd44780_process_read(
 {
 	uint32_t delay = 0; // uS
 	int four = !hd44780_get_flag(b, HD44780_FLAG_D_L);
-	int comp = four && hd44780_get_flag(b, HD44780_FLAG_LOWNIBBLE);
+	int comp = four && hd44780_get_private_flag(b, HD44780_PRIV_FLAG_LOWNIBBLE);
 	int done = 0;	// has something on the datapin we want
 
 	if (comp) {
 		// ready the 4 final bits on the 'actual' lcd pins
 		b->readpins <<= 4;
 		done++;
-		b->flags ^= (1 << HD44780_FLAG_LOWNIBBLE);
+		b->private_flags ^= (1 << HD44780_PRIV_FLAG_LOWNIBBLE);
 	}
 
 	if (!done) { // new read
 
 		if (b->pinstate & (1 << IRQ_HD44780_RS)) {	// read data
 			delay = 37;
 			b->readpins = b->vram[b->cursor];
+			hd44780_lock_state(b);
 			hd44780_kick_cursor(b);
+			hd44780_unlock_state(b);
 		} else {	// read 'command' ie status register
 			delay = 0;	// no raising busy when reading busy !
 
 			// low bits are the current cursor
 			b->readpins = b->cursor < 0x80 ? b->cursor : b->cursor-0x80;
-			int busy = hd44780_get_flag(b, HD44780_FLAG_BUSY);
+			int busy = hd44780_get_private_flag(b, HD44780_PRIV_FLAG_BUSY);
 			b->readpins |= busy ? 0x80 : 0;
 
 		//	if (busy) printf("Good boy, guy's reading status byte\n");
 			// now that we're read the busy flag, clear it and clear
 			// the timer too
-			hd44780_set_flag(b, HD44780_FLAG_BUSY, 0);
+			hd44780_set_private_flag(b, HD44780_PRIV_FLAG_BUSY, 0);
 			avr_raise_irq(b->irq + IRQ_HD44780_BUSY, 0);
 			avr_cycle_timer_cancel(b->avr, _hd44780_busy_timer, b);
 		}
 		avr_raise_irq(b->irq + IRQ_HD44780_DATA_OUT, b->readpins);
 
 		done++;
 		if (four)
-			b->flags |= (1 << HD44780_FLAG_LOWNIBBLE); // for next read
+			b->private_flags |= (1 << HD44780_PRIV_FLAG_LOWNIBBLE); // for next read
 	}
 
 	// now send the prepared output pins to send as IRQs
@@ -320,7 +324,7 @@ _hd44780_process_e_pinchange(
 {
 	hd44780_t *b = (hd44780_t *) param;
 
-	hd44780_set_flag(b, HD44780_FLAG_REENTRANT, 1);
+	hd44780_set_private_flag(b, HD44780_PRIV_FLAG_REENTRANT, 1);
 
 #if 0
 	uint16_t touch = b->oldstate ^ b->pinstate;
@@ -338,13 +342,13 @@ _hd44780_process_e_pinchange(
 		delay = hd44780_process_write(b);
 
 	if (delay) {
-		hd44780_set_flag(b, HD44780_FLAG_BUSY, 1);
+		hd44780_set_private_flag(b, HD44780_PRIV_FLAG_BUSY, 1);
 		avr_raise_irq(b->irq + IRQ_HD44780_BUSY, 1);
 		avr_cycle_timer_register_usec(b->avr, delay,
 			_hd44780_busy_timer, b);
 	}
 //	b->oldstate = b->pinstate;
-	hd44780_set_flag(b, HD44780_FLAG_REENTRANT, 0);
+	hd44780_set_private_flag(b, HD44780_PRIV_FLAG_REENTRANT, 0);
 	return 0;
 }
 
@@ -373,7 +377,7 @@ hd44780_pin_changed_hook(
 			return; // job already done!
 		case IRQ_HD44780_D0 ... IRQ_HD44780_D7:
 			// don't update these pins in read mode
-			if (hd44780_get_flag(b, HD44780_FLAG_REENTRANT))
+			if (hd44780_get_private_flag(b, HD44780_PRIV_FLAG_REENTRANT))
 				return;
 			break;
 	}

examples/parts/hd44780.h

@@ -79,17 +79,23 @@ enum {
     HD44780_FLAG_S,             // 1: Follow display shift
     HD44780_FLAG_I_D,           // 1: Increment, 0: Decrement
 
-    /*
-     * Internal flags, not HD44780
-     */
-    HD44780_FLAG_LOWNIBBLE,		// 1: 4 bits mode, write/read low nibble
-    HD44780_FLAG_BUSY,			// 1: Busy between instructions, 0: ready
-    HD44780_FLAG_REENTRANT,		// 1: Do not update pins
-
+	/*
+	 * Internal flags, not HD44780
+	 */
     HD44780_FLAG_DIRTY,			// 1: needs redisplay...
     HD44780_FLAG_CRAM_DIRTY,	// 1: Character memory has changed
 };
 
+/*
+ * Private internal flags. These are not protected by
+ * callbacks, so other threads must not access them.
+ */
+enum {
+    HD44780_PRIV_FLAG_BUSY = 0,			// 1: Busy between instructions, 0: ready
+    HD44780_PRIV_FLAG_LOWNIBBLE,		// 1: 4 bits mode, write/read low nibble
+    HD44780_PRIV_FLAG_REENTRANT,		// 1: Do not update pins
+};
+
 
 typedef struct hd44780_t
 {
@@ -106,6 +112,21 @@ typedef struct hd44780_t
 	uint8_t  readpins;
 
 	uint16_t flags;				// LCD flags ( HD44780_FLAG_*)
+	// LCD private flags, not protected by callbacks ( HD44780_PRIV_FLAG_*)
+	// You must not use these flags from a different thread than the simavr one.
+	uint8_t private_flags;
+
+	// These callbacks are called before and after a data or
+	// command packet is sent to the unit and as a 
+	// result the internal state of the device changes.
+	// If you lock and unlock a mutex in these, you can guard
+	// the cursor, vram and flags variables with it.
+	// The avr thread reads these variables outside
+	// of these functions, but writes always happen between them.
+	void *on_state_lock_parameter;
+	void (*on_state_lock)(void *);
+	void *on_state_unlock_parameter;
+	void (*on_state_unlock)(void *);
 } hd44780_t;
 
 void
@@ -134,4 +155,34 @@ hd44780_get_flag(
 	return (b->flags &  (1 << bit)) != 0;
 }
 
+static inline int
+hd44780_set_private_flag(
+		hd44780_t *b, uint16_t bit, int val)
+{
+	int old = b->private_flags &  (1 << bit);
+	b->private_flags = (b->private_flags & ~(1 << bit)) | (val ? (1 << bit) : 0);
+	return old != 0;
+}
+
+static inline int
+hd44780_get_private_flag(
+		hd44780_t *b, uint16_t bit)
+{
+	return (b->private_flags &  (1 << bit)) != 0;
+}
+
+static inline void
+hd44780_lock_state(hd44780_t *b)
+{
+	if (b->on_state_lock)
+		(*(b->on_state_lock))(b->on_state_lock_parameter);
+}
+
+static inline void
+hd44780_unlock_state(hd44780_t *b)
+{
+	if (b->on_state_unlock)
+		(*(b->on_state_unlock))(b->on_state_unlock_parameter);
+}
+
 #endif

examples/parts/hd44780_glut.c

@@ -27,13 +27,35 @@
 #else
 #include <GL/glut.h>
 #endif
+#include <pthread.h>
+#include <stdio.h>
 
 #include "lcd_font.h"	// generated with gimp
 
 static GLuint font_texture;
 static int charwidth = 5;
 static int charheight = 7;
 
+static pthread_mutex_t hd44780_state_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+void before_state_lock_cb(void *b)
+{
+	pthread_mutex_lock(&hd44780_state_mutex);
+}
+
+void after_state_lock_cb(void *b)
+{
+	pthread_mutex_unlock(&hd44780_state_mutex);
+}
+
+void hd44780_setup_mutex_for_gl(hd44780_t *b)
+{
+	b->on_state_lock = &before_state_lock_cb;
+	b->on_state_lock_parameter = b;
+	b->on_state_unlock = &after_state_lock_cb;
+	b->on_state_unlock_parameter = b;
+}
+
 void
 hd44780_gl_init()
 {
@@ -123,6 +145,13 @@ hd44780_gl_draw(
 		uint32_t text,
 		uint32_t shadow)
 {
+	if (b->on_state_lock != &before_state_lock_cb ||
+		b->on_state_unlock != &after_state_lock_cb)
+	{
+		printf("Error: the hd44780 instance is not using the mutex of the OpenGL thread!\nCall hd44780_setup_mutex_for_gl() first!\n");
+		exit(EXIT_FAILURE);
+	}
+
 	int rows = b->w;
 	int lines = b->h;
 	int border = 3;
@@ -139,16 +168,27 @@ hd44780_gl_draw(
 	        + (lines - 1) + border, 0);
 	glEnd();
 
+	// create a local copy (so we can release the mutex as fast as possible)
+	uint8_t vram[192];
+	pthread_mutex_lock(&hd44780_state_mutex);
+	// cgram is not updated yet
+	// int cgram_dirty = hd44780_get_flag(b, HD44780_FLAG_CRAM_DIRTY);
+	for (uint8_t i = 0; i < 192; i++)
+		vram[i] = b->vram[i];
+	// the values have been seen, they are not dirty anymore
+	hd44780_set_flag(b, HD44780_FLAG_CRAM_DIRTY, 0);
+	hd44780_set_flag(b, HD44780_FLAG_DIRTY, 0);
+	pthread_mutex_unlock(&hd44780_state_mutex);
+
 	glColor3f(1.0f, 1.0f, 1.0f);
 	const uint8_t offset[] = { 0x00, 0x40, 0x00 + 20, 0x40 + 20 };
-	for (int v = 0 ; v < b->h; v++) {
+	for (int v = 0 ; v < lines; v++) {
 		glPushMatrix();
-		for (int i = 0; i < b->w; i++) {
-			glputchar(b->vram[offset[v] + i], character, text, shadow);
+		for (int i = 0; i < rows; i++) {
+			glputchar(vram[offset[v] + i], character, text, shadow);
 			glTranslatef(6, 0, 0);
 		}
 		glPopMatrix();
 		glTranslatef(0, 8, 0);
 	}
-	hd44780_set_flag(b, HD44780_FLAG_DIRTY, 0);
 }