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

[Driver] ILI9486 on Quantum Painter #18521

Merged
merged 6 commits into from
Feb 17, 2024
Merged

[Driver] ILI9486 on Quantum Painter #18521

merged 6 commits into from
Feb 17, 2024

Conversation

elpekenin
Copy link
Contributor

@elpekenin elpekenin commented Sep 29, 2022

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.

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

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Sep 29, 2022
@zvecr zvecr changed the base branch from master to develop September 29, 2022 00:54
@tzarc
Copy link
Member

tzarc commented Sep 29, 2022

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 tzarc self-assigned this Sep 29, 2022
@tzarc tzarc requested review from tzarc and a team and removed request for a team September 29, 2022 01:23
@elpekenin
Copy link
Contributor Author

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 elpekenin marked this pull request as ready for review October 28, 2022 16:15
@elpekenin
Copy link
Contributor Author

elpekenin commented Dec 27, 2022

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
Copy link
Contributor Author

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 elpekenin marked this pull request as draft January 1, 2023 21:57
@elpekenin elpekenin changed the title Add ili9486 with optional SPI->parallel support [Driver] ILI9486 on Quantum Painter Jan 2, 2023
@elpekenin elpekenin marked this pull request as ready for review January 2, 2023 12:13
@elpekenin
Copy link
Contributor Author

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

@github-actions
Copy link

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 github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Feb 23, 2023
@tzarc tzarc added in progress and removed stale Issues or pull requests that have become inactive without resolution. labels Feb 23, 2023
@elpekenin
Copy link
Contributor Author

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

@drashna drashna requested a review from a team July 6, 2023 22:51
@neuhalje
Copy link

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

@tzarc tzarc merged commit 5383335 into qmk:develop Feb 17, 2024
3 of 4 checks passed
@elpekenin elpekenin deleted the ili9486 branch February 17, 2024 09:20
nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024
Co-authored-by: Nick Brassel <nick@tzarc.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants