Review feedback including NO_BRIGHTNESS_COMMAND macro

6 files changed, 14 insertions, 6 deletions

diff --git a/shared-bindings/displayio/ColorConverter.c b/shared-bindings/displayio/ColorConverter.c
index e170e0340..d9524870d 100644
--- a/shared-bindings/displayio/ColorConverter.c
+++ b/shared-bindings/displayio/ColorConverter.c
@@ -62,6 +62,8 @@ STATIC mp_obj_t displayio_colorconverter_make_new(const mp_obj_type_t *type, siz
 
 //|   .. method:: convert(color)
 //|
+//|   Converts the given RGB888 color to RGB565
+//|
 STATIC mp_obj_t displayio_colorconverter_obj_convert(mp_obj_t self_in, mp_obj_t color_obj) {
     displayio_colorconverter_t *self = MP_OBJ_TO_PTR(self_in);
 
diff --git a/shared-bindings/displayio/ColorConverter.h b/shared-bindings/displayio/ColorConverter.h
index 8f61e6642..24895500e 100644
--- a/shared-bindings/displayio/ColorConverter.h
+++ b/shared-bindings/displayio/ColorConverter.h
@@ -34,6 +34,6 @@
 extern const mp_obj_type_t displayio_colorconverter_type;
 
 void common_hal_displayio_colorconverter_construct(displayio_colorconverter_t* self);
-bool common_hal_displayio_colorconverter_convert(displayio_colorconverter_t *colorconverter, const _displayio_colorspace_t* colorspace, uint32_t input_color, uint32_t* output_color);
+void common_hal_displayio_colorconverter_convert(displayio_colorconverter_t *colorconverter, const _displayio_colorspace_t* colorspace, uint32_t input_color, uint32_t* output_color);
 
 #endif // MICROPY_INCLUDED_SHARED_BINDINGS_DISPLAYIO_COLORCONVERTER_H
diff --git a/shared-bindings/displayio/Display.c b/shared-bindings/displayio/Display.c
index 2586e562f..361a37ece 100644
--- a/shared-bindings/displayio/Display.c
+++ b/shared-bindings/displayio/Display.c
@@ -119,7 +119,7 @@ STATIC mp_obj_t displayio_display_make_new(const mp_obj_type_t *type, size_t n_a
         { MP_QSTR_write_ram_command, MP_ARG_INT | MP_ARG_KW_ONLY, {.u_int = 0x2c} },
         { MP_QSTR_set_vertical_scroll, MP_ARG_INT | MP_ARG_KW_ONLY, {.u_int = 0x0} },
         { MP_QSTR_backlight_pin, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_obj = mp_const_none} },
-        { MP_QSTR_brightness_command, MP_ARG_INT | MP_ARG_KW_ONLY, {.u_int = 0x100} },
+        { MP_QSTR_brightness_command, MP_ARG_INT | MP_ARG_KW_ONLY, {.u_int = NO_BRIGHTNESS_COMMAND} },
         { MP_QSTR_brightness, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_obj = MP_OBJ_NEW_SMALL_INT(1)} },
         { MP_QSTR_auto_brightness, MP_ARG_BOOL | MP_ARG_KW_ONLY, {.u_bool = false} },
         { MP_QSTR_single_byte_bounds, MP_ARG_BOOL | MP_ARG_KW_ONLY, {.u_bool = false} },
@@ -242,10 +242,14 @@ STATIC mp_obj_t displayio_display_obj_get_brightness(mp_obj_t self_in) {
 }
 MP_DEFINE_CONST_FUN_OBJ_1(displayio_display_get_brightness_obj, displayio_display_obj_get_brightness);
 
-STATIC mp_obj_t displayio_display_obj_set_brightness(mp_obj_t self_in, mp_obj_t brightness) {
+STATIC mp_obj_t displayio_display_obj_set_brightness(mp_obj_t self_in, mp_obj_t brightness_obj) {
     displayio_display_obj_t *self = native_display(self_in);
     common_hal_displayio_display_set_auto_brightness(self, false);
-    bool ok = common_hal_displayio_display_set_brightness(self, mp_obj_get_float(brightness));
+    mp_float_t brightness = mp_obj_get_float(brightness_obj);
+    if (brightness < 0 || brightness > 1.0) {
+        mp_raise_ValueError(translate("Brightness must be 0-1.0"));
+    }
+    bool ok = common_hal_displayio_display_set_brightness(self, brightness);
     if (!ok) {
         mp_raise_RuntimeError(translate("Brightness not adjustable"));
     }
diff --git a/shared-bindings/displayio/Display.h b/shared-bindings/displayio/Display.h
index a60a0cf5a..e765e6f25 100644
--- a/shared-bindings/displayio/Display.h
+++ b/shared-bindings/displayio/Display.h
@@ -36,6 +36,8 @@ extern const mp_obj_type_t displayio_display_type;
 
 #define DELAY 0x80
 
+#define NO_BRIGHTNESS_COMMAND 0x100
+
 void common_hal_displayio_display_construct(displayio_display_obj_t* self,
     mp_obj_t bus, uint16_t width, uint16_t height,
     int16_t colstart, int16_t rowstart, uint16_t rotation, uint16_t color_depth, bool grayscale, bool pixels_in_byte_share_row,
diff --git a/shared-bindings/displayio/I2CDisplay.c b/shared-bindings/displayio/I2CDisplay.c
index 2aac49b4d..1f74e8c9e 100644
--- a/shared-bindings/displayio/I2CDisplay.c
+++ b/shared-bindings/displayio/I2CDisplay.c
@@ -103,7 +103,7 @@ STATIC mp_obj_t displayio_i2cdisplay_make_new(const mp_obj_type_t *type, size_t
 STATIC mp_obj_t displayio_i2cdisplay_obj_send(mp_obj_t self, mp_obj_t command_obj, mp_obj_t data_obj) {
     mp_int_t command_int = MP_OBJ_SMALL_INT_VALUE(command_obj);
     if (!MP_OBJ_IS_SMALL_INT(command_obj) || command_int > 255 || command_int < 0) {
-        mp_raise_ValueError(translate("Command must be an int between 0 and 255"));
+        mp_raise_ValueError(translate("Command must be 0-255"));
     }
     uint8_t command = command_int;
     mp_buffer_info_t bufinfo;
diff --git a/shared-bindings/displayio/Palette.c b/shared-bindings/displayio/Palette.c
index 974cadb02..dda58e3c5 100644
--- a/shared-bindings/displayio/Palette.c
+++ b/shared-bindings/displayio/Palette.c
@@ -74,7 +74,7 @@ STATIC mp_obj_t displayio_palette_make_new(const mp_obj_type_t *type, size_t n_a
 STATIC mp_obj_t group_unary_op(mp_unary_op_t op, mp_obj_t self_in) {
     displayio_palette_t *self = MP_OBJ_TO_PTR(self_in);
     switch (op) {
-        case MP_UNARY_OP_BOOL: return mp_obj_new_bool(true);
+        case MP_UNARY_OP_BOOL: return mp_const_true;
         case MP_UNARY_OP_LEN:
             return MP_OBJ_NEW_SMALL_INT(common_hal_displayio_palette_get_len(self));
         default: return MP_OBJ_NULL; // op not supported