Add some entrypoints for DIODE_DIRECTION = CUSTOM_MATRIX

[zvecr]

Description

Before i take this any further, i wanted to put out the idea for feedback.

Currently if you set CUSTOM_MATRIX = yes you will have to implement the following:

• matrix_init
• matrix_scan
• matrix_get_row
• matrix_print

as well as calling the standard quantum and keyboard/user functions, implement debouncing (either through custom code, or calling the existing debounce.c functions)

There seems to be an existing comment, both within the docs, templates and code base, ```

/* COL2ROW, ROW2COL, or CUSTOM_MATRIX */
#define DIODE_DIRECTION CUSTOM_MATRIX

However digging into the implementation, it currently does not do much.

By reusing the existing matrix.c code and adding some functionality to DIODE_DIRECTION = CUSTOM_MATRIX we can eliminate quite a lot of the duplication
This has the following advantages

• Less code to duplicate
  • only an init and scan function
• Uses existing debouncing logic
• Implemented as week functions so you get defaults if required.

I have provided a refactor on the 40percent/nano to show the refactor potential, when all that is required is a different way to read the switch state (in this case, each switch is tied to ground).

If this is wanted, i will be happy to clean up any code, and split out the core changes.

Types of changes

• Core
• Bugfix
• New Feature
• Enhancement/Optimization
• Keyboard (addition or update)
• Keymap/Layout/Userspace (addition or update)
• Documentation

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
• 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]

To understand this, basically this is a "lite" custom matrix, correct?

[zvecr]

@drashna custom matrix "lite" is exactly the best way to describe it.

[drashna]

Okay. I'm%lad that I understood it properly!

Though, could you add documentation for this (in the config page) https://github.com/qmk/qmk_firmware/blob/master/docs/config_options.md

[zvecr]

@drashna happy to add docs, wanted to validate it was a worthy feature, and the direction i went with it.

As an alternative it could be implemented as CUSTOM_MATRIX = lite and some code could be shuffled around. However i felt this would be a more involved change, and i reused an option that already existed but was not fully fleshed out at the moment.

[pelrun]

I'm not sure about this. I'd like to see an example that actually needs more than the standard matrix code but doesn't need the "whole hog" (which isn't actually that complicated either.)

As it is I've been simplifying the custom matrix code to reduce the burden of implementing it, and this would complicate that.

[zvecr]

@pelrun this PR contains exactly that example, where the nano needs to read its state slightly differently. Its not like the code is complicated, its all the duplication it causes.

An example is amount of "custom" debounce routines in CUSTOM_MATRIX = yes that are all the same, and while they could call the standard debouce.c code, that leads to more call sites. Then when the debounce code gets refactored, say to add another parameter, its a unnecessary massive refactor task.

This PR really adds functionality to something that is inherently broken at the moment. Currently if you set the documented option (both in the existing template and online docs) DIODE_DIRECTION = CUSTOM_MATRIX you end up with a matrix.c being included that will not function at all, and no clean way to implement anything that fixes that.

As i hinted to above, there are other ways to implement the same overall objective. Adding a lot of default implementations with the weak attribute would also work fine.

[pelrun]

I see it now, but it doesn't need this. There's already a standard mechanism for direct pin mapping to keys, where scanning isn't required (and I did it literally to replace DIODE_DIRECTION = CUSTOM_MATRIX, to boot.) It's only present in split_common right now, but I can easily bring it across. The only reason I didn't already is because I thought non-split keyboards would have too many keys!

Debounce isn't getting refactored, especially since I literally just did that. Even if it did, it's not a "massive" refactor task.

[pelrun]

Have a look at miniaxe: https://github.com/qmk/qmk_firmware/blob/master/keyboards/miniaxe/config.h

[zvecr]

My example refactor is only one of many use cases, i provided it to start this thread of conversation. I could propose a process of reading a matrix over a spi/uart rf module. I still wouldn't want to reproduce all the logic of debouncing and calling the quantum function in the correct location.

While it could be stated that debouncing is not getting refactored because its just been completed, we cannot predict the future so should adapt the software for long term maintainability. For instance, there is this PR still outstanding, #3720.

[pelrun]

#3720 is only outstanding because I pulled a swifty on alex-ong and pre-empted his changes, so he needed to port his debouncing algorithms over to the new API. It's not going to change the API again, and even if I do, then it's my job to fix all the broken references. I'm happy to do that.

You're advocating adding multiple new API hooks to avoid calling a couple of existing ones, which doesn't seem right, and is guaranteed to be a maintenance hassle in the future. If there's a use case we haven't considered yet, the appropriate approach is to do it (messily if need be) in a single kb, and then it can get migrated into the core appropriately at a later date if it's genuinely useful.

Maybe the problem is we don't have many examples of a nice clean custom matrix implementation to work from yet, so you're getting discouraged by the large amount of existing messy ones?

[zvecr]

The main emphasis should be on the broken state of DIODE_DIRECTION = CUSTOM_MATRIX

If going with another solution, I would suggest at least adding the following logic to matrix.c

#elif (DIODE_DIRECTION == ROW2COL)
   ...
#else
  #pragma message ("DIODE_DIRECTION not supported")
#endif

And remove all references to DIODE_DIRECTION = CUSTOM_MATRIX

[zvecr]

@pelrun i think we might have to agree to disagree on the state of code duplication for CUSTOM_MATRIX=yes

[pelrun]

I'm all for cleaning up DIODE_DIRECTION.

[zvecr]

Instead of a "lite" CUSTOM_MATRIX, it does sound like a better long term solution is to refactor more.

I think the following items should help, i will mock up some of the changes for potential PR/review.

• Port split_common/matrix.c DIRECT_PINS to matrix.c - here
• Remove DIODE_DIRECTION = CUSTOM_MATRIX - here
• Add warning for bad DIODE_DIRECTION or DIRECT_PINS - TODO
• Extract "common" parts out from split and regular matrix implementations - TODO
  • This could also provide defaults for CUSTOM_MATRIX = yes, e.g.
    • default weak user/keyboard functions
    • print functions

This should help unify the implementations without touching split_common too much (as i know a lot of work is ongoing for split arm).

[zvecr]

Closed in favour of #5091 and #5090, and a pending PR for any CUSTOM_MATRIX = yes optimisations