Keychron / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
758 stars 1.02k forks source link

[Bug] Wrong LED count for V1 Max #228

Closed mklein994 closed 9 months ago

mklein994 commented 9 months ago

Describe the Bug

I think the count given here is wrong:

https://github.com/Keychron/qmk_firmware/blob/f9f4cf410fcc9c45810c2067ab6c8c052169a0a5/keyboards/keychron/v1_max/ansi_encoder/config.h#L21-L24

I propose this change:

diff --git a/keyboards/keychron/v1_max/ansi_encoder/config.h b/keyboards/keychron/v1_max/ansi_encoder/config.h
index 6a2ba7b89f..ed4b1630c2 100644
--- a/keyboards/keychron/v1_max/ansi_encoder/config.h
+++ b/keyboards/keychron/v1_max/ansi_encoder/config.h
@@ -19,8 +19,8 @@
 #ifdef RGB_MATRIX_ENABLE
 /* RGB Matrix driver configuration */
 #    define DRIVER_COUNT 2
-#    define DRIVER_1_LED_COUNT 45
-#    define DRIVER_2_LED_COUNT 39
+#    define DRIVER_1_LED_COUNT 44
+#    define DRIVER_2_LED_COUNT 37
 #    define RGB_MATRIX_LED_COUNT (DRIVER_1_LED_COUNT + DRIVER_2_LED_COUNT)

 #    define SPI_SCK_PIN A5

Keyboard Used

keychron/v1_max/ansi_encoder

Link to product page (if applicable)

https://keychron.ca/products/keychron-v1-max-qmk-via-wireless-custom-mechanical-keyboard?variant=41206028173395

Operating System

Arch Linux

qmk doctor Output

Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.2
Ψ QMK home: /home/matthew/qmk_firmware
Ψ Detected Linux (Arch Linux).
Ψ Userspace enabled: False
Ψ Git branch: dev
Ψ Repo version: 0.23.1
⚠ Git has unstashed/uncommitted changes.
Ψ - Latest dev: 2024-01-24 19:32:51 -0600 (a972dbf9b5) -- WIP: Disable all animations
Ψ - Latest upstream/master: 2024-02-06 18:09:22 +1100 (45ae4dec4d) -- WS2812: Better error message when trying to use `bitbang` driver on RP2040 (#23025)
Ψ - Latest upstream/develop: 2024-02-09 22:37:18 +1100 (a5ea619139) -- LED drivers: place I2C addresses into an array (#22975)
Ψ - Common ancestor with upstream/master: 2023-12-18 13:53:02 +1100 (9539f135d8) -- Remove obvious user keymaps, `keyboards/[0-9]*` edition. (#22691)
Ψ - Common ancestor with upstream/develop: 2023-12-18 13:53:02 +1100 (9539f135d8) -- Remove obvious user keymaps, `keyboards/[0-9]*` edition. (#22691)
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 13.2.0
Ψ Found avr-gcc version 13.2.0
⚠ We do not recommend avr-gcc newer than 8. Downgrading to 8.x is recommended.
Ψ Found avrdude version 7.3
Ψ Found dfu-programmer version 1.0.0
Ψ Found dfu-util version 0.11
Ψ Submodules are up to date.
Ψ Submodule status:
Ψ - lib/chibios: 2024-01-10 10:36:30 +0800 --  (86a3ed9ba)
Ψ - lib/chibios-contrib: 2023-07-17 11:39:05 +0200 --  (da78eb37)
Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 --  (e2239ee6)
Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 --  (549b97320)
Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 --  (819dbc1)
Ψ - lib/printf: 2022-06-29 23:59:58 +0300 --  (c2e3b4e)
Ψ - lib/pico-sdk: 2023-02-12 20:19:37 +0100 --  (a3398d8)
Ψ - lib/lvgl: 2022-04-11 04:44:53 -0600 --  (e19410f8)
Ψ QMK is ready to go, but minor problems were found

Is AutoHotKey / Karabiner installed

Other keyboard-related software installed

OpenRGB, hid_listen

Additional Context

Here's a modified diagram from qmk info -l -kb keychron/v1_max/ansi_encoder -km default:

                                                                   Count
┌──┐ ┌──┐┌──┐┌──┐┌──┐ ┌──┐┌──┐┌──┐┌──┐ ┌──┐┌──┐┌──┐┌──┐ ┌──┐ ╭──╮
│0 │ │0 ││0 ││0 ││0 │ │0 ││0 ││0 ││0 │ │0 ││0 ││0 ││0 │ │0 │ ▲  ▼  14
└──┘ └──┘└──┘└──┘└──┘ └──┘└──┘└──┘└──┘ └──┘└──┘└──┘└──┘ └──┘ ╰──╯
┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──────┐ ┌──┐
│0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0     │ │0 │  15
└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──────┘ └──┘
┌────┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌────┐ ┌──┐
│0   ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0 ││0   │ │0 │  15
└────┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└────┘ └──┘
┌─────┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌───────┐ ┌──┐
│1    ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1      │ │1 │  14
└─────┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└───────┘ └──┘
┌───────┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌──┐┌─────┐
│1      ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1 ││1    │ ┌──┐      13
└───────┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└──┘└─────┘ │1 │
┌───┐┌───┐┌───┐┌───────────────────────┐┌──┐┌──┐┌──┐     └──┘
│1  ││1  ││1  ││1                      ││1 ││1 ││1 │ ┌──┐┌──┐┌──┐  10
└───┘└───┘└───┘└───────────────────────┘└──┘└──┘└──┘ │1 ││1 ││1 │
                                                     └──┘└──┘└──┘

                                                            Total: 81

[!NOTE] The encoder doesn't have an LED, so there's no driver given.

The values inside the boxes say which driver each key is using. For example the escape key (top left) is using driver 0. The count on the right is the number of keys per row.

I got which driver goes where from here:

https://github.com/Keychron/qmk_firmware/blob/f9f4cf410fcc9c45810c2067ab6c8c052169a0a5/keyboards/keychron/v1_max/ansi_encoder/ansi_encoder.c#L22-L115

There are only 81 items listed, which seems to contradict what the config.h file says:

https://github.com/Keychron/qmk_firmware/blob/f9f4cf410fcc9c45810c2067ab6c8c052169a0a5/keyboards/keychron/v1_max/ansi_encoder/config.h#L21-L24

Math stuff ``` (14 + 15 + 15) + (14 + 13 + 10) = 44 + 37 = 81 ```

Are there some LEDs that I'm unaware of?

I compared this keyboard to others with the same 75% layout, and they're all different from the V1 Max:

Q1 Pro (ANSI) https://github.com/Keychron/qmk_firmware/blob/f9f4cf410fcc9c45810c2067ab6c8c052169a0a5/keyboards/keychron/q1_pro/ansi_knob/config.h#L25-L32

Q1 Max (ANSI) https://github.com/Keychron/qmk_firmware/blob/f9f4cf410fcc9c45810c2067ab6c8c052169a0a5/keyboards/keychron/q1_max/ansi_encoder/config.h#L21-L22

Q1 v2 (ANSI, master branch) https://github.com/Keychron/qmk_firmware/blob/b7468f47857ad20a031906ccbd654541222a0d26/keyboards/keychron/q1v2/ansi_encoder/config.h#L20-L22

lokher commented 9 months ago

This is indeed a bug, thanks for pointing it out, I will fix it later

mklein994 commented 9 months ago

Sweet, thanks!