hd-zero / hdzero-goggle

MIT License
261 stars 76 forks source link

Fix warning and offset voltage corruption after fw update or downgrade (#466 fix) #458

Closed Combinacijus closed 2 days ago

Combinacijus commented 5 days ago

Fixes #446 settings corruption bug.
I tested upgrade and downgrade both on emulator and goggles successfully Now after firmware upgrade Warning Cell Voltage and Voltage Calibration will be reinitialized to default values and saved as calibration_offset_mv and voltage_mv in setting.ini.

From now on setting.ini will contain both new and old settings: image

Should be good to merge

Master92 commented 5 days ago

Couldn't you implement some sort of migration so that when I upgrade, I don't have to re-set my limits?

if (ini_haskey("power", "calibration_offset", SETTING_INI)) {
    const long oldSetting = ini_getl("power", "calibration_offset", g_setting_defaults.power.calibration_offset, SETTING_INI);
    ini_puts("power", "calibration_offset", NULL, SETTING_INI); // Deletes the key/value from the file as per documentation 
    ini_puts("power", "calibration_offset_mv", oldSetting * 100, SETTING_INI); // I'm unsure about the factor, adjust as needed
}

and the same for voltage.

Combinacijus commented 5 days ago

@Master92 I could but I I'm not sure if it's ok to write migration code just for a single firmware upgrade which will be kept for all subsequent versions because you can't know from which old version user will upgrade Also it will use default values when downgrading and same default when upgrading again if no extra code is added to check for already existing *_mv setting

But I also see that such migration code will save all the users hassle of setup

@ligenxxxx what's your thoughts on this? Is such migration code ok? If so I can make a new commit

ligenxxxx commented 4 days ago

It is best not to do any migration, the simpler the better. We just need to make sure that both upgrades and downgrades are OK.

Master92 commented 4 days ago

It is best not to do any migration, the simpler the better. We just need to make sure that both upgrades and downgrades are OK.

Okay I respect your decision. But please add a remark to the next release that users have to re-configure that setting after an upgrade.

ligenxxxx commented 2 days ago

Okay I respect your decision. But please add a remark to the next release that users have to re-configure that setting after an upgrade.

Understand, this setting becomes the default value after the upgrade.