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

Add BOOTMAGIC_ENABLE=both which combines lite and full mode #13572

Closed
wants to merge 1 commit into from

Conversation

dkjer
Copy link

@dkjer dkjer commented Jul 16, 2021

Add BOOTMAGIC_ENABLE = both which combines lite and full mode

Description

Currently BOOTMAGIC_ENABLE can either be 'lite' ('yes') or 'full'. VIA currently forces 'lite' mode when enabled, presumably to have a method for clearing eeprom and entering bootloader mode that isn't reliant on a particular set of keys being available on layer 0 (which VIA may change).

This change allows for both 'full' mode and 'lite' mode to be enabled at the same time. VIA still defaults to enabling 'lite' mode, but will allow for 'both' mode if bootmagic 'full' functionally is also desired.

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

@dkjer dkjer changed the title Allowing BOOTMAGIC_ENABLE=full when VIA is enabled Allow BOOTMAGIC_ENABLE=full when VIA is enabled Jul 16, 2021
@tzarc
Copy link
Member

tzarc commented Jul 17, 2021

You'll need a via_eeprom_reset() call somewhere as per bootmagic lite.

@dkjer
Copy link
Author

dkjer commented Jul 17, 2021

You'll need a via_eeprom_reset() call somewhere as per bootmagic lite.

Is there a reason bootmagic full's eeconfig_init() won't work for this purpose?

Allowing BOOTMAGIC_ENABLE=both when VIA is enabled
@dkjer dkjer changed the title Allow BOOTMAGIC_ENABLE=full when VIA is enabled Add BOOTMAGIC_ENABLE=both which combines lite and full mode Jul 17, 2021
@dkjer
Copy link
Author

dkjer commented Jul 17, 2021

I have updated the PR to allow for both lite mode and full mode, so that there is always a 'failsafe' method to call via_eeprom_reset()

@zvecr
Copy link
Member

zvecr commented Jul 17, 2021

As full bootmagic is on the list of things that might be removed in the near future, could you go into some detail behind the use case for this change (and maybe touch on why the magic keycodes wont suffice).

@dkjer
Copy link
Author

dkjer commented Jul 18, 2021

As full bootmagic is on the list of things that might be removed in the near future, could you go into some detail behind the use case for this change (and maybe touch on why the magic keycodes wont suffice).

I honestly don't have a strong use case for bootmagic full, and if its on the list of things to be removed then we can just close this PR.

The main thing I liked keeping it enabled for is having the toggling of debug flags actually being written to eeprom via eeconfig_update_debug, as opposed to using MAGIC_KEY_DEBUG_* which doesn't update eeprom. Obviously someone could add their own functionality to suite their preferences, but it was convenient to have a built-in way to choose whether or not to have your debug settings be 'sticky' when iterating on firmware changes.

@drashna
Copy link
Member

drashna commented Jul 20, 2021

You'll need a via_eeprom_reset() call somewhere as per bootmagic lite.

Is there a reason bootmagic full's eeconfig_init() won't work for this purpose?

Because VIA doesn't properly hook into eeconfig_init. It's an oversight that I have a PR open to fix.
#13243

@drashna
Copy link
Member

drashna commented Jul 20, 2021

The main thing I liked keeping it enabled for is having the toggling of debug flags actually being written to eeprom via eeconfig_update_debug, as opposed to using MAGIC_KEY_DEBUG_* which doesn't update eeprom. Obviously someone could add their own functionality to suite their preferences, but it was convenient to have a built-in way to choose whether or not to have your debug settings be 'sticky' when iterating on firmware changes.

Well, bootmagic_lite can be overwritten to do more than just the reset, so ... that may be the better way to go.

@drashna drashna requested a review from a team July 20, 2021 00:56
@noroadsleft
Copy link
Member

noroadsleft commented Aug 2, 2021

As full bootmagic is on the list of things that might be removed in the near future, could you go into some detail behind the use case for this change (and maybe touch on why the magic keycodes wont suffice).

Full Bootmagic's removal was actually placed on the agenda for QMK back in early March, and this develop cycle is scheduled to remove Full Bootmagic as a feature.

https://docs.qmk.fm/#/ChangeLog/20210529?id=bootmagic-deprecation-and-refactor

@dkjer dkjer closed this Aug 12, 2021
@dkjer dkjer deleted the bootmagic-full-via branch August 12, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants