Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix data race when rendering an LCD display #507

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion examples/board_hd44780/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
47 changes: 47 additions & 0 deletions examples/board_hd44780/atmega48_fps_test.c
Original file line number Diff line number Diff line change
@@ -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++;
}
}
10 changes: 8 additions & 2 deletions examples/board_hd44780/charlcd.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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++) {
Expand Down
32 changes: 18 additions & 14 deletions examples/parts/hd44780.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 !
Expand All @@ -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++;
Expand All @@ -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;
}
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
65 changes: 58 additions & 7 deletions examples/parts/hd44780.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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
Expand Down Expand Up @@ -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
48 changes: 44 additions & 4 deletions examples/parts/hd44780_glut.c
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Loading