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

Add new boards for Keychron Q1 #19307

Closed
wants to merge 56 commits into from

Conversation

KeychronMacro
Copy link
Contributor

@KeychronMacro KeychronMacro commented Dec 13, 2022

Added Q1 Version 2 board and made some changes to the original Q1 board (often known as Q1 Version 1).
Thanks!

Description

  • Normally, Q1 Version 2 is based on STM32L432KBU6 while Q1 Version 1 ATmega32u4. There are still some Q1 Version 2 boards used ATmega32u4. So
  1. We moved their unique configuration from the top config.h into the lower config.h
  • The matrix size and the led light controller of both versions of Q1 are also different. So
  1. We added the mcu name as the suffix to distinguish one from another
  • Enable rgb matrix keypress effect for all keyboards, So
  1. We added the RGB_MATRIX_KEYPRESSES and RGB_MATRIX_FRAMEBUFFER_EFFECTS to the top config.h
  • Fixed the reverse order of the insert and delete key in ansi_encoder_atmega32u4.

  • And small format changes

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 keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Dec 13, 2022
@zvecr zvecr changed the base branch from master to develop December 13, 2022 13:42
@jrgiacone

This comment was marked as off-topic.

@jrgiacone
Copy link

@lalalademaxiya1 I tested out the new firmware for the q1v2, however I am not able to change the knob within VIA. Is there something I need to edit in the json to be able to configure it?

@adophoxia
Copy link
Contributor

adophoxia commented Jan 22, 2023

@jrgiacone, you're probably using a VIA Definitions JSON that's not made for Encoder Mapping support. You'll probably have better luck looking at the docs here to adapt the JSON to it.

@K900
Copy link

K900 commented Jan 23, 2023

Here's the Via config I'm using: https://gist.github.com/K900/f24eac85024ac9badfa573e945f12f7f

Note that you need to enable all the lighting effects as I've done here for the default Via effect names to line up.

@jrgiacone
Copy link

@jrgiacone, you're probably using a VIA Definitions JSON that's not made for Encoder Mapping support. You'll probably have better luck looking at the docs here to adapt the JSON to it.

I changed 0,15 to be 0,15\ne0 and it works as intended now! The only thing that is not present in the firmware here is the reset option from the stock firmware.

keyboards/keychron/q1/ansi_atmega32u4/config.h Outdated Show resolved Hide resolved
keyboards/keychron/q1/ansi_atmega32u4_encoder/config.h Outdated Show resolved Hide resolved
keyboards/keychron/q1/ansi_stm32l432/config.h Outdated Show resolved Hide resolved
keyboards/keychron/q1/ansi_stm32l432_encoder/config.h Outdated Show resolved Hide resolved
keyboards/keychron/q1/ansi_stm32l432/rules.mk Outdated Show resolved Hide resolved
keyboards/keychron/q1/jis_stm32l432/info.json Outdated Show resolved Hide resolved
keyboards/keychron/q1/jis_stm32l432/rules.mk Outdated Show resolved Hide resolved
keyboards/keychron/q1/jis_stm32l432_encoder/config.h Outdated Show resolved Hide resolved
keyboards/keychron/q1/jis_stm32l432_encoder/info.json Outdated Show resolved Hide resolved
keyboards/keychron/q1/jis_stm32l432_encoder/rules.mk Outdated Show resolved Hide resolved
KeychronMacro and others added 6 commits February 7, 2023 11:02
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
@KeychronMacro KeychronMacro requested review from drashna and removed request for fauxpark March 11, 2023 02:30
"device_version": "2.0.0"
},
"matrix_pins": {
"cols": ["C14", "C15", "A0", "A1", "A2", "A3", "A4", "A5", "NO_PIN", "NO_PIN", "NO_PIN", "NO_PIN", "NO_PIN", "NO_PIN", "NO_PIN", "NO_PIN"],
Copy link
Contributor

@vinorodrigues vinorodrigues Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null could (should?) be used instead of "NO_PIN". (same on the other json's)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, Thanks!

@keychron-dev

This comment was marked as duplicate.

@adophoxia
Copy link
Contributor

I still don't think that having each variant of the Q1 be denoted by their MCU would do well when it comes to identifying which model a user has, especially when it comes to troubleshooting since I doubt the average person will open up their board to check what MCU it has so they can flash the correct firmware. A suggestion like delegating each revision into each of their folders like how the GMMK Pro currently has it (Example below):

└── Keychron/
    └── q1/
        ├── v1/
        │   ├── ansi
        │   ├── ansi_knob
        │   ├── iso
        │   └── iso_knob
        └── v2/
            ├── ansi
            ├── ansi_knob
            ├── iso
            ├── iso_knob
            ├── jis
            └── jis_knob

This would look more simplified without having all of that MCU clutter in the way. Some food for thought if this is considered.

@keychron-dev

This comment was marked as duplicate.

@keychron-dev

This comment was marked as duplicate.

@Syphdias
Copy link

Syphdias commented Apr 3, 2023

Sorry, quick question. I cannot quite tell, if this will also implement support for the Q1 Pro ISO. Is this the case?

@vinorodrigues
Copy link
Contributor

No. Q1-Pro is not in this PR

@keychron-dev

This comment was marked as duplicate.

@keychron-dev

This comment was marked as off-topic.

@tzarc
Copy link
Member

tzarc commented Apr 11, 2023

This constant requesting of reviews doesn't help anyone, and to be honest, makes things worse.

Please refer to the PR Checklist for the list of items that need to be handled.

Key points:

  • PRs should contain the smallest amount of modifications required for a single change to the codebase
    • multiple keyboards at the same time is not acceptable
    • the smaller the PR, the higher likelihood of a quicker review, higher likelihood of quicker merge, and less chance of conflicts
  • info.json
    • almost all items, including really obvious things such as:
    • With the move to data driven keyboard configuration, we encourage contributors to utilise as many features as possible of the info.json schema.
    • If the keyboard only has a single electrical/switch layout:
      • use LAYOUT as your macro name, unless a community layout already exists
  • readme.md
    • images should be uploaded to an external image hosting service, such as imgur.

Feel free to go through and address everything in the PR checklist and then we'll consider a proper review.

These checklist items have been published for months, and the onus should not be on QMK maintainers to pick out all the issues that have already been published for general consumption. Complaining that nobody reviews when the PR is not in a reviewable state is not QMK's problem.

@tzarc tzarc added invalid awaiting changes pr_checklist_pending Needs changes as per the PR checklist labels Apr 11, 2023
@tzarc
Copy link
Member

tzarc commented Apr 11, 2023

@keychron-dev see above.

@KeychronMacro
Copy link
Contributor Author

Close this PR and submit a new PR again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes invalid keyboard keymap pr_checklist_pending Needs changes as per the PR checklist via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet