qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

[Driver] ILI9486 on Quantum Painter

Open elpekenin opened this issue 2 years ago • 5 comments

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).

elpekenin avatar Sep 29 '22 00:09 elpekenin

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.

tzarc avatar Sep 29 '22 01:09 tzarc

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

elpekenin avatar Sep 29 '22 15:09 elpekenin

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.

elpekenin avatar Dec 27 '22 21:12 elpekenin

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.

elpekenin avatar Jan 01 '23 21:01 elpekenin

Code has been fixed, simplified and abstracted now. Should be good to merge

elpekenin avatar Jan 02 '23 12:01 elpekenin

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.

github-actions[bot] avatar Feb 23 '23 02:02 github-actions[bot]

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

elpekenin avatar Jun 25 '23 22:06 elpekenin

I definitely would like an eink display — could this be reviewed and merged (after the drift to merge-Target has been resolved)?

neuhalje avatar Oct 22 '23 07:10 neuhalje