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

cleanup storage, variables and more #515

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LoQue90
Copy link
Member

@LoQue90 LoQue90 commented Sep 25, 2024

no need to stay compatible to old storage structure anymore -> remove reserved or freetouse

rename a lot of vars to be consistent and follow camelcase

deleted unused coldstart stuff

set PID Mode based on pidPonmOn for regular and brew pid

remove HA Discovery defaults for sensor objects. sensor types are too different than any default type would make sense

to test this PR, esp has to be erased before flash

no need to stay compatible to old storage structure, remove reserved or freetouse
rename a lot of vars
deleted missed coldstart stuff
set PID Mode based on pidPonmOn for regular and brew pid

d
@LoQue90 LoQue90 added the cleanup clean code label Sep 25, 2024
@LoQue90 LoQue90 requested review from kjyv and fiendie September 25, 2024 23:22
@LoQue90 LoQue90 self-assigned this Sep 25, 2024
.minValue = BREW_SETPOINT_MIN,
.maxValue = BREW_SETPOINT_MAX,
.ptr = (void*)&brewSetpoint};
editableVars["TEMP"] = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are some blocks formatted like before (e.g. for PID_PONM_ON above) and some like this inline? Not sure if this still needs to be auto-formatted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have no clue why this happens, but this is already formated. maybe @simonspa has an idea for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the entire thing fits into the total line length defined, it will not be broken up, and otherwise aligned.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so then the line length might be configured to be too long? 220 chars usually doesn't fit on screens ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was someone who wanted it so long, but it doesn't fit on my screen. (4k, only vsc with one side bar).
So I'm fine to break the line earlier.
Reading and editing the editiblevars isn't fun like that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, just "someone" shouldn't define the line lengths for the whole project ;) A more modern number might be 100 or 120, more traditional ones are 88.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80, for traditionalists. (Not counting the no-longer-common line numbers, which could leave you with 72.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm 120 does destroy the layout quit a lot, but formats all editibleVars as we want.
200 is still fine for my screen, editibleVars are still fine, but produce still some mess on other parts

@@ -66,97 +61,68 @@ typedef enum {

// storage data structure
typedef struct __attribute__((packed)) {
// Any 'freeToUse' areas ensure the compatibility to origin EEPROM layout and can be used by new value.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing all this means that saved configurations will not work when updating firmware and Erase Flash needs to be used, correct?

Copy link
Member Author

@LoQue90 LoQue90 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes right.
i did talked about a 4.0 release with fiendie. There is the idea of postpone the new storage/config system to a 4.1 release.
If so, i would move this PR also to a 4.1 release, so there is only one point where all users have to do a flash erase and not two times.

maybe i should take all cleanup stuff into its own PR and wait with the storage.

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

Successfully merging this pull request may close these issues.

4 participants