Add support for external userspace, encoder maps, and more#5
Add support for external userspace, encoder maps, and more#5schuay wants to merge 21 commits intotompi:masterfrom
Conversation
`ws812` -> `ws2812`
…-side mods and fix heading for Speculative Hold. (qmk#25924) * Note Chordal Hold supports multiple same-side mods. * Fix "Speculative Hold" heading from H3 -> H2.
Handle broken symlinks in 'qmk doctor' udev checks
) Bumps [JamesIves/github-pages-deploy-action](https://github.com/jamesives/github-pages-deploy-action) from 4.7.6 to 4.8.0. - [Release notes](https://github.com/jamesives/github-pages-deploy-action/releases) - [Commits](JamesIves/github-pages-deploy-action@v4.7.6...v4.8.0) --- updated-dependencies: - dependency-name: JamesIves/github-pages-deploy-action dependency-version: 4.8.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Update support S6xty5 * Delete chconf.h * Update manufacturer, community layout for hhkb * Update layout * fix row index * Update tyson60s * Update community layout and layout name * Update remove rgb test mode * Update capslock led * Apply suggestions from code review Co-authored-by: Duncan Sutherland <dunk2k_2000@hotmail.com> * Remove deprecated s6xty5 * Update tyson60 product id --------- Co-authored-by: Trần Thanh Sơn <son.tt1@teko.vn> Co-authored-by: Duncan Sutherland <dunk2k_2000@hotmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for the Cheapino keyboard to QMK firmware, including custom matrix scanning, encoder handling via matrix intersections, RGB lighting with startup effects, and ghosting mitigation for the keyboard's unique hardware design.
Changes:
- Added custom matrix scanning implementation to handle the Cheapino's unique wiring scheme
- Implemented encoder support using matrix intersections instead of dedicated pins
- Added RGB lighting with startup LED flash animation and ghosting detection/mitigation logic
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| keyboards/cheapino/rules.mk | Includes custom matrix.c implementation |
| keyboards/cheapino/readme.md | Adds keyboard documentation with build and flash instructions |
| keyboards/cheapino/mcuconf.h | Configures I2C peripheral for RP2040 |
| keyboards/cheapino/matrix.c | Implements custom matrix scanning with dual-direction scanning |
| keyboards/cheapino/keyboard.json | Defines keyboard configuration, layout, features, and pin mappings |
| keyboards/cheapino/config.h | Configures WS2812 RGB LED settings |
| keyboards/cheapino/cheapino.c | Implements keyboard initialization, encoder handling, and ghosting mitigation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| extern void fix_encoder_action(matrix_row_t current_matrix[]); | ||
| extern void fix_ghosting(matrix_row_t matrix[]); |
There was a problem hiding this comment.
External function declarations should typically be placed in header files rather than inside function bodies. Consider moving these declarations to a header file that can be included in both matrix.c and cheapino.c for better code organization and type safety.
| bool colA = encoder_row & ENC_A_BIT; | ||
| bool colB = encoder_row & ENC_B_BIT; | ||
|
|
||
| extern bool encoder_queue_event(uint8_t, bool); |
There was a problem hiding this comment.
The external function declaration should be placed at the top of the file or in a header file rather than inside the function body. This improves code organization and makes the dependency more explicit.
| for (short i = 0; i<3; i++) { | ||
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | ||
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | ||
| } | ||
|
|
||
| // Then exactly same procedure on the left side | ||
| for (short i = 0; i<3; i++) { | ||
| fix_ghosting_instance(matrix, i+4, possible_error_cause<<6, 4+((i+1)%3), possible_error<<6, error_fix<<6); | ||
| fix_ghosting_instance(matrix, i+4, possible_error_cause<<6, 4+((i+2)%3), possible_error<<6, error_fix<<6); |
There was a problem hiding this comment.
There's inconsistent spacing in the loop condition. Use consistent spacing around operators for better readability.
| for (short i = 0; i<3; i++) { | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | |
| } | |
| // Then exactly same procedure on the left side | |
| for (short i = 0; i<3; i++) { | |
| fix_ghosting_instance(matrix, i+4, possible_error_cause<<6, 4+((i+1)%3), possible_error<<6, error_fix<<6); | |
| fix_ghosting_instance(matrix, i+4, possible_error_cause<<6, 4+((i+2)%3), possible_error<<6, error_fix<<6); | |
| for (short i = 0; i < 3; i++) { | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i + 1) % 3, possible_error, error_fix); | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i + 2) % 3, possible_error, error_fix); | |
| } | |
| // Then exactly same procedure on the left side | |
| for (short i = 0; i < 3; i++) { | |
| fix_ghosting_instance(matrix, i + 4, possible_error_cause << 6, 4 + ((i + 1) % 3), possible_error << 6, error_fix << 6); | |
| fix_ghosting_instance(matrix, i + 4, possible_error_cause << 6, 4 + ((i + 2) % 3), possible_error << 6, error_fix << 6); |
| for (short i = 0; i<3; i++) { | ||
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | ||
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | ||
| } | ||
|
|
||
| // Then exactly same procedure on the left side | ||
| for (short i = 0; i<3; i++) { |
There was a problem hiding this comment.
There's inconsistent spacing in the loop condition. Use consistent spacing around operators for better readability.
| for (short i = 0; i<3; i++) { | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | |
| } | |
| // Then exactly same procedure on the left side | |
| for (short i = 0; i<3; i++) { | |
| for (short i = 0; i < 3; i++) { | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | |
| } | |
| // Then exactly same procedure on the left side | |
| for (short i = 0; i < 3; i++) { |
|  | ||
|
|
There was a problem hiding this comment.
The placeholder text "imgur.com image replace me!" should be replaced with an actual image URL or removed. This is incomplete documentation.
|  |
| // First the right side | ||
| for (short i = 0; i<3; i++) { | ||
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | ||
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | ||
| } | ||
|
|
||
| // Then exactly same procedure on the left side |
There was a problem hiding this comment.
The loop condition uses the modulo operator to cycle through indices, but the logic is complex and could benefit from clearer documentation explaining why indices (i+1)%3 and (i+2)%3 are used. This would improve code maintainability.
| // First the right side | |
| for (short i = 0; i<3; i++) { | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | |
| } | |
| // Then exactly same procedure on the left side | |
| // First the right side. | |
| // For each of the three rows on this side (i = 0..2), check for ghosting | |
| // interactions with the other two rows. The expressions (i+1)%3 and (i+2)%3 | |
| // cycle through the two remaining row indices in {0,1,2} without going out | |
| // of bounds, so every pair of rows on the right side is considered. | |
| for (short i = 0; i<3; i++) { | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+1)%3, possible_error, error_fix); | |
| fix_ghosting_instance(matrix, i, possible_error_cause, (i+2)%3, possible_error, error_fix); | |
| } | |
| // Then exactly same procedure on the left side. | |
| // Rows for the left side are stored starting at index 4, so we add 4 to the | |
| // row indices. The bit patterns are shifted by 6 to match the left-half | |
| // column positions, but the modulo math ((i+1)%3, (i+2)%3) still cycles | |
| // within the three left-side rows in the same way as on the right. |
|
|
||
|  | ||
|
|
||
| *A short description of the keyboard/project* |
There was a problem hiding this comment.
The placeholder text "A short description of the keyboard/project" should be replaced with an actual description of the Cheapino keyboard. This is incomplete documentation.
| *A short description of the keyboard/project* | |
| *Cheapino is a budget-friendly, QMK-powered custom keyboard designed as an accessible entry point for building and programming your own mechanical keyboard.* |
keyboards/cheapino/keyboard.json
Outdated
| @@ -0,0 +1,137 @@ | |||
| { | |||
| "manufacturer": "Thomas Haukland", | |||
| "keyboard_name": "cheapino2", | |||
There was a problem hiding this comment.
The keyboard_name is set to "cheapino2" but the directory and other references use "cheapino". This inconsistency may cause confusion. Consider using "cheapino" for consistency or updating all references to match "cheapino2".
| "keyboard_name": "cheapino2", | |
| "keyboard_name": "cheapino", |
| /* This is for debugging the matrix rows | ||
| void printBits(uint16_t n) | ||
| { | ||
| long i; | ||
| for (i = 15; i >= 0; i--) { | ||
| if ((n & (1 << i)) != 0) { | ||
| printf("1"); | ||
| } | ||
| else { | ||
| printf("0"); | ||
| } | ||
| } | ||
| printf("\n"); | ||
| } | ||
| */ | ||
|
|
There was a problem hiding this comment.
This commented-out debugging function should either be removed if not needed, or properly documented if it's intended to be kept for future debugging purposes. Leaving large blocks of commented code reduces code maintainability.
| /* This is for debugging the matrix rows | |
| void printBits(uint16_t n) | |
| { | |
| long i; | |
| for (i = 15; i >= 0; i--) { | |
| if ((n & (1 << i)) != 0) { | |
| printf("1"); | |
| } | |
| else { | |
| printf("0"); | |
| } | |
| } | |
| printf("\n"); | |
| } | |
| */ |
| // These are observed ghosts(following a pattern). TODO: need to fix this for v3 | ||
| // Might need to add 2 diodes(one in each direction) for every row, to increase voltage drop. |
There was a problem hiding this comment.
The comment references "v3" but it's unclear what version the current code is for. This comment should be updated to clarify which version it applies to, or the TODO should be addressed if this is for v3.
| // These are observed ghosts(following a pattern). TODO: need to fix this for v3 | |
| // Might need to add 2 diodes(one in each direction) for every row, to increase voltage drop. | |
| // These are observed ghosts (following a pattern). TODO: if a v3 PCB/hardware revision is created, | |
| // re-validate these patterns and update this table as needed. Might need to add 2 diodes | |
| // (one in each direction) for every row, to increase voltage drop. |
|
Just making sure - you're not waiting for me to respond to the copilot review, right? |
|
@schuay, I would like to test this. It seems there is a |
Yes, this PR only provides the firmware. Did you see tompi/cheapino#155 (comment) where others have used this successfully? Porting existing cheapino keymaps should only be a matter of updating the encoder mapping, which is now part of the LAYOUT_split_3x5_3 macro. See https://github.com/tompi/qmk_firmware/pull/5/files#diff-8512016014c07fd247117877fd233e3b170d1d3debed4eb98bdba00d954e0644R92. |
No, I hadn't seen it. I gives clear instructions from where to get the bits and pieces. Thank you for the hint!
Related to this is a comment of @FlorianGabelle. He writes:
Can you confirm this? |
Yeah exactly. It's there because that's where the encoder is physically located. Please keep further discussion in the issue (not this PR). Good luck! |
* Fixup tominabox1 le chiffre default keymap * Apply suggestions from code review Co-authored-by: Jack Sangdahl <jack@pngu.org> --------- Co-authored-by: Jack Sangdahl <jack@pngu.org>
|
@tompi can I do anything to unblock this PR? |
|
Hey, sorry about the delay, but tried to fetch the pr and compile, but seems like there is no default keymap? Should this be merged to the cheapino-branch? |
* Extract keymap definitions to follow the external userspace model. A default keymap should probably be added again as an example. * Move configuration to keyboard.json. * Enable LTO. * Move encoder button handling to the keymap for full qmk feature support (layers, mod-tap). * Inject encoder turn events into the qmk encoder pipeline, with the same motivation as above. * Rename files to avoid clashing with qmk-internal files (encoder.h). * Faster matrix store/compare primitives.
I've added one in the latest push a minute ago, based on your current default keymap but with the added encoder position. Please try again. Wrt merging: yes, into the cheapino branch, which additionally would have to be rebased on top of current QMK head. Maybe it's easiest to do this all locally on your side? Or, if you prefer, you could create a cheapino-next branch based off qmk_firmware HEAD and I can resubmit the PR there. |
I think in the 3. table, row 1 is mistake.
See tompi/cheapino#155
I ended up merging most things into cheapino.c, except matrix.c due to the license. What do you think? Which branch would you like to merge into?