Skip to content

Fix BusDisplay transaction handling #10571

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix BusDisplay transaction handling #10571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions shared-module/busdisplay/BusDisplay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,10 @@ static bool _refresh_area(busdisplay_busdisplay_obj_t *self, const displayio_are
}
remaining_rows -= rows_per_buffer;

displayio_display_bus_set_region_to_update(&self->bus, &self->core, &subrectangle);
if (!displayio_display_bus_set_region_to_update(&self->bus, &self->core, &subrectangle)) {
// This is a failure to acquire the bus, skip the rest of the refresh.
return false;
}

uint16_t subrectangle_size_bytes;
if (self->core.colorspace.depth >= 8) {
Expand All @@ -287,12 +290,11 @@ static bool _refresh_area(busdisplay_busdisplay_obj_t *self, const displayio_are

displayio_display_core_fill_area(&self->core, &subrectangle, mask, buffer);

// Can't acquire display bus; skip the rest of the data.
if (!displayio_display_bus_is_free(&self->bus)) {
// If we can't acquire display bus; skip the rest of the data.
if (!displayio_display_bus_begin_transaction(&self->bus)) {
return false;
}

displayio_display_bus_begin_transaction(&self->bus);
_send_pixels(self, (uint8_t *)buffer, subrectangle_size_bytes);
displayio_display_bus_end_transaction(&self->bus);

Expand All @@ -310,7 +312,7 @@ static bool _refresh_area(busdisplay_busdisplay_obj_t *self, const displayio_are

static void _refresh_display(busdisplay_busdisplay_obj_t *self) {
if (!displayio_display_bus_is_free(&self->bus)) {
// A refresh on this bus is already in progress. Try next display.
// Don't bother with the refresh if the bus is already busy
return;
}
displayio_display_core_start_refresh(&self->core);
Expand Down
18 changes: 9 additions & 9 deletions shared-module/displayio/bus_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ void displayio_display_bus_end_transaction(displayio_display_bus_t *self) {
self->end_transaction(self->bus);
}

void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, displayio_display_core_t *display, displayio_area_t *area) {
// Determine if this is run from a background task or not. Should we skip sending the data or wait for the bus?
bool displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, displayio_display_core_t *display, displayio_area_t *area) {
uint16_t x1 = area->x1 + self->colstart;
uint16_t x2 = area->x2 + self->colstart;
uint16_t y1 = area->y1 + self->rowstart;
Expand All @@ -127,8 +128,12 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
chip_select = CHIP_SELECT_TOGGLE_EVERY_BYTE;
}

if (!displayio_display_bus_begin_transaction(self)) {
// Can't acquire display bus; skip and report failure.
return false;
}

// Set column.
displayio_display_bus_begin_transaction(self);
uint8_t data[5];
data[0] = self->column_command;
uint8_t data_length = 1;
Expand Down Expand Up @@ -163,20 +168,16 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
}

self->send(self->bus, data_type, chip_select, data, data_length);
displayio_display_bus_end_transaction(self);

if (self->set_current_column_command != NO_COMMAND) {
uint8_t command = self->set_current_column_command;
displayio_display_bus_begin_transaction(self);
self->send(self->bus, DISPLAY_COMMAND, chip_select, &command, 1);
// Only send the first half of data because it is the first coordinate.
self->send(self->bus, DISPLAY_DATA, chip_select, data, data_length / 2);
displayio_display_bus_end_transaction(self);
}
Comment on lines -166 to 177
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried all of these deletes are going to break something due to changing chip select issues. I wonder if we should split out the bus locking from chip select.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I was unaware that some display drivers are fussy about when chip selects are toggled. I can change it back into individual bus transactions for now, and spin in any cmd sequences. This would eliminate the possibility of regressions.



// Set row.
displayio_display_bus_begin_transaction(self);
data[0] = self->row_command;
data_length = 1;
if (!self->data_as_commands) {
Expand Down Expand Up @@ -207,16 +208,15 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
}

self->send(self->bus, data_type, chip_select, data, data_length);
displayio_display_bus_end_transaction(self);

if (self->set_current_row_command != NO_COMMAND) {
uint8_t command = self->set_current_row_command;
displayio_display_bus_begin_transaction(self);
self->send(self->bus, DISPLAY_COMMAND, chip_select, &command, 1);
// Only send the first half of data because it is the first coordinate.
self->send(self->bus, DISPLAY_DATA, chip_select, data, data_length / 2);
displayio_display_bus_end_transaction(self);
}
displayio_display_bus_end_transaction(self);
return true;
}

void displayio_display_bus_collect_ptrs(displayio_display_bus_t *self) {
Expand Down
2 changes: 1 addition & 1 deletion shared-module/displayio/bus_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ bool displayio_display_bus_is_free(displayio_display_bus_t *self);
bool displayio_display_bus_begin_transaction(displayio_display_bus_t *self);
void displayio_display_bus_end_transaction(displayio_display_bus_t *self);

void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, displayio_display_core_t *display, displayio_area_t *area);
bool displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, displayio_display_core_t *display, displayio_area_t *area);

void release_display_bus(displayio_display_bus_t *self);

Expand Down
33 changes: 22 additions & 11 deletions shared-module/epaperdisplay/EPaperDisplay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,9 @@ static void send_command_sequence(epaperdisplay_epaperdisplay_obj_t *self,
data_size = ((data_size & ~DELAY) << 8) + *(cmd + 2);
data = cmd + 3;
}
displayio_display_bus_begin_transaction(&self->bus);
while (!displayio_display_bus_begin_transaction(&self->bus)) {
RUN_BACKGROUND_TASKS;
}
self->bus.send(self->bus.bus, DISPLAY_COMMAND, self->chip_select, cmd, 1);
self->bus.send(self->bus.bus, DISPLAY_DATA, self->chip_select, data, data_size);
displayio_display_bus_end_transaction(&self->bus);
Expand Down Expand Up @@ -310,14 +312,21 @@ static bool epaperdisplay_epaperdisplay_refresh_area(epaperdisplay_epaperdisplay
uint16_t remaining_rows = displayio_area_height(&clipped);

if (self->bus.row_command != NO_COMMAND) {
displayio_display_bus_set_region_to_update(&self->bus, &self->core, &clipped);
if (!displayio_display_bus_set_region_to_update(&self->bus, &self->core, &clipped)) {
// This is a failure to acquire the bus, skip the rest of the refresh.
return false;
}
}

uint8_t write_command = self->write_black_ram_command;
if (pass == 1) {
write_command = self->write_color_ram_command;
}
displayio_display_bus_begin_transaction(&self->bus);

if (!displayio_display_bus_begin_transaction(&self->bus)) {
// We can't acquire the bus skip the rest of the refresh.
return false;
}
self->bus.send(self->bus.bus, DISPLAY_COMMAND, self->chip_select, &write_command, 1);
displayio_display_bus_end_transaction(&self->bus);

Expand Down Expand Up @@ -394,24 +403,26 @@ static bool _clean_area(epaperdisplay_epaperdisplay_obj_t *self) {
memset(buffer, 0x77, width / 2);

uint8_t write_command = self->write_black_ram_command;
displayio_display_bus_begin_transaction(&self->bus);

if (!displayio_display_bus_begin_transaction(&self->bus)) {
// Can't acquire display bus; skip the rest of the data. Try next display.
return false;
}
self->bus.send(self->bus.bus, DISPLAY_COMMAND, self->chip_select, &write_command, 1);
displayio_display_bus_end_transaction(&self->bus);

for (uint16_t j = 0; j < height; j++) {
if (!displayio_display_bus_begin_transaction(&self->bus)) {
// Can't acquire display bus; skip the rest of the data. Try next display.
return false;
}
self->bus.send(self->bus.bus, DISPLAY_DATA, self->chip_select, buffer, width / 2);
displayio_display_bus_end_transaction(&self->bus);

// TODO(tannewt): Make refresh displays faster so we don't starve other
// background tasks.
#if CIRCUITPY_TINYUSB
usb_background();
displayio_display_bus_end_transaction(&self->bus);
do {
usb_background();
} while (!displayio_display_bus_begin_transaction(&self->bus));
#endif
}
displayio_display_bus_end_transaction(&self->bus);

return true;
}
Expand Down