copilot-instructions.md (18643B)
1 --- 2 applyTo: "**" 3 excludeAgent: 4 - "coding-agent" 5 --- 6 # GitHub Copilot Instructions for QMK Pull Request Review 7 This document provides automated review guidance based on the [QMK PR Checklist](https://docs.qmk.fm/pr_checklist) and it is intended only for use by GitHub Copilot code-review agent during pull request reviews. 8 9 ## General PR Requirements 10 11 ### Branch and Submission Standards 12 - **Source Branch Policy**: Verify PR is NOT submitted from submitter's own `master` branch 13 - Flag if submitter is using their own `master` branch as source 14 - Suggest using feature branches instead for cleaner fork management 15 - **Target Branch Policy**: 16 - **New keyboard additions** → `master` branch (new folders under `keyboards/`) 17 - **All other changes** → `develop` branch: 18 - Keyboard updates, refactors, or moves 19 - Core code changes 20 - Data-driven configuration migrations 21 - Any modifications to existing keyboards 22 - **PR Scope**: PRs should contain the smallest set of modifications for a single change 23 - Flag PRs that modify multiple keyboards simultaneously 24 - Suggest splitting large PRs into focused, incremental changes 25 - **Merge Conflicts**: Check for unresolved merge conflicts 26 27 ### File Naming and Structure 28 - **Lowercase Requirement**: All new directories and filenames must be lowercase 29 - Exception: Upstream sources with original uppercase (LUFA, ChibiOS) 30 - Exception: Core files with valid justification 31 - **Reject**: Board designer preference for uppercase is NOT valid justification 32 33 ### License Headers 34 - **Required**: Valid license headers on all `*.c` and `*.h` files 35 - **Recommended**: GPL2/GPL3 for consistency 36 - **Format**: Check for proper GPL2+ header or SPDX identifier 37 ```c 38 // Copyright 2024 Your Name (@yourgithub) 39 // SPDX-License-Identifier: GPL-2.0-or-later 40 ``` 41 - **Exception**: Simple assignment-only `rules.mk` files don't need headers 42 - **Flag**: Missing or ambiguous license headers (blocks merge) 43 44 ### QMK Best Practices 45 - **Include Guards**: Use `#pragma once` instead of `#ifndef` guards in headers 46 - **Abstractions Required**: No low-level GPIO/I2C/SPI functions 47 - Must use QMK abstractions (flag direct hardware access) 48 - **Timing Functions**: 49 - Use `wait_ms()` instead of `_delay_ms()` 50 - Remove `#include <util/delay.h>` 51 - Use `timer_read()`, `timer_read32()` from `timer.h` 52 - **New Abstractions**: If proposing new abstraction, suggest: 53 1. Prototype in own keyboard first 54 2. Discuss with QMK Collaborators on Discord 55 3. Refactor as separate core change 56 4. Remove the keyboard-specific implementation from board 57 58 --- 59 60 ## Keymap PR Reviews 61 62 **Scope**: These rules apply to files within `keyboards/*/keymaps/*` subdirectories. 63 64 ### Note on Personal Keymaps 65 - **Policy Change**: Personal keymap submissions no longer accepted 66 - **Permitted**: Vendor-specific keymaps only 67 - Naming convention: `default_${vendor}` (e.g., `default_clueboard`) 68 - Can be more feature-rich than stock `default` keymaps 69 70 ### Keymap Code Standards 71 - **Includes**: `#include QMK_KEYBOARD_H` preferred over specific board files 72 - **Enums**: Prefer layer enums to `#define`s 73 - **Custom Keycodes**: First entry must be `QK_USER` 74 - **Formatting**: Check spacing alignment on commas and keycodes (spaces, not tabs) 75 - **VIA**: Keymaps should NOT enable VIA 76 - VIA keymaps belong in [VIA QMK Userspace](https://github.com/the-via/qmk_userspace_via) 77 78 --- 79 80 ## Keyboard PR Reviews 81 82 **Scope**: These rules apply to keyboard-level files in `keyboards/*` directories, excluding files within the `keymaps/` subdirectories. This includes: 83 - `info.json` or `keyboard.json` (keyboard root or variant level) 84 - `readme.md` (keyboard level) 85 - `rules.mk` (keyboard level) 86 - `config.h` (keyboard level, not keymap level) 87 - `<keyboard>.c` and `<keyboard>.h` files 88 - Hardware configuration files (`halconf.h`, `mcuconf.h`, `chconf.h`) 89 90 ### Branch Targeting 91 - **New Keyboards**: Target `master` branch 92 - New additions to `keyboards/` folder submit to `master` 93 - **Keyboard Moves**: Must target `develop` branch 94 - Check `data/mappings/keyboard_aliases.hjson` is updated for moves 95 - **Keyboard Updates/Refactors**: Must target `develop` to reduce merge conflicts 96 - **Data Driven Migration**: Must target `develop` 97 98 ### info.json and keyboard.json Requirements 99 - **Data-Driven Configuration**: Encourage maximum use of `info.json` and `keyboard.json` schema features 100 - **Schema Validation**: All `info.json` and `keyboard.json` files must validate against `data/schemas/keyboard.jsonschema` 101 - Use QMK CLI: `qmk lint -kb <keyboard_name>` to validate 102 - Schema defines required fields, data types, and valid values 103 - Check for schema validation errors before submitting PR 104 - **Mandatory Elements**: 105 - Valid URL 106 - Valid maintainer 107 - Valid USB VID/PID and device version 108 - Displays correctly in Configurator (Ctrl+Shift+I to preview) 109 - `layout` definitions include matrix positions 110 - Standard layout definitions where applicable 111 - Community Layout macro names when applicable 112 - Microcontroller and bootloader specified 113 - Diode direction (if not using direct pins) 114 - **Layout Naming**: 115 - Single layout: Use `LAYOUT` or community layout name 116 - Multiple layouts: Include `LAYOUT_all` + alternate names 117 - Prefer community layout names (e.g., `LAYOUT_tkl_ansi`, `LAYOUT_ortho_4x4`) 118 - **Configuration in info.json or keyboard.json** (when applicable): 119 - Direct pin configuration 120 - Backlight, Split keyboard, Encoder, Bootmagic configs 121 - LED Indicator, RGB Light, RGB Matrix configs 122 - **Format**: Run `qmk format-json -i` before submitting 123 124 ### USB VID/PID Uniqueness 125 VID+PID combination must be unique across all keyboards. Individual VID or PID values can be reused with different partners. 126 **Validation Steps:** 127 1. Extract VID and PID from keyboard.json/info.json in the PR 128 2. Search for existing usage: `grep -r '"vid".*"0xVVVV"' keyboards/ --include="*.json" | grep -l '"pid".*"0xPPPP"'` 129 3. If results found: Check if BOTH VID AND PID match in same file 130 - Both match = **COLLISION** - request different PID 131 - Only one matches = **OK** - different keyboards can share individual values 132 4. For keyboard variants/revisions under same keyboard folder: 133 - Different PID recommended for functionally different variants 134 - Same PID acceptable if revisions only differ in hardware routing/pin assignments 135 **Quick Reference:** 136 - Same PID + Different VID = Valid 137 - Same VID + Different PID = Valid 138 - Same VID + Same PID = Invalid 139 **Review Response:** 140 For collision: 141 ``` 142 VID+PID collision: 0xVVVV:0xPPPP already used by keyboards/[path]/file.json 143 +Please assign a different PID. VID can remain the same. 144 ``` 145 For uniqueness confirmed: 146 ``` 147 VID+PID validation: 0xVVVV:0xPPPP is unique (no collisions found) 148 ``` 149 150 ### readme.md Requirements 151 - **Template**: Must follow [official template](https://github.com/qmk/qmk_firmware/blob/master/data/templates/keyboard/readme.md) 152 - **Flash Command**: Present with `:flash` at end 153 - **Hardware Link**: Valid availability link (unless handwired) 154 - Private groupbuys acceptable 155 - One-off prototypes will be questioned 156 - Open-source should link to files 157 - **Reset Instructions**: Clear bootloader mode instructions 158 - **Images Required**: 159 - Keyboard and PCB photos preferred 160 - Must be hosted externally (imgur, etc.) 161 - Direct image links required (not preview pages) 162 - Example: `https://i.imgur.com/vqgE7Ok.jpg` not `https://imgur.com/vqgE7Ok` 163 164 ### rules.mk Standards 165 - **Removed Items**: 166 - `MIDI_ENABLE`, `FAUXCLICKY_ENABLE`, `HD44780_ENABLE` 167 - Size comments like `(-/+size)` 168 - Alternate bootloader lists if one specified 169 - MCU parameter re-definitions matching defaults in `mcu_selection.mk` 170 - **Comment Updates**: Change bootloader comments to generic 171 - **Forbidden Features at Keyboard Level** (these belong in keymap-level `rules.mk` only): 172 - `COMBO_ENABLE` 173 - `ENCODER_MAP_ENABLE` 174 175 ### config.h Standards (Keyboard Level) 176 - **Prohibited**: 177 - `#define DESCRIPTION` 178 - Magic Key Options, MIDI Options, HD44780 configuration 179 - User preference `#define`s (belong in keymap) 180 - Re-defining default values (`DEBOUNCE`, RGB settings) 181 - Copy/pasted comment blocks explaining features 182 - Commented-out unused defines 183 - `#include "config_common.h"` 184 - `#define MATRIX_ROWS/COLS` (unless custom matrix) 185 - **Minimal Code**: Only critical board boot code required 186 - **No Vial**: Vial-related files/changes not accepted 187 188 ### Keyboard Implementation Files 189 190 #### `<keyboard>.c` 191 - **Remove Empty Functions**: Delete empty or commented-out weak-defined functions 192 - `xxxx_xxxx_kb()`, `xxxx_xxxx_user()` implementations 193 - **Migration**: `matrix_init_board()` → `keyboard_pre_init_kb()` 194 - **Custom Matrix**: Use `lite` variant when possible for standard debounce 195 - `CUSTOM_MATRIX = lite` preferred 196 - Full custom matrix (`yes`) requires justification 197 - **LED Indicators**: Prefer Configuration Options over custom `led_update_*()` implementations 198 - **Hardware Configuration**: Basic functionality for OLED, encoders, etc. at keyboard level 199 200 #### `<keyboard>.h` 201 - **Include**: `#include "quantum.h"` at top 202 - **Layout Macros**: Move to `info.json` or `keyboard.json` (no longer in header) 203 204 ### Default Keymap Standards 205 206 **Scope**: These rules specifically apply to files within `keyboards/*/keymaps/default/` directories. 207 208 - **Pristine Requirement**: Bare minimum clean slate 209 - No custom keycodes 210 - No advanced features (non-exhaustive list of examples: tap dance, macros) 211 - Basic mod taps and home row mods acceptable when necessary 212 - Standard layouts preferred -- see examples in `layouts/default/` and `layouts/community/` 213 - **Removed Examples**: Delete `QMKBEST`/`QMKURL` macros 214 - **Tri Layer**: Use Tri Layer feature instead of manual `layer_on/off()` + `update_tri_layer()` 215 - **Encoder Map**: Use encoder map feature, `encoder_update_user()` may not be present 216 - **No VIA**: Default keymap should not enable VIA 217 - **Additional Keymaps**: Example/bells-and-whistles keymaps acceptable in same PR (separate from default) 218 219 ### Prohibited Files 220 - **No VIA JSON**: Belongs in [VIA Keyboard Repo](https://github.com/the-via/keyboards) 221 - **No KLE JSON**: Not used within QMK 222 - **No Cross-Keyboard Sources**: Don't include files from other keyboard vendors 223 - Exception: Core files (e.g., `drivers/sensors/pmw3360.c`) 224 - Use of vendor-specific code (e.g., `wilba_tech/wt_main.c`) only when keyboard exists in the same enclosing vendor folder (e.g. a `wilba_tech` keyboard) 225 - Multi-board code is candidate for core refactoring when intended for use by multiple vendors 226 227 ### Wireless Keyboards 228 - **Policy**: Wireless/Bluetooth PRs rejected without complete wireless code 229 - Wireless code may not include anything resembling precompiled data such as `*.a` files or other libraries 230 - Firmware blobs are not permitted in raw form or as compiled C-style arrays either. 231 - GPL2+ license requires full source disclosure 232 - Historically abused for VIA compatibility without releasing sources 233 - PRs without wireless capability will be held indefinitely 234 - Existing merged wireless boards from same vendor held until sources provided 235 236 ### ChibiOS-Specific Requirements 237 - **Board Definitions**: Strong preference for existing ChibiOS board definitions 238 - Use equivalent Nucleo boards when possible 239 - Example: STM32L082KZ can use `BOARD = ST_NUCLEO64_L073RZ` 240 - QMK is eliminating custom board definitions due to maintenance burden 241 - **New Board Definitions**: 242 - Must NOT be embedded in keyboard PR 243 - Submit as separate Core PR 244 - `board.c` must have standard `__early_init()` and empty `boardInit()` 245 - Migrate code intended for `__early_init()` → keyboard-local `early_hardware_init_pre/post()` 246 - Migrate code intended for `boardInit()` → keyboard-local `board_init()` 247 248 --- 249 250 ## Core PR Reviews 251 252 ### Targeting and Scope 253 - **Branch**: All core PRs must target `develop` branch 254 - **Single Focus**: Smallest set of changes per PR 255 - PRs with multiple areas will be asked to split 256 - Keyboard/keymap changes only if affecting base builds or default-like keymaps 257 - Keymap modifications (non-default) should be followup PR after core merge 258 - Large refactoring PRs affecting other keymaps raised separately 259 260 ### Testing Requirements 261 - **New Hardware Support**: Requires test keyboard under `keyboards/handwired/onekey` 262 - New MCUs: Add child keyboard targeting new MCU for build verification 263 - New hardware (displays, matrix, peripherals): Provide associated keymap 264 - Exception: If existing keymap can leverage functionality (consult Collaborators) 265 - **Callbacks**: New `_kb`/`_user` callbacks must return `bool` for user override 266 - **Unit Tests**: Strongly recommended, may be required 267 - Critical code areas (keycode pipeline) will require tests 268 - Boost confidence in current and future correctness 269 270 ### Code Quality 271 - **Subjective Review**: Other requirements at QMK Collaborators' discretion 272 - **Documentation**: Core changes should be well-documented 273 274 --- 275 276 ## Automated Review Checklist 277 278 When reviewing PRs, check the following systematically: 279 280 ### File Changes Review 281 1. **License headers** on all C/H files (GPL2+ preferred, others must be GPL2+ compatible, SPDX format preferred) 282 2. **File naming** lowercase (flag exceptions needing justification) 283 3. **Include guards** use `#pragma once` 284 4. **No low-level hardware access** (GPIO, I2C, SPI direct register writes) 285 5. **Timing abstractions** (`wait_ms()`, `timer_read()` usage) 286 287 ### info.json and keyboard.json Validation 288 1. **Schema Compliance**: `keyboard.json` and `info.json` files validate against `data/schemas/keyboard.jsonschema` 289 - Both files are identical syntax, however the `keyboard.json` dictates a buildable target, `info.json` does not 290 - Run `qmk lint -kb <keyboard>` to check schema validation 291 - Check for proper data types (strings, integers, arrays, objects) 292 - Verify required fields are present 293 - Ensure enum values match allowed options in schema 294 2. All mandatory fields present and valid 295 3. `qmk format-json -i` has been run (formats and validates) 296 4. Layout macros moved from headers 297 5. Community layout names used where applicable 298 299 ### rules.mk Cleanup 300 1. Deprecated features removed 301 2. No size comments 302 3. No keymap-only features at keyboard level 303 4. No redundant MCU parameter definitions 304 305 ### config.h Cleanup 306 1. No `DESCRIPTION`, `config_common.h`, or prohibited includes 307 2. No default value re-definitions 308 3. No commented-out defines or feature documentation blocks 309 4. No user preference defines at keyboard level 310 311 ### Keymap Quality 312 1. Default keymaps are pristine (no custom keycodes/advanced features) 313 2. No `QMKBEST`/`QMKURL` macros 314 3. Encoder map feature used instead of `encoder_update_user()` 315 4. Tri Layer feature used for multi-layer access 316 5. No VIA enabled in default keymap 317 318 ### Documentation 319 1. readme.md follows template 320 2. Flash command present with `:flash` 321 3. Reset instructions clear 322 4. External image hosting (direct links) 323 5. Valid hardware availability link 324 325 ### Code Organization 326 1. Empty weak-defined functions removed from `<keyboard>.c` 327 2. Proper migration of init functions 328 3. No cross-vendor source files 329 4. No VIA/KLE JSON files 330 331 ### Branch and Scope 332 1. Not submitted from submitter's own `master` branch (use feature branches) 333 2. PR is focused on single change 334 3. Targets correct branch: 335 - `master` for new keyboard additions 336 - `develop` for keyboard updates/refactors/moves and core changes 337 4. No merge conflicts 338 339 --- 340 341 ## Review Response Templates 342 343 ### For source master branch usage: 344 ``` 345 ⚠️ 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. 346 347 See: [Best Practices: Your Fork's Master](https://docs.qmk.fm/newbs_git_using_your_master_branch) 348 ``` 349 350 ### For incorrect target branch: 351 ``` 352 ❌ This PR targets the wrong branch: 353 - **New keyboard additions** should target `master` 354 - **Keyboard updates/refactors/moves** should target `develop` 355 - **Core changes** should target `develop` 356 357 Please change the target branch accordingly. 358 ``` 359 360 ### For missing license headers: 361 ``` 362 ❌ Missing GPL-compatible license headers on the following files: 363 - [list files] 364 365 Please add GPL2+ headers (GPL2/GPL3 recommended). Example: 366 \`\`\`c 367 // Copyright 2024 Your Name (@yourgithub) 368 // SPDX-License-Identifier: GPL-2.0-or-later 369 \`\`\` 370 ``` 371 372 ### For non-lowercase filenames: 373 ``` 374 ❌ The following files/directories must be lowercase: 375 - [list files] 376 377 Exception: Only valid if from upstream sources (LUFA, ChibiOS) or justified by core consistency. 378 ``` 379 380 ### For config.h violations: 381 ``` 382 ⚠️ Found prohibited config.h elements: 383 - [list specific issues: DESCRIPTION, default value re-definitions, etc.] 384 385 Please remove these and refer to [Data Driven Configuration](https://docs.qmk.fm/data_driven_config). 386 ``` 387 388 ### For info.json or keyboard.json issues: 389 ``` 390 ⚠️ info.json or keyboard.json needs attention: 391 - [list missing mandatory fields] 392 - Please run: \`qmk format-json -i path/to/info.json\` (or keyboard.json) 393 - Validate with: \`qmk lint -kb <keyboard_name>\` 394 ``` 395 396 ### For schema validation errors: 397 ``` 398 ❌ Schema validation failed for info.json or keyboard.json: 399 - [list specific validation errors from schema] 400 - Check `data/schemas/keyboard.jsonschema` for valid field definitions 401 - Common issues: 402 - Invalid data types (e.g., string instead of integer) 403 - Missing required fields 404 - Invalid enum values 405 - Incorrectly formatted pin definitions 406 ``` 407 408 ### For non-pristine default keymap: 409 ``` 410 ⚠️ Default keymap should be pristine (clean slate for users): 411 - Remove: [custom keycodes/tap dance/macros/etc.] 412 - Keep it minimal with standard layouts where possible 413 414 Consider moving advanced features to a separate example keymap. 415 ``` 416 417 --- 418 419 ## Notes for GitHub Copilot 420 421 - Focus reviews on **objective checklist items** that can be automatically verified 422 - Flag **definite violations** with ❌ 423 - Suggest improvements for **recommendations** with ⚠️ 424 - **Provide specific file/line references** when flagging issues 425 - **Link to relevant QMK documentation** for each issue 426 - **Prioritize blocking issues** (license, merge conflicts, branch policy) 427 - **Be constructive**: Suggest fixes, not just problems 428 - **Acknowledge trade-offs**: Some guidelines have valid exceptions 429 430 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.