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

Keymap config ram #18373

Closed
wants to merge 1 commit into from
Closed

Keymap config ram #18373

wants to merge 1 commit into from

Conversation

osamuaoki
Copy link
Contributor

Provide an optional feature to keymap_config data to be non-persistent.

This means keyboard always turn on to its default map without surprize of swapped keys but you still retain capability to swap via quantum keycode.

Description

This is an implementation of my comment posted as
#18353 (comment)

Instead of storing the keymap_config struct in eeprom, this enables user to select storing it in RAM if KEYMAP_CONFIG_RAM is defined.

The QMK behavior doesn't change for existing keyboards.

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

No more surprise for some swapped keys. At least, power-on reset them all.

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 cli qmk cli command core dependencies documentation keyboard keymap python translation via Adds via keymap and/or updates keyboard for via support labels Sep 15, 2022
@sigprof sigprof changed the base branch from master to develop September 15, 2022 08:18
@github-actions github-actions bot removed via Adds via keymap and/or updates keyboard for via support documentation translation dependencies keymap python cli qmk cli command keyboard labels Sep 15, 2022
@sigprof
Copy link
Contributor

sigprof commented Sep 15, 2022

You need to select develop as the base branch when opening a PR based on develop. It is possible to edit it after the fact (I did it for this PR), but getting things right from the start would be better (e.g., no confusing label changes).

@osamuaoki
Copy link
Contributor Author

sigprof: sorry.

I just rebased to the latest develop.

@@ -47,7 +47,9 @@ bool process_magic(uint16_t keycode, keyrecord_t *record) {
case MAGIC_TOGGLE_CONTROL_CAPSLOCK:
case MAGIC_SWAP_ESCAPE_CAPSLOCK ... MAGIC_TOGGLE_ESCAPE_CAPSLOCK:
/* keymap config */
#ifndef KEYMAP_CONFIG_RAM
Copy link
Member

Choose a reason for hiding this comment

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

This might be better in the eeconfig_(read/update)_keymap function itself, since there are other places that these functions are called (bootmagic used to be one place, but the steno keycode in the planck, for instance).

@@ -16,7 +16,9 @@

#include "keycode_config.h"

extern keymap_config_t keymap_config;
#ifdef KEYMAP_CONFIG_RAM
keymap_config_t keymap_config;
Copy link
Member

Choose a reason for hiding this comment

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

If the magic.c declaration isn't changed, this will error out on complication, I think.

@osamuaoki
Copy link
Contributor Author

After good night sleep, I realized the changes to keycode_config.c is not needed for this PR purpose. (Though I still think it is redundant.)

I will force squashed patch for easier review.

If KEYMAP_CONFIG_RAM is defined, keymap_config is stored as a
non-persistent global variable on RAM instead of a persistent global
variable on EEPROM.  So for every power-on, we can reset the
keymap_config settings to its default values.

Signed-off-by: Osamu Aoki <[email protected]>
@osamuaoki
Copy link
Contributor Author

Copy link
Contributor

@sigprof sigprof left a comment

Choose a reason for hiding this comment

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

Unfortunately, just removing the EEPROM operations from process_magic() does not fix the problem completely, because keymap_config also contains flags used by other features, and some other code also can call eeconfig_update_keymap():

  • autocorrect_enable(), autocorrect_disable(), autocorrect_toggle();
  • oneshot_set(), oneshot_enable(), oneshot_disable(), oneshot_toggle().

Also keeping the nkro flag persistent could be useful.

So if the intent here is “force the power-on state of the magic keycode modifications to be off”, this may need to be done inside eeconfig_read_keymap() and eeconfig_update_keymap() instead (if we keep the current EEPROM layout).

@github-actions
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 Dec 15, 2022
@github-actions
Copy link

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants