From 60f642ba4f6a9bf2d976f133b85b8f6a7502c14c Mon Sep 17 00:00:00 2001 From: William Wilgus Date: Sat, 31 Oct 2020 00:18:57 -0400 Subject: lcd_framebuffer bugfixes, sanity checks several issues I saw that could pontentially cause problems scroll engine doesn't take text height into account when checking bounds NBELEMS was one whole row too large hopefully I got them right this time Change-Id: If303da8320429c3964fc675351cb088d46303745 --- firmware/drivers/lcd-16bit-common.c | 8 +++---- firmware/drivers/lcd-16bit-vert.c | 3 +++ firmware/drivers/lcd-16bit.c | 3 +++ firmware/drivers/lcd-24bit.c | 3 +++ firmware/drivers/lcd-bitmap-common.c | 42 +++++++++++++++++++++++++++--------- firmware/export/lcd-remote.h | 6 +++--- firmware/export/lcd.h | 14 ++++++------ 7 files changed, 55 insertions(+), 24 deletions(-) diff --git a/firmware/drivers/lcd-16bit-common.c b/firmware/drivers/lcd-16bit-common.c index 7c766dab8a..49e515f59f 100644 --- a/firmware/drivers/lcd-16bit-common.c +++ b/firmware/drivers/lcd-16bit-common.c @@ -81,11 +81,12 @@ void lcd_clear_viewport(void) } else { - if (!lcd_backdrop) + if (lcd_backdrop && lcd_current_viewport->buffer == &lcd_framebuffer_default) { do { - memset16(dst, lcd_current_viewport->bg_pattern, len); + memcpy(dst, PTR_ADD(dst, lcd_backdrop_offset), + len * sizeof(fb_data)); dst += step; } while (dst <= dst_end); @@ -94,8 +95,7 @@ void lcd_clear_viewport(void) { do { - memcpy(dst, PTR_ADD(dst, lcd_backdrop_offset), - len * sizeof(fb_data)); + memset16(dst, lcd_current_viewport->bg_pattern, len); dst += step; } while (dst <= dst_end); diff --git a/firmware/drivers/lcd-16bit-vert.c b/firmware/drivers/lcd-16bit-vert.c index 8631271482..4422fdea50 100644 --- a/firmware/drivers/lcd-16bit-vert.c +++ b/firmware/drivers/lcd-16bit-vert.c @@ -38,6 +38,9 @@ #include "bidi.h" #include "scroll_engine.h" +/*#define LOGF_ENABLE*/ +#include "logf.h" + #define ROW_INC 1 #define COL_INC lcd_current_viewport->buffer->stride diff --git a/firmware/drivers/lcd-16bit.c b/firmware/drivers/lcd-16bit.c index 6a76ba48ab..d6bf5a500d 100644 --- a/firmware/drivers/lcd-16bit.c +++ b/firmware/drivers/lcd-16bit.c @@ -38,6 +38,9 @@ #include "bidi.h" #include "scroll_engine.h" +// #define LOGF_ENABLE +#include "logf.h" + #define ROW_INC lcd_current_viewport->buffer->stride #define COL_INC 1 diff --git a/firmware/drivers/lcd-24bit.c b/firmware/drivers/lcd-24bit.c index 5ee290efe3..c3aa27f7ce 100644 --- a/firmware/drivers/lcd-24bit.c +++ b/firmware/drivers/lcd-24bit.c @@ -39,6 +39,9 @@ #include "bidi.h" #include "scroll_engine.h" +/*#define LOGF_ENABLE*/ +#include "logf.h" + #define ROW_INC lcd_current_viewport->buffer->stride #define COL_INC 1 diff --git a/firmware/drivers/lcd-bitmap-common.c b/firmware/drivers/lcd-bitmap-common.c index d195fd8ebe..170ad374ea 100644 --- a/firmware/drivers/lcd-bitmap-common.c +++ b/firmware/drivers/lcd-bitmap-common.c @@ -32,6 +32,10 @@ #include "string-extra.h" #include "diacritic.h" +#ifdef LOGF_ENABLE +#include "panic.h" +#endif + #ifndef LCDFN /* Not compiling for remote - define macros for main LCD. */ #define LCDFN(fn) lcd_ ## fn #define FBFN(fn) fb_ ## fn @@ -77,7 +81,10 @@ struct viewport* LCDFN(init_viewport)(struct viewport* vp) { struct frame_buffer_t *fb_default = &LCDFN(framebuffer_default); if (!vp) /* NULL vp grabs default viewport */ + { vp = &default_vp; + vp->buffer = fb_default; + } /* use defaults if no buffer is provided */ if (vp->buffer == NULL || vp->buffer->elems == 0) @@ -92,6 +99,19 @@ struct viewport* LCDFN(init_viewport)(struct viewport* vp) if (vp->buffer->get_address_fn == NULL) vp->buffer->get_address_fn = fb_default->get_address_fn; + +#ifdef LOGF_ENABLE + if ((size_t)LCD_NBELEMS(vp->width, vp->height) > vp->buffer->elems) + { + if (vp->buffer != fb_default) + panicf("viewport %d x %d > buffer", vp->width, vp->height); + logf("viewport %d x %d, %d x %d [%lu] > buffer [%lu]", vp->x, vp->y, + vp->width, vp->height, + (unsigned long) LCD_NBELEMS(vp->width, vp->height), + (unsigned long) vp->buffer->elems); + + } +#endif } return vp; } @@ -474,32 +494,34 @@ static bool LCDFN(puts_scroll_worker)(int x, int y, const unsigned char *string, int width, height; int w, h, cwidth; bool restart; + struct viewport * vp = LCDFN(current_viewport); if (!string) return false; /* prepare rectangle for scrolling. x and y must be calculated early * for find_scrolling_line() to work */ - cwidth = font_get(LCDFN(current_viewport)->font)->maxwidth; - height = font_get(LCDFN(current_viewport)->font)->height; + + cwidth = font_get(vp->font)->maxwidth; + /* get width (pixels) of the string */ + LCDFN(getstringsize)(string, &w, &h); + height = h; + y = y * (linebased ? height : 1); x = x * (linebased ? cwidth : 1); - width = LCDFN(current_viewport)->width - x; + width = vp->width - x; - if (y >= LCDFN(current_viewport)->height) + if (y >= vp->height || (height + y) > (vp->height)) return false; s = find_scrolling_line(x, y); restart = !s; - /* get width (pixeks) of the string */ - LCDFN(getstringsize)(string, &w, &h); - /* Remove any previously scrolling line at the same location. If * the string width is too small to scroll the scrolling line is * cleared as well */ if (w < width || restart) { - LCDFN(scroll_stop_viewport_rect)(LCDFN(current_viewport), x, y, width, height); + LCDFN(scroll_stop_viewport_rect)(vp, x, y, width, height); LCDFN(putsxyofs)(x, y, x_offset, string); /* nothing to scroll, or out of scrolling lines. Either way, get out */ if (w < width || LCDFN(scroll_info).lines >= LCDM(SCROLLABLE_LINES)) @@ -512,7 +534,7 @@ static bool LCDFN(puts_scroll_worker)(int x, int y, const unsigned char *string, strlcpy(s->linebuffer, string, sizeof(s->linebuffer)); /* scroll bidirectional or forward only depending on the string width */ if ( LCDFN(scroll_info).bidir_limit ) { - s->bidir = w < (LCDFN(current_viewport)->width) * + s->bidir = w < (vp->width) * (100 + LCDFN(scroll_info).bidir_limit) / 100; } else @@ -526,7 +548,7 @@ static bool LCDFN(puts_scroll_worker)(int x, int y, const unsigned char *string, s->y = y; s->width = width; s->height = height; - s->vp = LCDFN(current_viewport); + s->vp = vp; s->start_tick = current_tick + LCDFN(scroll_info).delay; LCDFN(scroll_info).lines++; } else { diff --git a/firmware/export/lcd-remote.h b/firmware/export/lcd-remote.h index 030b01c736..93ca1fac7e 100644 --- a/firmware/export/lcd-remote.h +++ b/firmware/export/lcd-remote.h @@ -63,13 +63,13 @@ #define LCD_REMOTE_STRIDE(w, h) (h) #define LCD_REMOTE_FBSTRIDE(w, h) ((h+7)/8) #define LCD_REMOTE_FBHEIGHT LCD_REMOTE_FBSTRIDE(LCD_REMOTE_WIDTH, LCD_REMOTE_HEIGHT) -#define LCD_REMOTE_NBELEMS(w, h) (((w*LCD_REMOTE_FBSTRIDE(w, h)) + h) / sizeof(fb_remote_data)) +#define LCD_REMOTE_NBELEMS(w, h) ((((w-1)*LCD_REMOTE_FBSTRIDE(w, h)) + h) / sizeof(fb_remote_data)) #elif LCD_REMOTE_DEPTH == 2 #if LCD_REMOTE_PIXELFORMAT == VERTICAL_INTERLEAVED #define LCD_REMOTE_STRIDE(w, h) (h) #define LCD_REMOTE_FBSTRIDE(w, h) ((h+7)/8) #define LCD_REMOTE_FBHEIGHT LCD_REMOTE_FBSTRIDE(LCD_REMOTE_WIDTH, LCD_REMOTE_HEIGHT) -#define LCD_REMOTE_NBELEMS(w, h) (((w*LCD_REMOTE_FBSTRIDE(w, h)) + h) / sizeof(fb_remote_data)) +#define LCD_REMOTE_NBELEMS(w, h) ((((w-1)*LCD_REMOTE_FBSTRIDE(w, h)) + h) / sizeof(fb_remote_data)) #endif #endif /* LCD_REMOTE_DEPTH */ @@ -84,7 +84,7 @@ #ifndef LCD_REMOTE_NBELEMS /* At this time (2020) known remote screens only have vertical stride */ -#define LCD_REMOTE_NBELEMS(w, h) ((w*STRIDE_REMOTE(w, h)) + h) / sizeof(fb_remote_data)) +#define LCD_REMOTE_NBELEMS(w, h) (((w-1)*STRIDE_REMOTE(w, h)) + h) / sizeof(fb_remote_data)) #define LCD_REMOTE_STRIDE(w, h) STRIDE_REMOTE(w, h) #define LCD_REMOTE_FBSTRIDE(w, h) STRIDE_REMOTE(w, h) #endif diff --git a/firmware/export/lcd.h b/firmware/export/lcd.h index f5a3b3f1f4..ffaf1a63d2 100644 --- a/firmware/export/lcd.h +++ b/firmware/export/lcd.h @@ -456,26 +456,26 @@ typedef void lcd_blockfunc_type(fb_data *address, unsigned mask, unsigned bits); #if LCD_PIXELFORMAT == HORIZONTAL_PACKING #define LCD_FBSTRIDE(w, h) ((w+7)/8) #define LCD_FBWIDTH LCD_FBSTRIDE(LCD_WIDTH, LCD_HEIGHT) -#define LCD_NBELEMS(w, h) (((h*LCD_FBSTRIDE(w, h)) + w) / sizeof(fb_data)) +#define LCD_NBELEMS(w, h) ((((h-1)*LCD_FBSTRIDE(w, h)) + w) / sizeof(fb_data)) #else /* LCD_PIXELFORMAT == VERTICAL_PACKING */ #define LCD_FBSTRIDE(w, h) ((h+7)/8) #define LCD_FBHEIGHT LCD_FBSTRIDE(LCD_WIDTH, LCD_HEIGHT) -#define LCD_NBELEMS(w, h) (((w*LCD_FBSTRIDE(w, h)) + h) / sizeof(fb_data)) +#define LCD_NBELEMS(w, h) ((((w-1)*LCD_FBSTRIDE(w, h)) + h) / sizeof(fb_data)) #endif /* LCD_PIXELFORMAT */ #elif LCD_DEPTH == 2 #if LCD_PIXELFORMAT == HORIZONTAL_PACKING #define LCD_FBSTRIDE(w, h) ((w+3)>>2) #define LCD_NATIVE_STRIDE(s) LCD_FBSTRIDE(s, s) #define LCD_FBWIDTH LCD_FBSTRIDE(LCD_WIDTH, LCD_HEIGHT) -#define LCD_NBELEMS(w, h) (((h*LCD_FBSTRIDE(w, h)) + w) / sizeof(fb_data)) +#define LCD_NBELEMS(w, h) ((((h-1)*LCD_FBSTRIDE(w, h)) + w) / sizeof(fb_data)) #elif LCD_PIXELFORMAT == VERTICAL_PACKING #define LCD_FBSTRIDE(w, h) ((h+3)/4) #define LCD_FBHEIGHT LCD_FBSTRIDE(LCD_WIDTH, LCD_HEIGHT) -#define LCD_NBELEMS(w, h) (((w*LCD_FBSTRIDE(w, h)) + h) / sizeof(fb_data)) +#define LCD_NBELEMS(w, h) ((((w-1)*LCD_FBSTRIDE(w, h)) + h) / sizeof(fb_data)) #elif LCD_PIXELFORMAT == VERTICAL_INTERLEAVED #define LCD_FBSTRIDE(w, h) ((h+7)/8) #define LCD_FBHEIGHT LCD_FBSTRIDE(LCD_WIDTH, LCD_HEIGHT) -#define LCD_NBELEMS(w, h) (((w*LCD_FBSTRIDE(w, h)) + h) / sizeof(fb_data)) +#define LCD_NBELEMS(w, h) ((((w-1)*LCD_FBSTRIDE(w, h)) + h) / sizeof(fb_data)) #endif /* LCD_PIXELFORMAT */ #endif /* LCD_DEPTH */ /* Set defaults if not defined different yet. The defaults apply to both @@ -494,9 +494,9 @@ typedef void lcd_blockfunc_type(fb_data *address, unsigned mask, unsigned bits); #ifndef LCD_NBELEMS #if defined(LCD_STRIDEFORMAT) && LCD_STRIDEFORMAT == VERTICAL_STRIDE -#define LCD_NBELEMS(w, h) ((w*STRIDE_MAIN(w, h)) + h) +#define LCD_NBELEMS(w, h) (((w-1)*STRIDE_MAIN(w, h)) + h) #else -#define LCD_NBELEMS(w, h) ((h*STRIDE_MAIN(w, h)) + w) +#define LCD_NBELEMS(w, h) (((h-1)*STRIDE_MAIN(w, h)) + w) #endif #define LCD_FBSTRIDE(w, h) STRIDE_MAIN(w, h) #endif -- cgit v1.2.3