18 KiB
| applyTo | excludeAgent | |
|---|---|---|
| ** |
|
GitHub Copilot Instructions for QMK Pull Request Review
This document provides automated review guidance based on the QMK PR Checklist and it is intended only for use by GitHub Copilot code-review agent during pull request reviews.
General PR Requirements
Branch and Submission Standards
- Source Branch Policy: Verify PR is NOT submitted from submitter's own
masterbranch- Flag if submitter is using their own
masterbranch as source - Suggest using feature branches instead for cleaner fork management
- Flag if submitter is using their own
- Target Branch Policy:
- New keyboard additions →
masterbranch (new folders underkeyboards/) - All other changes →
developbranch:- Keyboard updates, refactors, or moves
- Core code changes
- Data-driven configuration migrations
- Any modifications to existing keyboards
- New keyboard additions →
- PR Scope: PRs should contain the smallest set of modifications for a single change
- Flag PRs that modify multiple keyboards simultaneously
- Suggest splitting large PRs into focused, incremental changes
- Merge Conflicts: Check for unresolved merge conflicts
File Naming and Structure
- Lowercase Requirement: All new directories and filenames must be lowercase
- Exception: Upstream sources with original uppercase (LUFA, ChibiOS)
- Exception: Core files with valid justification
- Reject: Board designer preference for uppercase is NOT valid justification
License Headers
- Required: Valid license headers on all
*.cand*.hfiles - Recommended: GPL2/GPL3 for consistency
- Format: Check for proper GPL2+ header or SPDX identifier
// Copyright 2024 Your Name (@yourgithub) // SPDX-License-Identifier: GPL-2.0-or-later - Exception: Simple assignment-only
rules.mkfiles don't need headers - Flag: Missing or ambiguous license headers (blocks merge)
QMK Best Practices
- Include Guards: Use
#pragma onceinstead of#ifndefguards in headers - Abstractions Required: No low-level GPIO/I2C/SPI functions
- Must use QMK abstractions (flag direct hardware access)
- Timing Functions:
- Use
wait_ms()instead of_delay_ms() - Remove
#include <util/delay.h> - Use
timer_read(),timer_read32()fromtimer.h
- Use
- New Abstractions: If proposing new abstraction, suggest:
- Prototype in own keyboard first
- Discuss with QMK Collaborators on Discord
- Refactor as separate core change
- Remove the keyboard-specific implementation from board
Keymap PR Reviews
Scope: These rules apply to files within keyboards/*/keymaps/* subdirectories.
Note on Personal Keymaps
- Policy Change: Personal keymap submissions no longer accepted
- Permitted: Vendor-specific keymaps only
- Naming convention:
default_${vendor}(e.g.,default_clueboard) - Can be more feature-rich than stock
defaultkeymaps
- Naming convention:
Keymap Code Standards
- Includes:
#include QMK_KEYBOARD_Hpreferred over specific board files - Enums: Prefer layer enums to
#defines - Custom Keycodes: First entry must be
QK_USER - Formatting: Check spacing alignment on commas and keycodes (spaces, not tabs)
- VIA: Keymaps should NOT enable VIA
- VIA keymaps belong in VIA QMK Userspace
Keyboard PR Reviews
Scope: These rules apply to keyboard-level files in keyboards/* directories, excluding files within the keymaps/ subdirectories. This includes:
info.jsonorkeyboard.json(keyboard root or variant level)readme.md(keyboard level)rules.mk(keyboard level)config.h(keyboard level, not keymap level)<keyboard>.cand<keyboard>.hfiles- Hardware configuration files (
halconf.h,mcuconf.h,chconf.h)
Branch Targeting
- New Keyboards: Target
masterbranch- New additions to
keyboards/folder submit tomaster
- New additions to
- Keyboard Moves: Must target
developbranch- Check
data/mappings/keyboard_aliases.hjsonis updated for moves
- Check
- Keyboard Updates/Refactors: Must target
developto reduce merge conflicts - Data Driven Migration: Must target
develop
info.json and keyboard.json Requirements
- Data-Driven Configuration: Encourage maximum use of
info.jsonandkeyboard.jsonschema features - Schema Validation: All
info.jsonandkeyboard.jsonfiles must validate againstdata/schemas/keyboard.jsonschema- Use QMK CLI:
qmk lint -kb <keyboard_name>to validate - Schema defines required fields, data types, and valid values
- Check for schema validation errors before submitting PR
- Use QMK CLI:
- Mandatory Elements:
- Valid URL
- Valid maintainer
- Valid USB VID/PID and device version
- Displays correctly in Configurator (Ctrl+Shift+I to preview)
layoutdefinitions include matrix positions- Standard layout definitions where applicable
- Community Layout macro names when applicable
- Microcontroller and bootloader specified
- Diode direction (if not using direct pins)
- Layout Naming:
- Single layout: Use
LAYOUTor community layout name - Multiple layouts: Include
LAYOUT_all+ alternate names - Prefer community layout names (e.g.,
LAYOUT_tkl_ansi,LAYOUT_ortho_4x4)
- Single layout: Use
- Configuration in info.json or keyboard.json (when applicable):
- Direct pin configuration
- Backlight, Split keyboard, Encoder, Bootmagic configs
- LED Indicator, RGB Light, RGB Matrix configs
- Format: Run
qmk format-json -ibefore submitting
USB VID/PID Uniqueness
VID+PID combination must be unique across all keyboards. Individual VID or PID values can be reused with different partners. Validation Steps:
- Extract VID and PID from keyboard.json/info.json in the PR
- Search for existing usage:
grep -r '"vid".*"0xVVVV"' keyboards/ --include="*.json" | grep -l '"pid".*"0xPPPP"' - If results found: Check if BOTH VID AND PID match in same file
- Both match = COLLISION - request different PID
- Only one matches = OK - different keyboards can share individual values
- For keyboard variants/revisions under same keyboard folder:
- Different PID recommended for functionally different variants
- Same PID acceptable if revisions only differ in hardware routing/pin assignments Quick Reference:
- Same PID + Different VID = Valid
- Same VID + Different PID = Valid
- Same VID + Same PID = Invalid Review Response: For collision:
VID+PID collision: 0xVVVV:0xPPPP already used by keyboards/[path]/file.json
+Please assign a different PID. VID can remain the same.
For uniqueness confirmed:
VID+PID validation: 0xVVVV:0xPPPP is unique (no collisions found)
readme.md Requirements
- Template: Must follow official template
- Flash Command: Present with
:flashat end - Hardware Link: Valid availability link (unless handwired)
- Private groupbuys acceptable
- One-off prototypes will be questioned
- Open-source should link to files
- Reset Instructions: Clear bootloader mode instructions
- Images Required:
- Keyboard and PCB photos preferred
- Must be hosted externally (imgur, etc.)
- Direct image links required (not preview pages)
- Example:
https://i.imgur.com/vqgE7Ok.jpgnothttps://imgur.com/vqgE7Ok
rules.mk Standards
- Removed Items:
MIDI_ENABLE,FAUXCLICKY_ENABLE,HD44780_ENABLE- Size comments like
(-/+size) - Alternate bootloader lists if one specified
- MCU parameter re-definitions matching defaults in
mcu_selection.mk
- Comment Updates: Change bootloader comments to generic
- Forbidden Features at Keyboard Level (these belong in keymap-level
rules.mkonly):COMBO_ENABLEENCODER_MAP_ENABLE
config.h Standards (Keyboard Level)
- Prohibited:
#define DESCRIPTION- Magic Key Options, MIDI Options, HD44780 configuration
- User preference
#defines (belong in keymap) - Re-defining default values (
DEBOUNCE, RGB settings) - Copy/pasted comment blocks explaining features
- Commented-out unused defines
#include "config_common.h"#define MATRIX_ROWS/COLS(unless custom matrix)
- Minimal Code: Only critical board boot code required
- No Vial: Vial-related files/changes not accepted
Keyboard Implementation Files
<keyboard>.c
- Remove Empty Functions: Delete empty or commented-out weak-defined functions
xxxx_xxxx_kb(),xxxx_xxxx_user()implementations
- Migration:
matrix_init_board()→keyboard_pre_init_kb() - Custom Matrix: Use
litevariant when possible for standard debounceCUSTOM_MATRIX = litepreferred- Full custom matrix (
yes) requires justification
- LED Indicators: Prefer Configuration Options over custom
led_update_*()implementations - Hardware Configuration: Basic functionality for OLED, encoders, etc. at keyboard level
<keyboard>.h
- Include:
#include "quantum.h"at top - Layout Macros: Move to
info.jsonorkeyboard.json(no longer in header)
Default Keymap Standards
Scope: These rules specifically apply to files within keyboards/*/keymaps/default/ directories.
- Pristine Requirement: Bare minimum clean slate
- No custom keycodes
- No advanced features (non-exhaustive list of examples: tap dance, macros)
- Basic mod taps and home row mods acceptable when necessary
- Standard layouts preferred -- see examples in
layouts/default/andlayouts/community/
- Removed Examples: Delete
QMKBEST/QMKURLmacros - Tri Layer: Use Tri Layer feature instead of manual
layer_on/off()+update_tri_layer() - Encoder Map: Use encoder map feature,
encoder_update_user()may not be present - No VIA: Default keymap should not enable VIA
- Additional Keymaps: Example/bells-and-whistles keymaps acceptable in same PR (separate from default)
Prohibited Files
- No VIA JSON: Belongs in VIA Keyboard Repo
- No KLE JSON: Not used within QMK
- No Cross-Keyboard Sources: Don't include files from other keyboard vendors
- Exception: Core files (e.g.,
drivers/sensors/pmw3360.c) - Use of vendor-specific code (e.g.,
wilba_tech/wt_main.c) only when keyboard exists in the same enclosing vendor folder (e.g. awilba_techkeyboard) - Multi-board code is candidate for core refactoring when intended for use by multiple vendors
- Exception: Core files (e.g.,
Wireless Keyboards
- Policy: Wireless/Bluetooth PRs rejected without complete wireless code
- Wireless code may not include anything resembling precompiled data such as
*.afiles or other libraries - Firmware blobs are not permitted in raw form or as compiled C-style arrays either.
- GPL2+ license requires full source disclosure
- Historically abused for VIA compatibility without releasing sources
- PRs without wireless capability will be held indefinitely
- Existing merged wireless boards from same vendor held until sources provided
- Wireless code may not include anything resembling precompiled data such as
ChibiOS-Specific Requirements
- Board Definitions: Strong preference for existing ChibiOS board definitions
- Use equivalent Nucleo boards when possible
- Example: STM32L082KZ can use
BOARD = ST_NUCLEO64_L073RZ - QMK is eliminating custom board definitions due to maintenance burden
- New Board Definitions:
- Must NOT be embedded in keyboard PR
- Submit as separate Core PR
board.cmust have standard__early_init()and emptyboardInit()- Migrate code intended for
__early_init()→ keyboard-localearly_hardware_init_pre/post() - Migrate code intended for
boardInit()→ keyboard-localboard_init()
- Migrate code intended for
Core PR Reviews
Targeting and Scope
- Branch: All core PRs must target
developbranch - Single Focus: Smallest set of changes per PR
- PRs with multiple areas will be asked to split
- Keyboard/keymap changes only if affecting base builds or default-like keymaps
- Keymap modifications (non-default) should be followup PR after core merge
- Large refactoring PRs affecting other keymaps raised separately
Testing Requirements
- New Hardware Support: Requires test keyboard under
keyboards/handwired/onekey- New MCUs: Add child keyboard targeting new MCU for build verification
- New hardware (displays, matrix, peripherals): Provide associated keymap
- Exception: If existing keymap can leverage functionality (consult Collaborators)
- Callbacks: New
_kb/_usercallbacks must returnboolfor user override - Unit Tests: Strongly recommended, may be required
- Critical code areas (keycode pipeline) will require tests
- Boost confidence in current and future correctness
Code Quality
- Subjective Review: Other requirements at QMK Collaborators' discretion
- Documentation: Core changes should be well-documented
Automated Review Checklist
When reviewing PRs, check the following systematically:
File Changes Review
- License headers on all C/H files (GPL2+ preferred, others must be GPL2+ compatible, SPDX format preferred)
- File naming lowercase (flag exceptions needing justification)
- Include guards use
#pragma once - No low-level hardware access (GPIO, I2C, SPI direct register writes)
- Timing abstractions (
wait_ms(),timer_read()usage)
info.json and keyboard.json Validation
- Schema Compliance:
keyboard.jsonandinfo.jsonfiles validate againstdata/schemas/keyboard.jsonschema
- Both files are identical syntax, however the
keyboard.jsondictates a buildable target,info.jsondoes not - Run
qmk lint -kb <keyboard>to check schema validation - Check for proper data types (strings, integers, arrays, objects)
- Verify required fields are present
- Ensure enum values match allowed options in schema
- All mandatory fields present and valid
qmk format-json -ihas been run (formats and validates)- Layout macros moved from headers
- Community layout names used where applicable
rules.mk Cleanup
- Deprecated features removed
- No size comments
- No keymap-only features at keyboard level
- No redundant MCU parameter definitions
config.h Cleanup
- No
DESCRIPTION,config_common.h, or prohibited includes - No default value re-definitions
- No commented-out defines or feature documentation blocks
- No user preference defines at keyboard level
Keymap Quality
- Default keymaps are pristine (no custom keycodes/advanced features)
- No
QMKBEST/QMKURLmacros - Encoder map feature used instead of
encoder_update_user() - Tri Layer feature used for multi-layer access
- No VIA enabled in default keymap
Documentation
- readme.md follows template
- Flash command present with
:flash - Reset instructions clear
- External image hosting (direct links)
- Valid hardware availability link
Code Organization
- Empty weak-defined functions removed from
<keyboard>.c - Proper migration of init functions
- No cross-vendor source files
- No VIA/KLE JSON files
Branch and Scope
- Not submitted from submitter's own
masterbranch (use feature branches) - PR is focused on single change
- Targets correct branch:
masterfor new keyboard additionsdevelopfor keyboard updates/refactors/moves and core changes
- No merge conflicts
Review Response Templates
For source master branch usage:
⚠️ This PR appears to be submitted from your own `master` branch. For future PRs, we recommend using feature branches instead of committing to your `master`. This makes it easier to keep your fork updated and manage multiple PRs.
See: [Best Practices: Your Fork's Master](https://docs.qmk.fm/newbs_git_using_your_master_branch)
For incorrect target branch:
❌ This PR targets the wrong branch:
- **New keyboard additions** should target `master`
- **Keyboard updates/refactors/moves** should target `develop`
- **Core changes** should target `develop`
Please change the target branch accordingly.
For missing license headers:
❌ Missing GPL-compatible license headers on the following files:
- [list files]
Please add GPL2+ headers (GPL2/GPL3 recommended). Example:
\`\`\`c
// Copyright 2024 Your Name (@yourgithub)
// SPDX-License-Identifier: GPL-2.0-or-later
\`\`\`
For non-lowercase filenames:
❌ The following files/directories must be lowercase:
- [list files]
Exception: Only valid if from upstream sources (LUFA, ChibiOS) or justified by core consistency.
For config.h violations:
⚠️ Found prohibited config.h elements:
- [list specific issues: DESCRIPTION, default value re-definitions, etc.]
Please remove these and refer to [Data Driven Configuration](https://docs.qmk.fm/data_driven_config).
For info.json or keyboard.json issues:
⚠️ info.json or keyboard.json needs attention:
- [list missing mandatory fields]
- Please run: \`qmk format-json -i path/to/info.json\` (or keyboard.json)
- Validate with: \`qmk lint -kb <keyboard_name>\`
For schema validation errors:
❌ Schema validation failed for info.json or keyboard.json:
- [list specific validation errors from schema]
- Check `data/schemas/keyboard.jsonschema` for valid field definitions
- Common issues:
- Invalid data types (e.g., string instead of integer)
- Missing required fields
- Invalid enum values
- Incorrectly formatted pin definitions
For non-pristine default keymap:
⚠️ Default keymap should be pristine (clean slate for users):
- Remove: [custom keycodes/tap dance/macros/etc.]
- Keep it minimal with standard layouts where possible
Consider moving advanced features to a separate example keymap.
Notes for GitHub Copilot
- Focus reviews on objective checklist items that can be automatically verified
- Flag definite violations with ❌
- Suggest improvements for recommendations with ⚠️
- Provide specific file/line references when flagging issues
- Link to relevant QMK documentation for each issue
- Prioritize blocking issues (license, merge conflicts, branch policy)
- Be constructive: Suggest fixes, not just problems
- Acknowledge trade-offs: Some guidelines have valid exceptions
This is meant as a first-pass review to catch common issues before human review. Complex architectural decisions, code quality, and subjective assessments still require human QMK Collaborator review.