Eeconfg settings need a bit rethinking

[fredizzimo]

I think the current approach to eeconfig settings need a bit rethinking. The reason for bringing this up, is because the Infinity Ergodox will need at least three configurable values "LCD brightness", "LCD Contrast", and "LED Brightness". Some of these settings could use existing slots, while others probably need new allocated slots.

The current approach has four different problems

1. There's no versioning, so as soon as someone adds a new value, or change the meaning of an existing one, it gets corrupted.
2. It's not possible for keyboards and keymaps to add new values.
3. If you start with for example audio disabled, then the audio value will be randomly initialized, once you enable it. This is easy to solve, we just remove the ifdefs from the eeconfig_init function to always initialize them to the correct values.
4. The eeconfig_enable function does not really make sense as it is. We don't currently make any difference between disabled and uninitialized. Since the function seems to be unused, I suggest that we just remove it.

Additionally we have a constraint, the size can not be bigger than 32 bytes(at least not on the Infinity ones). This is a bit unfortunate, but unfortunately when @flabbergast ported TMK to ChibiOS he made the decision to initialize the EEPROM with a maximum size of 32 bytes, and I don't know of any way to change that once it already has been initialized. This means that the versioning by itself can't use too many bytes.

We already have the magic that could be used as a version number. The current value is 0xFEED, which we can define to mean version 0. 0xFEEC or 0xFEED-1 would mean version 1, 0xFEEB version 2 and so on. The reason why we go backwards instead of forwards, is because going forwards we could easily reach the dangerous values of 0xFFFF which means not initialized, and 0x0000, which could be the default value. So by going backwards we have a lot of versions to work with until we have issues.

In addition to the normal eeconfig_init, which always initializes the latest version, we should also define eeconfig_upgrade(prevVersion, currentVersion, uint8_t* prevBuffer, uint8_t* newBuffer) which will be called for all intermediate versions. So if the current version is 1 and the latest version is 3, we would call, obviously by an algorithm.

uint8_t buffer1[EEPROM_MAX_SIZE];
eeprom_read_block(EECONFIG_MAGIC, buffer1, EEPROM_MAX_SIZE);
read_eeprom_to_ram(buffer1);
uint8_t buffer2[EEPROM_MAX_SIZE]; 
eeconfig_upgrade(1, 2, buffer1, buffer2); 
eeconfig_upgrade(2, 3 buffer2, buffer1);
eeprom_write_block(buffer1, EECONFIG_MAGIC, EEPROM_MAX_SIZE);

We then implement the upgrade for all versions that we want to support. At some point we might want to disable support for old version, and in that case we can just initialize the whole thing with the newest version. Also note that the upgrade function can't use any defines to identify slot positions, it has to use plain numbers, or preferably defines like EECONFIG_MAGICv2

The EEPROM_MAX_SIZE could be bigger than 32 on keyboards that support them, but we should make sure we allocate all important slots at low addresses.

Rather than letting the keyboards and keymaps allocate the slots, I suggest that we have some informal "application" process, where we discuss what slots should be used and together make sure that we have a new version to initialize the slots, and that ids are properly defined.

Note that the upgrade mechanism is quite flexible and let us move slots around and change their sizes, so we don't need to be overly careful.

The difficulty comes if we start to run out of slots. I guess we could start to have multipurpose slots, but they might need different initial values. So that could be handled by keyboard and keymap specific defines.

A more lightweight version of the versioning would be to just reinitialize the EEPROM each time the version is changed.

What do you think?

[jackhumbert]

Nice. Agreed, and this sounds good! To figure out which options are enabled/if the current configuration is compatible with the data, we could do something like a checksum of all the available options. It could check that in addition to the version before taking action. The alternative to that is just force all features into the EEPROM storage at certain addresses, rather than omitting them when disabled (like audio right now).

I'm in favor of the latter (like you mentioned with removing the ifdefs), but for things like storing keymaps in the EEPROM, we may find that some features need to be disabled in order to fit them all in there. With keymaps specifically, there are different sized boards, and the possibility of more than one layer.

[belak]

I can only speak from a keymap perspective so far, but a way to do this for keymaps would be really nice... The only way I could currently see to do it is by reimplementing the magic check at the keyboard and keymap levels but this obviously isn't optimal. I also had to rely on a static offset (which looks like it isn't even a valid offset on infinity boards) so either a way to hook into the system or a way to know where the user section starts would be nice

[belak]

It looks like the other issue on a user data offset was closed, so my request for that can go here, I suppose.

Having a user offset is perhaps one of the most important parts of this so users can use the EEPROM without trampling all over the data used by eeconfig.

Having a bunch of additional eeconfig options would be nice but isn't completely needed for a user to use the eeprom. Not to mention that if you currently want to do anything outside what eeconfig already supports, you need to directly use the eeprom functions.

[jackhumbert]

I started messing around with this on the eeprom_update branch.

[3tilley]

Hello, was there any consensus on this? I'd like to store a few bytes in the EEPROM, but I have no idea where it's safe to do so, nor which files edit it. I could expand out macros and follow all the write functions, but it would be nice if it was stable between configuration.

Apologies if this is documented somewhere and I just didn't look hard enough.

[drashna]

I've also opened a PR that would address point 2: #3084

@3tilley my userspace has a good example of how to do that.
https://github.com/qmk/qmk_firmware/tree/master/users/drashna#userspace-eeprom-config

And I recommend something like 19-20, as that is a block that isn't too high, and shouldn't be used.

[3tilley]

Thanks a lot, I appreciate your help. I'll give that a go.

[drashna]

2 and 3 have been addressed, actually.

However, the rest still need to be addressed.

[stale]

This issue has been automatically marked as resolved because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.

[stale]

This issue has been automatically closed because it has not had activity in the last 30 days. If this issue is still valid, re-open the issue and let us know.