-
Notifications
You must be signed in to change notification settings - Fork 79
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 SmartEEPROM Support #49
Conversation
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 looks better than the original PR (in particular, there are less unexplained magic numbers in the code). There still might be some things that could be improved.
mdloader_common.c
Outdated
{ | ||
if (!write_word(NVMCTRL_USER + i * 4, data[i])) | ||
{ | ||
printf("Warning: Unable to write NVMCTRL_USER page %i\n", i); |
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 should be a fatal error, otherwise the subsequent code could write some random data to the user row.
printf("Warning: Unable to write NVMCTRL_USER page %i\n", i); | |
printf("Error: Unable to write NVMCTRL_USER page %i\n", i); | |
return 0; |
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.
Changed code to treat this as a fatal error.
mdloader_common.c
Outdated
puser_row1->bit.SBLK = 0x2; // 2 blocks | ||
puser_row1->bit.PSZ = 0x1; // 8 bytes |
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.
According to the Table 25-6 from the datasheet, this configuration is sub-optimal — some part of flash memory which is allocated for SmartEEPROM would always remain unused (apparently the maximum number of SEEP per SEES is limited to 144, even though that fact is not explicitly documented in the datasheet). However, increasing PSZ would mean that small writes to the QMK settings part of EEPROM would result in more flash writes, because every 1→0 bit change would result in allocating a larger page. One option is to decrease SBLK to 1, which apparently would result in the same wear leveling behavior while occupying less flash size.
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.
Okay, so I re-read the code and the manual regarding this. I'm setting it now to:
puser_row1->bit.SBLK = 0x1; // 1 block
puser_row1->bit.PSZ = 0x1; // 8 bytes
According to table 25-6, this will set the virtual size to 1024 bytes, in the optimal size.
Looking at qmk/qmk_firmware#6068, it seems it checks to see if either SBLK
or PSZ
is 0 - and if any of them are, disable the functionality.
Looking further into it, it seems the user configuration just requires 36 bytes for storage, so this could probably be set to:
puser_row1->bit.SBLK = 0x1; // 1 block
puser_row1->bit.PSZ = 0x0; // 4 bytes
Yielding a 512 bytes storage, in optimum setting - but this would require an update to the QMK PR.
For the meantime, I'm changing to code to the first option, but if you think we should push this further to the 2nd option, I can do the change - just waiting on your feedback.
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.
One of important users of EEPROM is VIA, and 1024 bytes is a commonly used EEPROM size (e.g., the ATmega32U4 chip (probably the most popular MCU for custom keyboards) has 1024 bytes of EEPROM). 1024 bytes is usually enough to store 4 keymap layers for a TKL keyboard, and going below that would be problematic. As for going to 2048 bytes of EEPROM instead, I'm not really sure — this would allow fitting more VIA keymap layers while occupying the same amount of flash as SBLK=2, PSZ=1, but maybe page rewrites for the settings part would happen more often because of larger page size. However, actually using the extra EEPROM space would require some changes in the QMK code (the current PR does not even try to support VIA, providing support just for a minimal amount of EEPROM required for the QMK core).
And actually the table gives 512 bytes as an optimum size for SBLK=1, PSZ=0 only because there is no way to go any lower than SBLK=1 — some flash space is still wasted in this configuration, so there is probably no point in going that low.
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.
@sigprof Fair enough, looks good for this PR, then.
Still, I didn't know about VIA existence (this one, right?), so might be a good idea to plan ahead for it.
Where can I get more info about VIA and/or how to future proof this PR to support it?
I can happily create a new PR in qmk repo to support it, if you give me some advice on how to proceed :)
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.
From the EEPROM side of things VIA just needs a large enough EEPROM (1024 bytes is enough, although 2048 might be better for a layout larger than TKL) with fast enough read access (every keymap lookup generates several EEPROM reads; there is no RAM cache, because AVR chips might not have enough RAM for that). Apparently SmartEEPROM already provides a RAM cache for the emulated EEPROM, therefore the read access time should not be a problem.
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.
@TheZoc @quaddy I kinda got VIA working on the CTRL thanks to all of the contributors awesome work using the following steps:
(moved complete step-by-step "guide" to #49 (comment) for readability, hadn't figured out the layout for VIA at this point)
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.
Now I remember that I spent a significant amount of time creating the layout file.
You can find the row column in the source code, the keymaps is organized as ROW, COL so it is a matter of creating the correct layout and filling the row, column accordingly
[0] = LAYOUT(
COL0, COL1 etc...
| |
ROW0 => KC_ESC, KC_F1, KC_F2, KC_F3, KC_F4, KC_F5, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_F11, KC_F12, KC_PSCR, KC_SLCK, KC_PAUS,
ROW1 => KC_GRV, KC_1, KC_2, KC_3, KC_4, KC_5, KC_6, KC_7, KC_8, KC_9, KC_0, KC_MINS, KC_EQL, KC_BSPC, KC_INS, KC_HOME, KC_PGUP,
ROW2 => KC_TAB, KC_Q, KC_W, KC_E, KC_R, KC_T, KC_Y, KC_U, KC_I, KC_O, KC_P, KC_LBRC, KC_RBRC, KC_BSLS, KC_DEL, KC_END, KC_PGDN,
ROW3 => KC_CAPS, KC_A, KC_S, KC_D, KC_F, KC_G, KC_H, KC_J, KC_K, KC_L, KC_SCLN, KC_QUOT, KC_ENT,
ROW4 => KC_LSFT, KC_Z, KC_X, KC_C, KC_V, KC_B, KC_N, KC_M, KC_COMM, KC_DOT, KC_SLSH, KC_RSFT, KC_UP,
ROW5 => KC_LCTL, KC_LGUI, KC_LALT, KC_SPC, KC_RALT, MO(1), KC_APP, KC_RCTL, KC_LEFT, KC_DOWN, KC_RGHT
),
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.
@daltona thats what I was first going to do as well, however according to https://github.com/qmk/qmk_firmware/blob/master/keyboards/massdrop/ctrl/config.h#L31-L33 we have 11 rows and 8 cols - I guess this means the hardware has it flipped around?
The second problem is, the keymap as it is in the code has a maximum of 6 rows and 17 cols, which doesn't fit the given rows/cols.
I guess https://github.com/qmk/qmk_firmware/blob/master/keyboards/massdrop/ctrl/ctrl.h#L12-L32 is the actual layout of the rows/cols, I'll try using that as a reference and add some trial and error on top.
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.
@kaeltis You're right sorry for the confusion, in fact the 6 rows that I believe existed are in fact split.
to get the matrix layout you should replace all Kxx in the layout view ( the parameters of the macro) with the position in the matrix of the corresponding keycode:
0,0 0,1 0,2 0,3 0,4 0,5 0,6 0,7 6,0 6,1 6,2 6,3 6,4 6,5 6,6
6,7 1,0 1,1 1,2 1,3 1,4 1,5 1,6 1,7 7,0 7,1 7,2 7,3 7,4 7,5
etc...
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.
@daltona thanks! With your help I figured it out :)
Here's the Massdrop CTRL ctrl.json
for VIA:
https://gist.github.com/kaeltis/c81ffbe79d7e8a92218dd8a610db39b7
Here's the layout on KLE if anyone needs it:
http://www.keyboard-layout-editor.com/#/gists/df6c13bb95885418bf5c6c8f280e93aa
Sadly I cannot help with this PR because I'm not into C++. But from a users perspective I'm hoping to see this feature soon on my CTRL! 🚀 |
c70bae9
to
f973cea
Compare
@sigprof Went through all your suggestions and added all them. Thank you very much! I did change the
If anyone used this PR before and want to improve the memory usage/wear level, just need to run like I did with the parameters (Also, if you're wondering how I did that on Windows, check #50 ) |
I would love creating the VIA config file for CTRL, but i do not have a CTRL keyboard to test with ;) |
I got VIA working on the CTRL thanks to all of the contributors awesome work using the following steps: Patch & Build mdloader
Patch & Build QMK
Use VIA
After that you should be able to reassign keys in VIA, lighting currently does not work due to the custom implementation (afaik). For reference, here's the layout on KLE if anyone needs it: |
Small update:
|
Closed in favor of #62 |
This is a rewrite of #16 to be used with qmk/qmk_firmware#6068, as a cleanup and an easier to read version, after looking up a bunch of stuff in the manual.
I fixed a couple issues while doing this:
0x41004000
(i.e. both CTRLA and CTRLB data), and toggling off0xf0
, which would change the CTRLB data.Also, it's not more verbose, to aid the user pinpoint if anything go wrong - as previously it would silently fail - and it will show a success message if everything works correctly.
As a last addition, I added support to
--restart
, as it was requested in the other PR.Please note that all credits of this work should be attributed to @daltona, as he was the one that made this possible :)