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

*** OLD *** Migrated parameters #2017

Closed
wants to merge 37 commits into from
Closed

*** OLD *** Migrated parameters #2017

wants to merge 37 commits into from

Conversation

caco3
Copy link
Collaborator

@caco3 caco3 commented Feb 12, 2023

replaced by #2023

@Slider0007
Copy link
Collaborator

@caco3, @jomjol: Some more discussion points in terms of config parameters:

main.ExtendedResolution in section [ANALOG]
--> Couldn't find in software

TimeServer = pool.ntp.org
--> I would remove the checkbox, because disabled parameter does not mean that service is disabled. Actual logic only disables timeservice when input field is empty.

Hostname = watermeter
--> I would remove the checkbox and set the default value (watermeter; to be aligned with logic). Disabling the parameter does nothing with actual logic (internally, default name is used anyway).

RSSIThreashold = 0
--> I would remove the checkbox, because function gets disabled with input = 0 and in my opinion there is no benefit to disable parameter additionally. It's hidden in expert anyway.

##############################
Defaults of parameters:
CNNGoodThreshold --> I would disable this by default, because it's experimental and only used for some models.
InitialRotate = 179 -> Default: 0?

main.MaxRateValue = 0.05
;main.MaxRateType = AbsoluteChange
--> MaxRateValue is enabled by default and MaxRateType is disabled? Is this by purpose? (AbsolutChange is default internally)

@Slider0007
Copy link
Collaborator

Slider0007 commented Feb 12, 2023

@caco3, @jomjol: Having an additional parameter adpation logic in firmware we have now two locations were parameters are adapted for migration/compability reason.
Just for my understanding: In future what will be handled in JS (readconfigparam.js) and what handled in directly firmware?

@caco3
Copy link
Collaborator Author

caco3 commented Feb 12, 2023

What do you mean with "additional parameter adpation logic in firmware"? I only added function to migrate the parameters. and it works on the plain text file.
Beside of this, change of boolean parameters and all the renaming, nothing should have changed.

@Slider0007
Copy link
Collaborator

What do you mean with "additional parameter adpation logic in firmware"? I only added function to migrate the parameters. and it works on the plain text file. Beside of this, change of boolean parameters and all the renaming, nothing should have changed.

I mean your migrate function. I was just thinking about this compabilty part in readconfigparam.js were parameters are also e.g. renamed and partly modified due to compability reason.

@caco3
Copy link
Collaborator Author

caco3 commented Feb 12, 2023

What do you mean with "additional parameter adpation logic in firmware"? I only added function to migrate the parameters. and it works on the plain text file. Beside of this, change of boolean parameters and all the renaming, nothing should have changed.

I mean your migrate function. I was just thinking about this compabilty part in readconfigparam.js were parameters are also e.g. renamed and partly modified due to compability reason.

The whole migration happens in the migration function in the firmware.
The rest of the firmware and Web UI only adapt the changed names.
The Web UI does not rename anything!

@Slider0007
Copy link
Collaborator

What do you mean with "additional parameter adpation logic in firmware"? I only added function to migrate the parameters. and it works on the plain text file. Beside of this, change of boolean parameters and all the renaming, nothing should have changed.

I mean your migrate function. I was just thinking about this compabilty part in readconfigparam.js were parameters are also e.g. renamed and partly modified due to compability reason.

The whole migration happens in the migration function in the firmware. The rest of the firmware and Web UI only adapt the changed names. The Web UI does not rename anything!

Here and in the following lines some parameters are "modified" due to compability reason:

// Make the downward compatiblity with MQTT (Maintopic --> topic)

Maybe @jomjol can tell more to this.

@caco3
Copy link
Collaborator Author

caco3 commented Feb 12, 2023

To simplify the PR, I only do the renaming in this PR.
The change of paths and migration of commented out boolean to enabled params with default will be done separately.

@caco3
Copy link
Collaborator Author

caco3 commented Feb 12, 2023

@Slider0007 Thanks for thinking into it!

main.ExtendedResolution in section [ANALOG] --> Couldn't find in software

Removed now (its only used in [PostProcessing])

TimeServer = pool.ntp.org --> I would remove the checkbox, because disabled parameter does not mean that service is disabled. Actual logic only disables timeservice when input field is empty.

Let's do that later to keep it as simple as possible.

Hostname = watermeter --> I would remove the checkbox and set the default value (watermeter; to be aligned with logic). Disabling the parameter does nothing with actual logic (internally, default name is used anyway).

Let's do that later to keep it as simple as possible. Also I would remove water from the default value.

RSSIThreashold = 0 --> I would remove the checkbox, because function gets disabled with input = 0 and in my opinion there is no benefit to disable parameter additionally. It's hidden in expert anyway.

Let's do that later to keep it as simple as possible.

Defaults of parameters: CNNGoodThreshold --> I would disable this by default, because it's experimental and only used for some models. InitialRotate = 179 -> Default: 0?

@jomjol What do you think? The default in the firmware seems to be 0.0!

main.MaxRateValue = 0.05 ;
main.MaxRateType = AbsoluteChange --> MaxRateValue is enabled by default and MaxRateType is disabled? Is this by purpose? (AbsolutChange is default internally)

@jomjol ?

@caco3
Copy link
Collaborator Author

caco3 commented Feb 12, 2023

What do you mean with "additional parameter adpation logic in firmware"? I only added function to migrate the parameters. and it works on the plain text file. Beside of this, change of boolean parameters and all the renaming, nothing should have changed.

I mean your migrate function. I was just thinking about this compabilty part in readconfigparam.js were parameters are also e.g. renamed and partly modified due to compability reason.

The whole migration happens in the migration function in the firmware. The rest of the firmware and Web UI only adapt the changed names. The Web UI does not rename anything!

Here and in the following lines some parameters are "modified" due to compability reason:

// Make the downward compatiblity with MQTT (Maintopic --> topic)

Maybe @jomjol can tell more to this.

That is old code, not sure if we still need it but I would keep it as is for now to keep the PR as simple as possible.

@caco3 caco3 marked this pull request as ready for review February 12, 2023 20:29
@caco3 caco3 changed the base branch from rolling to v14.1 February 12, 2023 20:49
@caco3 caco3 marked this pull request as draft February 12, 2023 20:59
@caco3
Copy link
Collaborator Author

caco3 commented Feb 12, 2023

As far as I tested, it should be complete now.
But I had to create another PR for this as I accidentally included the InfluxDB2 stuff.
How ever I really only want to have the parameter migration contained.

Please review and test #2023

@caco3 caco3 closed this Feb 12, 2023
@caco3 caco3 mentioned this pull request Feb 12, 2023
@caco3 caco3 changed the title Migrated parameters *** OLD *** Migrated parameters Feb 12, 2023
@caco3 caco3 deleted the migrate-parameters branch February 21, 2023 21:24
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.

6 participants