From 71c63c2812ce63ceabdc74aac1b2dda5eb1ca338 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Tue, 1 Sep 2020 10:30:14 -0500 Subject: FramebufferDisplay: Don't set rotation via core_construct The expectations of displayio.Display and frambufferio.FramebufferDisplay are different when it comes to rotation. In displayio.Display, if you call core_construct with a WxH = 64x32 and rotation=90, you get something that is 32 pixels wide and 64 pixels tall in the LCD's coordinate system. This is fine, as the existing definitions were written to work like this. With framebuffer displays, however, the underlying framebuffer (such as RGBMatrix) says "I am WxH pixels wide in my coordinate system" and the constructor is given a rotation; when the rotation indicates a transpose that means "exchange rows and columns, so that to the Groups displayed on it, there is an effectively HxW pixel region for use". Happily, we already have a set_rotation method. Thus (modulo the time spent debugging things anyway:) the fix is simple: Always request no rotation from core_construct, then immediately fix up the rotation to match what was requested. Testing performed: 32x16 RGBMatrix on Metro M4 Express (but using the Airlift firmware, as this is the configuration the error was reported on): * initially construct display at 0, 90, 180, 270 degrees * later change angle to 0, 90, 180, 270 degrees * no garbled display * no safe mode crashes --- shared-module/framebufferio/FramebufferDisplay.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'shared-module/framebufferio/FramebufferDisplay.c') diff --git a/shared-module/framebufferio/FramebufferDisplay.c b/shared-module/framebufferio/FramebufferDisplay.c index 6b5346877..5163c3a7b 100644 --- a/shared-module/framebufferio/FramebufferDisplay.c +++ b/shared-module/framebufferio/FramebufferDisplay.c @@ -66,7 +66,7 @@ void common_hal_framebufferio_framebufferdisplay_construct(framebufferio_framebu ram_height, 0, 0, - rotation, + 0, // rotation depth, fb_getter_default(get_grayscale, (depth < 8)), fb_getter_default(get_pixels_in_byte_share_row, false), @@ -92,6 +92,10 @@ void common_hal_framebufferio_framebufferdisplay_construct(framebufferio_framebu self->native_frames_per_second = fb_getter_default(get_native_frames_per_second, 60); self->native_ms_per_frame = 1000 / self->native_frames_per_second; + if (rotation != 0) { + common_hal_framebufferio_framebufferdisplay_set_rotation(self, rotation); + } + supervisor_start_terminal(self->core.width, self->core.height); // Set the group after initialization otherwise we may send pixels while we delay in -- cgit v1.2.3 From 774f2e16ba166b5976a46519c38ea894f897dca4 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Sun, 11 Oct 2020 21:09:08 -0500 Subject: displayio, framebufferio: Remove spurious call to supervisor_start_terminal A call to supervisor_start_terminal remained in common_hal_displayio_display_construct and was copied to other display _construct functions, even though it was also being done in displayio_display_core_construct when that was factored out. Originally, this was harmless, except it created an extra allocation. When investigating #3482, I found that this bug became harmful, especially for displays that were created in Python code, because it caused a supervisor allocation to leak. I believe that it is safe to merge #3482 after this PR is merged. --- shared-module/displayio/Display.c | 2 -- shared-module/displayio/EPaperDisplay.c | 2 -- shared-module/framebufferio/FramebufferDisplay.c | 2 -- 3 files changed, 6 deletions(-) (limited to 'shared-module/framebufferio/FramebufferDisplay.c') diff --git a/shared-module/displayio/Display.c b/shared-module/displayio/Display.c index bb7e40f41..499c00ffb 100644 --- a/shared-module/displayio/Display.c +++ b/shared-module/displayio/Display.c @@ -107,8 +107,6 @@ void common_hal_displayio_display_construct(displayio_display_obj_t* self, i += 2 + data_size; } - supervisor_start_terminal(width, height); - // Always set the backlight type in case we're reusing memory. self->backlight_inout.base.type = &mp_type_NoneType; if (backlight_pin != NULL && common_hal_mcu_pin_is_free(backlight_pin)) { diff --git a/shared-module/displayio/EPaperDisplay.c b/shared-module/displayio/EPaperDisplay.c index 5b055e62e..46c7ea82e 100644 --- a/shared-module/displayio/EPaperDisplay.c +++ b/shared-module/displayio/EPaperDisplay.c @@ -90,8 +90,6 @@ void common_hal_displayio_epaperdisplay_construct(displayio_epaperdisplay_obj_t* // TODO: Clear } - supervisor_start_terminal(width, height); - // Set the group after initialization otherwise we may send pixels while we delay in // initialization. common_hal_displayio_epaperdisplay_show(self, &circuitpython_splash); diff --git a/shared-module/framebufferio/FramebufferDisplay.c b/shared-module/framebufferio/FramebufferDisplay.c index 5163c3a7b..6b506c7fa 100644 --- a/shared-module/framebufferio/FramebufferDisplay.c +++ b/shared-module/framebufferio/FramebufferDisplay.c @@ -96,8 +96,6 @@ void common_hal_framebufferio_framebufferdisplay_construct(framebufferio_framebu common_hal_framebufferio_framebufferdisplay_set_rotation(self, rotation); } - supervisor_start_terminal(self->core.width, self->core.height); - // Set the group after initialization otherwise we may send pixels while we delay in // initialization. common_hal_framebufferio_framebufferdisplay_show(self, &circuitpython_splash); -- cgit v1.2.3 From 88d07ef35b686ffd66487de81564396aac0818b8 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Tue, 13 Oct 2020 16:57:09 -0500 Subject: displayio: further ensure just one start_terminal call @cwalther determined that for boards with 2 displays (monster m4sk), start_terminal would be called for each one, leaking supervisor heap entries. Determine, by comparing addresses, whether the display being acted on is the first display (number zero) and do (or do not) call start_terminal. stop_terminal can safely be called multiple times, so there's no need to guard against calling it more than once. Slight behavioral change: The terminal size would follow the displays[0] size, not the displays[1] size --- shared-module/displayio/Display.c | 6 ++++-- shared-module/displayio/display_core.c | 5 ++++- shared-module/framebufferio/FramebufferDisplay.c | 6 ++++-- 3 files changed, 12 insertions(+), 5 deletions(-) (limited to 'shared-module/framebufferio/FramebufferDisplay.c') diff --git a/shared-module/displayio/Display.c b/shared-module/displayio/Display.c index 499c00ffb..7c8c9280b 100644 --- a/shared-module/displayio/Display.c +++ b/shared-module/displayio/Display.c @@ -347,8 +347,10 @@ void common_hal_displayio_display_set_rotation(displayio_display_obj_t* self, in self->core.height = tmp; } displayio_display_core_set_rotation(&self->core, rotation); - supervisor_stop_terminal(); - supervisor_start_terminal(self->core.width, self->core.height); + if (self == &displays[0].display) { + supervisor_stop_terminal(); + supervisor_start_terminal(self->core.width, self->core.height); + } if (self->core.current_group != NULL) { displayio_group_update_transform(self->core.current_group, &self->core.transform); } diff --git a/shared-module/displayio/display_core.c b/shared-module/displayio/display_core.c index 60a8ef213..411f9f373 100644 --- a/shared-module/displayio/display_core.c +++ b/shared-module/displayio/display_core.c @@ -85,7 +85,10 @@ void displayio_display_core_construct(displayio_display_core_t* self, self->bus = bus; - supervisor_start_terminal(width, height); + // (offsetof core is equal in all display types) + if (self == &displays[0].display.core) { + supervisor_start_terminal(width, height); + } self->width = width; self->height = height; diff --git a/shared-module/framebufferio/FramebufferDisplay.c b/shared-module/framebufferio/FramebufferDisplay.c index 6b506c7fa..03e121c91 100644 --- a/shared-module/framebufferio/FramebufferDisplay.c +++ b/shared-module/framebufferio/FramebufferDisplay.c @@ -280,8 +280,10 @@ void common_hal_framebufferio_framebufferdisplay_set_rotation(framebufferio_fram self->core.height = tmp; } displayio_display_core_set_rotation(&self->core, rotation); - supervisor_stop_terminal(); - supervisor_start_terminal(self->core.width, self->core.height); + if (self == &displays[0].framebuffer_display) { + supervisor_stop_terminal(); + supervisor_start_terminal(self->core.width, self->core.height); + } if (self->core.current_group != NULL) { displayio_group_update_transform(self->core.current_group, &self->core.transform); } -- cgit v1.2.3