lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
250 stars 161 forks source link

generic st77xx, xpt2046 driver; rp2 DMA driver; example #210

Closed eudoxos closed 2 years ago

eudoxos commented 2 years ago

This PR contains

Thanks to @jgpeiro for the initial implementation, @amirgon for help in this discussion.

eudoxos commented 2 years ago

All the points resolved, except:

I added the constants for rotation even though I don't find them much useful, and they are now defined independently (thoguh identically) for ST77XX_ and XPT2046_.

eudoxos commented 2 years ago

@amirgon no pressure, just let me know if there is something more to do. I will be happy if the PR is merged (even with some small TODOs for later) so that my brain can mark it as done and drop it from the hot storage :)

amirgon commented 2 years ago

just let me know if there is something more to do

I actually wasn't sure if you are still working on that or if you are done.
Could you please go over our open discussions above and comment on each? (for example, you can write you implemented something as suggested, or differently, or not at all etc.)
Then we could either continue our discussions or resolve them when we come to an agreement.

I will be happy if the PR is merged (even with some small TODOs for later)

I would very much like to see this PR merged, but I want to merge it as cleanly as possible and not to leave TODOs for later, if we can avoid that. If we leave TODOs, there is a good chance some of them will never be addressed.

Another challenge for me is that I'm reviewing most of this "blindly" without testing (because I'm not using rp2). The best thing could be to have one more reviewer go over your PR, someone who is familiar with rp2 and able to test your code independently.

eudoxos commented 2 years ago

Thanks for your explanation. I added explanations to resolved conversations, still see some pending comments which I wrote though (they are marked as such by github, not sure what that means).

Outstanding points:

eudoxos commented 2 years ago

And yes, I understand it would be good if someone with the hardware could test and have a look, for sure. I appreciate your effort without having the hardware, really.

embeddedt commented 2 years ago

still see some pending comments which I wrote though (they are marked as such by github, not sure what that means).

Perhaps try to submit a review on your own PR using the "Review changes" button in the Files changed section. It seems that you have indeed left pending review comments, because we cannot see them. :slightly_smiling_face:

eudoxos commented 2 years ago

advanced_demo.py works nicely now. Just the colors seem to be wrong (blue shows red gradient, green shows green, red shows blue). Is it because I compiled with LV_COLOR_16_SWAP and should not, for this hardware?

image

amirgon commented 2 years ago

Just the colors seem to be wrong (blue shows red gradient, green shows green, red shows blue). Is it because I compiled with LV_COLOR_16_SWAP and should not, for this hardware?

The color distortion issue needs to be addressed.
LV_COLOR_16_SWAP is explained in the docs, and is usually needed when sending 16-bit values through SPI interface.

Your driver should verify the correct settings of LV_COLOR_DEPTH and LV_COLOR_16_SWAP and throw an error if they are incorrectly set. See an example of doing that in ili9XXX driver.

Color distortion may be a result of an error in your driver init sequence. For example selecting RGB/ BGR order. Please consult the display datasheet.

eudoxos commented 2 years ago

The color distortion issue needs to be addressed. LV_COLOR_16_SWAP is explained in the docs, and is usually needed when sending 16-bit values through SPI interface.

This is fixed now:

image

amirgon commented 2 years ago

Merged!

Thank you very much for this contribution.