qmk_firmware
qmk_firmware copied to clipboard
[Driver] ILI9486 on Quantum Painter
Description
~~This PR adds support for the ili9486 LCD screen into Quantum Painter, based on @tzarc's work and @sigprof's suggestions.
I implemented an optional flag #define QUANTUM_PAINTER_SPI_DC_RESET_SHIFT_REG_ENABLE
to handle this Waveshare module which has a hardware SPI->16 bit parallel converter that needs some special care.~~
~~This way of handling it may be problematic on scenarios which have several screens, and the name could maybe be improved~~
~~For this to work, the QP driver needed to get a new function added send_parameters
which sends values after the corresponding send_command
. In most of the situations it will just be the same as comms_send
but this addition allows to implement support for other screens with a similar circuit.~~
^ This description is outdated
This code is based on tzarc's code for the ILI9486 chip, however I also added a new vtable
which was needed for the code to run on my quirky hardware module (see link above).
I made this by changing the qp_comms_spi.c
file to abstract the common code instead of duplicating a good part of it.
This way, the new vtable
just wraps those functions passing the apropiate configuration arguments, with this approach we can easily add support for further extensions/variants in the future.
To select whether to use the "normal" or this modified vtable
, a boolean variable is passed to the make_spi_device
constructor function.
- This new
vtable
is also used(needed) on my IL91874 driver (#19437)
As far as I can test, existing code wasn't broken by these changes. Both my ILI9163 and ILI9341 still work just fine.
Types of Changes
- [X] Core
- [ ] Bugfix
- [X] New feature
- [ ] Enhancement/optimization
- [ ] Keyboard (addition or update)
- [ ] Keymap/layout/userspace (addition or update)
- [ ] Documentation
Issues Fixed or Closed by This PR
Checklist
- [X] My code follows the code style of this project: C, Python
- [x] I have read the PR Checklist document and have made the appropriate changes.
- [x] My change requires a change to the documentation.
- [X] I have updated the documentation accordingly.
- [x] I have read the CONTRIBUTING document.
- [ ] I have added tests to cover my changes.
- [X] I have tested the changes and verified that they work and don't break anything (as well as I can manage).
I'll have a look when I can, but at first glance I'd say that the #define
would need to be pulled out and turned into an option consulted at runtime. For QP, the intention is that any configuration deviations are handled at runtime.
I kept the define for firmware-size concerns. However I've just pushed a change from conditional vtable to having a whole new factory method for the shift-register variant. This gives a runtime option for having both kinds at the same time at the cost of an unused factory if you only have an screen with the hardware quirk, which isn't dramatic imo
Can I please get this reviewed to merge it before some conflicts appear in the future?
I'm using the shiftreg_vtable
on my WIP IL91874 driver (3color eink display), so this merge is a requirement to make a PR for it.
Latest refactor is still working on regular SPI devices, at least with the small testing i've done on my ILI9163 and ILI9341. But has broken the toggling
vtable, thus ili9486_shiftreg
driver is broken now, can't find the difference tho.
Code has been fixed, simplified and abstracted now. Should be good to merge
Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug
, awaiting review
, breaking_change
, in progress
, or on hold
to prevent the issue from being re-flagged.
Many months later, but i think this is ready for a review. Code is as clean as i could manage, driver works flawlessly as far as i've been playing with it. It's the display used on this video
Sorry for the force-push mess up, but didnt feel like taking the old branch up to date
I definitely would like an eink display — could this be reviewed and merged (after the drift to merge-Target has been resolved)?