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

[Feature] Add keycode PDF(layer) to set the default layer in EEPROM #21881

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

Nebuleon
Copy link
Contributor

@Nebuleon Nebuleon commented Sep 1, 2023

Description

This pull request introduces the PDF(layer) keycode, which acts like DF(layer) but sets the target layer in EEPROM with the existing set_single_persistent_default_layer function. The second proposed name, PDF, after the first which caused a name collision, stands for Persistent DeFault.

The keycode occupies the part of the 16-bit QMK keycode space starting at 0x52E0 and goes on for 32 entries, each corresponding to one of the 32 layers. It is, like DF, compiled unless NO_ACTION_LAYER is defined.

I have adjusted the core features and keyboards that have special processing for DF so that the same processing applies for PDF:

  • in process_auto_mouse(uint16_t keycode, keyrecord_t* record), so that it does nothing;
  • in process_autocorrect_default_handler(uint16_t *keycode, keyrecord_t *record, uint8_t *typo_buffer_size, uint8_t *mods), to exclude PDF from processing;
  • in bool music_mask_kb(uint16_t keycode) under moonlander and planck/ez, so that the keycodes are excluded from music mode processing.

The lint check is expected to fail here under either moonlander or planck/ez because the indentation style of the switch statement differs between the two and the linter only accepts one of them. I have preferred consistency within the surrounding code per the CONTRIBUTING document.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • 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).

@Nebuleon Nebuleon requested a review from tzarc September 1, 2023 19:59
@drashna
Copy link
Member

drashna commented Sep 2, 2023

modifying default_layer_state_set might be a simpler solution.

@Nebuleon
Copy link
Contributor Author

Nebuleon commented Sep 2, 2023

Do you mean that having the user do persistence in default_layer_state_set_user (and documenting that method, on top of the process_record_user method) would be better than introducing a keycode for it, or that doing the persistence in default_layer_state_set as part of this PR would be better than introducing this keycode?

I did it the way I did because I can envision a scenario where the user would like to have some kind of tentative layer switch keycode in one layer and a more committed switch in a special layer that needs more confirmation (like in Miryoku), and because of Via(l): a user can then switch the behaviour between the temporary switch and the permanent switch by changing the keycode.

quantum/action.c Outdated Show resolved Hide resolved
@Nebuleon
Copy link
Contributor Author

Nebuleon commented Sep 3, 2023

I would update docs/understanding_qmk.md with this:

      * [`bool process_default_layer(uint16_t keycode, keyrecord_t *record)`](https://github.com/qmk/qmk_firmware/blob/????????????????????????????????/quantum/process_keycode/process_default_layer.c#L22)

but don't know what the squashed commit hash will look like. In that sense, the documentation update cannot be atomic along with the code.

While looking at the surroundings of the documentation, I also found that the new process_tri_layer function is not in the documented chain, and neither is process_autocorrect.

@Nebuleon Nebuleon requested a review from zvecr September 3, 2023 08:40
@Nebuleon
Copy link
Contributor Author

Nebuleon commented Sep 3, 2023

Hmm.

QMK CI failure
In file included from quantum/quantum.h:40,
                 from ./.build/obj_ducky_one2mini_1861st_default/src/default_keyboard.h:27,
                 from .build/obj_ducky_one2mini_1861st_default/src/default_keyboard.c:26:
quantum/quantum_keycodes.h:96: error: "PD" redefined [-Werror]
 #define PD(layer) (QK_PERSISTENT_DEF_LAYER | ((layer)&0x1F))

In file included from ./lib/chibios-contrib/os/common/startup/ARMCMx/devices/NUC123/cmparams.h:72,
                 from ./lib/chibios/os/common/ports/ARMv6-M/chcore.h:32,
                 from ./lib/chibios/os/rt/include/chport.h:37,
                 from ./lib/chibios/os/rt/include/ch.h:106,
                 from ./lib/chibios/os/hal/osal/rt-nil/osal.h:32,
                 from ./lib/chibios/os/hal/include/hal.h:30,
                 from platforms/chibios/platform_deps.h:18,
                 from quantum/quantum.h:18,
                 from ./.build/obj_ducky_one2mini_1861st_default/src/default_keyboard.h:27,
                 from .build/obj_ducky_one2mini_1861st_default/src/default_keyboard.c:26:
./lib/chibios-contrib/os/common/ext/CMSIS/Nuvoton/NUMICRO/NUC123.h:8479: note: this is the location of the previous definition
 #define PD                  ((GPIO_T *) PD_BASE)                        /*!< GPIO PORTD Configuration Struct                        */

And the original definition is in ChibiOS so I'll have to rename the keycode.

@Nebuleon Nebuleon marked this pull request as draft September 3, 2023 20:03
@Nebuleon Nebuleon changed the title [Feature] Add keycode PD(layer) to set the default layer in EEPROM [Feature] Add keycode PDF(layer) to set the default layer in EEPROM Sep 4, 2023
@Nebuleon Nebuleon marked this pull request as ready for review September 4, 2023 03:50
@Nebuleon
Copy link
Contributor Author

Nebuleon commented Sep 4, 2023

And now things should be pretty much OK. I've renamed the keycode to PDF, and edited the original post and the PR title to suit.

@github-actions github-actions bot added the dd Data Driven Changes label Sep 4, 2023
@Nebuleon
Copy link
Contributor Author

This PR now depends on:

to be merged first so it can work better for layers 8..31. Could a maintainer please add the awaiting_pr tag? Thank you!

@drashna drashna added the awaiting_pr Relies on another PR to be merged first label Nov 8, 2023
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 24, 2023
@infinityis
Copy link
Contributor

infinityis commented Jan 17, 2024

The PR dependency adding support for layers 8..31 has been changed to #22933

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jan 18, 2024
@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 6, 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 6, 2024
Copy link

github-actions bot commented May 6, 2024

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 May 6, 2024
@zvecr zvecr reopened this May 6, 2024
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label May 7, 2024
@zvecr
Copy link
Member

zvecr commented May 18, 2024

To make this potentially mergeable, could the conflicts be resolved please. (Or alternatively the Allow maintainers to edit this pull request toggled.)

@Nebuleon Nebuleon force-pushed the feature/persistent-default-layer branch from cfc3dd1 to 33d1a26 Compare May 18, 2024 20:39
@Nebuleon
Copy link
Contributor Author

Thank you for pinging me, @zvecr. I have updated my branch to current develop by rebase, and the conflict in keyboards/moonlander/moonlander.c is resolved by following the rename to keyboards/zsa/moonlander/moonlander.c.

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
@zvecr zvecr changed the base branch from develop to develop_21881 November 22, 2024 11:16
@zvecr zvecr merged commit 9795490 into qmk:develop_21881 Nov 22, 2024
5 of 6 checks passed
zvecr added a commit that referenced this pull request Nov 23, 2024
…24630)

* [Feature] Add keycode PDF(layer) to set the default layer in EEPROM (#21881)

* Apply suggestions from code review

Co-authored-by: Nick Brassel <[email protected]>

---------

Co-authored-by: Nebuleon <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
ilham-agustiawan pushed a commit to ilham-agustiawan/qmk_firmware that referenced this pull request Nov 30, 2024
…mk#24630)

* [Feature] Add keycode PDF(layer) to set the default layer in EEPROM (qmk#21881)

* Apply suggestions from code review

Co-authored-by: Nick Brassel <[email protected]>

---------

Co-authored-by: Nebuleon <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
smallketchup82 pushed a commit to smallketchup82/qmk_firmware that referenced this pull request Dec 1, 2024
…mk#24630)

* [Feature] Add keycode PDF(layer) to set the default layer in EEPROM (qmk#21881)

* Apply suggestions from code review

Co-authored-by: Nick Brassel <[email protected]>

---------

Co-authored-by: Nebuleon <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
jlaptavi pushed a commit to jlaptavi/qmk_firmware that referenced this pull request Dec 3, 2024
…mk#24630)

* [Feature] Add keycode PDF(layer) to set the default layer in EEPROM (qmk#21881)

* Apply suggestions from code review

Co-authored-by: Nick Brassel <[email protected]>

---------

Co-authored-by: Nebuleon <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
DmNosachev pushed a commit to DmNosachev/qmk_firmware that referenced this pull request Dec 7, 2024
…mk#24630)

* [Feature] Add keycode PDF(layer) to set the default layer in EEPROM (qmk#21881)

* Apply suggestions from code review

Co-authored-by: Nick Brassel <[email protected]>

---------

Co-authored-by: Nebuleon <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
SyrupSplashin pushed a commit to SyrupSplashin/qmk_firmware that referenced this pull request Dec 10, 2024
…mk#24630)

* [Feature] Add keycode PDF(layer) to set the default layer in EEPROM (qmk#21881)

* Apply suggestions from code review

Co-authored-by: Nick Brassel <[email protected]>

---------

Co-authored-by: Nebuleon <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_pr Relies on another PR to be merged first awaiting review core dd Data Driven Changes develop-fast-track Intended to be merged early in the next develop cycle. documentation keyboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants