Skip to content
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.

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

transferImageDMA image pointer is non-const #1478

Closed
codewitch-honey-crisis opened this issue Dec 8, 2021 · 7 comments
Closed

transferImageDMA image pointer is non-const #1478

codewitch-honey-crisis opened this issue Dec 8, 2021 · 7 comments

Comments

@codewitch-honey-crisis
Copy link

void TFT_eSPI::pushImageDMA(int32_t x, int32_t y, int32_t w, int32_t h, uint16_t* image, uint16_t* buffer)
should be
void TFT_eSPI::pushImageDMA(int32_t x, int32_t y, int32_t w, int32_t h, const uint16_t* image, uint16_t* buffer)

The reason being is images are often const and it is impossible to do asynchronous transfers with const sources because of the way you have implemented this.

There's a better way to do it, but you're using buffer to point to image in the routine and that's a non-starter.

Please fix this, as I will have to fork your code otherwise.

@Bodmer
Copy link
Owner

Bodmer commented Dec 8, 2021

The pushImageDMA function will re-arrange the data if it needs to be clipped to a display window, or if bytes need to be swapped so a const will not work. It is important that it behaves just like pushImage in this respect.

In your case I suspect pushPixelsDMA is a better option with the byte swap option removed and the pointer being const.

Thus a new function is needed.

@codewitch-honey-crisis
Copy link
Author

codewitch-honey-crisis commented Dec 8, 2021 via email

@Bodmer
Copy link
Owner

Bodmer commented Dec 9, 2021

The DMA features in the library only work with SPI interface displays, not 8 bit parallel. SPI DMA is supported for a limited number of functions for ESP32, some STM32 processors and the RP2040.

I am not sure which processor you are using, if it is the ESP32 then it is possible to get 8 bit parallel DMA working using the I2S interface features, but my experiments have not been very successful.

With the RP2040 processor I do intend to add 8 bit parallel DMA using the PIO features but it will be quite a while before that hapens.

Given all that, I can add a const type DMA block transfer without window clipping and byte swapping, this would be quite a trivial change as is is just a case of cut paste and edit.

Yes, the macros have proliferated and there are better ways of managing this. It is just how the library evolved so don't look for some deep and meaningful reasons!

@codewitch-honey-crisis
Copy link
Author

codewitch-honey-crisis commented Dec 9, 2021

I would absolutely love it if you added that const function. I preclip everything anyway, so it would just be duplicating effort.***

I realize that the DMA isn't available for parallel since the whole thing is bit banged with no hardware support. The thing is, I want my bindings to work with your driver regardless, and take advantage of whatever is there, so if the user configures your driver to run SPI on an ESP32, async ops in my library will actually async instead of running synchronously.

I never figured on using the I2S that way. I didn't even think you could map more pins that way, but maybe I misunderstand you. I thought I2S was only a handful of pins (I haven't used it before)

Thank you for the clarification on the macros. It's totally understandable that the code would evolve, and there is no reason. I was just trying to sniff around as to why my code wasn't working, even though I (thought) I was following your code. I even diffed all the pin highs and lows (except the data pins) after dumping the output to the serial port for every pin change and data write, and compared the diffs between your driver and mine. No change. I am totally flummoxed, so I guess I'm binding to your driver.
I feel a little weird about that to be honest, because GFX basically takes over all of your line and shape drawing, image drawing, font drawing, color blending, etc, but I use it because it has a lot of high level features useful for rendering commercial screens.

I notice yours has antialiased fonts now, so it makes it a bit more competitive with my True Type engine in GFX (which is only fast on a WROVER due to memory constraints) but since I pick assets based on what clients want it's nice that they can have their designers send me a TTF or OTF font to use. =)

*** regarding clipping, rather than rearranging memory, when it's not just the bottom or top that's clipped in which case I just change offsets/lengths, I fall back to a line by line draw of the bitmap and offset+adjust the stride - it doesn't let you do full DMA that way, HOWEVER, on the ESP-IDF you can queue several async transactions, so in my ESP-IDF drivers it will actually transfer chunks of the clipped bitmap via DMA by queuing several (partial) lines of the bitmap at time. It's more expensive than your method either way, but it allows clipping in place without rearranging, sacrificing some performance as a result. Eventually I plan on making GFX able to dynamically allocate buffers on the fly for situations like this or alpha blending/anti-aliasing when using them can increase performance

Bodmer added a commit that referenced this issue Dec 9, 2021
@Bodmer
Copy link
Owner

Bodmer commented Dec 9, 2021

I have created a branch 1478 for you to try.

@Bodmer Bodmer closed this as completed Dec 9, 2021
@codewitch-honey-crisis
Copy link
Author

Thank you. I'll give it a shot and let you know it goes. I'm assuming if it works, that it will eventually be rolled into the main branch? That's what I'd need in order to viably redistribute my bindings for your driver with Async support added to them.

@codewitch-honey-crisis
Copy link
Author

I tried it. It works fantastically. Thank you so much. I'm curious though, since you made the pushImageDMA function const, I take it you're no longer clipping and such. Wouldn't it be advisable to make a second function for the const version so that you don't change the behavior of existing code? Of course this only applies if you intend to update the main branch with this revision, which I hope you do because that way my bindings can work for other people as well. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants