Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make default layer size 16-bit #15286

Merged
merged 13 commits into from
Jun 18, 2022
Merged

Conversation

drashna
Copy link
Member

@drashna drashna commented Nov 23, 2021

Description

Set the layer state to be 16-bit by default.

I would rather set this to 8-bit, but until the configurator can handle that without issue, ... it supports 16 layers, so this would work in the meanwhile.

And this allows for smaller compiled sizes.

Types of Changes

  • Core
  • Enhancement/optimization
  • Documentation

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

@github-actions github-actions bot added the core label Nov 23, 2021
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and is a sane default.

@precondition
Copy link
Contributor

I can already hear the unrest that would come from the havoc caused by all the biton32 in the wild no longer working. Other than that, I'm in favor of this.

@fauxpark
Copy link
Member

That's why you're supposed to use get_highest_layer() instead ;)

@precondition
Copy link
Contributor

Yes, indeed but I still see too many people on the QMK discord server posting code bits using biton32 instead of get_highest_layer :(

@precondition
Copy link
Contributor

Speaking of biton32, shouldn't the ~265 user files calling the biton32 function in place of get_highest_layer get changed in this PR too? For example:

keyboards/bpiphany/kitten_paw/keymaps/ickerwx/keymap.c:135:    layer = biton32(layer_state);

@fauxpark
Copy link
Member

fauxpark commented Nov 27, 2021

We try to avoid changing user code en masse like that...
Core and default-ish keymaps are fair game though.

@drashna
Copy link
Member Author

drashna commented Nov 27, 2021

Yes, indeed but I still see too many people on the QMK discord server posting code bits using biton32 instead of get_highest_layer :(

That's because it in a lot of the default keymaps. And people copy and paste code.

But I've been flagging that in any new PRs, so more copy-paste code hasn't been incorrectly added.

Edit: I've updated the default keymaps, docs, and associated kb files to use get_highest_layer when appropriate, now.

@drashna drashna force-pushed the core/set_default_layer_size branch from a2b90a6 to 5c98461 Compare November 27, 2021 18:30
@github-actions github-actions bot added documentation python translation via Adds via keymap and/or updates keyboard for via support labels Nov 27, 2021
@drashna drashna force-pushed the core/set_default_layer_size branch from 5c98461 to ade1290 Compare November 27, 2021 18:39
@github-actions github-actions bot removed the python label Nov 27, 2021
@drashna drashna marked this pull request as ready for review November 28, 2021 02:49
@drashna drashna requested a review from a team November 28, 2021 02:50
@drashna
Copy link
Member Author

drashna commented Nov 28, 2021

Speaking of biton32, shouldn't the ~265 user files calling the biton32 function in place of get_highest_layer get changed in this PR too? For example:

keyboards/bpiphany/kitten_paw/keymaps/ickerwx/keymap.c:135:    layer = biton32(layer_state);

Also, some of the rational behind not changing all of the user files doing this (besides, "that's a lot of work"), is that using biton32 will work and "fail gracefully". So there is no harm in leaving it. But future examples and the default keymps should use the correct function.

@zvecr
Copy link
Member

zvecr commented Dec 2, 2021

Also worth noting that the configurator currently supports more than 8 layers. And some keyboards also set DYNAMIC_KEYMAP_LAYER_COUNT greater than 8.

@drashna drashna force-pushed the core/set_default_layer_size branch from ade1290 to 4cf2296 Compare December 4, 2021 06:14
@drashna
Copy link
Member Author

drashna commented Dec 4, 2021

Also worth noting that the configurator currently supports more than 8 layers. And some keyboards also set DYNAMIC_KEYMAP_LAYER_COUNT greater than 8.

The dynamic keymap layer count is already taken into account, but github doesn't show a lot of context. Literally, right above:

https://github.com/qmk/qmk_firmware/blob/c76cfc2d36ead86a5ec9e4d5061a28987b0bca18/quantum/action_layer.h#L24-L41

However, the configurator is a good point, and this PR should wait to be merged until there is something in place to handle that.

@drashna drashna force-pushed the core/set_default_layer_size branch from 4cf2296 to 32f3006 Compare January 8, 2022 03:09
@drashna drashna changed the title Make default layer size 8-bit Make default layer size 16-bit Jan 8, 2022
@noroadsleft
Copy link
Member

Merge conflicts.

@drashna drashna force-pushed the core/set_default_layer_size branch from 32f3006 to 5db91e7 Compare January 13, 2022 04:49
@drashna drashna force-pushed the core/set_default_layer_size branch from 5db91e7 to fb64c4b Compare January 22, 2022 17:53
keyboards/helix/rev3_5rows/keymaps/five_rows/keymap.c Outdated Show resolved Hide resolved
keyboards/helix/rev2/keymaps/five_rows/keymap.c Outdated Show resolved Hide resolved
keyboards/helix/rev2/keymaps/edvorakjp/keymap.c Outdated Show resolved Hide resolved
@drashna drashna force-pushed the core/set_default_layer_size branch from ac28772 to dab2d54 Compare February 26, 2022 02:55
@drashna drashna requested a review from a team March 4, 2022 18:54
@drashna drashna force-pushed the core/set_default_layer_size branch from dab2d54 to 737732e Compare March 16, 2022 00:55
@tzarc
Copy link
Member

tzarc commented May 15, 2022

Seems to break the helix boards.

@tzarc
Copy link
Member

tzarc commented May 15, 2022

Deferred to Q3 so helix can be fixed and verified.

@drashna
Copy link
Member Author

drashna commented Jun 18, 2022

Deferred to Q3 so helix can be fixed and verified.

How was this breaking the helix boards?

I do see one keymap that won't compile, but that's because it's doing some really weird stuff. All the rest compile. However, I don't have a board to test on.

@tzarc
Copy link
Member

tzarc commented Jun 18, 2022

I don't remember, and at this stage I'm inclined to say "develop can break, we can fix over time if issues occur".

@tzarc tzarc merged commit 0da6562 into qmk:develop Jun 18, 2022
@drashna drashna deleted the core/set_default_layer_size branch June 21, 2022 00:23
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
Co-authored-by: James Young <18669334+noroadsleft@users.noreply.github.com>
rschenk added a commit to rschenk/qmk_keymaps that referenced this pull request Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation keyboard keymap python translation via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants