Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for latest QMK release #1

Closed
wants to merge 2 commits into from
Closed

Updates for latest QMK release #1

wants to merge 2 commits into from

Conversation

vinorodrigues
Copy link

@vinorodrigues vinorodrigues commented Dec 10, 2023

JonyLee,

If I may - this is with reference to your M2 PR on QMK (.qmk#19853 ), specifically the comment from zverc (.qmk#19853 (review) ) around not including "custom code at the keyboard level". This may need an explanation:

The default keymap is used by the QMK Configurator to allow users to create custom keymaps ... so if you hard code KB specific code ... for example in your process_record_kb function you specifically trap case DF(WIN_B), so if that user uses that specific key combo, but not for the default 3+3 layers, then they will get very unpredictable results. This is a no-no.

The best practice here is to stick to the default keymap to only support keys that match the electrical characteristics of the PCB. Same goes for the via keymap, as that keymap is generated by The-VIA project to populate their downloads site. So much so, that it's recommended that these keymaps be written in the keymap.json format.

So how does one create the "factory default" variant of the firmware. Not sure if it's best practice per-se, but it's common to create a keymap with the vendor name. In this case "monsgeek".

With that in mind - I offer you the code that brings the PR to that thinking... plus some other fixes.

  • Move the vendor specific code to the monsgeek keymap.
  • Convert the default keymap to json file style.
  • Convert the via keymap to the json file style.
  • Remove LOCKING_* as this is not needed in a default build.
  • Fixed duplicate "matrix": [0, 0] entry in the RGB matrix.
  • Fixed for the correct LED FLAGS in LED matrix, in case future keymap editors want to use the ALPHAS_MODS animation.
  • Added Scroll-lock as LED 3 to the default set, overriding as Win-Lock for the monsgeek keymap (as was in the original).
  • Removed the case RGB_TOG key handler as this is not needed here. There is no requirement to keep all the RGB's off as the caps-, num- & scroll-lock LED's are not operated from the under-key RGB's.
  • Moved the "wear_leveling" defines to the info.json file.
  • Fixed some keymap bugs (e.g. TG(WIN_WASD) in the Mac set) - but have left the monsgeek keymap functionally the same as the Monsgeek user manual.
  • I don't think DRIVER_x_LED_TOTAL defines are required - but left them in as I was unsure. [Moved RGB_MATRIX_LED_COUNT to info.json]

There is also some fixes to the VIA json file (here: https://github.com/vinorodrigues/the-via.keyboards/blob/vr-monsgeek-m2/v3/monsgeek/m2/m2.json).

[PS: On this branch you will need to bring QMK up to ver. 0.23.0, or as is current upstream master, as the whole CKLED rename thing is required.]

@senseivino senseivino closed this by deleting the head repository Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants