From 2af247eca4ed402078064abda29b7a14f10533cb Mon Sep 17 00:00:00 2001 From: Jon Burgess Date: Sun, 14 Sep 2014 21:39:55 +0100 Subject: [PATCH] Move mutex protection in GLCD up a layer. This fixes the occasional screen corruption seen in some of the apps. The locking helps avoid the situation where multiple tasks could call GotoXY() to update the internal position without taking a lock. This position determines what data is accessed by ReadData() and WriteData(). An invalid position could cause memory access beyond the framebuffer. Moving the locking up to the higher level drawing routines causes the mutex to be held for longer but also means the display update is more likely to be triggered after a complete drawing operation instead of in the middle. --- hardware/emfcamp/sam/libraries/GLCD/gText.cpp | 17 +++++ hardware/emfcamp/sam/libraries/GLCD/glcd.cpp | 9 +++ .../sam/libraries/GLCD/glcd_Device.cpp | 8 +++ .../emfcamp/sam/libraries/GLCD/glcd_Device.h | 9 ++- .../emfcamp/sam/libraries/GLCD/st7565.cpp | 62 ++++++++----------- 5 files changed, 66 insertions(+), 39 deletions(-) diff --git a/hardware/emfcamp/sam/libraries/GLCD/gText.cpp b/hardware/emfcamp/sam/libraries/GLCD/gText.cpp index ff3d0f9..adbcec4 100644 --- a/hardware/emfcamp/sam/libraries/GLCD/gText.cpp +++ b/hardware/emfcamp/sam/libraries/GLCD/gText.cpp @@ -286,6 +286,9 @@ void gText::ScrollUp(uint8_t x1, uint8_t y1, uint8_t x2, uint8_t y2, return; } + if (this->Acquire() != pdTRUE) + return; + for (col = x1; col <= x2; col++) { dy = y1; glcd_Device::GotoXY(col, dy & ~7); @@ -366,6 +369,7 @@ void gText::ScrollUp(uint8_t x1, uint8_t y1, uint8_t x2, uint8_t y2, this->WriteData(dbyte); } } + this->Release(); } #ifndef GLCD_NO_SCROLLDOWN @@ -390,6 +394,9 @@ void gText::ScrollDown(uint8_t x1, uint8_t y1, uint8_t x2, uint8_t y2, return; } + if (this->Acquire() != pdTRUE) + return; + /* * Process region from left to right */ @@ -467,6 +474,7 @@ void gText::ScrollDown(uint8_t x1, uint8_t y1, uint8_t x2, uint8_t y2, this->WriteData(dbyte); } } + this->Release(); } #endif // GLCD_NO_SCROLLDOWN @@ -753,6 +761,9 @@ int gText::PutChar(uint8_t c) { #ifdef GLCD_OLD_FONTDRAW /*================== OLD FONT DRAWING ============================*/ + if (this->Acquire() != pdTRUE) + return 0; + glcd_Device::GotoXY(this->x, this->y); /* @@ -805,6 +816,8 @@ int gText::PutChar(uint8_t c) { } this->x = this->x + width + 1; + this->Release(); + /*================== END of OLD FONT DRAWING ============================*/ #else @@ -839,6 +852,9 @@ int gText::PutChar(uint8_t c) { uint8_t dbyte; uint8_t fdata; + if (this->Acquire() != pdTRUE) + return 0; + for (p = 0; p < pixels;) { dy = this->y + p; @@ -1036,6 +1052,7 @@ int gText::PutChar(uint8_t c) { this->x = this->x + width + 1; + this->Release(); /*================== END of NEW FONT DRAWING ============================*/ #endif // NEW_FONTDRAW diff --git a/hardware/emfcamp/sam/libraries/GLCD/glcd.cpp b/hardware/emfcamp/sam/libraries/GLCD/glcd.cpp index a54bf34..56e662b 100644 --- a/hardware/emfcamp/sam/libraries/GLCD/glcd.cpp +++ b/hardware/emfcamp/sam/libraries/GLCD/glcd.cpp @@ -362,6 +362,9 @@ void glcd::InvertRect(uint8_t x, uint8_t y, uint8_t width, uint8_t height) { /* * First do the fractional pages at the top of the region */ + if (this->Acquire() != pdTRUE) + return; + this->GotoXY(x, y); for (i = 0; i <= width; i++) { data = this->ReadData(); @@ -398,6 +401,7 @@ void glcd::InvertRect(uint8_t x, uint8_t y, uint8_t width, uint8_t height) { this->WriteData(data); } } + this->Release(); } /** * Set LCD Display mode @@ -438,6 +442,10 @@ void glcd::DrawBitmap(Image_t bitmap, uint8_t x, uint8_t y, uint8_t color) { width = ReadPgmData(bitmap++); height = ReadPgmData(bitmap++); + + if (this->Acquire() != pdTRUE) + return; + #define BITMAP_FIX #ifdef BITMAP_FIX // temporary ifdef just to show what changes if a new // bit rendering routine is written. @@ -485,6 +493,7 @@ void glcd::DrawBitmap(Image_t bitmap, uint8_t x, uint8_t y, uint8_t color) { this->WriteData(~displayData); } } + this->Release(); } #ifdef NOTYET diff --git a/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.cpp b/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.cpp index be590e2..83c8c1c 100644 --- a/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.cpp +++ b/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.cpp @@ -60,6 +60,9 @@ void glcd_Device::SetDot(uint8_t x, uint8_t y, uint8_t color) { if ((x >= this->CurrentWidth()) || (y >= this->CurrentHeight())) return; + if (this->Acquire() != pdTRUE) + return; + this->GotoXY(x, y - y % 8); // read data from display memory data = this->ReadData(); @@ -69,6 +72,7 @@ void glcd_Device::SetDot(uint8_t x, uint8_t y, uint8_t color) { data &= ~(0x01 << (y % 8)); // clear dot } this->WriteData(data); // write data back to display + this->Release(); } /** @@ -111,6 +115,9 @@ void glcd_Device::SetPixels(uint8_t x, uint8_t y, uint8_t x2, uint8_t y2, } mask <<= pageOffset; + if (this->Acquire() != pdTRUE) + return; + this->GotoXY(x, y); for (i = 0; i < width; i++) { data = this->ReadData(); @@ -150,6 +157,7 @@ void glcd_Device::SetPixels(uint8_t x, uint8_t y, uint8_t x2, uint8_t y2, this->WriteData(data); } } + this->Release(); } /* diff --git a/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.h b/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.h index 4e9d32f..fffa0bf 100644 --- a/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.h +++ b/hardware/emfcamp/sam/libraries/GLCD/glcd_Device.h @@ -120,8 +120,6 @@ class glcd_Device : public Print { public: glcd_Device(); void WaitForUpdate(void); - uint8_t ReadData(void); - void WriteData(uint8_t); void Init(); void Display(); uint8_t CurrentWidth(); @@ -133,7 +131,14 @@ class glcd_Device : public Print { void SetDot(uint8_t x, uint8_t y, uint8_t color); void SetPixels(uint8_t x, uint8_t y, uint8_t x1, uint8_t y1, uint8_t color); + uint8_t Acquire(void); + void Release(void); + + // These must only be called after a successful Acquire() + // Screen will only be updated after Release() void GotoXY(uint8_t x, uint8_t y); + uint8_t ReadData(void); + void WriteData(uint8_t); }; #endif diff --git a/hardware/emfcamp/sam/libraries/GLCD/st7565.cpp b/hardware/emfcamp/sam/libraries/GLCD/st7565.cpp index e49af0d..1ef4dfe 100644 --- a/hardware/emfcamp/sam/libraries/GLCD/st7565.cpp +++ b/hardware/emfcamp/sam/libraries/GLCD/st7565.cpp @@ -318,6 +318,7 @@ void glcd_Device::_updateDisplay() { } } +// Must Acquire() mutex before this is called void glcd_Device::GotoXY(uint8_t x, uint8_t y) { if ((x > this->CurrentWidth() - 1) || (y > this->CurrentHeight() - 1)) // exit if coordinates are not legal @@ -328,8 +329,8 @@ void glcd_Device::GotoXY(uint8_t x, uint8_t y) { _y = y; } -// Never call this directly, as it has no mutex -uint8_t glcd_Device::_do_ReadData() { +// Must Acquire() mutex before you call this +uint8_t glcd_Device::ReadData() { if (_x >= this->CurrentWidth()) { return (0); } @@ -337,25 +338,8 @@ uint8_t glcd_Device::_do_ReadData() { return this->_framebuffer[_y /8 * this->CurrentWidth() + _x]; } -uint8_t glcd_Device::ReadData() { - uint8_t data; - if (xTaskGetSchedulerState() == taskSCHEDULER_RUNNING) { - if (frameBufferMutex == 0) { - frameBufferMutex = xSemaphoreCreateMutex(); - } - if (xSemaphoreTake(frameBufferMutex, (TickType_t)100) == pdTRUE) { - data = _do_ReadData(); - xSemaphoreGive(frameBufferMutex); - return data; - } else { - } - } else { - return _do_ReadData(); - } -} - -// Never call this directly, as it has no mutex -void glcd_Device::_do_WriteData(uint8_t data) { +// Must Acquire() mutex before you call this +void glcd_Device::WriteData(uint8_t data) { uint8_t displayData, yOffset, chip; uint8_t current_width = this->CurrentWidth(); if (_x >= current_width) { @@ -405,22 +389,6 @@ void glcd_Device::_do_WriteData(uint8_t data) { } } -void glcd_Device::WriteData(uint8_t data) { - if (xTaskGetSchedulerState() == taskSCHEDULER_RUNNING) { - if (frameBufferMutex == 0) { - frameBufferMutex = xSemaphoreCreateMutex(); - } - if (xSemaphoreTake(frameBufferMutex, (TickType_t)100) == pdTRUE) { - _do_WriteData(data); - xSemaphoreGive(frameBufferMutex); - } else { - } - } else { - _do_WriteData(data); - } - _updateDisplay(); -} - void glcd_Device::WaitForUpdate() { uint8_t discard; if (xTaskGetSchedulerState() == taskSCHEDULER_RUNNING) { @@ -471,3 +439,23 @@ void glcd_Device::SetRotation(rotation_t rotation) { _rotation = rotation; } + +uint8_t glcd_Device::Acquire(void) +{ + if (xTaskGetSchedulerState() == taskSCHEDULER_RUNNING) { + if (frameBufferMutex == 0) { + frameBufferMutex = xSemaphoreCreateMutex(); + } + return xSemaphoreTake(frameBufferMutex, (TickType_t)100); + } else { + return pdTRUE; + } +} + +void glcd_Device::Release(void) +{ + if (frameBufferMutex != 0) { + xSemaphoreGive(frameBufferMutex); + } + _updateDisplay(); +}