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

The Key Company project consolidation #9547

Merged
merged 8 commits into from
Jul 28, 2020

Conversation

TerryMathews
Copy link
Contributor

Description

Consolidated all projects into tkc project folder.
Update make commands referenced in readmes.
Increase keymap count in VIA keymaps to 4.

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

@wilba
Copy link
Contributor

wilba commented Jun 27, 2020

You can use #define DYNAMIC_KEYMAP_EEPROM_MAX_ADDR 2047 in the config.h to use more EEPROM. AT90USB1286 has 4K.

@TerryMathews
Copy link
Contributor Author

@Wilba6582 I thought there should be no way that it was running out of EEPROM... I can do that, but should the core maybe be fixed instead? It seems like the issue here is that the build isn't correctly detecting the amount of EEPROM space this MCU has.

@TerryMathews
Copy link
Contributor Author

I had meant for the EEPROM size detect fix to be a separate PR but it got rolled up in here. Sorry! Thoughts?

@wilba
Copy link
Contributor

wilba commented Jun 28, 2020

I chose not to do this initially and leave it up to the keyboards with more than 1K to explicitly set it.... but this seems reasonable enough for the AT90 and will give those keyboards more macro space as well.

@zvecr zvecr added breaking_change Changes that need to wait for a version increment keyboard labels Jun 28, 2020
@zvecr zvecr requested a review from a team June 28, 2020 22:13
quantum/dynamic_keymap.c Outdated Show resolved Hide resolved
@wilba
Copy link
Contributor

wilba commented Jun 29, 2020

All the memory between the end of the "keymaps" and the end of the "allocated" EEPROM is used for macros. It makes sense then to be explit about how much EEPROM gets used by dynamic keymaps rather than just use it all up.

This is the reason I didn't make it use E2END in the first place. Dynamic keymaps should use a sensible amount by default, not "all available EEPROM".

@fauxpark
Copy link
Member

All the memory between the end of the "keymaps" and the end of the "allocated" EEPROM is used for macros

So then there is no room for macros on the 32U4? 0x3FF == 1023, so you are using up all available EEPROM for dynamic keymaps on like 90% of boards.

@wilba
Copy link
Contributor

wilba commented Jun 29, 2020

"dynamic keymaps" as a feature uses up to DYNAMIC_KEYMAP_EEPROM_MAX_ADDR, the memory between the "keymaps" and that end address is used for macros. So for ATmega32u4, it is using all the EEPROM, giving a few hundred bytes for macros, depending on switch matrix size (i.e. macro pads have a lot more macro space than a TKL). For other MCUs, I thought it was preferrable not to set that end address by default to the maximum for the MCU, so it doesn't use something like 3K or so for macros on an AT90USB1286. But if you think it's better to do something like if AVR, use E2END then go for it.

quantum/dynamic_keymap.c Outdated Show resolved Hide resolved
@TerryMathews
Copy link
Contributor Author

So I've f-ed up my fork somehow pretty badly. Any possibility this gets merged into future fairly soon, so I can not worry about preserving the PR?

@TerryMathews TerryMathews requested a review from fauxpark July 2, 2020 03:48
@Erovia Erovia requested a review from a team July 2, 2020 15:37
@Erovia
Copy link
Member

Erovia commented Jul 2, 2020

This PR should target the develop branch instead of master as it is a breaking change.
I'm not very found of mixing keyboard/keymap stuff with core changes either.


How did you mess up your fork?
Feel free to reach out to me on Discord, it's probably fixable.

@TerryMathews
Copy link
Contributor Author

So is everything good with this now or do I need to make more changes to it?

@fauxpark fauxpark changed the base branch from master to develop July 14, 2020 05:26
@fauxpark
Copy link
Member

I've changed the base branch of this PR (you just click the Edit button):
image
The commit log afterwards looks the same 👍

@TerryMathews
Copy link
Contributor Author

OK, I didn't know how to do that. Thanks!

@drashna drashna requested a review from a team July 24, 2020 02:48
@noroadsleft noroadsleft merged commit 8422cd1 into qmk:develop Jul 28, 2020
@noroadsleft
Copy link
Member

Thanks!

noroadsleft pushed a commit that referenced this pull request Jul 31, 2020
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
noroadsleft pushed a commit that referenced this pull request Aug 11, 2020
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
noroadsleft pushed a commit that referenced this pull request Aug 27, 2020
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
noroadsleft pushed a commit that referenced this pull request Aug 29, 2020
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
nicocesar pushed a commit to nicocesar/qmk_firmware that referenced this pull request Sep 6, 2020
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Consolidate TKC projects and increase VIA keymap count to 4.

* Updated readme files.

* Removed config.h via limitation of 2 dynamic keymaps

* Reduce dynamic keymaps from 4 to 3 due to EEPROM space limitations.

* Update dynamic_keymap.c

* Restore 4 dynamic keymaps for VIA in TKC projects.

* Update quantum/dynamic_keymap.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change Changes that need to wait for a version increment keyboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants