Tweaks based on feedback

8 files changed, 50 insertions, 59 deletions

diff --git a/ports/atmel-samd/boards/hallowing_m0_express/board.c b/ports/atmel-samd/boards/hallowing_m0_express/board.c
index 69d7ecd37..85bcaabc7 100644
--- a/ports/atmel-samd/boards/hallowing_m0_express/board.c
+++ b/ports/atmel-samd/boards/hallowing_m0_express/board.c
@@ -84,7 +84,7 @@ void board_init(void) {
         } else if (*cmd == 0x11) {
             uint64_t start = ticks_ms;
             while (ticks_ms - start < 500) {}
-        } {
+        } else {
             uint64_t start = ticks_ms;
             while (ticks_ms - start < 10) {}
         }
diff --git a/ports/atmel-samd/common-hal/displayio/FourWire.c b/ports/atmel-samd/common-hal/displayio/FourWire.c
index 42038a58e..033970c77 100644
--- a/ports/atmel-samd/common-hal/displayio/FourWire.c
+++ b/ports/atmel-samd/common-hal/displayio/FourWire.c
@@ -63,22 +63,17 @@ bool common_hal_displayio_fourwire_begin_transaction(displayio_fourwire_obj_t* s
     if (!common_hal_busio_spi_try_lock(&self->bus)) {
         return false;
     }
+    // TODO(tannewt): Stop hardcoding SPI frequency, polarity and phase.
     common_hal_busio_spi_configure(&self->bus, 12000000, 0, 0, 8);
     common_hal_digitalio_digitalinout_set_value(&self->chip_select, false);
     return true;
 }
 
-void common_hal_displayio_fourwire_wait_for_send(displayio_fourwire_obj_t* self) {
-}
-
 void common_hal_displayio_fourwire_send(displayio_fourwire_obj_t* self, bool command, uint8_t *data, uint32_t data_length) {
-    common_hal_displayio_fourwire_wait_for_send(self);
     common_hal_digitalio_digitalinout_set_value(&self->command, !command);
     common_hal_busio_spi_write(&self->bus, data, data_length);
-
 }
 
-
 void common_hal_displayio_fourwire_end_transaction(displayio_fourwire_obj_t* self) {
     common_hal_digitalio_digitalinout_set_value(&self->chip_select, true);
     common_hal_busio_spi_unlock(&self->bus);
@@ -106,6 +101,7 @@ static uint16_t swap(uint16_t x) {
 }
 
 void displayio_fourwire_start_region_update(displayio_fourwire_obj_t* self, uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1) {
+    // TODO(tannewt): Handle displays with single byte bounds.
     common_hal_displayio_fourwire_begin_transaction(self);
     uint16_t data[2];
     common_hal_displayio_fourwire_send(self, true, &self->set_column_command, 1);
diff --git a/ports/atmel-samd/mpconfigport.h b/ports/atmel-samd/mpconfigport.h
index 58f74d7ec..14f72ccfb 100644
--- a/ports/atmel-samd/mpconfigport.h
+++ b/ports/atmel-samd/mpconfigport.h
@@ -239,8 +239,6 @@ extern const struct _mp_obj_module_t usb_hid_module;
         #define DISPLAYIO_MODULE
     #endif
 
-
-
     #ifndef EXTRA_BUILTIN_MODULES
     #define EXTRA_BUILTIN_MODULES \
         AUDIOIO_MODULE \
diff --git a/shared-bindings/displayio/Bitmap.c b/shared-bindings/displayio/Bitmap.c
index b01fe0e0c..d415127d2 100644
--- a/shared-bindings/displayio/Bitmap.c
+++ b/shared-bindings/displayio/Bitmap.c
@@ -45,30 +45,29 @@
 //|
 //| .. warning:: This will likely be changed before 4.0.0. Consider it very experimental.
 //|
-//| .. class:: Bitmap(width, height, value_size)
+//| .. class:: Bitmap(width, height, value_count)
 //|
-//|   Create a Bitmap object with the given fixed size.
+//|   Create a Bitmap object with the given fixed size. Each pixel stores a value that is used to
+//|   index into a corresponding palette. This enables differently colored sprites to share the
+//|   underlying Bitmap. value_count is used to minimize the memory used to store the Bitmap.
 //|
 //|   :param int width: The number of values wide
 //|   :param int height: The number of values high
-//|   :param int value_size: The value size in bits. Must be power of 2.
+//|   :param int value_count: The number of possible pixel values.
 //|
 STATIC mp_obj_t displayio_bitmap_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *pos_args) {
     mp_arg_check_num(n_args, n_kw, 3, 3, false);
     uint32_t width = mp_obj_get_int(pos_args[0]);
     uint32_t height = mp_obj_get_int(pos_args[1]);
-    uint32_t value_size = mp_obj_get_int(pos_args[2]);
+    uint32_t value_count = mp_obj_get_int(pos_args[2]);
     uint32_t power_of_two = 1;
-    while (value_size > power_of_two) {
+    while (value_count > (1U << power_of_two)) {
         power_of_two <<= 1;
     }
-    if (value_size != power_of_two) {
-        mp_raise_ValueError(translate("value_size must be power of two"));
-    }
 
     displayio_bitmap_t *self = m_new_obj(displayio_bitmap_t);
     self->base.type = &displayio_bitmap_type;
-    common_hal_displayio_bitmap_construct(self, width, height, value_size);
+    common_hal_displayio_bitmap_construct(self, width, height, power_of_two);
 
     return MP_OBJ_FROM_PTR(self);
 }
diff --git a/shared-bindings/displayio/Group.c b/shared-bindings/displayio/Group.c
index a2a879987..e6f1fbbf5 100644
--- a/shared-bindings/displayio/Group.c
+++ b/shared-bindings/displayio/Group.c
@@ -74,7 +74,7 @@ STATIC mp_obj_t displayio_group_make_new(const mp_obj_type_t *type, size_t n_arg
 
 //|   .. method:: append(layer)
 //|
-//|     Switches do displaying the given group of elements.
+//|     Append a layer to the group. It will be drawn above other layers.
 //|
 STATIC mp_obj_t displayio_group_obj_append(mp_obj_t self_in, mp_obj_t layer) {
     displayio_group_t *self = MP_OBJ_TO_PTR(self_in);
diff --git a/shared-bindings/displayio/Palette.c b/shared-bindings/displayio/Palette.c
index bb2af9410..99963f131 100644
--- a/shared-bindings/displayio/Palette.c
+++ b/shared-bindings/displayio/Palette.c
@@ -41,8 +41,8 @@
 //| :class:`Palette` -- Stores a mapping from bitmap pixel values to display colors
 //| ===============================================================================
 //|
-//| Manage updating a display over SPI four wire protocol in the background while Python code runs.
-//| It doesn't handle display initialization.
+//| Map a pixel value to a full color. Colors are transformed to the display's format internally to
+//| save memory.
 //|
 //| .. warning:: This will be changed before 4.0.0. Consider it very experimental.
 //|
@@ -76,42 +76,41 @@ STATIC mp_obj_t palette_subscr(mp_obj_t self_in, mp_obj_t index_in, mp_obj_t val
     if (value == MP_OBJ_NULL) {
         // delete item
         return MP_OBJ_NULL; // op not supported
-    } else {
-        displayio_palette_t *self = MP_OBJ_TO_PTR(self_in);
-        if (MP_OBJ_IS_TYPE(index_in, &mp_type_slice)) {
-            return MP_OBJ_NULL; // Slicing not supported. Use a duplicate Palette to swap multiple colors atomically.
-        } else {
-            if (value == MP_OBJ_SENTINEL) {
-                return MP_OBJ_NULL; // index read is not supported
-            } else {
-                size_t index = mp_get_index(&displayio_palette_type, self->max_value, index_in, false);
+    }
+    // Slicing not supported. Use a duplicate Palette to swap multiple colors atomically.
+    if (MP_OBJ_IS_TYPE(index_in, &mp_type_slice)) {
+        return MP_OBJ_NULL;
+    }
+    // index read is not supported
+    if (value == MP_OBJ_SENTINEL) {
+        return MP_OBJ_NULL;
+    }
+    displayio_palette_t *self = MP_OBJ_TO_PTR(self_in);
+    size_t index = mp_get_index(&displayio_palette_type, self->max_value, index_in, false);
 
-                uint32_t color;
-                mp_int_t int_value;
-                mp_buffer_info_t bufinfo;
-                if (mp_get_buffer(value, &bufinfo, MP_BUFFER_READ)) {
-                    if (bufinfo.typecode != 'b' && bufinfo.typecode != 'B' && bufinfo.typecode != BYTEARRAY_TYPECODE) {
-                        mp_raise_ValueError(translate("color buffer must be a bytearray or array of type 'b' or 'B'"));
-                    }
-                    uint8_t* buf = bufinfo.buf;
-                    if (bufinfo.len == 3 || bufinfo.len == 4) {
-                        color = buf[0] << 16 | buf[1] << 8 | buf[2];
-                    } else {
-                        mp_raise_ValueError(translate("color buffer must be 3 bytes (RGB) or 4 bytes (RGBA)"));
-                    }
-                } else if (mp_obj_get_int_maybe(value, &int_value)) {
-                    if (int_value < 0 || int_value > 0xffffff) {
-                        mp_raise_TypeError(translate("color must be between 0x000000 and 0xffffff"));
-                    }
-                    color = int_value;
-                } else {
-                    mp_raise_TypeError(translate("color buffer must be a buffer or int"));
-                }
-                common_hal_displayio_palette_set_color(self, index, color);
-                return mp_const_none;
-            }
+    uint32_t color;
+    mp_int_t int_value;
+    mp_buffer_info_t bufinfo;
+    if (mp_get_buffer(value, &bufinfo, MP_BUFFER_READ)) {
+        if (bufinfo.typecode != 'b' && bufinfo.typecode != 'B' && bufinfo.typecode != BYTEARRAY_TYPECODE) {
+            mp_raise_ValueError(translate("color buffer must be a bytearray or array of type 'b' or 'B'"));
         }
+        uint8_t* buf = bufinfo.buf;
+        if (bufinfo.len == 3 || bufinfo.len == 4) {
+            color = buf[0] << 16 | buf[1] << 8 | buf[2];
+        } else {
+            mp_raise_ValueError(translate("color buffer must be 3 bytes (RGB) or 4 bytes (RGBA)"));
+        }
+    } else if (mp_obj_get_int_maybe(value, &int_value)) {
+        if (int_value < 0 || int_value > 0xffffff) {
+            mp_raise_TypeError(translate("color must be between 0x000000 and 0xffffff"));
+        }
+        color = int_value;
+    } else {
+        mp_raise_TypeError(translate("color buffer must be a buffer or int"));
     }
+    common_hal_displayio_palette_set_color(self, index, color);
+    return mp_const_none;
 }
 
 //|   .. method:: make_transparent(value)
diff --git a/shared-bindings/displayio/__init__.h b/shared-bindings/displayio/__init__.h
index 5c3fc2b55..a6663bf57 100644
--- a/shared-bindings/displayio/__init__.h
+++ b/shared-bindings/displayio/__init__.h
@@ -31,8 +31,4 @@
 
 // Nothing now.
 
-// typedef enum {
-//     PIXEL_
-// } displayio_pixel_format;
-
 #endif  // MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYIO___INIT___H
diff --git a/shared-module/displayio/Bitmap.c b/shared-module/displayio/Bitmap.c
index 7c6a2ee80..453dc1c59 100644
--- a/shared-module/displayio/Bitmap.c
+++ b/shared-module/displayio/Bitmap.c
@@ -56,7 +56,7 @@ void common_hal_displayio_bitmap_construct(displayio_bitmap_t *self, uint32_t wi
 }
 
 void common_hal_displayio_bitmap_load_row(displayio_bitmap_t *self, uint16_t y, uint8_t* data, uint16_t len) {
-    if (len != self->stride * 4) {
+    if (len != self->stride * sizeof(uint32_t)) {
         mp_raise_ValueError(translate("row must be packed and word aligned"));
     }
     uint32_t* row_value = self->data + (y * self->stride);
@@ -77,6 +77,9 @@ void common_hal_displayio_bitmap_load_row(displayio_bitmap_t *self, uint16_t y,
     }
 }
 uint32_t common_hal_displayio_bitmap_get_pixel(displayio_bitmap_t *self, int16_t x, int16_t y) {
+    if (x >= self->width || x < 0 || y >= self->height || y < 0) {
+        return 0;
+    }
     int32_t row_start = y * self->stride;
     if (self->bits_per_value < 8) {
         uint32_t word = self->data[row_start + (x >> self->x_shift)];