Merge pull request #7922 from tannewt/dvi320

Take in framebuffer resolution, not output res

Author: tannewt

ports/raspberrypi/bindings/picodvi/Framebuffer.c

@@ -36,8 +36,6 @@
 #include "shared-module/framebufferio/FramebufferDisplay.h"
 
 //| class Framebuffer:
-//|     """A PicoDVI managed frame buffer."""
-//|
 //|     def __init__(
 //|         self,
 //|         width: int,
@@ -53,36 +51,37 @@
 //|         blue_dn: microcontroller.Pin,
 //|         color_depth: int = 8,
 //|     ) -> None:
-//|         """Create a Framebuffer object with the given dimensions (640x480 or 800x480). Memory is
-//|            allocated outside of onto the heap and then moved outside on VM
-//|            end.
+//|         """Create a Framebuffer object with the given dimensions. Memory is
+//|         allocated outside of onto the heap and then moved outside on VM end.
+//|
+//|         .. warning:: This will change the system clock speed to match the DVI signal.
+//|            Make sure to initialize other objects after this one so they account
+//|            for the changed clock.
 //|
-//|         This will change the system clock speed to match the DVI signal.
-//|         Make sure to initialize other objects after this one so they account
-//|         for the changed clock. This also allocates a very large framebuffer
-//|         and is most likely to succeed the earlier it is attempted.
+//|         This allocates a very large framebuffer and is most likely to succeed
+//|         the earlier it is attempted.
 //|
 //|         Each dp and dn pair of pins must be neighboring, such as 19 and 20.
 //|         They must also be ordered the same way. In other words, dp must be
 //|         less than dn for all pairs or dp must be greater than dn for all pairs.
 //|
 //|         The framebuffer pixel format varies depending on color_depth:
+//|
 //|         * 1 - Each bit is a pixel. Either white (1) or black (0).
 //|         * 2 - Each 2 bits is a pixels. Grayscale between white (0x3) and black (0x0).
 //|         * 8 - Each byte is a pixels in RGB332 format.
 //|         * 16 - Each two bytes are a pixel in RGB565 format.
 //|
-//|         Monochrome framebuffers (color_depth=1 or 2) will be full resolution.
-//|         Color framebuffers will be half resolution and pixels will be
-//|         duplicated to create a signal with the target dimensions.
+//|         Two output resolutions are currently supported, 640x480 and 800x480.
+//|         Monochrome framebuffers (color_depth=1 or 2) must be full resolution.
+//|         Color framebuffers must be half resolution (320x240 or 400x240) and
+//|         pixels will be duplicated to create the signal.
 //|
 //|         A Framebuffer is often used in conjunction with a
 //|         `framebufferio.FramebufferDisplay`.
 //|
-//|         :param int width: the width of the target display signal. It will be halved when
-//|           color_depth >= 8 when creating the framebuffer. Only 640 or 800 is currently supported.
-//|         :param int height: the height of the target display signal. It will be halved when
-//|           color_depth >= 8 when creating the framebuffer. Only 480 is currently supported.
+//|         :param int width: the width of the target display signal. Only 320, 400, 640 or 800 is currently supported depending on color_depth.
+//|         :param int height: the height of the target display signal. Only 240 or 480 is currently supported depending on color_depth.
 //|         :param ~microcontroller.Pin clk_dp: the positive clock signal pin
 //|         :param ~microcontroller.Pin clk_dn: the negative clock signal pin
 //|         :param ~microcontroller.Pin red_dp: the positive red signal pin
@@ -160,8 +159,7 @@ static void check_for_deinit(picodvi_framebuffer_obj_t *self) {
 }
 
 //|     width: int
-//|     """The width of the framebuffer, in pixels. It may be doubled for output (and half of what
-//|        width was given to __init__.)"""
+//|     """The width of the framebuffer, in pixels. It may be doubled for output."""
 STATIC mp_obj_t picodvi_framebuffer_get_width(mp_obj_t self_in) {
     picodvi_framebuffer_obj_t *self = (picodvi_framebuffer_obj_t *)self_in;
     check_for_deinit(self);
@@ -172,8 +170,7 @@ MP_PROPERTY_GETTER(picodvi_framebuffer_width_obj,
     (mp_obj_t)&picodvi_framebuffer_get_width_obj);
 
 //|     height: int
-//|     """The width of the framebuffer, in pixels. It may be doubled for output (and half of what
-//|        width was given to __init__.)"""
+//|     """The width of the framebuffer, in pixels. It may be doubled for output."""
 //|
 STATIC mp_obj_t picodvi_framebuffer_get_height(mp_obj_t self_in) {
     picodvi_framebuffer_obj_t *self = (picodvi_framebuffer_obj_t *)self_in;

ports/raspberrypi/boards/adafruit_feather_rp2040_dvi/board.c

@@ -33,7 +33,7 @@
 void board_init(void) {
     picodvi_framebuffer_obj_t *fb = &allocate_display_bus()->picodvi;
     fb->base.type = &picodvi_framebuffer_type;
-    common_hal_picodvi_framebuffer_construct(fb, 640, 480,
+    common_hal_picodvi_framebuffer_construct(fb, 320, 240,
         &pin_GPIO17, &pin_GPIO16,
         &pin_GPIO19, &pin_GPIO18,
         &pin_GPIO21, &pin_GPIO20,

ports/raspberrypi/common-hal/picodvi/Framebuffer.c

@@ -138,21 +138,29 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
     const mcu_pin_obj_t *green_dp, const mcu_pin_obj_t *green_dn,
     const mcu_pin_obj_t *blue_dp, const mcu_pin_obj_t *blue_dn,
     mp_uint_t color_depth) {
+    if (active_picodvi != NULL) {
+        mp_raise_msg_varg(&mp_type_RuntimeError, translate("%q in use"), MP_QSTR_picodvi);
+    }
 
+    bool color_framebuffer = color_depth >= 8;
     const struct dvi_timing *timing = NULL;
-    if (width == 640 && height == 480) {
+    if ((width == 640 && height == 480) ||
+        (width == 320 && height == 240)) {
         timing = &dvi_timing_640x480p_60hz;
-    } else if (width == 800 && height == 480) {
+    } else if ((width == 800 && height == 480) ||
+               (width == 400 && height == 240)) {
         timing = &dvi_timing_800x480p_60hz;
     } else {
-        if (height == 480) {
-            mp_raise_ValueError_varg(translate("%q must be %d"), MP_QSTR_width, 480);
+        if (height != 480 && height != 240) {
+            mp_raise_ValueError_varg(translate("Invalid %q"), MP_QSTR_height);
         }
-        mp_raise_ValueError_varg(translate("Invalid %q"), MP_QSTR_height);
+        mp_raise_ValueError_varg(translate("Invalid %q"), MP_QSTR_width);
     }
 
-    if (active_picodvi != NULL) {
-        mp_raise_msg_varg(&mp_type_RuntimeError, translate("%q in use"), MP_QSTR_picodvi);
+    // If the width is > 400, then it must not be color frame buffer and vice
+    // versa.
+    if ((width > 400) == color_framebuffer) {
+        mp_raise_ValueError_varg(translate("Invalid %q"), MP_QSTR_color_depth);
     }
 
     bool invert_diffpairs = clk_dn->number < clk_dp->number;
@@ -214,12 +222,12 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
     self->height = height;
 
     size_t tmds_bufs_per_scanline;
-    if (color_depth >= 8) {
+    size_t scanline_width = width;
+    if (color_framebuffer) {
         dvi_vertical_repeat = 2;
         dvi_monochrome_tmds = false;
-        self->width /= 2;
-        self->height /= 2;
         tmds_bufs_per_scanline = 3;
+        scanline_width *= 2;
     } else {
         dvi_vertical_repeat = 1;
         dvi_monochrome_tmds = true;
@@ -233,8 +241,7 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
     }
     self->pitch /= sizeof(uint32_t);
     size_t framebuffer_size = self->pitch * self->height;
-    // use width here because it hasn't been downsized for the frame buffer
-    self->tmdsbuf_size = tmds_bufs_per_scanline * width / DVI_SYMBOLS_PER_WORD + 1;
+    self->tmdsbuf_size = tmds_bufs_per_scanline * scanline_width / DVI_SYMBOLS_PER_WORD + 1;
     size_t total_allocation_size = sizeof(uint32_t) * (framebuffer_size + DVI_N_TMDS_BUFFERS * self->tmdsbuf_size);
     self->allocation = allocate_memory(total_allocation_size, false, true);
     if (self->allocation == NULL) {