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

Support Irregular Matrices #17

Open
3 tasks
Shredder opened this issue Jan 31, 2021 · 10 comments
Open
3 tasks

Support Irregular Matrices #17

Shredder opened this issue Jan 31, 2021 · 10 comments
Labels
Bilateral Combinations bug Something isn't working question Further information is requested

Comments

@Shredder
Copy link

Shredder commented Jan 31, 2021

Describe the Bug

I have merged your branches manna-harbour/miryoku and manna-harbour/bilateral-combinations in my copy of qmk_firmware/master and enabled the BILATERAL_COMBINATIONS feature in config.h. I have noticed that keyboard combinations Ctrl-Backspace and Ctrl-Delete (e.g., delete previous and next work in Notepad) do not work anymore. Without the merge of manna-harbour/bilateral-combinations (but with manna-harbour/miryoku), both work as expected.

System Information

  • Keyboard: Planck
    • Revision (if applicable): 6
  • Operating system: Windows 10
  • AVR GCC version: 5.4.0
  • ARM GCC version: 9.2.1
  • QMK Firmware version: 0.11.56 (with manna-harbour/miryoku and manna-harbour/bilateral-combinations merged in)
  • Any keyboard related software installed?
    • AutoHotKey
    • Karabiner
    • Other:

Additional Context

You can find the branch here: https://github.com/Shredder/qmk_firmware/tree/dev_jz_bilateral_combinations.

@Shredder Shredder added bug Something isn't working help wanted Extra attention is needed labels Jan 31, 2021
@manna-harbour
Copy link
Owner

Thanks for your report. I'll try building from your fork when I have a chance. It's possible something has changed upstream. If you'd like to do more testing, please try with the miryoku branch plus bilateral combinations only (i.e. without merging into qmk master) to see if that's the issue.

@manna-harbour
Copy link
Owner

Can you confirm that this is with left hand mod-tap control? Also, can you specify you build command line?

@manna-harbour manna-harbour added question Further information is requested and removed help wanted Extra attention is needed labels May 28, 2021
@Shredder
Copy link
Author

Shredder commented May 31, 2021

Yes, I do have Ctrl mod-taps (in fact, also the other modifiers like in default Miryoku) on both sides (i.e., my left and right hands). Let me get back to you on the build command line.

@urob
Copy link

urob commented Jun 15, 2021

I have the same problem, and in my case I could trace it down to the right half of the bottom row being treated as if it were on the left half. To give a bit more details, consider the following layout: image

Here, HOME_S is an alias to LCTL_T(KC_S). I have permissive_hold enabled, so nested taps of the opposite hand should immediately trigger the mod. For the KC_BSPC in the middle column, this works fine. However, for the one in the bottom row, the mod does not activate (when held less than the bilateral_combinations timeout.) If on the other hand, I nest the same bottom row KC_BSPC with the right hand control (HOME_E), everything works fine.

So (for now this just being a guess) I suspect that this means that the problem is caused by how the key matrix for the Planck is defined. Maybe it's more of a problem with the Planck than with bilateral combinations?

Shredder, can you confirm this behavior? That is, does Ctrl-backspace and Ctrl-delete work in your case with the right hand mod as well?

@manna-harbour
Copy link
Owner

Planck has an irregular matrix on the bottom row, see https://github.com/qmk/qmk_firmware/blob/2538d341d828d04392898a9d3ce691bcd4a79e1f/keyboards/planck/rev6/rev6.h#L99

You would have to use #28 and supply your own custom bilateral_combinations_left() function.

For the future implementation (see #29) it would be nice to either use the keymap's rows / cols rather than those from the matrix (then something like BILATERAL_COMBINATIONS_COLS from #28 would be sufficient), or to specify the hand for each key with an array using the keymap layout.

urob added a commit to urob/qmk_firmware that referenced this issue Jun 17, 2021
disables bl for bottom row to work around manna-harbour#17
@urob
Copy link

urob commented Jun 17, 2021

Here's a quick workaround, which disables BL for Planck's bottom row using the custom bilateral combinations commit by theol0403 (605ecc6)

urob@3ef7605#diff-6b5eecce5cfac61a489d60ddc296d779ee957cb48b677973f0e5ce069fab49fc

obviously this is just a quick fix for fellow planck users and no replacement for the more general solutions outlined by manna-harbour

edit: I got the exception for the bottom row wrong on my first commit. This is how it should look like: 2bc16f8

@manna-harbour
Copy link
Owner

The general solution for an irregular matrix is probably to add a new layer to your keymap and populate with KC_L and KC_R as appropriate for each key position (and something else for keys that can be used with either hand, if adding that feature too), then use keymap_key_to_keycode() on that layer to lookup handedness for the key in question. That way the keyboard's own LAYOUT macro can be used to handle the matrix irregularities.

@ZenghaoWang
Copy link

ZenghaoWang commented Jul 24, 2021

The general solution for an irregular matrix is probably to add a new layer to your keymap and populate with KC_L and KC_R as appropriate for each key position (and something else for keys that can be used with either hand, if adding that feature too), then use keymap_key_to_keycode() on that layer to lookup handedness for the key in question. That way the keyboard's own LAYOUT macro can be used to handle the matrix irregularities.

For anyone else using a planck, this is how I implemented the fix:

  1. Merge Add customisable hand determination #28
  2. Add KC_LH and KC_RH to my keycodes enum
  3. Add another layer to my keymap:
    [LAYER_HANDS] = LAYOUT_ortho_4x12(
        KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH,
        KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH,
        KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH,
        KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_LH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH, KC_RH
    )};
  1. override bilateral_combinations_left in my keymap.c
bool bilateral_combinations_left(keypos_t key) {
    uint16_t keycode = keymap_key_to_keycode(LAYER_HANDS, key);
    return keycode == KC_LH;
}

manna-harbour added a commit that referenced this issue Jul 26, 2021
- supports irregular matrix e.g. Planck
- resolves #17

Co-authored-by: zenghao.wang@mail.utoronto.ca
@manna-harbour
Copy link
Owner

@ZenghaoWang Thanks for trying it! This function might as well be built-in, so here it is in a new branch:

Please try this branch. If it's ok I'll merge it into the main BC branch (well, force push, as this branch is rebased on current master).

@ZenghaoWang
Copy link

Works fine on my end! I merged your branch, deleted my bilateral_combinations_left function, and defined BILATERAL_COMBINATIONS_HANDS in my config.h.

@manna-harbour manna-harbour changed the title [Bug] BILATERAL_COMBINATIONS break Ctrl-Backspace and Ctrl-Del Support Irregular Matrices Jun 9, 2022
manna-harbour pushed a commit that referenced this issue Jul 13, 2022
* [keyboard] Initial support for Anne Pro 2

* [keyboard][AnnePro2] Keymap:update to a reasonable keymap with caps+hjkl => arrow

* :(

* changed to use HSI

* support for annepro2 c18

* keyboard/annepro2: Very stupid matrix scan bug fix.

* typo

* swap COL14/13

* keyboard/annepro2: startup secondary LED MCU

* keyboard/annepro2: typo fix

* Add IO Values

* Disable Combo feature

* Update default keymap to Anne Pro 2 Official Keymap

* keyboard/annepro2: keymap layer name changes

* keyboard/annepro2 BLE Support

* Fix keymap comment

FN1 ESC was listed as ~ instead of `

* keyboard/annepro2: Bluetooth path

* Keyboard annepro2 bidir led comms (#5)

* Added bidirectional shine comms and moved led functionality to new file

* Added bidirectional shine comms and moved led functionality to new file

* Restore original functionality to existing keymaps using new shine commands

* Fix dangling bracketless if statements

* PR cleanup

* add custom keycodes to switch led profiles

* Optimize code

* switch to prev profile before turning leds off

* Add persistent led support with eeprom (qmk#9)

* adding HT32 support to chibios SPI master driver

* add support for W25X20CL SPI eeprom

* add makefile flag for eeprom feature

* add spi support to keyboard startup and config

* example keymap using eeprom profile loading

* Cleanup to fix C15 eeprom/spi build errors (qmk#11)

* Cleanup to fix C15 eeprom/spi build errors

* add newline at eof

* LED Masking support for Shine

Introduce companion update to ledSetMask and ledClearMask.
In keymap `codetector` there is example of how to map caps_lock
to the caps_lock key light on the keyboard.

* [AnnePro2]: update bluetooth connection

* Merge the custom keys enums on annepro2.h (qmk#13)

* Keyboard annepro2 ble caps lock (#12)

* Move matrix_scan_kb out of board.c to annepro2.c

* add buffer clear after init and caplock polling

* Add support for LED intensity (qmk#15)

* Improve logic for switching off and on of LEDs (#16)

* Implement animation speed (#17)

* Include logic to send solid colors as foreground to shine and add sample profiles (qmk#14)

Include the logic to send a solid color from qmk to shine. That solid color will act as a foreground (will override the current profile) until reset (witch will reactivate the current profile).
This functionality depends on changes made for shine as well.

Include 3 new profiles:

    default-full-caps -> same as default, but with the logic of using the red foreground color on caps lock.
    default-layer-indicators -> same as default, but with the logic of red foreground on caps lock, green foreground on FN1 and blue foreground on FN2.
    thomazmoura -> my own profile as a sample of an over-engineered advanced case scenario.

* Implement reactive lighting effects (qmk#18)

* Added multiarrow keymap (#19)

* Add LED documentation (#26)

* add LED documentation

* add LED documentation to other default profiles

* Implement QMK's IAP default keybind (#29)

* Add keymap for going into IAP

* switch to default QMK keybind for IAP mode

* implement bluetooth IAP mode

* Make default config more like Obins stock default (qmk#30)

* Add new message type for resetting foreground color (qmk#31)

* annepro2(bluetooth): add media keys support (#41)

* Asynchronous, robust serial protocol. (qmk#39)

* bla personal ap2-c18 keymap.

* Bidirectional, asynchronous message-based communication with Shine.

- Requires a matching Shine version.
- Protocol is resiliant to loosing bytes during communication, chips won't lock
  waiting for bytes that aren't coming.
- Chips resynchronize in event of loosing a byte using a AA0D header.

Regressions:
- Key masking/locking doesn't work right now. (did it work before?)
- Not all user keymaps build against it.

* Clang-format + code to ease reducing speed of LED UART.

- Did clang-format --style=file -i on multiple files according to
  coding_conventions_c.md

- Added separate serial speed for IAP boot and Led communication, it's possible
  that reducing this to 9600 helped someone with faulty HW. With this code they
  can do it with simple replacing of a value.

* Main chip can set/clear foreground using a mask mechanism.

- Some preparations for selective colouring.

* Selective mask works - tested on capslock.

- Migrated personal keymaps to new status API.

* Clear the foreground colors to show profile when it's modified.

- Show example of achieving selective caps-lock painting + foreground painting
  for layers.
- annepro2LedMaskSetRow is implemented, but still requires testing.

* Implement the QMK side of led blinking to indicate the command was received.

- This stupidly blinks the key when user presses one of the bluetooth commands
to let the user know that the command was received and forwarded to the BT chip.

- TODO: Row/col key positions are hardcoded and not taken from the keymap.

* Reduce memory footprint.

Applying code review suggestions. Moved msgId to globals - preparing for
transmission without copying payload when no retries are necessary.

Added empty readme.md files - required by QMK lint.

Co-authored-by: Tomasz bla Fortuna <bla@thera.be>

* Let the LED chip settle a bit before waking it from the bootloader. (#42)

At least for one person that helps to reliably get the LEDs working without
disconnecting/reconnecting the power to the board multiple times.

Co-authored-by: Tomasz bla Fortuna <bla@thera.be>

* annepro2: rename KEYMAP to LAYOUT, as required by new version of QMK

* annepro2: update ChibiOS configuration files

* annepro2: fix undefined reference to dprint and timer_read32

* annepro2: update ChibiOS MCU name

* update spi driver, fix bad merging with master

* annepro2: add readme and info.json

* annepro2: make code compatible with QMK coding conventions

* tmk_core: temporary fix to allow HT32 based keyboards to work without patched ChibiOS-contrib (AnnePro2)

* AnnePro2: removed core changes

* AnnePro2: Leave only default keymaps

Missing keymaps will be restored in another PR

* annepro2: add licence information

* annepro2: satisfy qmk lint

* annepro2: fix drashna's suggestions

* annepro2: fix matrix

* annepro2: apply code review suggestions

* annepro2: apply remaining code review suggestions

* annepro2: update info.json

* annepro2: remove include

* annepro2: rename keymap to layout

* annepro2: fix typing

* annepro2: apply suggestions from tzarc's code review

Co-authored-by: Nick Brassel <nick@tzarc.org>

* annepro2: more fixes

* annepro2: apply suggestions from code review

Co-authored-by: Joel Challis <git@zvecr.com>

* annepro2: rename file

* more fixes

* Apply suggestions from @tzarc code review

Co-authored-by: Nick Brassel <nick@tzarc.org>

* Update keyboards/annepro2/protocol.h

Co-authored-by: Nick Brassel <nick@tzarc.org>

* Update keyboards/annepro2/chconf.h

Co-authored-by: Nick Brassel <nick@tzarc.org>

* apply CR suggestions

* upgrade readme

* IAP

* update IAP comments, defines

* led fix

* init fix

* annepro2: GPIO cleanup

* annepro2: ioline

* change waiting time

* Start develop for 2022q2

* [Core] Squeeze AVR some more with `-mrelax` and `-mcall-prologues` (qmk#16269)

* Rework generate-api CLI command to use .build directory (qmk#16441)

* Remove `send_unicode_hex_string()` (qmk#16518)

* Change data driven "str" type to represent a quoted string literal (qmk#16516)

* Change data driven "str" type to represent a quoted string literal

* Update docs

* Map data driven `DESCRIPTION` as string literal (qmk#16523)

* update bootloader

* Revert "Merge pull request qmk#2 from qmk/develop"

This reverts commit 9c76065, reversing
changes made to 240745d.

* Revert "update bootloader"

This reverts commit 240745d.

* fix rules.mk

* change PROGRAM_CMD

Co-authored-by: codetector <codetector@codetector.cn>
Co-authored-by: Fagl4 <18francisco18@gmail.com>
Co-authored-by: Jakob Gillich <jakob@gillich.me>
Co-authored-by: tech2077 <tech2077@gmail.com>
Co-authored-by: jcdeA <31413538+JcdeA@users.noreply.github.com>
Co-authored-by: Thomaz Moura <5599621+thomazmoura@users.noreply.github.com>
Co-authored-by: Darkhan <darkhanu@gmail.com>
Co-authored-by: Paco <70448173+packorf@users.noreply.github.com>
Co-authored-by: jmarmstrong1207 <32995055+jmarmstrong1207@users.noreply.github.com>
Co-authored-by: 1Conan <7620342+1Conan@users.noreply.github.com>
Co-authored-by: Tomasz bla Fortuna <blagh@thera.be>
Co-authored-by: Tomasz bla Fortuna <bla@thera.be>
Co-authored-by: Nick Brassel <nick@tzarc.org>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: QMK Bot <hello@qmk.fm>
Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
Co-authored-by: Ryan <fauxpark@gmail.com>
manna-harbour pushed a commit that referenced this issue Nov 29, 2022
* bastardkb: cleanup blackpill configuration

Fixes #17

* bastardkb: fix splinky configuration

The pinout of the splinky changed between the last beta batch, and the
production one. This commit updates the keyboard definition to support
the new pinout by default, while offering backward compatibility.

Define `SPLINKY_BETA_PINOUT` to build the firmware with pre-production
pinout.

Fixes qmk#15

* bastardkb: add support for STeMCell

* Update scylla/tbkmini/skeletyl outdated readmes

* bastardkb/dilemma: enable circular scroll

* bastardkb/dilemma: add initial version of the `via` keymap

* bastardkb/dilemma/assembled: add new keyboard

Fixes qmk#20

* bastardkb/dilemma: remove elite-c

* Initial support for the Dilemma 3x5+3 Assembled RGB

* Address code review comments

* Address more comments

* Address review comments

* Address more nits

* bastardkb: split splinky-based keyboards to distinguish between Splinky v2 and v3 pinout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bilateral Combinations bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants