Add rgeorge/model_c keyboard#26341
Conversation
6dbbe29 to
b09c38e
Compare
|
This keyboard should be under Also https://github.com/rgeorge/model-c does not seem to be accessible at the moment. |
| /* Matrix pins are driven by MCP23S17 port expanders via SPI - NOT direct GPIO */ | ||
| #define MATRIX_ROW_PINS \ | ||
| { NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN } | ||
| #define MATRIX_COL_PINS \ | ||
| { NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN } | ||
|
|
||
| /* COL2ROW, ROW2COL */ | ||
| #define DIODE_DIRECTION ROW2COL | ||
|
|
There was a problem hiding this comment.
These definitions are not needed (only the default matrix implementation needs them, but your custom matrix implementation does not use them).
| /* Matrix pins are driven by MCP23S17 port expanders via SPI - NOT direct GPIO */ | |
| #define MATRIX_ROW_PINS \ | |
| { NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN } | |
| #define MATRIX_COL_PINS \ | |
| { NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN, NO_PIN } | |
| /* COL2ROW, ROW2COL */ | |
| #define DIODE_DIRECTION ROW2COL |
| /* Bootmagic Lite config */ | ||
| #define BOOTMAGIC_LITE_ROW 0 | ||
| #define BOOTMAGIC_LITE_COLUMN 5 | ||
|
|
There was a problem hiding this comment.
The bootmagic config should be moved to JSON (set bootmagic.matrix).
| /* Bootmagic Lite config */ | |
| #define BOOTMAGIC_LITE_ROW 0 | |
| #define BOOTMAGIC_LITE_COLUMN 5 |
|
|
||
| /* LED pins - active low (sinking current to VDD) */ | ||
| #define LED1_PIN E6 /* Arduino D7 -> PE6 -> QMK E6 */ | ||
| #define LED2_PIN B4 /* Arduino D8 -> PB4 -> QMK B4 */ | ||
| #define LED3_PIN B5 /* Arduino D9 -> PB5 -> QMK B5 */ |
There was a problem hiding this comment.
LEDs also should be configured in JSON (indicators.{caps_lock,num_lock,scroll_lock,on_state}).
| /* LED pins - active low (sinking current to VDD) */ | |
| #define LED1_PIN E6 /* Arduino D7 -> PE6 -> QMK E6 */ | |
| #define LED2_PIN B4 /* Arduino D8 -> PB4 -> QMK B4 */ | |
| #define LED3_PIN B5 /* Arduino D9 -> PB5 -> QMK B5 */ |
| "vid": "0x04D8", | ||
| "pid": "0xED10", |
There was a problem hiding this comment.
This VID belongs to Microchip Technology Inc.; did you actually request and receive a PID from them? If not, the common convention used in QMK is to select a random unregistered VID value above 0x4000 which does not collide with anything in https://yanfali.github.io/qmk_usb_usage/ and https://www.the-sz.com/products/usbid/, and any PID value you like. Alternatively, there are some places which can hand out presumably unique USB VID:PID combinations for Open Hardware projects.
There was a problem hiding this comment.
The custom matrix implementation should be placed in a file named matrix.c.
Any other custom code should be placed in a *.c file with exactly the same name as the last component of the keyboard name (for the currently used name it would be model_c.c, not model-c.c); this file should not be mentioned in SRC (it would be used automatically). However, you may not need such a file at all (the code which inits the matrix pins could be moved to matrix_init_custom(), and the custom LED code won't be needed if you move the LED pin definitions into JSON).
|
|
||
| CUSTOM_MATRIX = lite | ||
|
|
||
| SRC += model-c.c |
There was a problem hiding this comment.
| SRC += model-c.c | |
| SRC += matrix.c |
The custom matrix implementation should be moved to matrix.c, and even if you end up needing <keyboard>.c, it does not need to be mentioned in SRC.
There was a problem hiding this comment.
https://docs.qmk.fm/pr_checklist#keyboard-prs:
default keymaps should be "pristine"
So you should not have your Unicode setup in the default keymap — just make a plain QWERTY keymap.
And also your LAYOUT macro included from model-c.h is completely wrong — you should use #include QMK_KEYBOARD_H to get the generated macro (which would expect key mappings in the same order as defined in JSON, which should roughly match the physical keyboard layout, not the electrical one).
There was a problem hiding this comment.
Use qmk format-json to reformat keyboard.json, so that the file (especially the layout section) would be more readable.
| "bootmagic": true, | ||
| "command": true, | ||
| "extrakey": true, | ||
| "ghost": false, |
There was a problem hiding this comment.
| "ghost": false, |
features.ghost does not exist, so this definition just does nothing. The proper JSON key is matrix_pins.ghost, and I suspect that you actually need to set it to true (because the ghosting behavior when pressing multiple keys at once would be present in a diodeless matrix; the high-impedance deselection which you use only avoids overloading the chip outputs when multiple outputs get connected together).
| "extrakey": true, | ||
| "ghost": false, | ||
| "mousekey": true, | ||
| "nkro": true |
There was a problem hiding this comment.
This is probably useless if your hardware is actually 2KRO.
| "processor": "atmega32u4", | ||
| "bootloader": "caterina", |
There was a problem hiding this comment.
| "processor": "atmega32u4", | |
| "bootloader": "caterina", | |
| "development_board": "promicro", |
This will set the controller and bootloader, and setup compatibility with converters.
IBM Model M compatible keyboard controller using ATmega32U4 (Arduino Pro Micro) with two MCP23S17 SPI port expanders to scan the 8x16 key matrix.
b09c38e to
27367e2
Compare
zvecr
left a comment
There was a problem hiding this comment.
The folder structure here is incorrect. The files need to be inside a sub-folder of keyboards/ibm/model_m and the keymaps need to be within a keymaps folder.
| // Custom Matrix Implementation | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| void matrix_init_custom(void) { |
There was a problem hiding this comment.
| void matrix_init_custom(void) { | |
| void matrix_init_custom(void) { | |
| // Initialize SPI master | |
| spi_init(); | |
| // Set chip select pins as output | |
| gpio_set_pin_output(MCP23S17_CS1_PIN); | |
| gpio_set_pin_output(MCP23S17_CS2_PIN); | |
| // Deselect both chips (active low, so write HIGH) | |
| gpio_write_pin_high(MCP23S17_CS1_PIN); | |
| gpio_write_pin_high(MCP23S17_CS2_PIN); | |
| void keyboard_pre_init_kb(void) { | ||
| // Initialize SPI master | ||
| spi_init(); | ||
|
|
||
| // Set chip select pins as output | ||
| gpio_set_pin_output(MCP23S17_CS1); | ||
| gpio_set_pin_output(MCP23S17_CS2); | ||
|
|
||
| // Deselect both chips (active low, so write HIGH) | ||
| gpio_write_pin_high(MCP23S17_CS1); | ||
| gpio_write_pin_high(MCP23S17_CS2); | ||
|
|
||
| keyboard_pre_init_user(); | ||
| } | ||
|
|
There was a problem hiding this comment.
| void keyboard_pre_init_kb(void) { | |
| // Initialize SPI master | |
| spi_init(); | |
| // Set chip select pins as output | |
| gpio_set_pin_output(MCP23S17_CS1); | |
| gpio_set_pin_output(MCP23S17_CS2); | |
| // Deselect both chips (active low, so write HIGH) | |
| gpio_write_pin_high(MCP23S17_CS1); | |
| gpio_write_pin_high(MCP23S17_CS2); | |
| keyboard_pre_init_user(); | |
| } |
| #define MCP23S17_CS1_PIN C6 /* D5 -> PC6 (CS1 - Expander A for columns) */ | ||
| #define MCP23S17_CS2_PIN D7 /* D6 -> PD7 (CS2 - Expander B for rows) */ |
There was a problem hiding this comment.
Doesnt really add value as the only file thats ever going to use it is the keyboards matrix.c
| #define MCP23S17_CS1_PIN C6 /* D5 -> PC6 (CS1 - Expander A for columns) */ | |
| #define MCP23S17_CS2_PIN D7 /* D6 -> PD7 (CS2 - Expander B for rows) */ |
|
|
||
| // MCP23S17 chip select pins (for two expanders) | ||
| #define MCP23S17_CS1 (uint8_t) MCP23S17_CS1_PIN // CS for expander A (columns), address 0x01 | ||
| #define MCP23S17_CS2 (uint8_t) MCP23S17_CS2_PIN // CS for expander B (rows), address 0x00 |
There was a problem hiding this comment.
| #define MCP23S17_CS2 (uint8_t) MCP23S17_CS2_PIN // CS for expander B (rows), address 0x00 | |
| #define MCP23S17_CS1_PIN C6 /* D5 -> PC6 (CS1 - Expander A for columns) */ | |
| #define MCP23S17_CS2_PIN D7 /* D6 -> PD7 (CS2 - Expander B for rows) */ |
| static void mcp23s17_init_expander(uint8_t cs_pin, uint8_t addr, uint8_t iodir_a, uint8_t iodir_b); | ||
| static void mcp23s17_write_reg(uint8_t cs_pin, uint8_t addr, uint8_t reg_addr, uint8_t data); | ||
| static uint8_t mcp23s17_read_reg(uint8_t cs_pin, uint8_t addr, uint8_t reg_addr); |
There was a problem hiding this comment.
| static void mcp23s17_init_expander(uint8_t cs_pin, uint8_t addr, uint8_t iodir_a, uint8_t iodir_b); | |
| static void mcp23s17_write_reg(uint8_t cs_pin, uint8_t addr, uint8_t reg_addr, uint8_t data); | |
| static uint8_t mcp23s17_read_reg(uint8_t cs_pin, uint8_t addr, uint8_t reg_addr); | |
| static void mcp23s17_init_expander(pin_t cs_pin, uint8_t addr, uint8_t iodir_a, uint8_t iodir_b); | |
| static void mcp23s17_write_reg(pin_t cs_pin, uint8_t addr, uint8_t reg_addr, uint8_t data); | |
| static uint8_t mcp23s17_read_reg(pin_t cs_pin, uint8_t addr, uint8_t reg_addr); |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRA, 0xFF); | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRB, 0xFF); | ||
|
|
||
| if (col < 8) { | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_GPIOA, ~(1 << col)); | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRA, ~(1 << col)); | ||
| } else { | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_GPIOB, ~(1 << (col - 8))); | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRB, ~(1 << (col - 8))); |
There was a problem hiding this comment.
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRA, 0xFF); | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRB, 0xFF); | |
| if (col < 8) { | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_GPIOA, ~(1 << col)); | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRA, ~(1 << col)); | |
| } else { | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_GPIOB, ~(1 << (col - 8))); | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRB, ~(1 << (col - 8))); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_IODIRA, 0xFF); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_IODIRB, 0xFF); | |
| if (col < 8) { | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_GPIOA, ~(1 << col)); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_IODIRA, ~(1 << col)); | |
| } else { | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_GPIOB, ~(1 << (col - 8))); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_IODIRB, ~(1 << (col - 8))); |
| mcp23s17_write_reg(MCP23S17_CS2, 0x00, MCP23S17_IOCON, MCP23S17_IOCON_HAEN); | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x00, MCP23S17_IOCON, MCP23S17_IOCON_HAEN); | ||
|
|
||
| // Expander B (CS2, addr 0x00) - Short 8-pin connector (Rows), all inputs | ||
| mcp23s17_init_expander(MCP23S17_CS2, 0x00, 0xFF, 0xFF); | ||
| // Enable pull-ups on row expander GPA0-GPA7 | ||
| mcp23s17_write_reg(MCP23S17_CS2, 0x00, MCP23S17_GPPUA, 0xFF); | ||
|
|
||
| // Expander A (CS1, addr 0x01) - Long 16-pin connector (Columns) | ||
| // First init as all outputs with OLAT=0xFF (HIGH) so the output latches are set, | ||
| // then switch to all inputs (high-impedance) for diodeless-safe scanning. | ||
| mcp23s17_init_expander(MCP23S17_CS1, 0x01, 0x00, 0x00); | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRA, 0xFF); | ||
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRB, 0xFF); |
There was a problem hiding this comment.
| mcp23s17_write_reg(MCP23S17_CS2, 0x00, MCP23S17_IOCON, MCP23S17_IOCON_HAEN); | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x00, MCP23S17_IOCON, MCP23S17_IOCON_HAEN); | |
| // Expander B (CS2, addr 0x00) - Short 8-pin connector (Rows), all inputs | |
| mcp23s17_init_expander(MCP23S17_CS2, 0x00, 0xFF, 0xFF); | |
| // Enable pull-ups on row expander GPA0-GPA7 | |
| mcp23s17_write_reg(MCP23S17_CS2, 0x00, MCP23S17_GPPUA, 0xFF); | |
| // Expander A (CS1, addr 0x01) - Long 16-pin connector (Columns) | |
| // First init as all outputs with OLAT=0xFF (HIGH) so the output latches are set, | |
| // then switch to all inputs (high-impedance) for diodeless-safe scanning. | |
| mcp23s17_init_expander(MCP23S17_CS1, 0x01, 0x00, 0x00); | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRA, 0xFF); | |
| mcp23s17_write_reg(MCP23S17_CS1, 0x01, MCP23S17_IODIRB, 0xFF); | |
| mcp23s17_write_reg(MCP23S17_CS2_PIN, 0x00, MCP23S17_IOCON, MCP23S17_IOCON_HAEN); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x00, MCP23S17_IOCON, MCP23S17_IOCON_HAEN); | |
| // Expander B (CS2, addr 0x00) - Short 8-pin connector (Rows), all inputs | |
| mcp23s17_init_expander(MCP23S17_CS2_PIN, 0x00, 0xFF, 0xFF); | |
| // Enable pull-ups on row expander GPA0-GPA7 | |
| mcp23s17_write_reg(MCP23S17_CS2_PIN, 0x00, MCP23S17_GPPUA, 0xFF); | |
| // Expander A (CS1, addr 0x01) - Long 16-pin connector (Columns) | |
| // First init as all outputs with OLAT=0xFF (HIGH) so the output latches are set, | |
| // then switch to all inputs (high-impedance) for diodeless-safe scanning. | |
| mcp23s17_init_expander(MCP23S17_CS1_PIN, 0x01, 0x00, 0x00); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_IODIRA, 0xFF); | |
| mcp23s17_write_reg(MCP23S17_CS1_PIN, 0x01, MCP23S17_IODIRB, 0xFF); |
Description
Adds the Model-C keyboard — a custom IBM Model M compatible keyboard controller using QMK firmware.
Hardware
Implementation
Uses CUSTOM_MATRIX=lite with MCP23S17 port expanders connected via hardware SPI. The matrix scanning uses high-impedance column deselection to avoid sneak paths in the diodeless matrix.
Checklist