Added option to allow Displays with Single Byte Boundaries #1708
Conversation
shared-module/displayio/Display.c
Outdated
| if (self->init_cs_toggle && self->set_cs != NULL) { | ||
| self->set_cs(self->bus, true); | ||
| self->set_cs(self->bus, false); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you. It's initialization code, so it doesn't hurt us much if it takes a few ms longer.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it was appropriate, and a small delay is good. Microcontrollers will only get faster.
|
Please rebase this so it doesn't include the chip select changes. Thanks! |
…to ssd1351-fix
|
Done. |
|
Looks like Travis is failing with "git submodule update --init --recursive". Any ideas? |
|
@makermelissa The lwip server is unhappy. Waiting and then retry should work. |
|
Ok, thanks! |
| 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This fixes #1683 and allows the SSD1351 display to work with displayio.