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

Allow QMK to save a single default layers in the full range of 0-31 #22933

Closed
wants to merge 2 commits into from

Conversation

infinityis
Copy link
Contributor

@infinityis infinityis commented Jan 20, 2024

Description

Currently QMK saves the default layer persistently through EECONFIG as an 8-bit wide bit field which allows for saving multiple simultaneous default layers, but only for layers 0 through 7. This PR adds the ability to save the default layer as a value instead of a bit field, so that layers 0 through 31 can be the default layer, subject to the constraint that there is only one default layer selected at a time.

Pull request #21881 is written to allow default layers in the range from 0 to 31, but that is currently not possible with the existing implementation. Functions like set_single_persistent_default_layer indicate that the more common use case is a single default layer. There are no instances of multiple default layers being saved in the QMK repository, but there is awareness of one instance of multiple default layers being used in the recently removed user code.

This PR changes the default/presumed paradigm to a single default layer, and the original bit field method can be the re-enabled with #define DEFAULT_LAYER_BITMASK_ENABLE. If desired, I can update the PR to preserve the original implementation by default instead.

Additionally:

  • All bespoke implementations that save default layers by calling eeconfig_update_default_layer to set a single layer have been replaced with API calls to set_single_persistent_default_layer Removed from this PR, was implemented in Tidy up default layer handling in keymaps #23436
  • EECONFIG magic number has been decremented No longer decremented as a decrement has already been performed for this breaking chances cycle

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

@github-actions github-actions bot added core keymap documentation via Adds via keymap and/or updates keyboard for via support labels Jan 20, 2024
@drashna drashna requested a review from a team January 23, 2024 23:59
@tzarc tzarc added breaking_change_2024q2 develop-fast-track Intended to be merged early in the next develop cycle. labels Feb 20, 2024
Copy link

github-actions bot commented Apr 8, 2024

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

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Apr 8, 2024
@github-actions github-actions bot removed the via Adds via keymap and/or updates keyboard for via support label Apr 8, 2024
@infinityis
Copy link
Contributor Author

Rebased on develop after merge of #23436 which implemented all the keymap cleanup changes; what remains of this PR is much smaller and focused on the primary goal of changing how eeconfig saves the default layer information.

I'm not sure if EECONFIG_MAGIC_NUMBER should decremented once per breaking changes release, or if it is decremented anytime a change is made to eeconfig storage implementation (in which case I would need to add in another decrement).

@tzarc
Copy link
Member

tzarc commented Apr 8, 2024

There's a bit of debate internally as to whether or not we should reshuffle the eeconfig definitions and allow for auint32_t here. Will get back to you.

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Apr 9, 2024
Copy link

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

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label May 25, 2024
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label May 28, 2024
Copy link

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

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jul 12, 2024
@tzarc tzarc added awaiting review and removed stale Issues or pull requests that have become inactive without resolution. labels Jul 12, 2024
Comment on lines +54 to 59
#if defined(DEFAULT_LAYER_BITMASK_ENABLE)
default_layer_state = (layer_state_t)1 << 0;
#else
default_layer_state = 0;
#endif
eeprom_update_byte(EECONFIG_DEFAULT_LAYER, default_layer_state);
Copy link
Member

Choose a reason for hiding this comment

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

Setting the global default_layer_state to zero would be incorrect as its expected to be a bitmask of enabled layers, and should probably look more like...

Suggested change
#if defined(DEFAULT_LAYER_BITMASK_ENABLE)
default_layer_state = (layer_state_t)1 << 0;
#else
default_layer_state = 0;
#endif
eeprom_update_byte(EECONFIG_DEFAULT_LAYER, default_layer_state);
default_layer_state = (layer_state_t)1 << 0;
#if defined(DEFAULT_LAYER_BITMASK_ENABLE)
eeprom_update_byte(EECONFIG_DEFAULT_LAYER, default_layer_state);
#else
eeprom_update_byte(EECONFIG_DEFAULT_LAYER, 0);
#endif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review breaking_change_2025q1 Targeting breaking changes Q1 2025 core develop-fast-track Intended to be merged early in the next develop cycle. documentation keymap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants