-
-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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
[Bug] Realign and size check EECONFIG structures #20541
Conversation
Can revert if needed
Co-authored-by: Nick Brassel <nick@tzarc.org>
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.
How does this affect the Input Club boards, if at all? Their EEPROM becomes locked to 32 bytes after flashing QMK for the first time due to misconfiguration in the driver.
A quick look at it would seem to indicate that the val, speed, and flags would get truncated. Would it be worth moving the reserved block? It would mean that the led and rgb matrix would not be as parity like it is now, but it could fit in 32bit structure and would be fully under the 32byte limit, I think. |
You could always reorder and bump haptic to the end, far less users on that feature. |
Haptic isn't used on their boards, as far as I'm aware. So sounds "safe" |
Apparently, because of how the struct was set up for haptic config, the size of the config was actually larger than 32 bits. Seems like AVR-GCC was okay with this, but arm-gcc did not like this. Rearranging struct seems to fix this, and since we're already decrementing the Magic number... |
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.
I'm still not completely convinced that we should waste 4 bytes of EEPROM to store the data that the RGBLIGHT subsystem does not actually use in any way. But if we choose to do it that way, at least we should clean up the misleading comments from that code.
Can definitely understand that. And part of that was me wanting to avoid with bitwise stuff. Reverted some of the reorganization, and split off a block for rgblight speed. Can decide to nuke it or whatnot later. |
Co-authored-by: Nick Brassel <nick@tzarc.org>
Co-authored-by: Nick Brassel <nick@tzarc.org>
Description
RGB/LED matrix eeconfig is overwriting addresses pass 34, which is not good.
Technically, it only takes up 6 bytes, but there is no uint48_t. So for correctness, extends the config to uint64_t to properly fit everything, which requires extending the EECONFIG blocks too.
Types of Changes
Issues Fixed or Closed by This PR
Checklist