From f477848ac1de15d023d049261b9083ee9b618c45 Mon Sep 17 00:00:00 2001 From: Radomir Dopieralski Date: Sat, 5 Mar 2022 17:06:18 +0100 Subject: [PATCH 1/3] paralleldisplay: reset and read pins should be optional The ``reset`` and ``read`` pins should be optional, but the espressif code had several places where it assumed they are not, and a bug that caused a crash on ``release_displays`` if they were made optional. The bug was caused by the fields for storing pin numbers being set to ``NO_PIN``, which has value of -1, while the fields have type ``uint8_t``. That set the actual value to 255, and a subsequent comparison to ``NO_PIN`` returned false. --- .../common-hal/paralleldisplay/ParallelBus.c | 17 +++++++---------- .../common-hal/paralleldisplay/ParallelBus.h | 4 ++-- shared-bindings/paralleldisplay/ParallelBus.c | 14 +++++++------- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/ports/espressif/common-hal/paralleldisplay/ParallelBus.c b/ports/espressif/common-hal/paralleldisplay/ParallelBus.c index b9ee1f055d..1b6ee41c40 100644 --- a/ports/espressif/common-hal/paralleldisplay/ParallelBus.c +++ b/ports/espressif/common-hal/paralleldisplay/ParallelBus.c @@ -72,13 +72,6 @@ void common_hal_paralleldisplay_parallelbus_construct_nonsequential(paralleldisp .buffer_size = 512, }; - if (reset != NULL) { - common_hal_never_reset_pin(reset); - self->reset_pin_number = reset->number; - } else { - self->reset_pin_number = NO_PIN; - } - for (uint8_t i = 0; i < n_pins; i++) { common_hal_never_reset_pin(data_pins[i]); config.pin_data_num[i] = common_hal_mcu_pin_number(data_pins[i]); @@ -98,10 +91,14 @@ void common_hal_paralleldisplay_parallelbus_construct_nonsequential(paralleldisp gpio_set_level(read->number, true); } + self->reset_pin_number = NO_PIN; + if (reset != NULL) { + common_hal_never_reset_pin(reset); + self->reset_pin_number = reset->number; + } + common_hal_never_reset_pin(chip_select); common_hal_never_reset_pin(command); - common_hal_never_reset_pin(read); - common_hal_never_reset_pin(reset); common_hal_never_reset_pin(write); self->config = config; @@ -140,8 +137,8 @@ void common_hal_paralleldisplay_parallelbus_deinit(paralleldisplay_parallelbus_o reset_pin_number(self->config.pin_num_cs); reset_pin_number(self->config.pin_num_wr); - reset_pin_number(self->read_pin_number); reset_pin_number(self->config.pin_num_rs); + reset_pin_number(self->read_pin_number); reset_pin_number(self->reset_pin_number); port_i2s_reset_instance(0); diff --git a/ports/espressif/common-hal/paralleldisplay/ParallelBus.h b/ports/espressif/common-hal/paralleldisplay/ParallelBus.h index 1c84d9b421..b5727a9605 100644 --- a/ports/espressif/common-hal/paralleldisplay/ParallelBus.h +++ b/ports/espressif/common-hal/paralleldisplay/ParallelBus.h @@ -32,8 +32,8 @@ typedef struct { mp_obj_base_t base; - uint8_t read_pin_number; - uint8_t reset_pin_number; + int8_t read_pin_number; + int8_t reset_pin_number; i2s_lcd_config_t config; i2s_lcd_handle_t handle; } paralleldisplay_parallelbus_obj_t; diff --git a/shared-bindings/paralleldisplay/ParallelBus.c b/shared-bindings/paralleldisplay/ParallelBus.c index be69ce1f95..6e6e778259 100644 --- a/shared-bindings/paralleldisplay/ParallelBus.c +++ b/shared-bindings/paralleldisplay/ParallelBus.c @@ -42,7 +42,7 @@ //| protocol may be refered to as 8080-I Series Parallel Interface in datasheets. It doesn't handle //| display initialization.""" //| -//| def __init__(self, *, data0: microcontroller.Pin, command: microcontroller.Pin, chip_select: microcontroller.Pin, write: microcontroller.Pin, read: microcontroller.Pin, reset: microcontroller.Pin, frequency: int = 30_000_000) -> None: +//| def __init__(self, *, data0: microcontroller.Pin, command: microcontroller.Pin, chip_select: microcontroller.Pin, write: microcontroller.Pin, read: Optional[microcontroller.Pin], reset: Optional[microcontroller.Pin] = None, frequency: int = 30_000_000) -> None: //| """Create a ParallelBus object associated with the given pins. The bus is inferred from data0 //| by implying the next 7 additional pins on a given GPIO port. //| @@ -56,8 +56,8 @@ //| :param microcontroller.Pin command: Data or command pin //| :param microcontroller.Pin chip_select: Chip select pin //| :param microcontroller.Pin write: Write pin -//| :param microcontroller.Pin read: Read pin -//| :param microcontroller.Pin reset: Reset pin +//| :param microcontroller.Pin read: Read pin, optional +//| :param microcontroller.Pin reset: Reset pin, optional //| :param int frequency: The communication frequency in Hz for the display on the bus""" //| ... //| @@ -69,8 +69,8 @@ STATIC mp_obj_t paralleldisplay_parallelbus_make_new(const mp_obj_type_t *type, { MP_QSTR_command, MP_ARG_OBJ | MP_ARG_KW_ONLY | MP_ARG_REQUIRED }, { MP_QSTR_chip_select, MP_ARG_OBJ | MP_ARG_KW_ONLY | MP_ARG_REQUIRED }, { MP_QSTR_write, MP_ARG_OBJ | MP_ARG_KW_ONLY | MP_ARG_REQUIRED }, - { MP_QSTR_read, MP_ARG_OBJ | MP_ARG_KW_ONLY | MP_ARG_REQUIRED }, - { MP_QSTR_reset, MP_ARG_OBJ | MP_ARG_KW_ONLY | MP_ARG_REQUIRED }, + { MP_QSTR_read, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_obj = mp_const_none } }, + { MP_QSTR_reset, MP_ARG_OBJ | MP_ARG_KW_ONLY, {.u_obj = mp_const_none } }, { MP_QSTR_frequency, MP_ARG_INT | MP_ARG_KW_ONLY, {.u_int = 30000000 } }, }; mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; @@ -79,8 +79,8 @@ STATIC mp_obj_t paralleldisplay_parallelbus_make_new(const mp_obj_type_t *type, const mcu_pin_obj_t *command = validate_obj_is_free_pin(args[ARG_command].u_obj); const mcu_pin_obj_t *chip_select = validate_obj_is_free_pin(args[ARG_chip_select].u_obj); const mcu_pin_obj_t *write = validate_obj_is_free_pin(args[ARG_write].u_obj); - const mcu_pin_obj_t *read = validate_obj_is_free_pin(args[ARG_read].u_obj); - const mcu_pin_obj_t *reset = validate_obj_is_free_pin(args[ARG_reset].u_obj); + const mcu_pin_obj_t *read = validate_obj_is_free_pin_or_none(args[ARG_read].u_obj); + const mcu_pin_obj_t *reset = validate_obj_is_free_pin_or_none(args[ARG_reset].u_obj); paralleldisplay_parallelbus_obj_t *self = &allocate_display_bus_or_raise()->parallel_bus; self->base.type = ¶lleldisplay_parallelbus_type; From b5ad78715ce03ad475777047169a791be6237c5e Mon Sep 17 00:00:00 2001 From: Radomir Dopieralski Date: Sat, 5 Mar 2022 21:48:00 +0100 Subject: [PATCH 2/3] Update ports/espressif/common-hal/paralleldisplay/ParallelBus.h Co-authored-by: Dan Halbert --- ports/espressif/common-hal/paralleldisplay/ParallelBus.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ports/espressif/common-hal/paralleldisplay/ParallelBus.h b/ports/espressif/common-hal/paralleldisplay/ParallelBus.h index b5727a9605..c46d3cb0b8 100644 --- a/ports/espressif/common-hal/paralleldisplay/ParallelBus.h +++ b/ports/espressif/common-hal/paralleldisplay/ParallelBus.h @@ -32,8 +32,8 @@ typedef struct { mp_obj_base_t base; - int8_t read_pin_number; - int8_t reset_pin_number; + gpio_num_t read_pin_number; + gpio_num_t reset_pin_number; i2s_lcd_config_t config; i2s_lcd_handle_t handle; } paralleldisplay_parallelbus_obj_t; From b69a06b2ed2a85a282d59fab4a428cbfb8742f4e Mon Sep 17 00:00:00 2001 From: Radomir Dopieralski Date: Sun, 6 Mar 2022 11:33:07 +0100 Subject: [PATCH 3/3] Also fix the read pin in the atmel and rp2040 ports --- .../common-hal/paralleldisplay/ParallelBus.c | 11 +++++++---- .../common-hal/paralleldisplay/ParallelBus.c | 19 +++++++++++++------ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/ports/atmel-samd/common-hal/paralleldisplay/ParallelBus.c b/ports/atmel-samd/common-hal/paralleldisplay/ParallelBus.c index 490c595e74..10407d3b3a 100644 --- a/ports/atmel-samd/common-hal/paralleldisplay/ParallelBus.c +++ b/ports/atmel-samd/common-hal/paralleldisplay/ParallelBus.c @@ -69,9 +69,13 @@ void common_hal_paralleldisplay_parallelbus_construct(paralleldisplay_parallelbu common_hal_digitalio_digitalinout_construct(&self->write, write); common_hal_digitalio_digitalinout_switch_to_output(&self->write, true, DRIVE_MODE_PUSH_PULL); - self->read.base.type = &digitalio_digitalinout_type; - common_hal_digitalio_digitalinout_construct(&self->read, read); - common_hal_digitalio_digitalinout_switch_to_output(&self->read, true, DRIVE_MODE_PUSH_PULL); + self->read.base.type = &mp_type_NoneType; + if (read != NULL) { + self->read.base.type = &digitalio_digitalinout_type; + common_hal_digitalio_digitalinout_construct(&self->read, read); + common_hal_digitalio_digitalinout_switch_to_output(&self->read, true, DRIVE_MODE_PUSH_PULL); + never_reset_pin_number(read->number); + } self->data0_pin = data_pin; self->write_group = &PORT->Group[write->number / 32]; @@ -89,7 +93,6 @@ void common_hal_paralleldisplay_parallelbus_construct(paralleldisplay_parallelbu never_reset_pin_number(command->number); never_reset_pin_number(chip_select->number); never_reset_pin_number(write->number); - never_reset_pin_number(read->number); for (uint8_t i = 0; i < 8; i++) { never_reset_pin_number(data_pin + i); } diff --git a/ports/raspberrypi/common-hal/paralleldisplay/ParallelBus.c b/ports/raspberrypi/common-hal/paralleldisplay/ParallelBus.c index 2e0ae4def2..51187a3429 100644 --- a/ports/raspberrypi/common-hal/paralleldisplay/ParallelBus.c +++ b/ports/raspberrypi/common-hal/paralleldisplay/ParallelBus.c @@ -67,9 +67,13 @@ void common_hal_paralleldisplay_parallelbus_construct(paralleldisplay_parallelbu common_hal_digitalio_digitalinout_construct(&self->chip_select, chip_select); common_hal_digitalio_digitalinout_switch_to_output(&self->chip_select, true, DRIVE_MODE_PUSH_PULL); - self->read.base.type = &digitalio_digitalinout_type; - common_hal_digitalio_digitalinout_construct(&self->read, read); - common_hal_digitalio_digitalinout_switch_to_output(&self->read, true, DRIVE_MODE_PUSH_PULL); + self->read.base.type = &mp_type_NoneType; + if (read != NULL) { + self->read.base.type = &digitalio_digitalinout_type; + common_hal_digitalio_digitalinout_construct(&self->read, read); + common_hal_digitalio_digitalinout_switch_to_output(&self->read, true, DRIVE_MODE_PUSH_PULL); + never_reset_pin_number(read->number); + } self->data0_pin = data_pin; self->write = write_pin; @@ -86,7 +90,6 @@ void common_hal_paralleldisplay_parallelbus_construct(paralleldisplay_parallelbu never_reset_pin_number(command->number); never_reset_pin_number(chip_select->number); never_reset_pin_number(write_pin); - never_reset_pin_number(read->number); for (uint8_t i = 0; i < 8; i++) { never_reset_pin_number(data_pin + i); } @@ -121,8 +124,12 @@ void common_hal_paralleldisplay_parallelbus_deinit(paralleldisplay_parallelbus_o reset_pin_number(self->command.pin->number); reset_pin_number(self->chip_select.pin->number); reset_pin_number(self->write); - reset_pin_number(self->read.pin->number); - reset_pin_number(self->reset.pin->number); + if (self->read.base.type != &mp_type_NoneType) { + reset_pin_number(self->read.pin->number); + } + if (self->reset.base.type != &mp_type_NoneType) { + reset_pin_number(self->reset.pin->number); + } } bool common_hal_paralleldisplay_parallelbus_reset(mp_obj_t obj) {