-
Notifications
You must be signed in to change notification settings - Fork 19
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
Ported writeRect to write 16bpp pixmaps from PaulStoffregen/ILI9341_t3 as well as DemoSauce examples #1
Conversation
This is not my code, it was ported from https://github.com/PaulStoffregen/ILI9341_t3
requires this patch: espressif/WROVER_KIT_LCD#1
Adafruit_ILI9341.cpp
Outdated
|
||
// This code was ported/adapted from https://github.com/PaulStoffregen/ILI9341_t3 | ||
// by Marc MERLIN | ||
void Adafruit_ILI9341::setAddr(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method already exists ;) it's setAddressWindow
Adafruit_ILI9341.cpp
Outdated
void Adafruit_ILI9341::writeRect(int16_t x, int16_t y, int16_t w, int16_t h, const uint16_t *pcolors) { | ||
startWrite(); | ||
setAddr(x, y, x+w-1, y+h-1); | ||
// setaddrwindow does not work if x!=1 (the picture gets offset) but setAddr works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I get this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read setAddressWindow ;) setAddr(x, y, x+w-1, y+h-1); == setAddessWindow(x,y,w,h)
Adafruit_ILI9341.cpp
Outdated
setAddr(x, y, x+w-1, y+h-1); | ||
// setaddrwindow does not work if x!=1 (the picture gets offset) but setAddr works | ||
//setAddrWindow(x, y, x+w-1, y+h-1); | ||
for(y=h; y>0; y--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is equal to writePixels(pcolors, w*h)
Adafruit_ILI9341.cpp
Outdated
startWrite(); | ||
setAddr(x, y, x+w-1, y+h-1); | ||
// setaddrwindow does not work if x!=1 (the picture gets offset) but setAddr works | ||
//setAddrWindow(x, y, x+w-1, y+h-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling it wrong in this comment
Adafruit_ILI9341.cpp
Outdated
} | ||
SPI_WRITE16(*pcolors++); | ||
} | ||
endWrite(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this method calls startWrite and endWrite, it should be called drawRect not writeRect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
void Adafruit_ILI9341::drawRect(int16_t x, int16_t y, int16_t w, int16_t h, const uint16_t *pcolors) {
startWrite();
setAddressWindow(x,y,w,h);
writePixels(pcolors, w*h);
endWrite();
} |
Thank you for your review, clearly it was a bit too late last night for me to get rid of setAddr from the other code branch. |
The original suggestion of using drawRect was a bad idea in the end since it conflicts with another totally different drawRect method in the T3 lib. drawBitmap is a better name to say what this does anyway.
Note that this requires espressif/WROVER_KIT_LCD#1 for the graphics support missing from the stock Adafruit library. - changed baud rate to 115200 - on my iotuz board, some demos crash the board, likely due to lack of power from the VREG. I've enabled DEBUG_ANIM = true Please try setting it to false to see if the other demos work in sequence. - analogWrite does not exist on ESP32, change to digitalWrite. - disable font support (not ported/impleneted)
…ith lib. - added missing setScroll and alternate interface for drawRect - modified DemoSauce to work with new Adafruit_ILI9341 lib. Awesome demo! Please try to reset 'DEBUG_ANIM = false' in DemoSauce.
Ok, I used the time to finish porting the methods from https://github.com/PaulStoffregen/ILI9341_t3 This should be a great stress test for your library. Sadly, on my board some of the demos crash, but this may due to the weak voltage regulator on my board. |
Adafruit_ILI9341.cpp
Outdated
void Adafruit_ILI9341::drawBitmap(int16_t x, int16_t y, int16_t w, int16_t h, const uint16_t *pcolors) { | ||
startWrite(); | ||
setAddrWindow(x, y, w, h); | ||
for(y=h; y>0; y--) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I said. those two for loops equal writePixels(pcolors, w*h)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, I missed that bit in your comment. Fixed, tested, and it works fine with writePixels
Adafruit_ILI9341.cpp
Outdated
endWrite(); | ||
} | ||
|
||
void Adafruit_ILI9341::setScroll(uint16_t offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already in the library ;) scrollTo ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, thank you for having a look at all this, and you are of course totally correct. I got mislead by the fact that the ILI9341_VSCRSADD define was missing and assumed the method was too.
I've updated scrollTo to use ILI9341_VSCRSADD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@me-no-dev so scrollTo wsa buggy and didn't work, but I just fixed it with this last uploaded commit.
I'm now able to display a bitmap and scroll it around.
@@ -155,6 +156,19 @@ class Adafruit_ILI9341 : public Adafruit_GFX { | |||
void drawFastHLine(int16_t x, int16_t y, int16_t w, uint16_t color); | |||
void fillRect(int16_t x, int16_t y, int16_t w, int16_t h, uint16_t color); | |||
|
|||
// Compat with ILI9341_T3 lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to figure out if the names for the methods make sense with the GFX library, or if we need to move them there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, this is your call. It's kind of a shame that Paul had a fork for so long and nothing got merged, and now we have diverging names.
If you want to remove the #defines and instead change the code to use the 'native' names, I'd understand.
there are many many places where you call multiple draws in a loop of some sort. writing and not drawing could make things faster. See my other comments and look for math issues where the examples do not run. |
Added #define setScroll scrollTo to make the lib compatible with Paul's T3 library.
My apologies, this was already in my tree, but I messed up copying this over and those were missing from this PR.
I tried to address all your comments (thanks again for the feedback), but about this one: "there are many many places where you call multiple draws in a loop of some sort. writing and not drawing could make things faster. See my other comments and look for math issues where the examples do not run." |
Ok, I fixed up the pictureEmbed.ino code a bit, and added a scrollTo example in it, which also validates the now fixed and working library code. Also, I think I'm done with the changes/improvement I needed on my side, so feel free to pull my changes, modify them in whichever way you see fit, including removing things you don't like if any, and I'll resync with you after that. |
Actually commit ae3517d isn't quite good. |
about writePixels, no it does the same exact thing that the for loops did. you increment the input address with each loop. And if you see the inner for loop, it counts down to 1 and not 0, but then it writes another pixel, which is the same exact thing. As for scroll to, let me see what I have here (cuz I use it)... Yes damned... I must have translated it wrong when I was moving from C to C++ |
@me-no-dev I understand they should be the same, but they are not. So, clearly the picture looks damaged with |
I would rather get writePixels to work ;) if it is as you say, it's unusable |
Code change suggestion from me-no-dev
@me-no-dev |
I'm close to having my code ready for release, but having it depend on an unmerged pull request against the TFT lib, is non ideal :) |
@marcmerlin I have started working with the LCD and the code that I need to create (which includes images and whatnot) but have been delayed because the new wrover kits come with ST7789V instead of ILI9341 and I need to adjust the drivers. @ladyada has already merged the changes and we actually need to sync to that and then add the needed method in both drivers or if I can go with just one driver for AVR it will be even better. |
@marcmerlin so... I fixed writePixels ;) and talked to ladyada to fork the ili driver to WROVER_LCD and work on that instead. New WROVERs have different LCD chip ST7789V which is really similar to the ILI but has some differences in init and rotation procedures (so far). |
So, I think we can close this pull request, but you said you had a new drawBitmap and much more such methods you wrote, but I'm not seeing them in this code. |
this PR is now obsolete, but #2 replaces it by adding the one method that's still missing to draw bitmaps |
This is not my code, it was ported from
https://github.com/PaulStoffregen/ILI9341_t3
Tested on ESP32, allows writing 16bpp pixmaps defined in a C file.