feat: default layer setter #2222
Conversation
|
If I may add an idea to be considered in your follow up PR. Might be worth to consider being able to trigger automatic layout changes on BLE profile change. Not sure how this would be best made configurable (maybe via KConfig?, Could ZMK remember default layer per profile?), but it would be convenient if someone with Windows and macOS system for example, could configure a default layer for each BLE profile and one for USB to automatically trigger a default layer change depending on which profile is activated. |
|
I acknowledge the potential advantage of the planned behavior for users with dual systems, but after thinking about it a bit more I'm wondering whether this proposed change should be directly integrated into the feature's implementation itself rather than as its separate PR. |
|
I dont mind either way, the idea of making a small PR was to start rolling it and see if it had missed a small edge case or whatever. Also making two different PRs with the respective self-contained logic seemed better review-wise. I already have the code (or at least, its first revision) written & slightly tested it (with to instead of setting default layer at ZMK level). So, if collabs think it is better to keep everything together just let me know and i will push it on this branch too |
Sorry, I have been quite busy lately. Personally, I would recommend adding everything related to this feature in one PR so when it's merged it adds the full feature. I tested your code from your personal repo and I would recommend to incorporate it into this PR for full review/testing of the complete feature. |
|
Aight, will do. Again, my main idea was go test if default layer setting really worked well before relying on it for the behavior, for easier debug when (if) edge cases poped up. Will add it to make the process faster and keep it together. Glad to hear the current implementation (layer toggle instead of default layer, which is worse) is working fine for you |
| struct default_layer_settings_t { | ||
| uint8_t usb[ZMK_ENDPOINT_USB_COUNT]; | ||
| uint8_t ble[ZMK_ENDPOINT_BLE_COUNT]; | ||
| }; |
There was a problem hiding this comment.
This is too tied to the details of the transports, IMHO. You should be more agnostic, e.g.
| struct default_layer_settings_t { | |
| uint8_t usb[ZMK_ENDPOINT_USB_COUNT]; | |
| uint8_t ble[ZMK_ENDPOINT_BLE_COUNT]; | |
| }; | |
| struct default_layer_settings_t { | |
| uint8_t endpoint_defaults[ZMK_ENDPOINT_COUNT]; | |
| }; |
And then code that needs to set/load the setting would simply call zmk_endpoint_instance_to_index to determine which setting to pull out of that array consistently.
The advantage here, whenever we do eventually support some other new transport (e.g. some custom 2.4Ghz protocol, or maybe HID over i2c), this code will adjust automatically.
There was a problem hiding this comment.
That's what i get for not looking closely at the codebase, i ended up reinventing the wheel 😝
Im still a bit worried about issues when user has old settings stored (eg move layers around and whatnot).
Any ideas that could help with that? Imho code shouldn't worry too much about those scenarios and let the user be the one in charge of deteting/fixing. But if there is some easy/robust idea, i can sure add it.
If nothing gets added i will add some warn messge in docs as in "beware the stored settings might break when you tweak the keymap"
There was a problem hiding this comment.
Two other docs pieces need updating in addition to below comments:
- https://zmk.dev/docs/behaviors#layer-navigation-behaviors table
- The default layer remark in https://zmk.dev/docs/config/keymap#devicetree
| break; | ||
| } | ||
|
|
||
| int ret = settings_save_one("default_layer/settings", &default_layers, sizeof(default_layers)); |
There was a problem hiding this comment.
Do we want to apply debouncing here like most other settings that save to flash?
|
|
||
| ## Default Layer | ||
|
|
||
| The default layer behavior allows configuring a different default layer, for example to test DVORAK while keeping QWERTY on another layer, or moving a couple keycodes around for Windows/Mac usage. |
There was a problem hiding this comment.
I feel like this needs a little more information on the default layer concept, since it wasn't modifiable from 0 until this. I'd at least note here that the default (default) layer is the first defined one, that it cannot be changed or deactivated by other behaviors in this section.
There was a problem hiding this comment.
On this note, I've used the default layer concept differently on the ROTR to have an encoder scroll in a momentary layer change the layer you return to when that momentary layer is released, skipping layers tagged to be momentary only. I think there's reasoning behind an endpoint agnostic default layer mechanism too, perhaps a pair of bool props in the behaviour to choose whether it respects endpoints and saves to flash?
There was a problem hiding this comment.
Thanks for the docs check, will get them updated tomorrow.
As per the delayed writing, Im not sure whether it is needed. Based on my understanding of Zephyr's docs, it only triggers a write if the value has changed (eg: differs from what the flash contains already).
If that is correct, I dont think an inmediate write is bad (wearing down memory - wise) but for sure is in terms of consistency with codebase, will update too.
|
#1500 seems like it could be relevant here and hasn't been mentioned yet, just dropping a link. |
I plan to do a PR to persistently change the default layer, for multi-layout uses (eg win/mac or qwerty/dvorak)
However, while activating the layer or to'ing into it is similar, it wouldn't mimic the behavior of default layer. Thus, the follow-up PR needs an API of this kind.
Notes: