implementing layer_lock_timer

[mwpardue]

Hi. This is 02ranger on reddit. This is the pull request for the layer_lock_timer function we discussed. I've tested and it compiles with and without LAYER_LOCK_IDLE_TIMEOUT defined and the layer lock timer does work when compiled with the timeout defined and the timer is absent when no timeout is defined. Please do test if you're interested in merging because I am VERY new at all this and could have made a mistake.

Thanks for all your help!

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[getreuer]

D'oh, I lost my review comments somehow. I'll add them again.

[mwpardue]

Question for you, I see it's possible to use a 32 bit unsigned integer for a timer. Other than firmware size, are there any drawbacks to using a 32-bit integer? IDK if you want to include it in your published version of layer lock but I might toy with it on my personal repo. I'm using a Bonsai C4 MCU on my KB so for once I don't really have to be concerned with firmware size.

[getreuer]

Great question. That's awesome that you have a Bonsai C4. I heard that's a good one.

It is worthwhile to go with 16-bit timers and make firmware size smaller where we can. For better or worse, some potential users of this repo are on custom keyboards running AVR microcontrollers. Flash size might be very limited, e.g. 28 KB usable flash space on the ATmega32U4. AVR is an 8-bit architecture, where narrower types are preferable. For instance, to compute time elapsed (current_time - last_time) in the Atmel AVR instruction set, I think that would be a SUB instruction and a SBC (subtract with carry) instruction for 16-bit timers vs. a SUB and three SBCs for 32-bit timers. Not that extra 3 instructions on it's own is a huge deal, but this adds up when multiplied across every operation for every feature.

OTOH, Bonsai C being a 32-bit ARM chip can compute 32-bit time elapsed in a single instruction.

[mwpardue]

Yeah, I don't think it would be a great idea to use a 32-bit timer for a publicly available feature. I just didn't know if there were other concerns with it. I may play with it on my own because I'd kinda like to set the timeout to 60 seconds, maybe higher, but not something to add to the published feature. Of course if all I want is 60 seconds I suppose I could do some kind of boolean variable that gets flipped true when the timer reaches 30 and resets the timer or if already true turns the layers off at 30. Sorry, none of that is stuff I'm trying to get you to add. Just thinking 'out loud' so to speak.

[getreuer]

Aha, thanks for that context. You have persuaded me =) I meant my previous post as "all else being equal" when comparing 16-bit vs. 32-bit timers, with the difference not being huge. If you find a 60-second timeout more practical, that's a compelling reason to go up to 32-bit timers. It's likely that other people would like that option, too, and I think we should do it.

[mwpardue]

Works for me! 😁 Im thinking just automatically select 16 or 32 bit based on the timeout value they define? I'll look at what kind of size difference there is and put a warning in the files and on compile if timer >30000.

BTW, this my first time collaborating with anybody on GitHub. Should I be resolving your comments as I make changes or would you normally do that?

[mwpardue]

I've pushed the version with both 16- and 32-bit timers. Let me know what you think. I did several test compiles for an Atmega32u4 version of the Corne and the 32-bit timer actually only resulted in an additional 20 bytes. I think this version will work very well, but do you think we should go strictly 32 bit for simplicity's sake?

[getreuer]

Works for me! grin Im thinking just automatically select 16 or 32 bit based on the timeout value they define? I'll look at what kind of size difference there is and put a warning in the files and on compile if timer >30000.

Thanks for looking into it and measuring! I agree, at least in this use, it's better to go with 32-bit timers unconditionally for simplicity.

BTW, this my first time collaborating with anybody on GitHub. Should I be resolving your comments as I make changes or would you normally do that?

I'm not sure if there's an etiquette about such things, I don't have much experience with GitHub myself. Resolving comments is fine by me.

[getreuer]

BTW, I flashed your layer lock timer on my keyboard, and it is working great. Thanks for all your work on this!

[mwpardue]

I'm glad to know the feature worked well for you. After the latest round of updates everything seems to be working as expected, including compiling without the LAYER_LOCK_IDLE_TIMEOUT defined or just defined to 0. It does not activate the timer in those cases, as we would expect. Let me know what you think and if you see anything else we need to change just let me know. I'm having fun with this, it's the first time I've been able to contribute in even a tiny way to the custom KB community.

[getreuer]

Thanks for all your work on this PR! This is good to go. I will merge it now.

I'm having fun with this, it's the first time I've been able to contribute in even a tiny way to the custom KB community.

Fantastic, it's really great that you are contributing and enjoying it.

On the main qmk_firmware repo, there is an open feature request https://github.com/qmk/qmk_firmware/issues/16861 proposing to make Layer Lock a core QMK feature. This would be great to make Layer Lock available to a wider audience, for instance users who use Oryx or other QMK frontend where adding custom code is hard to do. What's needed to get this going is motivation on why Layer Lock would be worth adding, considering that tap-toggle TT is similar.

It would be really helpful if you could comment on the issue thread about how Layer Lock helps your workflow and why you prefer it over alternatives methods.

Thanks again for your contribution!