kmk_firmware
kmk_firmware copied to clipboard
KMKfw
Refactor split module
In relation to #323, I tried starting such abstraction of Split module. The structure is following:
AbstractSplit
└─ UartSplit
│ └─ PioSplit
└─ BleSplit
I tried to extract common code to AbstractSplit, but also not to change any logic so far. There is still a bit of common code, but I didn't want to introduce any drastic changes yet.
It is possible now to just initialize UartSplit() or PioSplit(), but old Split(split_type=SplitType.UART, use_pio=True) will still work as I left it as wrapper for backward compatibility. I don't know which form would you rather see in docs. I even tested the situation where left board got new code after these changes and right board got current master and everything worked. The only drastic change is removal of target_left argument as it did nothing at all.
Having an opportunity, I did minor improvements of BLE. While my #328 draft got more drastic changes, here I just made minimal required changes to make BLE split work again and introduced the same header/checksum as in UartSplit. It worked on my tiny test bed.
Regarding future, I see we will probably introduce different headers, for different message type. Still I don't have clear vision how to make other modules and extensions to plug in to such messages in best way.
I'm looking forward to feedback.
I like these changes. I started BLE splits similarly, but merged them for stupid reasons, and regretted it. Glad to see them split back out. As for integrating other modules/extensions integration, maybe have a preamble byte that lets you know what the protocol will be. We can extend extensions/modules with before/after for split as well if that's integral to making things work smoothly. Powersave is an example of a module that has core hooks, and splits can get the same core hooks. It just "acts like a dud" when the module isn't loaded, and while it takes up memory and a few cycles when you aren't using it, it's incredibly minimal, so I'd be willing to add those hooks for splits. It could allow for integration between modules/extensions to not only get data from splits, but also give a way to instruct splits to retrieve data in ways they want if a spec is built to handle that. For example, you can keep RGB animations in sync if you can poll the secondary half to send it's "animation position", and correct on one side to deal with clock cycles not always matching.
I totally agree with having different headers, hooks are also idea I head somewhere in mind.
What concerns me now is what relation should have modules of two sides and if we should follow master/slave relation. It sounds perfect for modules which works simultaneously on both sides like RGB (master can only sporadically poll child for sync, but probably mode changes after key presses should be interpreted only by one side) or Encoders (so only master store the state and changing the map requires only change it map, like with key matrix).
But what in situation when the master is left side and you got trackball only on right? Should left side have dummy trackball which get real values from second split instead of I2C?
And what with people who use one directional split? (Are there many users of that?) Master cannot poll the child in this situation about status, but if it was implemented other way, child could just send sync message from time to time and it would work on one directional split.
BTW, concerning this PR, would you rather have users use BleSplit, UartSplit and so on or still use existing Split(split_mode=X) wrapper? I'm not talking here about removing backward compatibility, just future direction.
A few things that some don't realize about splits.
- Modules don't always have to match
- Even the firmware doesn't have to match. KMK isn't required on the secondary half, only the communication protocol matters
- There can't be a "only one directional split" as then it becomes a single piece keyboard the moment it needs to send HID. Sure the keymap may have keys that don't exist, but it doesn't require a second piece if set to UART and works independently.
In terms of the trackball only on secondary(initiator), the module should "gracefully fail" when trying to assign hardware, but still be ready to take data, and send the mouse movements or whatever is needed of it. The split protocol will have to expand to let the data through, and the module becomes a "pass through" module, which is what split module has always basically been. To get the firmware state matching (or as close to) on both sides, regardless of what installed hardware is in there.
"would you rather have users use BleSplit, UartSplit and so on or still use existing Split(split_mode=X) wrapper?": I don't have a strong opinion on this. Changing it will take a lot of work for anyone that has forked KMK, however if there is benefit in changing it, the sooner the better, especially if there's a wrapper to support the "wrong" old way.
Disclaimer: I'll probably never use a split with two controllers, outside of testing, so I my opinion is rather tangential. Anyway here's an idea for split module communication. Introduce two new attributes to modules/extensions:
- The header byte identifying the module, ex.
module.SplitID - A setter-getter method, ex:
def split_sync(self, keyboard, in_data: bytearray):
# de-serialize data from other split and interpret, apply, whatever (if not None).
# return serialized own data if there's any, else None.
return out_data
The split module may call this method either if it received a message with matching ID, or periodically to see if there's data to be send. Split doesn't have to know anything about the modules internals, just shuffle data back and forth.
As for this PR: sure, I'm all for it if it works. Enums vs classes... I'd prefer classes, looks more pythonic to me, easier to extend -- but it's a bit more verbose and may trip up non-programmers.
"As for this PR: sure, I'm all for it if it works. Enums vs classes... I'd prefer classes, looks more pythonic to me, easier to extend -- but it's a bit more verbose and may trip up non-programmers." This is how we have done things for both memory reasons, and consistency. If memory isn't increased in a significant way, and the two of you believe that classes are easier, I am fine with this change as long as you hit EVERY instance of that in the entire project. Consistency is what we need most.
Any other changes that you want to make, or should I prep for final review/merge?
To be honest, when I created this PR, I thought that instead of one huge refactor, I would rather do these changes gradually, checking on each step if I didn't break anything. Especially that I don't have much time lately (btw I'm sorry I respond just now). This PR was tested by me on my Kyria and two pairs of standalone controllers (one for two-way UART, and one for BLE), but I would appreciate other tests.
Memory reason you mentioned is something I'm curious to hear more as with RP2040 I may not detect some compatibility-breaking changes. How should I profile it? I don't have much experience with memory profiling. I tried using gc.mem_free() (only thing I found so far) but it returns me just a sinusoid. When the memory doubts are solved, I would call it merge ready.
Concerning your split ideas for future module compatibility - these are great.
Even the firmware doesn't have to match. In terms of the trackball only on secondary(initiator), the module should "gracefully fail" when trying to assign hardware, but still be ready to take data, and send the mouse movements or whatever is needed of it.
I'm aware and often run two halves with different firmware. I wasn't sure what's you attitude of customizing firmware to each side. Currently I don't init some oled stuff and trackball on half which don't have it. I thought that firmware for each side should be more "manually fitted", but when I hear you idea of "gracefully fail" I like it as it may really make some stuff easier.
The header byte identifying the module, ex. module.SplitID
Oh, that's better idea than list of headers in split module!
A setter-getter method, ex:
split_sync(self, keyboard, in_data: bytearray)
I see that all of us three are having similar method in mind :)
Does this change make encoders work on the secondary half of the split keyboard
@Tonasz I would like to add the I2C protocol and it looks like I better wait for this before. But this PR seems dead... What is the status of this ?
I would like to add the I2C protocol and it looks like I better wait for this before. But this PR seems dead... What is the status of this ?
I'm hoping to adopt this PR and continue it. I hope to have the time in the next few days.