Eddddddddy / Songguo-PTS200

165 stars 37 forks source link

Issue 19 fix - Doesn't save temp settings #25

Open dexter323i opened 10 months ago

dexter323i commented 10 months ago

https://github.com/Eddddddddy/Songguo-PTS200/issues/19

This part causes it at line 612 in Thermostat() function:

if (SetTemp != DefaultTemp) {
    DefaultTemp = SetTemp;  // 把设置里面的默认温度也修改了
    update_default_temp_EEPROM();
}

When user finishes setting up things, and exists to main screen, then updateEEPROM() is called, which saves everything correctly. Even DefaultTemp too! Now normal operation starts, Thermostat() called from loop, but SetTemp won't be equal to DefaultTemp, so DefaultTemp is overwitten with SetTemp, then it is also saved in eeprom. Which is wrong, because it overwrites user's previous DefaultTemp modification.

My fix:

hurynovich commented 10 months ago

IMHO Default Temp menu point should be removed from settings at all. This value is set with + and - buttons in main screen. No sense to duplicate it in menu.

dexter323i commented 10 months ago

I had the same thoughts: Why to give two diffetent options for users to change / save Default temp?

But I think it is better to keep it in menu! Chaging with + and - buttons should be a temporary change. Like you know you usually solder at 280 degrees. So set it up as default. But one day you want to solder some very fine things, so you lower the temp. For that job only. It is a temporary need. Next time when you turn on the iron, then maybe you will do a different job, so you will need the usual 280 again.

hurynovich commented 10 months ago

Makes sense. But in this case I would also added option like Save temp: on/off. Because it is a matter of taste.

hurynovich commented 10 months ago

Any way master branch has a bug, or at least I don't understand idea behind the behavior.

This PR gives better behavior, which is: Keep Set temp on main screen and Default temp in setting menu in sync and save it between restarts.

This PR does not implement:

Chaging with + and - buttons should be a temporary change

Right?

dexter323i commented 10 months ago

This PR does not implement:

Chaging with + and - buttons should be a temporary change

Right?

Right. It is a brand new idea, which is not in the PR yet.