Proper procedure for new settings

[MrMep]

I see that Andrew added this:

https://github.com/amowry/WARBL2/blob/f900aacfb407cc3c845ba6b4295daf6f51a564ce/warbl2_firmware/Functions.ino#L2772-L2774

Still, it's unclear to me:

• What if that EEPROM byte was by chance at value 1 (first run with new firmware) and 1 is not the intended default value?
• What about the Factory Settings?

Shouldn't we catch a first run with a new FW version and adjust the default values for the new preferences (and the related factory settings)? I kind of remember there was something like that in WARBL(1): has it been moved somewhere else?

Thanks, gl

[amowry]

Yes, I've done this different ways, but you're right-- I guess it should check when it re-writes the firmware version and change the EEPROM value to 0 for both the factory setting and current setting. I'm not sure the best way to do that without it getting messy after a number of firmware changes? In the past I've sometimes had a new firmware version completely rewrite settings, but that's no a popular option.

[MrMep]

I think it's a common problem, and that there should be a "migration manager "somewhere at startup. In this case, if the trigger is the new value for Firmware version, we should simply have a manual check for each new release.

For every new setting we should define a default value and write it both in current settings and factory settings. This way, we could also ideally re-write whole pieces of settings and manage the migration without loosing the existing prefs. (And we should do the same with saved settings via Config Tool, but that's for another day ;)

I'll cook up something, if you want. Also, we could "patch" existing factory settings, if needed. For example, this looks fishy:

https://github.com/amowry/WARBL2/blob/21a46a19bbf4467465b5170a09028481bb73d131/warbl2_firmware/warbl2_firmware.ino#L185-L188

the values for previous CUSTOM_FINGERING (that now are unused variables) should be set at zero. With a migration manager we could apply a patch.

What do you think?

[amowry]

Yes, that sounds fine to me. You're welcome to do it if you'd like, or I can before releasing v 4.2. Thanks!

[amowry]

How about something like this for the migration manager? This is called from settings if the version has changed. I'll address the unused variables you mentioned above as well. Or did you have something else in mind?

void manageFirmwareUpdate() {
    switch (VERSION) {
        case 42:
            for (byte i = 0; i < 3; i++) {
                writeEEPROM((EEPROM_SWITCHES_START + i + (BUTTON_DOUBLE_CLICK * 3)), 0);                                  // Initialize button double-click preferences as false (0) for all three modes.
                writeEEPROM((EEPROM_SWITCHES_START + i + (BUTTON_DOUBLE_CLICK * 3)) + EEPROM_FACTORY_SETTINGS_START, 0);  // Initialize factory settings for same.
            }
            break;
        default:
            break;
    }
}

[MrMep]

How about something like this for the migration manager?

It looks good, but I wouldn't presume what the current version is: if someone upgrades, say, from 41 to 44, it has to apply all the patches sequentially. What do you think?

[MrMep]

Here's how I would go:

void checkFirmwareVersion() {
    byte currentVersion =     readEEPROM(EEPROM_FIRMWARE_VERSION);  // Previous firmware version.

    if (VERSION != currentVersion) {

        if (currentVersion < 42) { //Here manage all changes made in version 42

            for (byte i = 0; i < 3; i++) {
                writeEEPROM((EEPROM_SWITCHES_START + i + (BUTTON_DOUBLE_CLICK * 3)), 0);                                  // Initialize button double-click preferences as false (0) for all three modes.
                writeEEPROM((EEPROM_SWITCHES_START + i + (BUTTON_DOUBLE_CLICK * 3)) + EEPROM_FACTORY_SETTINGS_START, 0);  // Initialize factory settings for same.
            }

        }

        writeEEPROM(EEPROM_FIRMWARE_VERSION, VERSION);  // Update the firmware version if it has changed.
    }
}

We will then add an If statement for each future release that requires changes to settings.

[amowry]

Makes sense, thanks! I just pushed this change and a few other fixes.