Skip to content

Added option to allow Displays with Single Byte Boundaries #1708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 29, 2019
Merged

Added option to allow Displays with Single Byte Boundaries #1708

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c3329e2
Added Single Byte Boundaries option for certain displays
Mar 25, 2019
cc96c39
Fixed wrong operator
Mar 26, 2019
a54493b
Added small delay inside toggle for edge cases
Mar 26, 2019
5f0e71c
Merge branch 'master' of https://github.com/adafruit/circuitpython in…
Mar 27, 2019
f4cede4
Removed parameter so CS is always toggled
Mar 27, 2019
2e0268c
Simplified cs toggling into fourwire only
Mar 28, 2019
8087cb4
Merge branch 'master' of https://github.com/adafruit/circuitpython in…
Mar 28, 2019
e254597
Merge branch 'master' of https://github.com/adafruit/circuitpython in…
Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ports/atmel-samd/boards/hallowing_m0_express/board.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ void board_init(void) {
display_init_sequence,
sizeof(display_init_sequence),
&pin_PA00,
true);
true, // init_cs_toggle
false); // single_byte_bounds
common_hal_displayio_display_set_auto_brightness(display, true);
}

Expand Down
3 changes: 2 additions & 1 deletion ports/atmel-samd/boards/pybadge/board.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ void board_init(void) {
display_init_sequence,
sizeof(display_init_sequence),
&pin_PA00,
true);
true, // init_cs_toggle
false); // single_byte_bounds
common_hal_displayio_display_set_auto_brightness(display, true);
}

Expand Down
4 changes: 3 additions & 1 deletion ports/atmel-samd/boards/pyportal/board.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ void board_init(void) {
display_init_sequence,
sizeof(display_init_sequence),
&pin_PB31,
true);
true, // init_cs_toggle
false); // single_byte_bounds

common_hal_displayio_display_set_auto_brightness(display, true);
}

Expand Down
11 changes: 7 additions & 4 deletions shared-bindings/displayio/Display.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
//| Most people should not use this class directly. Use a specific display driver instead that will
//| contain the initialization sequence at minimum.
//|
//| .. class:: Display(display_bus, init_sequence, *, width, height, colstart=0, rowstart=0, rotation=0, color_depth=16, set_column_command=0x2a, set_row_command=0x2b, write_ram_command=0x2c, set_vertical_scroll=0, backlight_pin=None, init_cs_toggle=False)
//| .. class:: Display(display_bus, init_sequence, *, width, height, colstart=0, rowstart=0, rotation=0, color_depth=16, set_column_command=0x2a, set_row_command=0x2b, write_ram_command=0x2c, set_vertical_scroll=0, backlight_pin=None, init_cs_toggle=True, single_byte_bounds=False)
//|
//| Create a Display object on the given display bus (`displayio.FourWire` or `displayio.ParallelBus`).
//|
Expand Down Expand Up @@ -91,10 +91,11 @@
//| :param int write_ram_command: Command used to write pixels values into the update region
//| :param int set_vertical_scroll: Command used to set the first row to show
//| :param microcontroller.Pin backlight_pin: Pin connected to the display's backlight
//| :param bool init_cs_toggle: Toggle the Chip Select between each initialization command.
//| :param bool init_cs_toggle: Toggle the Chip Select between each initialization command
//| :param bool single_byte_bounds: Display column and row commands use single bytes
//|
STATIC mp_obj_t displayio_display_make_new(const mp_obj_type_t *type, size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
enum { ARG_display_bus, ARG_init_sequence, ARG_width, ARG_height, ARG_colstart, ARG_rowstart, ARG_rotation, ARG_color_depth, ARG_set_column_command, ARG_set_row_command, ARG_write_ram_command, ARG_set_vertical_scroll, ARG_backlight_pin, ARG_init_cs_toggle };
enum { ARG_display_bus, ARG_init_sequence, ARG_width, ARG_height, ARG_colstart, ARG_rowstart, ARG_rotation, ARG_color_depth, ARG_set_column_command, ARG_set_row_command, ARG_write_ram_command, ARG_set_vertical_scroll, ARG_backlight_pin, ARG_init_cs_toggle, ARG_single_byte_bounds };
static const mp_arg_t allowed_args[] = {
{ MP_QSTR_display_bus, MP_ARG_REQUIRED | MP_ARG_OBJ },
{ MP_QSTR_init_sequence, MP_ARG_REQUIRED | MP_ARG_OBJ },
Expand All @@ -110,6 +111,7 @@ STATIC mp_obj_t displayio_display_make_new(const mp_obj_type_t *type, size_t n_a
{ 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_init_cs_toggle, MP_ARG_BOOL | MP_ARG_KW_ONLY, {.u_bool = true} },
{ MP_QSTR_single_byte_bounds, MP_ARG_BOOL | MP_ARG_KW_ONLY, {.u_bool = false} },
};
mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
mp_arg_parse_all(n_args, pos_args, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args);
Expand Down Expand Up @@ -149,7 +151,8 @@ STATIC mp_obj_t displayio_display_make_new(const mp_obj_type_t *type, size_t n_a
args[ARG_write_ram_command].u_int,
args[ARG_set_vertical_scroll].u_int,
bufinfo.buf, bufinfo.len, MP_OBJ_TO_PTR(backlight_pin),
args[ARG_init_cs_toggle].u_bool);
args[ARG_init_cs_toggle].u_bool,
args[ARG_single_byte_bounds].u_bool);

return self;
}
Expand Down
3 changes: 2 additions & 1 deletion shared-bindings/displayio/Display.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ 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,
uint8_t set_column_command, uint8_t set_row_command, uint8_t write_ram_command, uint8_t set_vertical_scroll,
uint8_t* init_sequence, uint16_t init_sequence_len, const mcu_pin_obj_t* backlight_pin, bool init_cs_toggle);
uint8_t* init_sequence, uint16_t init_sequence_len, const mcu_pin_obj_t* backlight_pin, bool init_cs_toggle,
bool single_byte_bounds);

int32_t common_hal_displayio_display_wait_for_frame(displayio_display_obj_t* self);

Expand Down
27 changes: 19 additions & 8 deletions shared-module/displayio/Display.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ 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, uint8_t set_column_command, uint8_t set_row_command,
uint8_t write_ram_command, uint8_t set_vertical_scroll, uint8_t* init_sequence, uint16_t init_sequence_len,
const mcu_pin_obj_t* backlight_pin, bool init_cs_toggle) {
const mcu_pin_obj_t* backlight_pin, bool init_cs_toggle, bool single_byte_bounds) {
self->color_depth = color_depth;
self->set_column_command = set_column_command;
self->set_row_command = set_row_command;
Expand All @@ -55,6 +55,7 @@ void common_hal_displayio_display_construct(displayio_display_obj_t* self,
self->rowstart = rowstart;
self->auto_brightness = false;
self->init_cs_toggle = init_cs_toggle;
self->single_byte_bounds = single_byte_bounds;

if (MP_OBJ_IS_TYPE(bus, &displayio_parallelbus_type)) {
self->begin_transaction = common_hal_displayio_parallelbus_begin_transaction;
Expand Down Expand Up @@ -85,6 +86,7 @@ void common_hal_displayio_display_construct(displayio_display_obj_t* self,
uint8_t *data = cmd + 2;
if (self->init_cs_toggle && self->set_cs != NULL) {
self->set_cs(self->bus, true);
common_hal_time_delay_ms(1);
self->set_cs(self->bus, false);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is going to be reliable, especially on the faster MCUs and with longer wires going to the display — the voltage might have not enough time to grow to TTL levels. I wonder if it wouldn't be better to toggle the CS pin at the start and at the end of the delay later in this loop.

Copy link

@deshipu deshipu Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically leave the self->set_cs(self->bus, false) here, and move the self->set_cs(self->bus, true) to right after the send commands. Of course then we will need one more just before the loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason I have the toggle commands right next to each other is because that’s how it functions in the Arduino driver.

I originally had it after the send command and it required adding an additional reset for the ST7789 display to function for it to work, which is the main reason I added this. When I moved it to before the send commands based on the comments by @ladyada, it worked much better.

Also, I was testing with relatively long wires hooked up (about 6” or so) using an M4 processor and it worked consistently in this manner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it for a bit, I could dd an arbitrary amount of delay between (1-2ms) between the commands if that would make you feel more comfortable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. It's initialization code, so it doesn't hurt us much if it takes a few ms longer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked in the datasheet to see about minimum CS toggle times, and the minimum times for CS setup are in the tens of nanoseconds, so it seems like this will not be an issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, sorry for being paranoid.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was appropriate, and a small delay is good. Microcontrollers will only get faster.

self->send(self->bus, true, cmd, 1);
Expand Down Expand Up @@ -218,16 +220,25 @@ void displayio_display_end_transaction(displayio_display_obj_t* self) {
}

void displayio_display_set_region_to_update(displayio_display_obj_t* self, uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1) {
// TODO(tannewt): Handle displays with single byte bounds.
uint16_t data[2];
self->send(self->bus, true, &self->set_column_command, 1);
data[0] = __builtin_bswap16(x0 + self->colstart);
data[1] = __builtin_bswap16(x1 - 1 + self->colstart);
self->send(self->bus, false, (uint8_t*) data, 4);
if (self->single_byte_bounds) {
data[0] = __builtin_bswap16(((x0 + self->colstart) << 8) | ((x1 - 1 + self->colstart) & 0xFF));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using bswap and shifts here, put the data declaration inside the if and change it to uint8_t. That will isolate the individual values and make it clearer to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on readability. I was just hoping it would make the program a little smaller when I did it this way. Thanks for merging it in. I can make the readability improvement for the next release.

self->send(self->bus, false, (uint8_t*) data, 2);
} else {
data[0] = __builtin_bswap16(x0 + self->colstart);
data[1] = __builtin_bswap16(x1 - 1 + self->colstart);
self->send(self->bus, false, (uint8_t*) data, 4);
}
self->send(self->bus, true, &self->set_row_command, 1);
data[0] = __builtin_bswap16(y0 + self->rowstart);
data[1] = __builtin_bswap16(y1 - 1 + self->rowstart);
self->send(self->bus, false, (uint8_t*) data, 4);
if (self->single_byte_bounds) {
data[0] = __builtin_bswap16(((y0 + self->rowstart) << 8) | ((y1 - 1 + self->rowstart) & 0xFF));
self->send(self->bus, false, (uint8_t*) data, 2);
} else {
data[0] = __builtin_bswap16(y0 + self->rowstart);
data[1] = __builtin_bswap16(y1 - 1 + self->rowstart);
self->send(self->bus, false, (uint8_t*) data, 4);
}
self->send(self->bus, true, &self->write_ram_command, 1);
}

Expand Down
1 change: 1 addition & 0 deletions shared-module/displayio/Display.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ typedef struct {
int16_t colstart;
int16_t rowstart;
bool init_cs_toggle;
bool single_byte_bounds;
display_bus_begin_transaction begin_transaction;
display_bus_send send;
display_bus_end_transaction end_transaction;
Expand Down