Rework based on Dan's review

Author: tannewt

ports/atmel-samd/boards/monster_m4sk/mpconfigboard.h

@@ -27,5 +27,3 @@
 // USB is always used internally so skip the pin objects for it.
 #define IGNORE_PIN_PA24     1
 #define IGNORE_PIN_PA25     1
-
-#define CIRCUITPY_FS_NAME "MONSTERM4SK"

ports/atmel-samd/common-hal/displayio/ParallelBus.c

@@ -131,9 +131,9 @@ bool common_hal_displayio_parallelbus_begin_transaction(mp_obj_t obj) {
     return true;
 }
 
-void common_hal_displayio_parallelbus_send(mp_obj_t obj, bool command, bool toggle_every_byte, uint8_t *data, uint32_t data_length) {
+void common_hal_displayio_parallelbus_send(mp_obj_t obj, display_byte_type_t byte_type, display_chip_select_behavior_t chip_select, uint8_t *data, uint32_t data_length) {
     displayio_parallelbus_obj_t* self = MP_OBJ_TO_PTR(obj);
-    common_hal_digitalio_digitalinout_set_value(&self->command, !command);
+    common_hal_digitalio_digitalinout_set_value(&self->command, byte_type == DISPLAY_DATA);
     uint32_t* clear_write = (uint32_t*) &self->write_group->OUTCLR.reg;
     uint32_t* set_write = (uint32_t*) &self->write_group->OUTSET.reg;
     uint32_t mask = self->write_mask;

ports/nrf/common-hal/displayio/ParallelBus.c

@@ -142,9 +142,10 @@ bool common_hal_displayio_parallelbus_begin_transaction(mp_obj_t obj) {
     return true;
 }
 
-void common_hal_displayio_parallelbus_send(mp_obj_t obj, bool command, bool toggle_every_byte, uint8_t *data, uint32_t data_length) {
+// This ignores chip_select behaviour because data is clocked in by the write line toggling.
+void common_hal_displayio_parallelbus_send(mp_obj_t obj, display_byte_type_t byte_type, display_chip_select_behavior_t chip_select, uint8_t *data, uint32_t data_length) {
     displayio_parallelbus_obj_t* self = MP_OBJ_TO_PTR(obj);
-    common_hal_digitalio_digitalinout_set_value(&self->command, !command);
+    common_hal_digitalio_digitalinout_set_value(&self->command, byte_type == DISPLAY_DATA);
     uint32_t* clear_write = (uint32_t*) &self->write_group->OUTCLR;
     uint32_t* set_write = (uint32_t*) &self->write_group->OUTSET;
     uint32_t mask = self->write_mask;

py/circuitpy_mpconfig.h

@@ -657,8 +657,4 @@ void run_background_tasks(void);
 #define CIRCUITPY_FILESYSTEM_FLUSH_INTERVAL_MS 1000
 #define CIRCUITPY_BOOT_OUTPUT_FILE "/boot_out.txt"
 
-#ifndef CIRCUITPY_FS_NAME
-#define CIRCUITPY_FS_NAME "CIRCUITPY"
-#endif
-
 #endif  // __INCLUDED_MPCONFIG_CIRCUITPY_H

shared-bindings/_stage/__init__.c

@@ -107,7 +107,7 @@ STATIC mp_obj_t stage_render(size_t n_args, const mp_obj_t *args) {
     area.y2 = y1;
     displayio_display_core_set_region_to_update(&display->core, display->set_column_command, display->set_row_command, NO_COMMAND, NO_COMMAND, display->data_as_commands, false, &area);
 
-    display->core.send(display->core.bus, true, true, &display->write_ram_command, 1);
+    display->core.send(display->core.bus, DISPLAY_COMMAND, CHIP_SELECT_TOGGLE_EVERY_BYTE, &display->write_ram_command, 1);
     render_stage(x0, y0, x1, y1, layers, layers_size, buffer, buffer_size,
                  display, scale);
     displayio_display_core_end_transaction(&display->core);

shared-bindings/displayio/Display.c

@@ -218,16 +218,22 @@ STATIC mp_obj_t displayio_display_obj_show(mp_obj_t self_in, mp_obj_t group_in)
 }
 MP_DEFINE_CONST_FUN_OBJ_2(displayio_display_show_obj, displayio_display_obj_show);
 
-//|   .. method:: refresh(*, target_frames_per_second=None, minimum_frames_per_second=1)
+//|   .. method:: refresh(*, target_frames_per_second=60, minimum_frames_per_second=1)
 //|
-//|     When auto refresh is off, waits for the target frame rate and then refreshes the display. If
-//|     the call is too late for the given target frame rate, then the refresh returns immediately
-//|     without updating the screen to hopefully help getting caught up. If the current frame rate
-//|     is below the minimum frame rate, then an exception will be raised.
+//|     When auto refresh is off, waits for the target frame rate and then refreshes the display,
+//|     returning True. If the call has taken too long since the last refresh call for the given
+//|     target frame rate, then the refresh returns False immediately without updating the screen to
+//|     hopefully help getting caught up.
+//|
+//|     If the time since the last successful refresh is below the minimum frame rate, then an
+//|     exception will be raised. Set minimum_frames_per_second to 0 to disable.
 //|
 //|     When auto refresh is on, updates the display immediately. (The display will also update
 //|     without calls to this.)
 //|
+//|   :param int target_frames_per_second: How many times a second `refresh` should be called and the screen updated.
+//|   :param int minimum_frames_per_second: The minimum number of times the screen should be updated per second.
+//|
 STATIC mp_obj_t displayio_display_obj_refresh(size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     enum { ARG_target_frames_per_second, ARG_minimum_frames_per_second };
     static const mp_arg_t allowed_args[] = {
@@ -238,8 +244,12 @@ STATIC mp_obj_t displayio_display_obj_refresh(size_t n_args, const mp_obj_t *pos
     mp_arg_parse_all(n_args - 1, pos_args + 1, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args);
 
     displayio_display_obj_t *self = native_display(pos_args[0]);
-    common_hal_displayio_display_refresh(self, 1000 / args[ARG_target_frames_per_second].u_int, 1000 / args[ARG_minimum_frames_per_second].u_int);
-    return mp_const_none;
+    uint32_t maximum_ms_per_real_frame = 0xffffffff;
+    mp_int_t minimum_frames_per_second = args[ARG_minimum_frames_per_second].u_int;
+    if (minimum_frames_per_second > 0) {
+        maximum_ms_per_real_frame = 1000 / minimum_frames_per_second;
+    }
+    return mp_obj_new_bool(common_hal_displayio_display_refresh(self, 1000 / args[ARG_target_frames_per_second].u_int, maximum_ms_per_real_frame));
 }
 MP_DEFINE_CONST_FUN_OBJ_KW(displayio_display_refresh_obj, 1, displayio_display_obj_refresh);
 

shared-bindings/displayio/Display.h

@@ -51,7 +51,7 @@ bool common_hal_displayio_display_show(displayio_display_obj_t* self,
                                        displayio_group_t* root_group);
 
 // Times in ms.
-void common_hal_displayio_display_refresh(displayio_display_obj_t* self, uint32_t target_frame_time, uint32_t maximum_frame_time);
+bool common_hal_displayio_display_refresh(displayio_display_obj_t* self, uint32_t target_ms_per_frame, uint32_t maximum_ms_per_real_frame);
 
 bool common_hal_displayio_display_get_auto_refresh(displayio_display_obj_t* self);
 void common_hal_displayio_display_set_auto_refresh(displayio_display_obj_t* self, bool auto_refresh);

shared-bindings/displayio/EPaperDisplay.c

@@ -55,7 +55,7 @@
 //|
 //|   Create a EPaperDisplay object on the given display bus (`displayio.FourWire` or `displayio.ParallelBus`).
 //|
-//|   The ``start_sequence`` and ``stop_sequence`` bitpacked to minimize the ram impact. Every
+//|   The ``start_sequence`` and ``stop_sequence`` are bitpacked to minimize the ram impact. Every
 //|   command begins with a command byte followed by a byte to determine the parameter count and if
 //|   a delay is need after. When the top bit of the second byte is 1, the next byte will be the
 //|   delay time in milliseconds. The remaining 7 bits are the parameter count excluding any delay

shared-bindings/displayio/FourWire.c

@@ -146,8 +146,12 @@ STATIC mp_obj_t displayio_fourwire_obj_send(size_t n_args, const mp_obj_t *pos_a
     while (!common_hal_displayio_fourwire_begin_transaction(self)) {
         RUN_BACKGROUND_TASKS;
     }
-    common_hal_displayio_fourwire_send(self, true, args[ARG_toggle_every_byte].u_bool, &command, 1);
-    common_hal_displayio_fourwire_send(self, false, args[ARG_toggle_every_byte].u_bool, ((uint8_t*) bufinfo.buf), bufinfo.len);
+    display_chip_select_behavior_t chip_select = CHIP_SELECT_UNTOUCHED;
+    if (args[ARG_toggle_every_byte].u_bool) {
+        chip_select = CHIP_SELECT_TOGGLE_EVERY_BYTE;
+    }
+    common_hal_displayio_fourwire_send(self, DISPLAY_COMMAND, chip_select, &command, 1);
+    common_hal_displayio_fourwire_send(self, DISPLAY_DATA, chip_select, ((uint8_t*) bufinfo.buf), bufinfo.len);
     common_hal_displayio_fourwire_end_transaction(self);
 
     return mp_const_none;

shared-bindings/displayio/FourWire.h

@@ -28,6 +28,8 @@
 #define MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYBUSIO_FOURWIRE_H
 
 #include "shared-module/displayio/FourWire.h"
+
+#include "shared-bindings/displayio/__init__.h"
 #include "common-hal/microcontroller/Pin.h"
 
 #include "shared-module/displayio/Group.h"
@@ -45,7 +47,7 @@ bool common_hal_displayio_fourwire_bus_free(mp_obj_t self);
 
 bool common_hal_displayio_fourwire_begin_transaction(mp_obj_t self);
 
-void common_hal_displayio_fourwire_send(mp_obj_t self, bool command, bool toggle_every_byte, uint8_t *data, uint32_t data_length);
+void common_hal_displayio_fourwire_send(mp_obj_t self, display_byte_type_t byte_type, display_chip_select_behavior_t chip_select, uint8_t *data, uint32_t data_length);
 
 void common_hal_displayio_fourwire_end_transaction(mp_obj_t self);
 

shared-bindings/displayio/I2CDisplay.c

@@ -131,7 +131,7 @@ STATIC mp_obj_t displayio_i2cdisplay_obj_send(mp_obj_t self, mp_obj_t command_ob
     uint8_t full_command[bufinfo.len + 1];
     full_command[0] = command;
     memcpy(full_command + 1, ((uint8_t*) bufinfo.buf), bufinfo.len);
-    common_hal_displayio_i2cdisplay_send(self, true, false, full_command, bufinfo.len + 1);
+    common_hal_displayio_i2cdisplay_send(self, DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, full_command, bufinfo.len + 1);
     common_hal_displayio_i2cdisplay_end_transaction(self);
 
     return mp_const_none;

shared-bindings/displayio/I2CDisplay.h

@@ -28,6 +28,8 @@
 #define MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYBUSIO_I2CDISPLAY_H
 
 #include "shared-module/displayio/I2CDisplay.h"
+
+#include "shared-bindings/displayio/__init__.h"
 #include "common-hal/microcontroller/Pin.h"
 
 extern const mp_obj_type_t displayio_i2cdisplay_type;
@@ -42,7 +44,7 @@ bool common_hal_displayio_i2cdisplay_bus_free(mp_obj_t self);
 
 bool common_hal_displayio_i2cdisplay_begin_transaction(mp_obj_t self);
 
-void common_hal_displayio_i2cdisplay_send(mp_obj_t self, bool command, bool toggle_every_byte, uint8_t *data, uint32_t data_length);
+void common_hal_displayio_i2cdisplay_send(mp_obj_t self, display_byte_type_t byte_type, display_chip_select_behavior_t chip_select, uint8_t *data, uint32_t data_length);
 
 void common_hal_displayio_i2cdisplay_end_transaction(mp_obj_t self);
 

shared-bindings/displayio/ParallelBus.c

@@ -139,8 +139,8 @@ STATIC mp_obj_t displayio_parallelbus_obj_send(mp_obj_t self, mp_obj_t command_o
     while (!common_hal_displayio_parallelbus_begin_transaction(self)) {
         RUN_BACKGROUND_TASKS;
     }
-    common_hal_displayio_parallelbus_send(self, true, false, &command, 1);
-    common_hal_displayio_parallelbus_send(self, false, false, ((uint8_t*) bufinfo.buf), bufinfo.len);
+    common_hal_displayio_parallelbus_send(self, DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, &command, 1);
+    common_hal_displayio_parallelbus_send(self, DISPLAY_DATA, CHIP_SELECT_UNTOUCHED, ((uint8_t*) bufinfo.buf), bufinfo.len);
     common_hal_displayio_parallelbus_end_transaction(self);
 
     return mp_const_none;

shared-bindings/displayio/ParallelBus.h

@@ -28,8 +28,9 @@
 #define MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYBUSIO_PARALLELBUS_H
 
 #include "common-hal/displayio/ParallelBus.h"
-#include "common-hal/microcontroller/Pin.h"
 
+#include "common-hal/microcontroller/Pin.h"
+#include "shared-bindings/displayio/__init__.h"
 #include "shared-module/displayio/Group.h"
 
 extern const mp_obj_type_t displayio_parallelbus_type;
@@ -45,7 +46,7 @@ bool common_hal_displayio_parallelbus_bus_free(mp_obj_t self);
 
 bool common_hal_displayio_parallelbus_begin_transaction(mp_obj_t self);
 
-void common_hal_displayio_parallelbus_send(mp_obj_t self, bool command, bool toggle_every_byte, uint8_t *data, uint32_t data_length);
+void common_hal_displayio_parallelbus_send(mp_obj_t self, display_byte_type_t byte_type, display_chip_select_behavior_t chip_select, uint8_t *data, uint32_t data_length);
 
 void common_hal_displayio_parallelbus_end_transaction(mp_obj_t self);
 

shared-bindings/displayio/__init__.h

@@ -27,6 +27,25 @@
 #ifndef MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYIO___INIT___H
 #define MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYIO___INIT___H
 
+#include "py/obj.h"
+
+typedef enum {
+    DISPLAY_COMMAND,
+    DISPLAY_DATA
+} display_byte_type_t;
+
+
+typedef enum {
+    CHIP_SELECT_UNTOUCHED,
+    CHIP_SELECT_TOGGLE_EVERY_BYTE
+} display_chip_select_behavior_t;
+
+typedef bool (*display_bus_bus_reset)(mp_obj_t bus);
+typedef bool (*display_bus_bus_free)(mp_obj_t bus);
+typedef bool (*display_bus_begin_transaction)(mp_obj_t bus);
+typedef void (*display_bus_send)(mp_obj_t bus, display_byte_type_t byte_type, display_chip_select_behavior_t chip_select, uint8_t *data, uint32_t data_length);
+typedef void (*display_bus_end_transaction)(mp_obj_t bus);
+
 void common_hal_displayio_release_displays(void);
 
 #endif  // MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYIO___INIT___H

shared-module/_stage/__init__.c

@@ -57,7 +57,7 @@ void render_stage(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1,
                     index += 1;
                     // The buffer is full, send it.
                     if (index >= buffer_size) {
-                        display->core.send(display->core.bus, false, false, ((uint8_t*)buffer),
+                        display->core.send(display->core.bus, DISPLAY_DATA, CHIP_SELECT_UNTOUCHED, ((uint8_t*)buffer),
                                       buffer_size * 2);
                         index = 0;
                     }
@@ -67,6 +67,6 @@ void render_stage(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1,
     }
     // Send the remaining data.
     if (index) {
-        display->core.send(display->core.bus, false, false, ((uint8_t*)buffer), index * 2);
+        display->core.send(display->core.bus, DISPLAY_DATA, CHIP_SELECT_UNTOUCHED, ((uint8_t*)buffer), index * 2);
     }
 }

shared-module/displayio/ColorConverter.c

@@ -26,6 +26,8 @@
 
 #include "shared-bindings/displayio/ColorConverter.h"
 
+#include "py/misc.h"
+
 void common_hal_displayio_colorconverter_construct(displayio_colorconverter_t* self) {
 }
 
@@ -45,49 +47,21 @@ uint8_t displayio_colorconverter_compute_luma(uint32_t color_rgb888) {
     return (r8 * 19) / 255 + (g8 * 182) / 255 + (b8 + 54) / 255;
 }
 
-void compute_bounds(uint8_t r8, uint8_t g8, uint8_t b8, uint8_t* min, uint8_t* max) {
-    if (r8 > g8) {
-        if (b8 > r8) {
-            *max = b8;
-        } else {
-            *max = r8;
-        }
-        if (b8 < g8) {
-            *min = b8;
-        } else {
-            *min = g8;
-        }
-    } else {
-        if (b8 > g8) {
-            *max = b8;
-        } else {
-            *max = g8;
-        }
-        if (b8 < r8) {
-            *min = b8;
-        } else {
-            *min = r8;
-        }
-    }
-}
-
 uint8_t displayio_colorconverter_compute_chroma(uint32_t color_rgb888) {
     uint32_t r8 = (color_rgb888 >> 16);
     uint32_t g8 = (color_rgb888 >> 8) & 0xff;
     uint32_t b8 = color_rgb888 & 0xff;
-    uint8_t max;
-    uint8_t min;
-    compute_bounds(r8, g8, b8, &min, &max);
+    uint8_t max = MAX(r8, MAX(g8, b8));
+    uint8_t min = MIN(r8, MIN(g8, b8));
     return max - min;
 }
 
 uint8_t displayio_colorconverter_compute_hue(uint32_t color_rgb888) {
     uint32_t r8 = (color_rgb888 >> 16);
     uint32_t g8 = (color_rgb888 >> 8) & 0xff;
     uint32_t b8 = color_rgb888 & 0xff;
-    uint8_t max;
-    uint8_t min;
-    compute_bounds(r8, g8, b8, &min, &max);
+    uint8_t max = MAX(r8, MAX(g8, b8));
+    uint8_t min = MIN(r8, MIN(g8, b8));
     uint8_t c = max - min;
     if (c == 0) {
         return 0;