logo

qmk_firmware

custom branch of QMK firmware git clone https://anongit.hacktivis.me/git/qmk_firmware.git

pr_checklist.md (20608B)

    

  1. # PR checklists
  3. This is a non-exhaustive checklist of what the QMK Collaborators will be checking when reviewing submitted PRs.
  5. If there are any inconsistencies with these recommendations, you're best off [creating an issue](https://github.com/qmk/qmk_firmware/issues/new) against this document, or getting in touch with a QMK Collaborator on [Discord](https://discord.gg/qmk).
  7. ## Requirements for all PRs
  9. - PR should be submitted using a non-`master` branch on the source repository
  10.     - this does not mean you target a different branch for your PR, rather that you're not working out of your own master branch
  11.     - if submitter _does_ use their own `master` branch, they'll be given a link to the ["how to git"](newbs_git_using_your_master_branch) page after merging -- (end of this document will contain the contents of the message)
  12.     - Note, frequently merging upstream with your branch is not needed and is discouraged. Valid reason for updating your branch may be resolving merge conflicts and pulling in new changes relevant to your PR.
  13. - PRs should contain the smallest amount of modifications required for a single change to the codebase
  14.     - multiple keyboards at the same time is not acceptable
  15.     - **the smaller the PR, the higher likelihood of a quicker review, higher likelihood of quicker merge, and less chance of conflicts**
  16. - newly-added directories and filenames must be lowercase
  17.     - the lowercase requirement may be relaxed if upstream sources originally had uppercase characters (e.g. LUFA, ChibiOS, or imported files from other repositories etc.)
  18.     - if there is valid justification (i.e. consistency with existing core files etc.) this can be relaxed
  19.         - a board designer naming their keyboard with uppercase letters is not enough justification
  20. - valid license headers on all `*.c` and `*.h` source files
  21.     - GPL2/GPL3 recommended for consistency
  22.     - an example GPL2+ license header may be copied (and author modified) from the bottom of this document
  23.     - other licenses are permitted, however they must be GPL-compatible and must allow for redistribution. Using a different license will almost certainly delay a PR getting merged
  24.     - missing license headers will prevent PR merge due to ambiguity with license compatibility
  25.         - simple assignment-only `rules.mk` files should not need a license header - where additional logic is used in an `*.mk` file a license header may be appropriate
  26. - QMK Codebase "best practices" followed
  27.     - this is not an exhaustive list, and will likely get amended as time goes by
  28.     - `#pragma once` instead of `#ifndef` include guards in header files
  29.     - no "old-school" or other low-level GPIO/I2C/SPI functions may be used -- must use QMK abstractions unless justifiable (and laziness is not valid justification)
  30.     - timing abstractions should be followed too:
  31.         - `wait_ms()` instead of `_delay_ms()` (remove `#include <util/delay.h>` too)
  32.         - `timer_read()` and `timer_read32()` etc. -- see [timer.h](https://github.com/qmk/qmk_firmware/blob/master/platforms/timer.h) for the timing APIs
  33.     - if you think a new abstraction is useful, you're encouraged to:
  34.         - prototype it in your own keyboard until it's feature-complete
  35.         - discuss it with QMK Collaborators on Discord
  36.         - refactor it as a separate core change
  37.         - remove your specific copy in your board
  38. - fix all merge conflicts before opening the PR (in case you need help or advice, reach out to QMK Collaborators on Discord)
  39.     - PR submitters will need to keep up-to-date with their base branch, resolving conflicts along the way
  41. ## Keymap PRs
  43. ::: warning
  44. Note that personal keymap submissions will no longer be accepted. This section applies to manufacturer-supported keymaps. Please see this [issue](https://github.com/qmk/qmk_firmware/issues/22724) for more information.
  45. :::
  47. - PRs for vendor specific keymaps will be permitted. The naming convention for these should be `default_${vendor}` i.e. `default_clueboard`.
  48.     - vendor specific keymaps do not necessarily need to be "vanilla" and can be more richly featured than `default` stock keymaps.
  49. - `#include QMK_KEYBOARD_H` preferred to including specific board files
  50. - prefer layer enums to #defines
  51. - custom keycode enums must have first entry = `QK_USER`
  52. - some care with spacing (e.g., alignment on commas or first char of keycodes) makes for a much nicer-looking keymap. Spaces are preferred to tabs
  53. - keymaps should not enable VIA
  54.     - keymaps targeting VIA support should be submitted to the [VIA QMK Userspace](https://github.com/the-via/qmk_userspace_via) repository
  56. ## Keyboard PRs
  58. Closed PRs (for inspiration, previous sets of review comments will help you eliminate ping-pong of your own reviews):
  59. https://github.com/qmk/qmk_firmware/pulls?q=is%3Apr+is%3Aclosed+label%3Akeyboard
  61. - keyboard moves within the repository *must* go through the `develop` branch instead of `master`, so as to ensure compatibility for users
  62.     - `data/mappings/keyboard_aliases.hjson` must be updated to reflect the move, so users with pre-created configurator keymap.json files continue to detect the correct keyboard
  63. - keyboard updates and refactors (eg. to data driven) *must* go through `develop` to reduce `master` -> `develop` merge conflicts
  64. - PR submissions from a `kbfirmware` export (or equivalent) will not be accepted unless converted to new QMK standards -- try `qmk import-kbfirmware` first
  65. - `info.json`
  66.     - With the move to [data driven](data_driven_config) keyboard configuration, we encourage contributors to utilise as many features as possible of the info.json [schema](https://github.com/qmk/qmk_firmware/blob/master/data/schemas/keyboard.jsonschema).
  67.     - the mandatory elements for a minimally complete `info.json` at present are:
  68.         - valid URL
  69.         - valid maintainer
  70.         - valid USB VID/PID and device version
  71.         - displays correctly in Configurator (press Ctrl+Shift+I to preview local file, turn on fast input to verify ordering)
  72.         - `layout` definitions must include matrix positions, so that `LAYOUT` macros can be generated at build time
  73.             - should use standard definitions if applicable
  74.             - use the Community Layout macro names where they apply (preferred above `LAYOUT`/`LAYOUT_all`)
  75.             - If the keyboard only has a single electrical/switch layout:
  76.                 - use `LAYOUT` as your macro name, unless a community layout already exists
  77.             - If the keyboard has multiple electrical/switch layouts:
  78.                 - include a `LAYOUT_all` which specifies all possible layout positions in the electrical matrix
  79.                 - use alternate layout names for all other possible layouts, preferring community layout names if an equivalent is available (e.g. `LAYOUT_tkl_ansi`, `LAYOUT_ortho_4x4` etc.)
  80.         - Microcontroller and bootloader
  81.         - Diode Direction (if not using direct pins)
  82.     - the following are required to be configured in `info.json` if necessary
  83.       - Direct pin configuration
  84.       - Backlight Configuration (where applicable)
  85.       - Split keyboard configuration (where applicable)
  86.       - Encoder Configuration
  87.       - Bootmagic Configuration
  88.       - LED Indicator Configuration
  89.       - RGB Light Configuration
  90.       - RGB Matrix Configuration
  91.     - Run `qmk format-json` on this file before submitting your PR. Be sure to append the `-i` flag to directly modify the file, or paste the outputted code into the file. 
  92. - `readme.md`
  93.     - must follow the [template](https://github.com/qmk/qmk_firmware/blob/master/data/templates/keyboard/readme.md)
  94.     - flash command is present, and has `:flash` at end
  95.     - valid hardware availability link (unless handwired) -- private groupbuys are okay, but one-off prototypes will be questioned. If open-source, a link to files should be provided.
  96.     - clear instructions on how to reset the board into bootloader mode
  97.     - a picture about the keyboard and preferably about the PCB, too
  98.         - images are not to be placed in the `qmk_firmware` repository
  99.         - images should be uploaded to an external image hosting service, such as [imgur](https://imgur.com/).
  100.         - image links should link directly to the image, not a "preview" -- i.e. [https://imgur.com/vqgE7Ok](https://imgur.com/vqgE7Ok) should be [https://i.imgur.com/vqgE7Ok.jpg](https://i.imgur.com/vqgE7Ok.jpg) when using imgur
  101. - `rules.mk`
  102.     - removed `MIDI_ENABLE`, `FAUXCLICKY_ENABLE` and `HD44780_ENABLE`
  103.     - modified `# Enable Bluetooth with the Adafruit EZ-Key HID` -> `# Enable Bluetooth`
  104.     - no `(-/+size)` comments related to enabling features
  105.     - remove the list of alternate bootloaders if one has been specified
  106.     - no re-definitions of the default MCU parameters if same value, when compared to the equivalent MCU in [mcu_selection.mk](https://github.com/qmk/qmk_firmware/blob/master/builddefs/mcu_selection.mk)
  107.     - no "keymap only" features enabled
  108.       - `COMBO_ENABLE`
  109.       - `ENCODER_MAP_ENABLE`
  110. - keyboard `config.h`
  111.     - no `#define DESCRIPTION`
  112.     - no Magic Key Options, MIDI Options or HD44780 configuration
  113.     - user preference configurable `#define`s should not be placed at the keyboard level
  114.     - default values should not be redefined, such as `DEBOUNCE`, RGB related settings, etc.
  115.       - feature specific documentation contains most default values
  116.       - `grep` or alternative tool can be used to search for default values in core directories (e.g. `grep -r "define DEBOUNCE" quantum`)
  117.     - no copy/pasted comment blocks explaining a feature and/or its caveats -- this is what the docs are for
  118.       - `Force NKRO to be enabled ... toggled again during a power-up`
  119.       - commented-out unused defines, such as RGB effects
  120.     - no `#include "config_common.h`
  121.     - no `#define MATRIX_ROWS/COLS`, unless necessary (e.g. a keyboard with a custom matrix)
  122.     - bare minimum required code for a board to boot into QMK should be present
  123.         - initialisation code for the matrix and critical devices
  124.         - mirroring existing functionality of a commercial board (like custom keycodes and special animations etc.) should be handled through non-`default` keymaps
  125.     - Vial-related files or changes will not be accepted, as they are not used by QMK firmware (no Vial-specific core code has been submitted or merged)
  126. - `<keyboard>.c`
  127.     - empty `xxxx_xxxx_kb()`, `xxxx_xxxx_user()`, or other weak-defined default implemented functions removed
  128.     - commented-out functions removed too
  129.     - `matrix_init_board()` etc. migrated to `keyboard_pre_init_kb()`, see: [keyboard_pre_init*](custom_quantum_functions#keyboard_pre_init_-function-documentation)
  130.     - when configuring custom matrix, the 'lite' variant (`CUSTOM_MATRIX = lite`) must be used where possible, as this allows for standard debounce. See [custom matrix 'lite'](custom_matrix#lite)
  131.         - justification for full custom matrix (`CUSTOM_MATRIX = yes`) must be provided when used
  132.     - prefer LED indicator [Configuration Options](features/led_indicators#configuration-options) to custom `led_update_*()` implementations where possible
  133.     - hardware that's enabled at the keyboard level and requires configuration such as OLED displays or encoders should have basic functionality implemented here
  134. - `<keyboard>.h`
  135.     - `#include "quantum.h"` appears at the top
  136.     - `LAYOUT` macros are no longer accepted and should instead be moved to `info.json`
  137. - keymap `config.h`
  138.     - no duplication of `rules.mk` or `config.h` from keyboard
  139. - `keymaps/default/keymap.c`
  140.     - `QMKBEST`/`QMKURL` example macros removed
  141.     - if using `MO(1)` and `MO(2)` keycodes together to access a third layer, the [Tri Layer](features/tri_layer) feature should be used, rather than manually implementing this using `layer_on/off()` and `update_tri_layer()` functions in the keymap's `process_record_user()`.
  142. - default keymaps should be "pristine"
  143.     - bare minimum to be used as a "clean slate" for another user to develop their own user-specific keymap
  144.     - what does pristine mean? no custom keycodes. no advanced features like tap dance or macros. basic mod taps and home row mods would be acceptable where their use is necessary
  145.     - standard layouts preferred in these keymaps, if possible
  146.     - should use [encoder map feature](features/encoders#encoder-map), rather than `encoder_update_user()`
  147.     - default keymap should not enable VIA -- keymaps targeting VIA support should be submitted to the [VIA QMK Userspace](https://github.com/the-via/qmk_userspace_via) repository
  148. - submitters can add an example (or bells-and-whistles) keymap showcasing capabilities in the same PR but it shouldn't be embedded in the 'default' keymap
  149. - submitters can also have a "manufacturer-matching" keymap that mirrors existing functionality of the commercial product, if porting an existing board
  150. - Do not include VIA json files in the PR. These do not belong in the QMK repository as they are not used by QMK firmware -- they belong in the [VIA Keyboard Repo](https://github.com/the-via/keyboards)
  151. - Do not include KLE json files in the PR. These have no use within QMK.
  152. - Do not include source files from another keyboard or vendors keyboard folder. Including core files is fine.
  153.   - For instance, only `wilba_tech` boards shall include `keyboards/wilba_tech/wt_main.c` and  `keyboards/wilba_tech/wt_rgb_backlight.c`. But including `drivers/sensors/pmw3360.c` is absolutely fine for any and all boards that require it.
  154.   - Code that needs to be used by multiple boards is a candidate for core code changes, and should be separated out.
  156. Wireless-capable boards:
  157. - Given license abuse from vendors, QMK does not accept any vendor PRs for wireless- or Bluetooth-capable keyboards without wireless and/or Bluetooth code
  158.     - Historically, vendors have done this in bad faith in order to attain downstream VIA compatibility with no intention of releasing wireless sources
  159.     - QMK's license, the GPL2+, requires full source disclosure for any distributed binary -- including full sources for any keyboard shipped by vendors containing QMK and/or firmware-side VIA code
  160.     - If a vendor's wireless-capable keyboard PR submission is lacking wireless capability, then the PR will be left on-hold and unmergeable until wireless bindings are provided
  161.     - If a vendor's wireless-capable keyboard is merged into QMK before it's known that the board is wireless, then all existing and future PRs from the same vendor will be put on hold until wireless bindings for the offending keyboard are provided
  163. Also, specific to ChibiOS:
  164. - **strong** preference to using existing ChibiOS board definitions.
  165.     - a lot of the time, an equivalent Nucleo board can be used with a different flash size or slightly different model in the same family
  166.         - example: For an STM32L082KZ, given the similarity to an STM32L073RZ, you can use `BOARD = ST_NUCLEO64_L073RZ` in rules.mk
  167.     - QMK is migrating to not having custom board definitions if at all possible, due to the ongoing maintenance burden when upgrading ChibiOS
  168. - New board definitions must not be embedded in a keyboard PR
  169.     - See [Core PRs](#core-pr) below for the procedure for adding a new board to QMK
  170. - if a board definition is unavoidable, `board.c` must have a standard `__early_init()` (as per normal ChibiOS board defs) and an empty `boardInit()`:
  171.     - see Arm/ChibiOS [early initialization](platformdev_chibios_earlyinit#board-init)
  172.     - `__early_init()` should be replaced by either `early_hardware_init_pre()` or `early_hardware_init_post()` as appropriate
  173.     - `boardInit()` should be migrated to `board_init()`
  175. ## Core PRs {#core-pr}
  177. - all core PRs must now target `develop` branch, which will subsequently be merged back to `master` on the breaking changes timeline
  178. - as indicated above, the smallest set of changes to core components should be included in each PR
  179.     - PRs containing multiple areas of change will be asked to be split up and raised separately
  180.     - keyboard and keymap changes should only be included if they affect base keyboard builds, or the default-like `default`, `default_????` keymaps etc.
  181.         - keymap modifications for anything other than the default-like keymaps **should not be included in the initial PR** in order to simplify the review process
  182.         - the core PR submitter should submit a followup PR affecting other keymaps after initial PR merge
  183.         - large-scale refactoring or consolidation PRs that affect other keymaps (such as renaming keycodes) should always be raised separately
  184. - any new boards adding support for new hardware now requires a corresponding test board under `keyboards/handwired/onekey`
  185.     - for new MCUs, a new "child" keyboard should be added that targets your newly-added MCU, so that builds can be verified
  186.     - for new hardware support such as display panels, core-side matrix implementations, or other peripherals, an associated keymap should be provided
  187.     - if an existing keymap exists that can leverage this functionality this may not be required (e.g. a new RGB driver chip, supported by the `rgb` keymap) -- consult with the QMK Collaborators on Discord to determine if there is sufficient overlap already
  188. - any features adding `_kb`/`_user` callbacks must return a `bool`, to allow for user override of keyboard-level callbacks.
  189. - where relevant, unit tests are strongly recommended -- they boost the confidence level that changes behave correctly
  190.     - critical areas of the code -- such as the keycode handling pipeline -- will almost certainly require unit tests accompanying them to ensure current and future correctness
  191.     - you should not be surprised if a QMK collaborator requests unit tests to be included in your PR if it's critical functionality
  192. - other requirements are at the discretion of QMK collaborators
  193.     - core is a lot more subjective given the breadth of posted changes
  195. ---
  197. ## Notes
  199. For when people use their own `master` branch, post this after merge:
  200. ```
  201. For future reference, we recommend against committing to your `master` branch as you've done here, because pull requests from modified `master` branches can make it more difficult to keep your QMK fork updated. It is highly recommended for QMK development – regardless of what is being done or where – to keep your master updated, but **NEVER** commit to it. Instead, do all your changes in a branch (branches are basically free in Git) and issue PRs from your branches when you're developing.
  203. There are instructions on how to keep your fork updated here:
  205. [**Best Practices: Your Fork's Master: Update Often, Commit Never**](newbs_git_using_your_master_branch)
  207. [Fixing Your Branch](newbs_git_resynchronize_a_branch) will walk you through fixing up your `master` branch moving forward. If you need any help with this just ask.
  209. Thanks for contributing!
  210. ```
  212. ## Review Process
  214. In general, we want to see two (or more) approvals that are meaningful (e.g. that have inspected code) before a PR will be considered for merge. These reviews are not limited to collaborators -- any community member willing to put in the time is welcomed (and encouraged). The only difference is that your checkmark won't be green, and that's fine!
  216. Additionally, PR reviews are something that is done in our free time. We are not paid nor compensated for the time we spend reviewing, as it is a labor of love. As such, this means that it can take time for us to get to your Pull Request.  Things like family, or life can get in the way of us getting to PRs, and burnout is a serious concern. The QMK firmware repository averages 200 PRs opened and 200 PRs merged every month, so please have patience.
  218. ## Example GPLv2 Header
  220. ```
  221. /* Copyright 2024 Your Name (@yourgithub)
  222.  *
  223.  * This program is free software: you can redistribute it and/or modify
  224.  * it under the terms of the GNU General Public License as published by
  225.  * the Free Software Foundation, either version 2 of the License, or
  226.  * (at your option) any later version.
  227.  *
  228.  * This program is distributed in the hope that it will be useful,
  229.  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  230.  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  231.  * GNU General Public License for more details.
  232.  *
  233.  * You should have received a copy of the GNU General Public License
  234.  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  235.  */
  236. ```
  238. Or, optionally, using [SPDX identifier](https://spdx.org/licenses/) instead:
  240. ```
  241. // Copyright 2024 Your Name (@yourgithub)
  242. // SPDX-License-Identifier: GPL-2.0-or-later
  243. ```