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

MidEvil keyboard support #22259

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

MidEvil keyboard support #22259

wants to merge 36 commits into from

Conversation

dcpedit
Copy link
Contributor

@dcpedit dcpedit commented Oct 12, 2023

Description

Support for MidEvil keyboard PCB

Types of Changes

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

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 keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Oct 12, 2023
keyboards/dcpedit/midevil/info.json Outdated Show resolved Hide resolved
keyboards/dcpedit/midevil/info.json Outdated Show resolved Hide resolved
keyboards/dcpedit/midevil/keymaps/via/keymap.json Outdated Show resolved Hide resolved
keyboards/dcpedit/midevil/keymaps/via/rules.mk Outdated Show resolved Hide resolved
keyboards/dcpedit/midevil/readme.md Outdated Show resolved Hide resolved
@elpekenin
Copy link
Contributor

elpekenin commented Oct 12, 2023

Does the code work? Like, the exact same files on the PR

  • Not assigning a MISO pin should fine since Chibios SPI driver: allow some SPI pins to be left unassigned #20315, but it is only available on develop as of now
  • Didnt specify the SPI driver to be used
  • These names for MOSI/SCK aren't the ones that spi_master.h(or .c) use -and i dont see any sort of aliasing on this PR files-, so im pretty sure it shouldn't even compile. For example, fallback value for SCK is B15, which wouldn't even be a valid pin under RP2040.

@dcpedit
Copy link
Contributor Author

dcpedit commented Oct 12, 2023

Yes, it compiles and works on my keyboard. Here's a video: https://imgur.com/a/JDhwkuG

I basically just followed the documentation for quantum painter, and used the handwired onekey lvgl code as an example:
https://github.com/qmk/qmk_firmware/blob/master/docs/quantum_painter.md?id=quantum-painter-cli#-ili9488-
https://github.com/qmk/qmk_firmware/tree/master/keyboards/handwired/onekey/keymaps/lvgl

Isn't the driver defined in rules.mk:

QUANTUM_PAINTER_DRIVERS += ili9488_spi

And the pins defined when calling qp_ili9488_make_spi_device?

@elpekenin
Copy link
Contributor

I mean the SPI driver (hardware on the RP to emit such protocol), as the MCU has two of them and it needs to be setup.
Not the device driver (low level code to interact with the display)

As per pins, the example you linked uses SPI_SCK_PIN (and same for MISO and MOSI), which as you can see arent used on the make_device function as they are hardcoded in the file i named (spi_master). Thus, naming them LCD_ shouldnt work afaik

@dcpedit
Copy link
Contributor Author

dcpedit commented Oct 12, 2023

I think I understand. I made another commit to fix the defines. Tested the code again to make sure it compiles and works with the keyboard. Not sure why it was fine before the fix. Also, the display doesn't use a MISO pin, which should be fine according to the QP docs:

Most TFT display panels use a 5-pin interface -- SPI SCK, SPI MOSI, SPI CS, D/C, and RST pins.

If it has to be used, I don't have any extra pins I left on my Pi Pico. Can it be left unassigned, or can I assign a bogus value?

@elpekenin
Copy link
Contributor

Will be fine with the changes on develop for the SPI code, you can change the target branch. And you'd then set it to NO_PIN

@sigprof
Copy link
Contributor

sigprof commented Oct 13, 2023

and i dont see any sort of aliasing on this PR files-, so im pretty sure it shouldn't even compile

The defaults are somewhat hidden:

  • platforms/chibios/mcu_selection.mk sets BOARD ?= GENERIC_PROMICRO_RP2040 as the default board for RP2040.
  • platforms/chibios/boards/GENERIC_PROMICRO_RP2040/configs/config.h sets the default values for I2C, SPI, USART pins to match the Pro Micro RP2040 pinout:
#if !defined(SPI_DRIVER)
#    define SPI_DRIVER SPID0
#endif

#if !defined(SPI_SCK_PIN)
#    define SPI_SCK_PIN GP18
#endif

#if !defined(SPI_MISO_PIN)
#    define SPI_MISO_PIN GP20
#endif

#if !defined(SPI_MOSI_PIN)
#    define SPI_MOSI_PIN GP19
#endif

So the default SPI_MISO_PIN is GP20, which actually conflicts with one of the row pins. However, with a COL2ROW matrix the row pins get reconfigured on every matrix scan, and the SPI code sets up the pins only once during init, therefore the GP20 pin ends up being reconfigured as a plain GPIO; this does not prevent the SPI peripheral from working (on RP2040 it probably results in the peripheral always seeing 0 on the unmapped input), but still looks unclean in comparison with the proper solution (using NO_PIN with the code in the develop branch).

@dcpedit dcpedit changed the base branch from master to develop October 13, 2023 14:50
@elpekenin
Copy link
Contributor

I would maybe add some way for user to disable the animation being done by the keyboard-level file. And a few keys on the JSON have too many decimal places (precision will be lost when drawing, and doesnt need to be 100% perfect anyway). Other than that... Looks good to me

@dcpedit
Copy link
Contributor Author

dcpedit commented Oct 13, 2023

I would maybe add some way for user to disable the animation being done by the keyboard-level file. And a few keys on the JSON have too many decimal places (precision will be lost when drawing, and doesnt need to be 100% perfect anyway). Other than that... Looks good to me

Can I instead set QUANTUM_PAINTER_DISPLAY_TIMEOUT = 30000 so that it turns off automatically?

@elpekenin
Copy link
Contributor

elpekenin commented Oct 13, 2023

That wouldn't help at all.

  • Code still being run (and slowing down scan rate), just not seen due to screen being off
  • Main concern i had: Users have no way to disable this default behaviour from their keymaps. So they would have to edit keyboard-level code if they wanted to draw anything else

@dcpedit
Copy link
Contributor Author

dcpedit commented Oct 13, 2023

I've added a way to toggle the animation with a keypress

@dcpedit dcpedit requested a review from waffle87 October 18, 2023 22:16
keyboards/dcpedit/midevil/midevil.c Show resolved Hide resolved
keyboards/dcpedit/midevil/midevil.c Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team December 12, 2023 09:00
keyboards/dcpedit/midevil/config.h Outdated Show resolved Hide resolved
@dcpedit dcpedit changed the base branch from develop to master February 14, 2024 22:28
Copy link

github-actions bot commented May 5, 2024

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 May 5, 2024
@dcpedit
Copy link
Contributor Author

dcpedit commented May 16, 2024

Awaiting review

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label May 17, 2024
Copy link

github-actions bot commented Jul 1, 2024

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 Jul 1, 2024
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jul 28, 2024
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 Sep 20, 2024
Copy link

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Oct 20, 2024
@dcpedit
Copy link
Contributor Author

dcpedit commented Nov 1, 2024

Please review

@zvecr zvecr reopened this Nov 1, 2024
keyboards/dcpedit/midevil/info.json Outdated Show resolved Hide resolved
keyboards/dcpedit/midevil/midevil.c Outdated Show resolved Hide resolved
keyboards/dcpedit/midevil/keymaps/via/keymap.json Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Nov 2, 2024
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 Dec 17, 2024
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants