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

gBoards GergoPlex #8928

Closed
wants to merge 8 commits into from
Closed

gBoards GergoPlex #8928

wants to merge 8 commits into from

Conversation

germ
Copy link
Contributor

@germ germ commented Apr 26, 2020

Part two of the gboards-consolidation project. The Mega PR has been separated out into smaller per-keyboard PRs.

This PR contains a single keyboard as per @zvecr , in this case GergoPlex. This code was pulled from #8262

  • Enhancement/optimization
  • Keymap/layout/userspace (addition or update)

Issues Fixed or Closed by This PR

@zvecr zvecr added the awaiting_pr Relies on another PR to be merged first label Apr 26, 2020
keyboards/gboards/k/gergoplex/matrix.c Outdated Show resolved Hide resolved
@zvecr zvecr added keyboard and removed awaiting_pr Relies on another PR to be merged first labels May 4, 2020
@germ germ requested a review from drashna May 5, 2020 01:44
@drashna drashna requested a review from a team May 19, 2020 10:03
@germ
Copy link
Contributor Author

germ commented May 30, 2020

Single line change to GergoPlex dicts, forgot that the eng-words dict was left enabled.

@germ
Copy link
Contributor Author

germ commented Jun 7, 2020

@pirj Make sure your master branch is updated, the gboards folder was merged in a little while ago. This branch itself depends on those being avalible

@pirj
Copy link

pirj commented Jun 7, 2020

Good point, I was confused that I have to use this tree https://github.com/germ/qmk_firmware/tree/gboards-gergoplex, and was not aware of this pull request. Good to know gBoards are going mainstream in QMK.

So, to help you reproduce (using hub.github.com, macOS 10.14.6):

$ hub clone --recurse-submodules qmk/qmk_firmware
Cloning into 'qmk_firmware'...
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 210046 (delta 2), reused 2 (delta 2), pack-reused 210040
Receiving objects: 100% (210046/210046), 147.09 MiB | 2.16 MiB/s, done.
Resolving deltas: 100% (129217/129217), done.
Updating files: 100% (15888/15888), done.
Submodule 'lib/chibios' (https://github.com/qmk/ChibiOS) registered for path 'lib/chibios'
Submodule 'lib/chibios-contrib' (https://github.com/qmk/ChibiOS-Contrib) registered for path 'lib/chibios-contrib'
Submodule 'lib/googletest' (https://github.com/qmk/googletest) registered for path 'lib/googletest'
Submodule 'lib/lufa' (https://github.com/qmk/lufa) registered for path 'lib/lufa'
Submodule 'lib/printf' (https://github.com/qmk/printf) registered for path 'lib/printf'
Submodule 'lib/ugfx' (https://github.com/qmk/uGFX) registered for path 'lib/ugfx'
Submodule 'lib/vusb' (https://github.com/qmk/v-usb) registered for path 'lib/vusb'
Cloning into '/Users/pirj/source/qmk_firmware/lib/chibios'...
remote: Enumerating objects: 182932, done.
remote: Total 182932 (delta 0), reused 0 (delta 0), pack-reused 182932
Receiving objects: 100% (182932/182932), 137.17 MiB | 2.53 MiB/s, done.
Resolving deltas: 100% (138803/138803), done.
Cloning into '/Users/pirj/source/qmk_firmware/lib/chibios-contrib'...
remote: Enumerating objects: 13695, done.
remote: Total 13695 (delta 0), reused 0 (delta 0), pack-reused 13695
Receiving objects: 100% (13695/13695), 5.52 MiB | 1.34 MiB/s, done.
Resolving deltas: 100% (8755/8755), done.
Cloning into '/Users/pirj/source/qmk_firmware/lib/googletest'...
remote: Enumerating objects: 20198, done.
remote: Total 20198 (delta 0), reused 0 (delta 0), pack-reused 20198
Receiving objects: 100% (20198/20198), 7.43 MiB | 1.33 MiB/s, done.
Resolving deltas: 100% (14937/14937), done.
Cloning into '/Users/pirj/source/qmk_firmware/lib/printf'...
remote: Enumerating objects: 3, done.
remote: Counting objects: 100% (3/3), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 526 (delta 0), reused 3 (delta 0), pack-reused 523
Receiving objects: 100% (526/526), 392.80 KiB | 676.00 KiB/s, done.
Resolving deltas: 100% (312/312), done.
Cloning into '/Users/pirj/source/qmk_firmware/lib/vusb'...
remote: Enumerating objects: 1296, done.
remote: Total 1296 (delta 0), reused 0 (delta 0), pack-reused 1296
Receiving objects: 100% (1296/1296), 429.12 KiB | 1.04 MiB/s, done.
Resolving deltas: 100% (797/797), done.
Cloning into '/Users/pirj/source/qmk_firmware/lib/lufa'...
remote: Enumerating objects: 23, done.
remote: Counting objects: 100% (23/23), done.
remote: Compressing objects: 100% (22/22), done.
remote: Total 73792 (delta 2), reused 7 (delta 1), pack-reused 73769
Receiving objects: 100% (73792/73792), 18.67 MiB | 1.53 MiB/s, done.
Resolving deltas: 100% (59127/59127), done.
Cloning into '/Users/pirj/source/qmk_firmware/lib/ugfx'...
remote: Enumerating objects: 4686, done.
remote: Counting objects: 100% (4686/4686), done.
remote: Compressing objects: 100% (1719/1719), done.
remote: Total 29592 (delta 3346), reused 3729 (delta 2930), pack-reused 24906
Receiving objects: 100% (29592/29592), 20.10 MiB | 2.75 MiB/s, done.
Resolving deltas: 100% (19347/19347), done.
Submodule path 'lib/chibios': checked out '313416b8fda90d9973a749a0a35970956852c286'
Submodule path 'lib/chibios-contrib': checked out 'e3a3a24047717af33b098266e11c2e4e5102206a'
Submodule path 'lib/googletest': checked out 'ec44c6c1675c25b9827aacd08c02433cccde7780'
Submodule path 'lib/lufa': checked out 'ce10f7642b0459e409839b23cc91498945119b4d'
Submodule path 'lib/printf': checked out 'd3b984684bb8a8bdc48cc7a1abecb93ce59bbe3e'
Submodule path 'lib/ugfx': checked out '40b48f470addad6a4fb1177de1a69a181158739b'
Submodule path 'lib/vusb': checked out '9a42d205eb60faca494ff4eabce8d59f0ec0fc7f'

$ cd qmk_firmware

$ hub pr checkout 8928
remote: Enumerating objects: 21, done.
remote: Counting objects: 100% (21/21), done.
remote: Total 66 (delta 21), reused 21 (delta 21), pack-reused 45
Unpacking objects: 100% (66/66), 26.58 KiB | 283.00 KiB/s, done.
From git://github.com/qmk/qmk_firmware
 * [new ref]             refs/pull/8928/head -> gboards-gergoplex
warning: unable to rmdir 'lib/printf': Directory not empty
warning: unable to rmdir 'lib/vusb': Directory not empty
Switched to branch 'gboards-gergoplex'

$ util/macos_install.sh
Updating Homebrew...
==> Auto-updated Homebrew!
Updated 1 tap (homebrew/core).

Already up-to-date.
Warning: osx-cross/avr/avr-gcc@8 8.4.0 is already installed and up-to-date
To reinstall 8.4.0, run `brew reinstall avr-gcc@8`
Warning: osx-cross/arm/arm-gcc-bin@8 8-2019-q3-update is already installed and up-to-date
To reinstall 8-2019-q3-update, run `brew reinstall arm-gcc-bin@8`
Warning: dfu-programmer 0.7.2 is already installed and up-to-date
To reinstall 0.7.2, run `brew reinstall dfu-programmer`
Warning: avrdude 6.3_1 is already installed and up-to-date
To reinstall 6.3_1, run `brew reinstall avrdude`
Warning: clang-format 10.0.0 is already installed and up-to-date
To reinstall 10.0.0, run `brew reinstall clang-format`
Warning: dfu-util 0.9 is already installed and up-to-date
To reinstall 0.9, run `brew reinstall dfu-util`
Warning: python 3.7.7 is already installed and up-to-date
To reinstall 3.7.7, run `brew reinstall python`
Warning: Calling Non-checksummed download of bootloadhid formula file from an arbitrary URL is deprecated! Use 'brew extract' or 'brew create' and 'brew tap-new' to create a formula file in a tap on GitHub instead.
######################################################################## 100.0%
Warning: bootloadhid HEAD-e3585b2 is already installed and up-to-date
To reinstall HEAD, run `brew reinstall bootloadhid`
Warning: Already linked: /usr/local/Cellar/avr-gcc@8/8.4.0
To relink:
  brew unlink avr-gcc@8 && brew link --force avr-gcc@8
Warning: Already linked: /usr/local/Cellar/arm-gcc-bin@8/8-2019-q3-update
To relink:
  brew unlink arm-gcc-bin@8 && brew link --force arm-gcc-bin@8
Requirement already satisfied: appdirs in /usr/local/lib/python3.7/site-packages (from -r util/../requirements.txt (line 3)) (1.4.4)
Requirement already satisfied: argcomplete in /usr/local/lib/python3.7/site-packages (from -r util/../requirements.txt (line 4)) (1.11.1)
Requirement already satisfied: colorama in /usr/local/lib/python3.7/site-packages (from -r util/../requirements.txt (line 5)) (0.4.3)
Requirement already satisfied: hjson in /usr/local/lib/python3.7/site-packages (from -r util/../requirements.txt (line 6)) (3.0.1)
Requirement already satisfied: importlib-metadata<2,>=0.23; python_version == "3.7" in /usr/local/lib/python3.7/site-packages (from argcomplete->-r util/../requirements.txt (line 4)) (1.6.1)
Requirement already satisfied: zipp>=0.5 in /usr/local/lib/python3.7/site-packages (from importlib-metadata<2,>=0.23; python_version == "3.7"->argcomplete->-r util/../requirements.txt (line 4)) (3.1.0)

$ make gboards/gergoplex:default
QMK Firmware 0.8.133
Making gboards/gergoplex with keymap default

avr-gcc (Homebrew AVR GCC 8.4.0) 8.4.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiling: keyboards/gboards/gergoplex/keymaps/default/keymap.c                                    keyboards/gboards/gergoplex/keymaps/default/keymap.c:10:10: fatal error: g/keymap_combo.h: No such file or directory
 #include "g/keymap_combo.h"
          ^~~~~~~~~~~~~~~~~~
compilation terminated.
 [ERRORS]
 |
 |
 |
make[1]: *** [.build/obj_gboards_gergoplex_default/keyboards/gboards/gergoplex/keymaps/default/keymap.o] Error 1
make: *** [gboards/gergoplex:default] Error 1
Make finished with errors

I've edited keyboards/gboards/gergoplex/keymaps/default/rules.mk and added COMBO_ENABLE=yes like http://combos.gboards.ca/docs/install/ suggests. But the error remains.

What am I doing wrong?

@pirj
Copy link

pirj commented Jun 7, 2020

There's one thing that confused me, if you run make on master, it reports:

QMK Firmware 0.9.7

while on this branch:

QMK Firmware 0.8.133

make dz60:default passes for me on both this branch and master.

make gboards/gergoplex:via passes for me after commenting out

#define COMBO_TERM 40

in either keyboards/gboards/gergoplex/config.h or keyboards/gboards/gergoplex/keymaps/via/config.h.

@pirj
Copy link

pirj commented Jun 7, 2020

Ok, the culprit seems to be that this branch is not up to date with master.

$ git pull --rebase origin master
$ make gboards/gergoplex:default
QMK Firmware 0.9.7
...
Creating load file for flashing: .build/gboards_gergoplex_default.hex                               [OK]
Copying gboards_gergoplex_default.hex to qmk_firmware folder                                        [OK]
Checking file size of gboards_gergoplex_default.hex                                                 [OK]
 * The firmware size is fine - 22242/28672 (77%, 6430 bytes free)

Also notice the 0.9.7 vs 0.8.133 difference.
Rebase went fine with no conflicts.

make gboards/gergoplex:via error still remains, but it's a minor one, I guess this fixes it in keyboards/gboards/gergoplex/keymaps/via/config.h:

  #define COMBO_COUNT 16
  #define COMBO_ALLOW_ACTION_KEYS 
+ #ifdef COMBO_TERM
+   #undef COMBO_TERM
+ #endif COMBO_TERM
  #define COMBO_TERM 50

Copy link

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@germ
Copy link
Contributor Author

germ commented Jun 8, 2020

I'll get that updated tonight! Thanks for doing a dig on this, these PRs have been hanging around for a hot minute :(

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from a clang-format, looks good

@germ
Copy link
Contributor Author

germ commented Jun 18, 2020

Sorry @drashna didn't realize that was holding this up. Formatted lol

@keyboard-magpie
Copy link
Contributor

Dear QMK higher powers,
Very keen to try and get hacking on my gergoplex at home, poor germ has been moving house so not able to chase stuff up much. Is there any progress that might be made here?

@pirj
Copy link

pirj commented Jul 2, 2020

I can lend a hand as well with this.

@germ
Copy link
Contributor Author

germ commented Jul 2, 2020

Yeah, I'm backlogged and in the middle of a move right now. All the other gboards- PRs are held up as well :(

* |ESC/META|ENT/ALT|SPC(SYM)| |SPC(NUM)|BSPC|TAB|
* '-------------------------' '-----------------'
*/
[BASE] = LAYOUT_gergoplex(KC_Q, KC_W, KC_E, KC_R, KC_T, KC_Y, KC_U, KC_I, KC_O, KC_P, MT(MOD_LCTL, KC_A), KC_S, KC_D, KC_F, KC_G, KC_H, KC_J, KC_K, KC_L, MT(MOD_LCTL, KC_SCLN), MT(MOD_RSFT, KC_Z), KC_X, KC_C, KC_V, KC_B, KC_N, KC_M, KC_COMM, KC_DOT, MT(MOD_RSFT, KC_SLSH),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[BASE] = LAYOUT_gergoplex(KC_Q, KC_W, KC_E, KC_R, KC_T, KC_Y, KC_U, KC_I, KC_O, KC_P, MT(MOD_LCTL, KC_A), KC_S, KC_D, KC_F, KC_G, KC_H, KC_J, KC_K, KC_L, MT(MOD_LCTL, KC_SCLN), MT(MOD_RSFT, KC_Z), KC_X, KC_C, KC_V, KC_B, KC_N, KC_M, KC_COMM, KC_DOT, MT(MOD_RSFT, KC_SLSH),
[BASE] = LAYOUT_split_3x5_3(KC_Q, KC_W, KC_E, KC_R, KC_T, KC_Y, KC_U, KC_I, KC_O, KC_P, MT(MOD_LCTL, KC_A), KC_S, KC_D, KC_F, KC_G, KC_H, KC_J, KC_K, KC_L, MT(MOD_LCTL, KC_SCLN), MT(MOD_RSFT, KC_Z), KC_X, KC_C, KC_V, KC_B, KC_N, KC_M, KC_COMM, KC_DOT, MT(MOD_RSFT, KC_SLSH),

* |MMB | ; | = | | = | ; | DEL |
* '-----------------' '------------------'
*/
[SYMB] = LAYOUT_gergoplex(KC_EXLM, KC_AT, KC_LCBR, KC_RCBR, KC_PIPE, KC_GRV, KC_TILD, KC_TRNS, KC_TRNS, KC_BSLS, KC_HASH, KC_DLR, KC_LPRN, KC_RPRN, KC_BTN2, KC_PLUS, KC_MINS, KC_SLSH, KC_ASTR, KC_QUOT, KC_PERC, KC_CIRC, KC_LBRC, KC_RBRC, KC_BTN1, KC_AMPR, KC_EQL, KC_COMM, KC_DOT, KC_MINS, CMB_TOG, KC_SCLN, KC_EQL, KC_EQL, KC_SCLN, KC_DEL),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[SYMB] = LAYOUT_gergoplex(KC_EXLM, KC_AT, KC_LCBR, KC_RCBR, KC_PIPE, KC_GRV, KC_TILD, KC_TRNS, KC_TRNS, KC_BSLS, KC_HASH, KC_DLR, KC_LPRN, KC_RPRN, KC_BTN2, KC_PLUS, KC_MINS, KC_SLSH, KC_ASTR, KC_QUOT, KC_PERC, KC_CIRC, KC_LBRC, KC_RBRC, KC_BTN1, KC_AMPR, KC_EQL, KC_COMM, KC_DOT, KC_MINS, CMB_TOG, KC_SCLN, KC_EQL, KC_EQL, KC_SCLN, KC_DEL),
[SYMB] = LAYOUT_split_3x5_3(KC_EXLM, KC_AT, KC_LCBR, KC_RCBR, KC_PIPE, KC_GRV, KC_TILD, KC_TRNS, KC_TRNS, KC_BSLS, KC_HASH, KC_DLR, KC_LPRN, KC_RPRN, KC_BTN2, KC_PLUS, KC_MINS, KC_SLSH, KC_ASTR, KC_QUOT, KC_PERC, KC_CIRC, KC_LBRC, KC_RBRC, KC_BTN1, KC_AMPR, KC_EQL, KC_COMM, KC_DOT, KC_MINS, CMB_TOG, KC_SCLN, KC_EQL, KC_EQL, KC_SCLN, KC_DEL),

* | F11 | F12| | | | PLY | SKP |
* '-----------------' '-----------------'
*/
[NUMB] = LAYOUT_gergoplex(KC_1, KC_2, KC_3, KC_4, KC_5, KC_6, KC_7, KC_8, KC_9, KC_0, KC_F1, KC_F2, KC_F3, KC_F4, KC_F5, KC_LEFT, KC_DOWN, KC_UP, KC_RGHT, KC_VOLU, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_MS_L, KC_MS_D, KC_MS_U, KC_MS_R, KC_VOLD, KC_F11, KC_F12, KC_TRNS, KC_TRNS, KC_MPLY, KC_MNXT)};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[NUMB] = LAYOUT_gergoplex(KC_1, KC_2, KC_3, KC_4, KC_5, KC_6, KC_7, KC_8, KC_9, KC_0, KC_F1, KC_F2, KC_F3, KC_F4, KC_F5, KC_LEFT, KC_DOWN, KC_UP, KC_RGHT, KC_VOLU, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_MS_L, KC_MS_D, KC_MS_U, KC_MS_R, KC_VOLD, KC_F11, KC_F12, KC_TRNS, KC_TRNS, KC_MPLY, KC_MNXT)};
[NUMB] = LAYOUT_split_3x5_3(KC_1, KC_2, KC_3, KC_4, KC_5, KC_6, KC_7, KC_8, KC_9, KC_0, KC_F1, KC_F2, KC_F3, KC_F4, KC_F5, KC_LEFT, KC_DOWN, KC_UP, KC_RGHT, KC_VOLU, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_MS_L, KC_MS_D, KC_MS_U, KC_MS_R, KC_VOLD, KC_F11, KC_F12, KC_TRNS, KC_TRNS, KC_MPLY, KC_MNXT)};

uint8_t init_mcp23018(void);

/* -------- LEFT HAND------RIGHT HAND ---------- */
#define LAYOUT_gergoplex(L00, L01, L02, L03, L04, R00, R01, R02, R03, R04, L10, L11, L12, L13, L14, R10, R11, R12, R13, R14, L20, L21, L22, L23, L24, R20, R21, R22, R23, R24, L30, L31, L32, R30, R31, R32) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define LAYOUT_gergoplex(L00, L01, L02, L03, L04, R00, R01, R02, R03, R04, L10, L11, L12, L13, L14, R10, R11, R12, R13, R14, L20, L21, L22, L23, L24, R20, R21, R22, R23, R24, L30, L31, L32, R30, R31, R32) \
#define LAYOUT_split_3x5_3(L00, L01, L02, L03, L04, R00, R01, R02, R03, R04, L10, L11, L12, L13, L14, R10, R11, R12, R13, R14, L20, L21, L22, L23, L24, R20, R21, R22, R23, R24, L30, L31, L32, R30, R31, R32) \

This is a new "standardized" macro from #9562.
The purpose of this is to make this macro generic and allow users to share keymaps between keyboards with the same layout.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Erovia I really love the cleaniness of defining layouts with LAYOUT_kc, i.e:

    [_ALPHA] = LAYOUT_kc(
    // ,-------------------------------.      ,-------------------------------.
           Q   ,  W  ,  E  ,  R  ,  T  ,         Y  ,  U  ,  I  ,  O  ,   P   ,
    // |-------+-----+-----+-----+-----|      |-----+-----+-----+-----+-------|
         CTL_A ,  S  ,  D  ,  F  ,  G  ,         H  ,  J  ,  K  ,  L  , CTL_CL,
    // |-------+-----+-----+-----+-----|      |-----+-----+-----+-----+-------|
         SFT_Z ,  X  ,  C  ,  V  ,  B  ,         N  ,  M  ,COMMA, DOT , SFT_SL,
    // '-------------------------------'      '-------------------------------'

There's a comment I've found:

/*This special definition is used for S60-X keymaps that were ported from TMK
 * QMK has a lot of keycodes that don't start with KC_, so using the regular KEYMAP macro is recommended
 */
#define LAYOUT_kc( \

There's actually just one non-KC keycode used in the keymap, CMB_TOG.
Also, there's a number of gergoplex-specific definitions:

#define KC_CMB_TOG CMB_TOG  // A hack to allow KC_-less keycode along with KC_-ful ones

#define KC_CTL_A  MT(MOD_LCTL, KC_A)     // Tap for A, hold for Control
#define KC_CTL_CL MT(MOD_LCTL, KC_SCLN)  // Tap for colon, hold for Control

What prevents users of a different keyboard from picking the layout above and using it? How is it supposed to be done?

Is it sufficient to add LAYOUTS = split_3x5_3 to rules.mk?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +14 to +19
CUSTOM_MATRIX = yes
COMBO_ENABLE = yes
EXTRAKEY_ENABLE = yes
CONSOLE_ENABLE = yes
COMMAND_ENABLE = yes
BOOTMAGIC_ENABLE = lite
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CUSTOM_MATRIX = yes
COMBO_ENABLE = yes
EXTRAKEY_ENABLE = yes
CONSOLE_ENABLE = yes
COMMAND_ENABLE = yes
BOOTMAGIC_ENABLE = lite
CUSTOM_MATRIX = yes
COMBO_ENABLE = yes
EXTRAKEY_ENABLE = yes
CONSOLE_ENABLE = yes
COMMAND_ENABLE = yes
BOOTMAGIC_ENABLE = lite

# know what you're doing.
#
# No touchy, capiche?
SRC += matrix.c i2c_master.c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SRC += matrix.c i2c_master.c

Comment on lines +13 to +26
DDRB &= ~(1 << 0 | 1 << 4 | 1 << 5 | 1 << 6 | 1 << 7);
PORTB |= (1 << 0 | 1 << 4 | 1 << 5 | 1 << 6 | 1 << 7);

DDRC &= ~(1 << 6);
PORTC |= (1 << 6);

PORTD |= (1 << 5 | 1 << 4 | 1 << 6 | 1 << 7);
DDRD &= ~(1 << 5 | 1 << 4 | 1 << 6 | 1 << 7);

DDRE &= ~(1 << 6);
PORTE |= (1 << 6);

DDRF &= ~(1 << 0 | 1 << 7);
PORTF |= (1 << 0 | 1 << 7);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We rather use the GPIO control functions: https://docs.qmk.fm/#/internals_gpio_control?id=functions

Also, if this is not needed for the matrix initialization, they should be moved to keyboard_post_init_kb.

@@ -0,0 +1,80 @@
#include QMK_KEYBOARD_H
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include QMK_KEYBOARD_H
#include "gergoplex.h"

Comment on lines +4 to +5
#include <stdint.h>
#include <stdbool.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <stdint.h>
#include <stdbool.h>

I believe these are implicitly included. Maybe the others, too.
Test it though. :D

@@ -0,0 +1,288 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a custom matrix really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to get these suggested changes into their own branch so I can test them without trashing the PR? Need to verify these all work and compile

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if Github has a feature like this, but you could commit all the suggestions as one and pull in the PR into new branch on your computer.
You could check it out, fine-tune it and drop it if it's a disaster.

Feel free to ping me on QMK Discord if the above doesn't make any sense. Words are hard... :D

This was referenced Jul 9, 2020
Copy link
Contributor

@manna-harbour manna-harbour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to @Erovia's change request for split_3x5_3 layout support, I believe this is also needed.


DEBOUNCE_TYPE = eager_pr
SRC += matrix.c
QUANTUM_LIB_SRC += i2c_master.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QUANTUM_LIB_SRC += i2c_master.c
QUANTUM_LIB_SRC += i2c_master.c
LAYOUTS = split_3x5_3

@pirj
Copy link

pirj commented Sep 17, 2020

@Erovia I'm currently bringing this PR to shape. Is it correct that it's made against develop, not master? Is this because it's marked as breaking changes? May I kindly ask you to change the target branch?

Speaking of the breaking changes, this PR (along with the others gBoards-related https://github.com/qmk/qmk_firmware/pull/germ) don't seem to fall under "modifies how QMK behaves in a way that in incompatible or potential..ly dangerous" clause. Do you think it's possible to remove it and merge normally (when it's in a good condition)?

@pirj
Copy link

pirj commented Sep 18, 2020

@drashna Do you think it's makes sense to remove the breaking_change status from this and other related pulls, and change their target to master?

@zvecr
Copy link
Member

zvecr commented Sep 18, 2020

@pirj not all those PRs can have their label removed as they relocate a existing keyboard, which falls under our current rules for 'breaking changes'. This one, and a few of the others are newly added keyboards so might not have that restriction (dependent on changed files).

@pirj
Copy link

pirj commented Nov 28, 2020

It's ok to close this PR, I'm handling it in #10406. Sorry, I was busy with other things, so it's taking me longer, but I plan to wrap it up this year.

@tzarc tzarc closed this Nov 28, 2020
@tzarc
Copy link
Member

tzarc commented Nov 28, 2020

Cheers.

@pirj pirj mentioned this pull request May 28, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change Changes that need to wait for a version increment keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants