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

Migrate parameters #2023

Merged
merged 21 commits into from
Feb 21, 2023
Merged

Migrate parameters #2023

merged 21 commits into from
Feb 21, 2023

Conversation

caco3
Copy link
Collaborator

@caco3 caco3 commented Feb 12, 2023

This PR adds a migration function of some parameters:

Section Old Name New Name Note
MakeImage - - Renamed section from MakeImage to TakeImage
TakeImage LogImageLocation RawImagesLocation Renamed Parameter
LogfileRetentionInDays RawImagesRetention Renamed Parameter
Demo - If disabled, set to default value, enable it
FixedExposure - If disabled, set to default value, enable it
InitialMirror - If disabled, set to default value, enable it
FlipImageSize - If disabled, set to default value, enable it
Digits LogImageLocation ROIImagesLocation Renamed Parameter
LogfileRetentionInDays ROIImagesRetention Renamed Parameter
Analog LogImageLocation ROIImagesLocation Renamed Parameter
LogfileRetentionInDays ROIImagesRetention Renamed Parameter
ExtendedResolution Removed as no longer used (use <NUMBER>.ExtendedResolution instead)
PostProcessing PreValueUse - If disabled, set to default value, enable it
<NUMBER>.ExtendedResolution - If disabled, set to default value, enable it
<NUMBER>.IgnoreLeadingNaN - If disabled, set to default value, enable it
AllowNegativeRates <NUMBER>.AllowNegativeRates If disabled, set to default value, enable it
N.B. the <NUMBER> prefix was missing in the default config
ErrorMessage - If disabled, set to default value, enable it
MQTT SetRetainFlag RetainMessages If disabled, set to default value, enable it
Topic MainTopic Renamed Parameter
InfluxDB DataLogActive - If disabled, set to default value, enable it
DataLogging DataLogRetentionInDays DataFilesRetention Renamed Parameter
AutoTimer Intervall Interval Renamed Parameter
AutoStart - If disabled, set to default value, enable it
Debug Logfile LogLevel And switch form boolean True/False to WARNING
LogfileRetentionInDays LogfilesRetention If disabled, set to default value, enable it
System RSSIThreashold RSSIThreshold Renamed Parameter
AutoAdjustSummertime - Removed, no longer in use

Also it switches all disabled boolean parameters to their default value . Then enable them.

The corresponding parts in the firmware and Web UI got also updated accordingly.

The migration function gets called at startup and modifies the config file as needed. If one or more parameter needs a migration, it stores the config.ini as config.bak before any change to it.
The migration has some simplifications:

  • Case Sensitiveness must be like in the initial config.ini
  • No Whitespace after a semicollon
  • Only one whitespace before/after the equal sign

And all changes are documented in the log file (as ERROR so they show up for sure):


[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line '[MakeImage]' to '[TakeImage]'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';LogImageLocation = /log/source' to ';CamImagesLocation = /log/source'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';LogfileRetentionInDays = 15' to ';CamImagesRetention = 15'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';LogImageLocation = /log/digit' to ';ROIImagesLocation = /log/digit'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';LogfileRetentionInDays = 3' to ';ROIImagesRetention = 3'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';[Analog]' to '[Analog]'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';LogImageLocation = /log/analog' to ';ROIImagesLocation = /log/analog'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';LogfileRetentionInDays = 3' to ';ROIImagesRetention = 3'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';SetRetainFlag = true' to ';RetainMessages = true'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';[InfluxDB]' to '[InfluxDB]'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';[GPIO]' to '[GPIO]'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line 'Intervall = 2' to 'Interval = 2'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line 'DataLogRetentionInDays = 3' to 'DataFilesRetention = 3'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line 'Logfile = 3' to 'LogLevel = 3'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line 'LogfileRetentionInDays = 30' to 'LogfilesRetention = 30'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Migrated Configfile line ';AutoAdjustSummertime = false' to ';;UNUSED_PARAMETER = false'
[0d00h00m04s] 2023-02-12T18:41:30 <ERR> [MAIN] Config file migrated. Saved backup to /sdcard/config/config.bak

See also discussions in:

@caco3 caco3 marked this pull request as ready for review February 12, 2023 21:23
@caco3 caco3 changed the title Migrate parameters to v14.1 branch *** OLD *** Migrate parameters to v14.1 branch Feb 12, 2023
@caco3 caco3 changed the title *** OLD *** Migrate parameters to v14.1 branch Migrate parameters to v14.1 branch Feb 12, 2023
@jomjol
Copy link
Owner

jomjol commented Feb 13, 2023

Started to test and found the following issue with removing the Enableling Checkbox for true/false values.

;main.IgnoreLeadingNaN = false
;ErrorMessage = true

In both cases the leading ";" should be removed. The second one is easy in the migration function:
migrated = migrated | replace(configLines[i], ";ErrorMessage", "ErrorMessage");

The first one is a problem, as the name of the number is not known!

We need to solve this.

@Slider0007
Copy link
Collaborator

Slider0007 commented Feb 13, 2023

@caco3: I did a quick test. Renaming works like described :-)

Some first comments / remarks:

  • Saving CamImages, digit ROIImages and analog ROIImages not working anymore. Files are not stored when activating respective functions (I haven't checked retention).

  • Set Retain Flag keeps greyed out if selected + text in UI needs to be adapted to new name RetainMessages
    image

  • Name Logfile Log Level differs from parameter name LogLevel, by purpose?

  • CamImages... --> maybe RawImages...?

  • LogfilesRetention / DataFilesRetention --> Align ...filesRetention or ...FilesRetention?

@Slider0007
Copy link
Collaborator

Slider0007 commented Feb 13, 2023

  • HomeAssistant keeps unchecked after saving...

image

@caco3: I'm a little confused. Is the removing of the checkboxes for boolean parameters already included in this PR or will this be done with a separate PR?

@jomjol
Copy link
Owner

jomjol commented Feb 13, 2023

I have removed the checkboxes (again? - thought I already did it). Additionally I have updated the migrate funtion to remove the ";" for the simple parameters (without a number).
For the one with a number (e.g. HT.AllowNegativeRates = true) we still need to extend the migration function.

@caco3
Copy link
Collaborator Author

caco3 commented Feb 13, 2023

I am sorry for he confusion I caused about the boolean parameters.
I wanted to keep this PR as simple as possible and thus removed the checkbox-removal again. But I think it just caused extra work and we can also add it within this PR.

I will extend the migration function to also support <NUMBER>.param functions later this evening.

The naming (labels) in the UI is quite random, i wanted to clean that up later, eg. when I add the tooltips again.

code/main/main.cpp Outdated Show resolved Hide resolved
CaCO3 added 3 commits February 14, 2023 00:16
Removed checkbox in UI for ErrorMessage
Added migration of pboolean parameters: enable them if they where disabled, set them to their default value, then enable them
Switch SetRetainFlag internally to a boolean
@caco3
Copy link
Collaborator Author

caco3 commented Feb 13, 2023

* `CamImages...` --> maybe `RawImages...`?

Done

* `LogfilesRetention` / `DataFilesRetention` --> Align ...filesRetention or ...FilesRetention?

I wanted to do as suggested, but then i learned that there is a common class for all them, so I wanted to keep it consistent.

@jomjol
Copy link
Owner

jomjol commented Feb 16, 2023

Is everything done and we can merge it?

@caco3
Copy link
Collaborator Author

caco3 commented Feb 19, 2023

Saving CamImages, digit ROIImages and analog ROIImages not working anymore. Files are not stored when activating respective functions (I haven't checked retention).
  • I can confirm this, but do not understand the reason yet.

@caco3
Copy link
Collaborator Author

caco3 commented Feb 19, 2023

Saving CamImages, digit ROIImages and analog ROIImages is working now.
and MQTT retain as well.

@caco3
Copy link
Collaborator Author

caco3 commented Feb 19, 2023

@Slider0007 @jomjol I hope I have now everything. Please give it another try.

@jomjol
Copy link
Owner

jomjol commented Feb 20, 2023

@Slider0007 @jomjol I hope I have now everything. Please give it another try.

I cannot test before the weekend.

@Slider0007
Copy link
Collaborator

@caco3: I did a quick test of UI config page.

  • Extended resolution is greyed out
    image

  • Could the checkboxes of those parameters also get removed?
    image
    image

Does it make sense to add an indication that this config.ini is a migrated one or is it clear due to the new names? (config.ini - firmware version > 14.1).
I was just thinking about the fact when somebody goes back to an older version... (I tried it, system is behaving wierd)

I'll do further testing tomorrow...

@caco3
Copy link
Collaborator Author

caco3 commented Feb 20, 2023

* Extended resolution is greyed out
  ![image](https://user-images.githubusercontent.com/115730895/220187820-70b8d09a-d8f3-45b8-ba54-b1acf1efb8c9.png)

Oh bugger, somehow I missed that, thanks!

* Could the checkboxes of those parameters also get removed?
  ![image](https://user-images.githubusercontent.com/115730895/220187602-5746b5ac-163e-41d6-a4bf-bae30ae7e453.png)
  ![image](https://user-images.githubusercontent.com/115730895/220187722-1b8131e3-98e5-4742-be52-cd95ce85ef60.png)

That question leads to the question if a parameter should be able to be disabled at all.
Personally I think the way the configuration is setup is highly overcomplicated. A parameter should either be set, or use its default if it is missing. Here we have some tri-state situation.
But I do not want to dig into it more and break it further.
My main goal was to rename them as needed so we can properly document them.
We can think of removing all those checkboxes for all parameters, but lets do it at a later time.

Does it make sense to add an indication that this config.ini is a migrated one or is it clear due to the new names? (config.ini - firmware version > 14.1). I was just thinking about the fact when somebody goes back to an older version... (I tried it, system is behaving wierd)

Based on the naming it should be clear.
But I did not plan to provide downgrade support below 14.1.
Thats actually the main reason I want it in 14.1. 14.0 is quite stable as it looks, and in 14.x we will change a lot again. so people can go back to the 14.1 release without issues.
Notabene this will need to be state clearly in the release notes.
And like we already always say, a backup is highly recommended. Additionally before a migration it does a backup.

I'll do further testing tomorrow...

Thank you!

@Slider0007
Copy link
Collaborator

That question leads to the question if a parameter should be able to be disabled at all.
Personally I think the way the configuration is setup is highly overcomplicated. A parameter should either be set, or use its default if it is missing. Here we have some tri-state situation.
But I do not want to dig into it more and break it further.
My main goal was to rename them as needed so we can properly document them.
We can think of removing all those checkboxes for all parameters, but lets do it at a later time.

That's true, so it's totally fine for me 👍

@Slider0007
Copy link
Collaborator

@caco3: From my prespective everthing seems OK so far :-)

Only one minor: ClassFlowMakeImage.h and ClassFlowMakeImage.cpp are now empty in project. Can they be deleted?

@caco3
Copy link
Collaborator Author

caco3 commented Feb 21, 2023

@caco3: From my prespective everthing seems OK so far :-)

Thanks!

Only one minor: ClassFlowMakeImage.h and ClassFlowMakeImage.cpp are now empty in project. Can they be deleted?

Right, they got re-added by accident.

@jomjol I will merge now and rebase rolling to it so I can proceed with #1986

If you find some issues, we can add another PR to the 14.1 branch

@caco3 caco3 merged commit e1d3920 into v14.1 Feb 21, 2023
@caco3 caco3 deleted the migrate-parameters2-to-v14.1-branch branch February 21, 2023 21:24
caco3 pushed a commit that referenced this pull request Feb 21, 2023
caco3 added a commit that referenced this pull request Feb 21, 2023
* Migrate parameters to v14.1 branch (#2023)

* Migrated parameters

* -

* .

* .

* .

* .

* .

* Remove unneeded checkboxes for true/false

* Remove ";"

* Correct MaintTopic

* Added missing parameters to UI: FlipImageSize, InitialMirror
Removed checkbox in UI for ErrorMessage
Added migration of pboolean parameters: enable them if they where disabled, set them to their default value, then enable them
Switch SetRetainFlag internally to a boolean

* .

* CamImages -> RawImages

* CamImages -> RawImages

* catch error on unknown parameter

* fix missing case insensitivity

* fix typo

* fixmissing rename

* fix migration of ExtendedResolution

* Delete ClassFlowMakeImage.cpp

* Delete ClassFlowMakeImage.h

---------

Co-authored-by: CaCO3 <[email protected]>
Co-authored-by: jomjol <[email protected]>

* Update Changelog.md

---------

Co-authored-by: CaCO3 <[email protected]>
Co-authored-by: jomjol <[email protected]>
@Slider0007
Copy link
Collaborator

@caco3: Using the latest rolling I figured out two more things which needs to be adressed:

@caco3
Copy link
Collaborator Author

caco3 commented Feb 22, 2023

AllowNegativeRates

hmm, kannst du das reproduzieren?

;Topic =

Ah, now I understand.
If I understand @jomjol correctly, Topic was used a long time ago but is no longer used. not sure why you ended up to have this line

@jomjol
Copy link
Owner

jomjol commented Feb 22, 2023

It is still in the config.ini because due to downwards compatibility is was still as a parameter in the readconfigparam.js and every parameter in there get written back to the config.ini once it is saved again. Solved in the latest rolling

caco3 added a commit that referenced this pull request Feb 22, 2023
* Migrated parameters, see #2023

* remove no longer used "topic" parameter. This is a backport from b21e3c6

* Fix wrong url-encoding, see #2036 resp. #2036

* Threashold -> Threshold

* updated changelog

---------

Co-authored-by: CaCO3 <[email protected]>
@caco3 caco3 changed the title Migrate parameters to v14.1 branch Migrate parameters Feb 22, 2023
caco3 added a commit that referenced this pull request Feb 24, 2023
* Replace deprecated actions (#2016)

* Update build.yaml

* Update build.yaml

* Update manual-update-webinstaller.yml

* Update manual-update-webinstaller.yml

* Update manual-update-webinstaller.yml

* Update manual-update-webinstaller.yml

* Update build.yaml

* preparations for v15.0 (#2063)

* Migrated parameters, see #2023

* remove no longer used "topic" parameter. This is a backport from b21e3c6

* Fix wrong url-encoding, see #2036 resp. #2036

* Threashold -> Threshold

* updated changelog

---------

Co-authored-by: CaCO3 <[email protected]>

* DataLogActive is true by default

* updated changelog

---------

Co-authored-by: CaCO3 <[email protected]>
jomjol added a commit that referenced this pull request Mar 1, 2023
* Replace deprecated actions (#2016)

* Update build.yaml

* Update build.yaml

* Update manual-update-webinstaller.yml

* Update manual-update-webinstaller.yml

* Update manual-update-webinstaller.yml

* Update manual-update-webinstaller.yml

* Update build.yaml

* preparations for v15.0 (#2063)

* Migrated parameters, see #2023

* remove no longer used "topic" parameter. This is a backport from b21e3c6

* Fix wrong url-encoding, see #2036 resp. #2036

* Threashold -> Threshold

* updated changelog

---------

Co-authored-by: CaCO3 <[email protected]>

* DataLogActive is true by default

* updated changelog

* Bugfix #1933 (again :-))

* Update Influxdb

* re-add missing dropdownbox filling for Postprocessing Individual Parameters

* stop auto filling the release notes, it causes more confusion than it helps

* Update Changelog.md

---------

Co-authored-by: CaCO3 <[email protected]>
Co-authored-by: jomjol <[email protected]>
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.

3 participants