Split transport mirror

[XScorpion2]

Description

Added a define option for split keyboards to transport the master matrix to the slave side. This allows systems (such as rgb matrix's key reactive animations) to be processed on the slave side instead of the master side, or mirrored in the case of the up coming split rgb matrix split support.

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

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).

[zvecr]

Can we back out all the non SPLIT_TRANSPORT_MIRROR to decouple it from the sync timer PR. That way this one can potentially land first, and allow some of the previous non split common behaviours on split keyboards with oled.

[XScorpion2]

Ya, let me do that right now @zvecr
Edit: done

[tzarc]

shakes fist in #10730

[XScorpion2]

Also, doesn't should_process_keypress() need to return true, if SPLIT_TRANSPORT_MIRROR is defined?

No, transporting the matrix data to the slave side (SPLIT_TRANSPORT_MIRROR) should be an independent choice from further processing that data and converting it into keycodes (should_process_keypress())

[drashna]

So, correct me if I'm wrong, then if you want to process stuff on the secondary side, you'd need hooks like the rgb matrix one to be able to handle keypress events, for stuff such as audio clicky, haptic feedback, etc? Or to manually enable the keypress processing?

[XScorpion2]

So, correct me if I'm wrong, then if you want to process stuff on the secondary side, you'd need hooks like the rgb matrix one to be able to handle keypress events, for stuff such as audio clicky, haptic feedback, etc? Or to manually enable the keypress processing?

Correct, in theory, depending on the system, many only care about electrical key presses, in which case they can do what split rgb matrix does and just hooks in at that level reducing the need to process the keycode record system. This approach also reduces the need to worry about triggering certain keycodes on the slave side if you do not intend to do so (Like reset)

[drashna]

So, in that case, anything that wants or needs to use the matrix info on the secondary side would then need to edit the keyboard.c file, so that it can get updates, or set the should_process_keycodes() to return true?

If that's the case, then we shouldn't we have a separate callback to handle that? This way, other features can hook into it (such as haptic feedback or audio clicky, oled) Or just ... revert the code changes to keyboard.c, and add should_process_keycodes to split_util, so that it returns true when mirroring is enabled.

As for RESET, there is already a check to only run on the master side. Could that be removed in this case? Yeah. But I think keeping that would be the best option, regardless (if only as a "just in case somebody does something wrong")n

[XScorpion2]

So, in that case, anything that wants or needs to use the matrix info on the secondary side would then need to edit the keyboard.c file, so that it can get updates, or set the should_process_keycodes() to return true?

Correct

If that's the case, then we shouldn't we have a separate callback to handle that? This way, other features can hook into it (such as haptic feedback or audio clicky, oled)

Right, that is what I was thinking, but didn't want to go down that path just yet until more systems want to use it. Plus I hate naming new apis. Ideally, we would have a call registration system where new systems could register for certain types of events instead of having to hard code the call sites like this in the first place. But that is more of a ground up rewrite of a lot of the core tmk / qmk code base.

As for RESET, there is already a check to only run on the master side. Could that be removed in this case? Yeah. But I think keeping that would be the best option, regardless (if only as a "just in case somebody does something wrong")n

Reset today does not have that check you are talking about. That check only existed on the older split rgb matrix pr and never landed in QMK. (Just looked to verify)

[drashna]

Plus I hate naming new apis

HAHAHAHAHA. I'm 1000% right there with you.

But I think that having the API there first is very important, as it heads off weird/bad issues.

As for the reset, I swear it had it. :(

[XScorpion2]

But I think that having the API there first is very important, as it heads off weird/bad issues.

Alright, I'll add an API to start this new logic branch handling and use that in rgb matrix.

[stale]

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 awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[XScorpion2]

@drashna Updated the PR to include the new entry point we talked about.

[tzarc]

Just a heads-up, something with this commit has broken the massdrop boards -- probably something to do with RGB, considering they're not split. Will do some investigation.

[sigprof]

And this change apparently also broke 3 keyboards which used SPLIT_TRANSPORT = custom:

keyboards/handwired/dactyl_manuform/5x6_right_trackball/rules.mk:SPLIT_TRANSPORT = custom
keyboards/handwired/lagrange/rules.mk:SPLIT_TRANSPORT = custom
keyboards/whale/sk/v3/rules.mk:SPLIT_TRANSPORT = custom

handwired/lagrange is being fixed at the moment, not sure what to do about the others. The usage of what looks like an old API in ai03/orbit is probably safe, because that keyboard actually has its own matrix implementation instead of using split_common, therefore the transport_master() and transport_slave() calls from there match the old API too.

[tzarc]

Then I'd suggest we find out whether these boards can migrate to split_common, given that there's more changes in the pipeline.

[sigprof]

Then I'd suggest we find out whether these boards can migrate to split_common

But these boards actually use the matrix implementation from split_common, replacing just the transport layer with a custom implementation (this is even “documented” in the sense that the split keyboard doc mentions the SPLIT_TRANSPORT = custom option, but does not actually describe the custom transport API).

For handwired/dactyl_manuform/5x6_right_trackball the proper solution is probably #11930, because the reason why a custom split transport is used there is that some way to sync the pointing device state between halves is needed (pointer_transport.c in that keyboard is a copy of an old version of quantum/split_common/transport.c with some additions to handle the extra synced bits).

handwired/lagrange actually has a different physical layer implementation of split communications — it uses SPI between halves. whale/sk/v3 also has a different physical layer implementation — it uses duplex USART communications (even with real RS-422 converters). So these keyboards actually need a custom transport implementation in some form — except that it could be possible to add some alternative way to replace just the lower communication layer, while still keeping the rest of transport code shared?

Looks like the real bug here is that quantum/split_common/transport.c does not contain #include "transport.h", therefore a prototype mismatch between transport.h and an obsolete patched copy of transport.c is not detected automatically (and the other two custom transport implementations also missed that #include "transport.h").

(Ideally the code should compile with -Werror=missing-prototypes, but that would probably break the existing code all over the place, and it would still be possible to “fix” compile errors in a wrong way — by adding a prototype locally instead of including the proper header file.)

[tzarc]

Right... though I'm currently looking towards pulling out the specific functions that are actually transport-specific, so that we have a proper abstraction over the top. SPI-based comms can in theory implement a single function and pick up the rest for free.

That's the intention with the #11930 branch as it stands, at least.