Skip to content
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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dexter323i
Copy link

#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:

  • In SetupScreen() function there is no reason to store SetTemp value into SaveSetTemp variable, then in the end write it back. Nothing changes SetTemp within this part.
  • But if user changed DefaultTemp then we have to save it to SetTemp too. Because Thermostat() function will be called, and it would overwrite user's previous setting.

@hurynovich
Copy link

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
Copy link
Author

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
Copy link

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

@hurynovich
Copy link

hurynovich commented Nov 26, 2023

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants