Add multilayer VIA support to Hasu usb_usb

[jshuf]

Description

Adds VIA support to the Hasu usb_usb converter. 'multilayer' comes from some additional complexity above and beyond most VIA ports - in order to fit more than one dynamic keymap layer into the atmega32u4 EEPROM, the converter's default scheme of using USB HID codes to directly index a 16x16 scan matrix is replaced with an intermediate mapping. This mapping translates from the smaller (142-code) set of USB HID codes actually used by the existing usb_usb layout macros, to cells in a smaller 9x16 scan matrix. This reduced matrix size allows three dynamic keymap layers to be stored in the atmega32u4 EEPROM. A more detailed explanation can be found in the comments in keymaps/via/keymap.c.

All code changes to support this are ifdef'd under VIA_ENABLE, and hence should have no impact on existing usage of the QMK usb_usb converter code. Because the LAYOUT macro for this VIA implementation has scan matrix dimensions that differ from the others, it lives in keymaps/via/keymap.c to avoid confusion with existing LAYOUT macros and info.json.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

• None

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drashna]

why is there via specific implementation here?

[jshuf]

To avoid code repetition - the keyboard report parsing and matrix scan logic for the converter remains unchanged. Only the scan matrix access macros (CODE, ROW, COL) had to be altered to support multiple dynamic keymaps.

In theory it would be possible to change all of the existing layouts to use the smaller-sized scan matrix and its associated HID-to-row,col translation, but that seemed to be a larger change with no benefit to existing QMK usb_usb users.

I could easily be missing a preferable way of handling the code separation - suggestions welcomed.

[drashna]

That doesn't really answer the question, though. Why does there need to be via specific handling of this?

VIA should be unaware of the hardware, here. The dynamic keymap feature, which via leverages changes the keymap, and that reads from the matrix config.

Eg, there is not really a good reason to have via specific code if the matrix is properly implemented, as far as I can see.

[jshuf]

I'm still not 100% sure I understand the issue, but is it that you'd prefer a different ifdef to wrap that code?

There are two scan matrices (the original, which supports translation of all 256 USB keycodes but only one layer, and the new one I've added, which supports fewer keycodes in order to allow for three layers). Both matrices are implemented correctly, and the USB-to-USB converter can support both - but you have to choose one. Since only one of the two matrices supports multiple layers, it made sense to me to put it under an ifdef so that you can make that compile-time choice. If the issue you're getting at is that using VIA_ENABLE is too specific a toggle in this hardware-specific code to make that choice (since any dynamic keymap user might want to enable it) then I could put the smaller one under some other ifdef and then enable that new ifdef only in the via keymap, which would take the VIA ifdef out of custom_matrix.cpp.

[fauxpark]

I'd like to convert this board to Custom Matrix Lite first - this change should still be doable after that, but hopefully with a little less boilerplate.

[FREEWING-JP]

This PR is very helpful to me for Reduce program size .
http://www.neko.ne.jp/~freewing/hardware/qmk_usb_converter_max3421e_host_shield/

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[jshuf]

Still active (at least as far as I'm concerned).

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[jshuf]

Still active.

[jshuf]

Updated PR to reflect changes to master, and replaced VIA_ENABLE with COMPRESS_MATRIX, to better reflect that the changes to the keymap are not VIA-specific.

[jshuf]

The remaining CI failures are outside the scope of this PR - they're in userspace code, and are failing to compile in master.

[bbecker-inlogik]

@jshuf In an attempt to free up as much EEPROM as possible, In my local code base, I added a keymaps/via_ansi folder which removed 34 keys in total ( F13-F24, International keys, legacy key codes ). So that makes 108 HID keycodes for a US ANSI layout, which could fit into 16x7 with 4 spare.
How would I go about removing 2 rows from hidindex? I suppose I don't fully understand the meaning of val = ((row) << 4) + (col)) and how that fits in with the various codes ie. 0x5D. Thanks for your PR. I'd like to see this get included in the code base.

[bbecker-inlogik]

Merge conflicts fixed in attached custom_matrix.cpp file.
custom_matrix.zip

[jshuf]

@bbecker-inlogik thanks for the code changes - I'll try to integrate and resubmit when I have time. There have been other QMK changes since the last review that I need to factor back into this code. In the meantime...

If foo is the HID code emitted by the keyboard and received by the converter, then hidindex[matrixindex[foo]] is the HID code that the converter will send to the host. That extra level of indirection is what allows us to reduce the size of each VIA layer in EEPROM.
So, to make 16x7 work in your local code base, you'd want to do at least these things (there may be more, I haven't looked at this in a while, but this should get you started):

• in via_ansi/config.h, set MATRIX_ROWS to 7
• make hidindex an array of length 112, and make sure it includes all 108 of the HID codes you want, filling the remaining entries of the array with 0x00. The order in which you enumerate the 108 HID codes doesn't really matter here.
• matrixindex remains an array of length 256, representing all 256 HID codes that could theoretically be received by the converter. Each value of this matrix is an index into the hidindex array. So, for each of those 256 HID codes, matrixindex will contain either XBAD (meaning that the HID code won't be supported) or it will contain the index 0x{R}{C}, where (R<<4) + C is the index into hidindex where you've stored the HID code you want the converter to send to the host.
• in via_ansi/keymap.c, the LAYOUT_via macro and keymaps[][][] would need to be modified to correspond to your 16x7 array - this is what would initialize the layers in VIA.

[sigprof]

Suggested change

	uint8_t matrix_key_count(void) {
	uint8_t count = 0;
	count += bitpop(local_keyboard_report.mods);
	for (uint8_t i = 0; i < KEYBOARD_REPORT_KEYS; i++) {
	if (IS_ANY(local_keyboard_report.keys[i])) {
	count++;
	}
	}
	return count;
	}

matrix_key_count() was removed back in #16603; no need to implement it.

(The implementation is also not correct in case the same key is pressed on multiple keyboards, or if the pressed key is filtered out by the matrix compression.)

[sigprof]

This variable is modified in several places, but never read — that looks wrong.

[sigprof]

Using 0x00 for unused matrix locations would break the current matrix_is_on() implementation — it would report these locations as pressed, because the unused entries in local_keyboard_report.keys contain 0. (For the uncompressed matrix the same problem would happen for the [0,0] location.)

One way to fix this without changing matrix_is_on() is to use something like 0x01 here, which is not accepted by IS_ANY() (so can't get into local_keyboard_report.keys), but also is not 0; this won't fix the uncompressed matrix case though. Another way is to add the IS_ANY(code) check to matrix_is_on(); this is probably safer.

[sigprof]

The matrix compression feature could be useful even without VIA (the current implementation uses 400 bytes for the data tables, but reduces each layer by 224 bytes, so it may be a win even starting from 2 layers, depending on the code size increase). So the implementation probably should not be coupled with VIA_ENABLE.

One major issue is that adding an option which changes the matrix layout makes all layouts which are currently defined in info.json invalid — all matrix locations in there must be adjusted to match the new virtual matrix. The solutions that I could think of are:

1. Use the compressed matrix unconditionally; change all layout definitions in info.json accordingly. This makes the converter work mostly like “normal” keyboards (no weird extra options), but might break things for users of strange keyboards which could send keycodes outside of the normal set (however, I'm not sure whether such USB keyboards actually exist in the wild, so this might not be a problem in practice).
2. Split the board into multiple revisions — one with the existing uncompressed matrix and layout definitions (and no VIA support), another with the compressed matrix and updated layout definitions. Unfortunately, this board already has 3 subrevisions, which would then need to be duplicated for the uncompressed/compressed revisions.

[sigprof]

In any case, changes like this are rather major, and need to go to the develop branch, not directly to master.

[sigprof]

Also you should think about converting the matrix to the “lite” custom matrix — even though it would have some overhead due to the debounce logic (the debounce itself could be disabled, but the matrix and raw_matrix arrays would still remain separate), the compressed matrix would take just 18 bytes of RAM, which would probably be an acceptable price for using more generic code (and might actually end up being faster than the custom matrix_get_row() implementation). And changing the code to convert the HID reports into bit arrays all at once would allow dropping the hidindex table (only matrixindex would be needed to determine the bit position for each pressed key).