-
-
Notifications
You must be signed in to change notification settings - Fork 40.3k
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
Don't make EEPROM size assumptions with dynamic keymaps. #16054
Conversation
/* These bits are used for optimizing encoding of bytes, 0 and 1 */ | ||
#define FEE_WORD_ENCODING 0x8000 | ||
#define FEE_VALUE_NEXT 0x6000 | ||
#define FEE_VALUE_RESERVED 0x4000 | ||
#define FEE_VALUE_ENCODED 0x2000 | ||
#define FEE_BYTE_RANGE 0x80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These defines are not used in the size calculations below and may stay internal (although their values are related to FEE_ADDRESS_MAX_SIZE
, because that value is determined by the encoding used here, and FEE_ADDRESS_MAX_SIZE
is needed in the size checks).
/* Flash word value after erase */ | ||
#define FEE_EMPTY_WORD ((uint16_t)0xFFFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also does not need to be in the header file.
@sigprof look good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The qmk multibuild
change should be made in a separate PR, but the EEPROM changes look good (hope that they won't break existing boards — now some of them would probably get much more available space for macros).
I've converted to draft while I go through and confirm binary reproducibility. As you've said, some of them have gained support for more space, I want to confirm that this is indeed the only side-effect. |
Looking at diff's between a handful of builds looks very much isolated to sizing changes -- traditionally more eeprom was available with the wear-leveling EEPROM-on-flash implementation but VIA was still artificially limited to 1kB by default. |
Will raise it. |
… attempts to build a board without specifying EEPROM size.
920756a
to
807fc6f
Compare
Description
Individuals have broken their boards by using the default assumption of having 1kB of EEPROM available.
Falling back to the transient EEPROM driver, which defaults to 36 bytes, means enabling VIA is going to attempt to obliterate up to 1kB of RAM past the start of the transient EEPROM array.
This PR fixes the dynamic keymap code such that it actually uses the selected driver's overall limit.
Types of Changes
Checklist