[Feature Request] Support NO_PIN in ROW2COL and COL2ROW matrix scans

[infinityis]

Feature Request Type

• Core functionality
• Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
• Alteration (enhancement/optimization) of existing feature(s)
• New behavior

Description

Some keyboard creators have started creating virtual entries in their switch matrix to support dynamic keycaps for encoder rotations. That is, they will define a row or column in config.h that doesn't actually exist in hardware, then use encoder rotation events to "press" these virtual keys in the switch matrix. This provides a straightforward way to reconfigure encoder rotation effects using software like VIA by enabling visual representing of virtual keys that get pressed upon encoder cc/ccw rotation.

Examples of this implementation technique can be found here:
https://github.com/qmk/qmk_firmware/blob/master/keyboards/nightly_boards/octopad/config.h
https://github.com/qmk/qmk_firmware/blob/master/keyboards/ramonimbao/herringbone/pro/config.h

Note that the MATRIX_ROW_PINS definitions include a NO_PIN value.

The problem is that NO_PIN is only checked in direct matrix scans, not r2c and c2r matrix sans. The way NO_PIN is defined sets it equal to 0xFF. Thus, when a matrix scan occurs, instead of just skipping the NO_PIN, a memory location at the base I/O memory address + 255 is affected, which is at best unintended, and at worst, a hard-to-catch bug with undefined side-effects.

Some possible solutions:
-Throw a compile-time error if NO_PIN is used in the rows or column definitions for ROW2COL or COL2ROW
-Add it to a checklist for pull requests ("Verify NO_PIN is not used in a ROW2COL or COL2ROW switch matrix")
-Mimic the direct matrix scan approach and check to see if the pin assignment is NO_PIN even for ROW2COL and COL2ROW scans, and then take no action if the value is NO_PIN.

I believe the last option would be ideal as it allows NO_PIN to be safely be used in consistent manner for all matrix definitions. However, I'm interested in QMK maintainer's thoughts on the matter (and also to confirm whether or not my analysis is accurate), especially since I may be biased: I only looked into this because I was hoping to implement the same functionality for one of my encoders, but wanted to verify whether NO_PIN could really be used in that manner...and it doesn't appear to be the case currently.

[drashna]

Honestly, I don't feel that this is a good idea. Yeah, there are a few boards that do this, but ... it's a hack. A better solution is needed, IMO. Like an encoder map, instead.

I have a mockup/hack for this, but I'm not happy with it at all.

[infinityis]

Completely understandable; checking for NO_PIN in the matrix scan also adds conditionals in one of the tightest loops, so it's not great performance-wise either.

One option that works today is if there is a spare, unconnected pin, it can be can be assigned to the virtual row/col, but ultimately it is still a hack...albeit one with less potential to cause problems. And it only works if you have an I/O pin to spare.

I'd be happy to see/review your mockup/hack as a starting point, but I fear that may also be the wrong starting point.

Based on the existence of alternatives to VIA (like Vial), as well as the staunchly closed-source status of VIA (https://github.com/the-via/keyboards/issues/491#issuecomment-757603213), it feels like the correct approach here is to define an QMK API for dynamic key mapping software independent of any particular GUI software like VIA or Vial (which seems to be proposed here #11567). The current implementation designed around VIA certainly works, but Vial's method of handling encoders seems much cleaner and intentional, doesn't feel like a hack:
vial-kb/vial-qmk@10e15c0
https://get.vial.today/gettingStarted/encoders.html

I assume the decision to fork QMK for Vial was based on some fundamental differences compared to how VIA operates, like how Vial optionally requires an "unlock" to modify the dynamic keymaps for security purposes.

Also, incidentally, having a defined interface for dynamic keymap software should help avoid shifting key code issues like this in the future: #11157. The "it's a VIA problem" and "VIA is closed source" created a bit of confusion, consternation, and helplessness: QMK is well within their right to shift key codes, but then since VIA is closed source, users' hands are tied as far as being able to fix it outside of QMK. And actually...it looks like tzarc commented basically the same thing two days ago, I'm now thinking this issue should be closed to instead focus on ensuring encoders are better supported in the plans for the QMK API (and just use hacks like a spare pin in the interim).

[drashna]

Those are all very good points. And something that has ben on our minds. The biggest issue is how to implement them, and doing.

hooking the encoder into the actual matrix isn't a great solution, and may/will create problems down the line.

That said, in my personal repo, I do have a hacky solution to this:
drashna@48837dd

Line 39 of encoder_stuff.c could be changed so that it calls another function, one that could be replaced, much how via does so with the keymap, actually. And it supports all of the quantum codes, too. (such as RGB_HUI, and the like).

But my code is pretty hacky, but is better than adding spots into the matrix for this stuff.

[infinityis]

Thank you! It does indeed look better. Closing this issue to keep digging through it and learning. Will also start tracking the API discussion on that other issue thread and Discord.

Thanks for everything you do!