Add Log File Configuration from INI

[oscarfonloz]

• [x] This contribution adheres to CONTRIBUTING.md.

TODO: Check the above box with an 'x' indicating you've read and followed CONTRIBUTING.md.

What does this Pull Request accomplish?

Add the ability to configure the default log files from an INI configuration.

Why should this Pull Request be merged?

It also allows for additional extensibility, because the configuration files can be used in multiple creation nodes

What testing has been done?

Manual testing both with and without the file

[oscarfonloz]

@debryant let me know what you think, especially about the file name. I am using ~Configuration.ini~ Embedded Data Logger.ini, but we can move it to something else if it makes more sense.

[oscarfonloz]

@rtzoeller I realize this code change might be a bit of an overkill because I am introducing a function that can parse multiple properties of different types. I see the value on it if we want to expand the default configuration capability into default log file names, segmentation periods, etc

[niveristand-diff-bot]

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

Embedded Data Logger System Explorer.lvlib--Add Log File RTM.vi.png


Embedded Data Logger System Explorer.lvlib--Load Default Properties from Configuration File.vi.png


Embedded Data Logger System Explorer.lvlib--Main Page.vi.png


Embedded Data Logger System Explorer.lvlib--New Log File Configuration.vi.png


[oscarfonloz]

Lastly, this is how the INI file looks like... I am unsure where we want to put it in this repo as an example (in case we want to)

[New Log File] Archive Files = True Segment Files = True Append Waveform Properties = True

I also realized I have a typo in "Waveform", so a bunch of VIs changed. I'm sorry!!!!

[niveristand-diff-bot]

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

Embedded Data Logger Engine.lvlib--Get Data Log File Settings.vi.png


Embedded Data Logger Shared.lvlib--Constants.vi.png


Embedded Data Logger System Explorer.lvlib--Add Log File RTM.vi.png


Embedded Data Logger System Explorer.lvlib--File Page.vi.png


Embedded Data Logger System Explorer.lvlib--Load Default Properties from Configuration File.vi.png


Embedded Data Logger System Explorer.lvlib--Main Page.vi.png


Embedded Data Logger System Explorer.lvlib--New Log File Configuration.vi.png


[debryant]

@debryant let me know what you think, especially about the file name. I am using Configuration.ini, but we can move it to something else if it makes more sense.

Maybe "Settings.ini" or "Embedded Data Logger.ini"?

[rtzoeller]

My feedback is largely centered around Load Properties from Configuration File.vi.

• [ ] The path to the configuration file should be a parameter, not hard-coded in this VI. By taking in a path to the configuration file, we can unit test this VI much more easily. A helper VI can be created to compute the default path, to be passed as an input.

• [ ] Instead of checking for the configuration file's existence, we can unconditionally call Open Config Data.vi and then use the error to indicate if the file was opened successfully. This removes a potential race condition.

• [ ] The name is a bit confusing to me. The VI does two things, but the name only indicates it does one (reading config file, setting properties on node). Incorporating the above feedback, a better name might be Apply Properties from Configuration File.vi.

• [ ] Something in the INI file should indicate that these keys are default values, not overrides. That can either be the filename or the section name. It's a bit ambiguous what these keys actually do if you are just looking at the INI file.

[oscarfonloz]

My feedback is largely centered around Load Properties from Configuration File.vi.

• [x] The path to the configuration file should be a parameter, not hard-coded in this VI. By taking in a path to the configuration file, we can unit test this VI much more easily. A helper VI can be created to compute the default path, to be passed as an input.

Agreed. I added the control and created a subVI that handles the file path.

• [x] Instead of checking for the configuration file's existence, we can unconditionally call Open Config Data.vi and then use the error to indicate if the file was opened successfully. This removes a potential race condition.

I am using now an error case structure to proceed, and I filter error 7 to ensure that a missing file will not make the creation system throw an error.

• [x] The name is a bit confusing to me. The VI does two things, but the name only indicates it does one (reading config file, setting properties on node). Incorporating the above feedback, a better name might be Apply Properties from Configuration File.vi.

I agree with your name suggestion.

• [x] Something in the INI file should indicate that these keys are default values, not overrides. That can either be the filename or the section name. It's a bit ambiguous what these keys actually do if you are just looking at the INI file.

I added "Defaults" to the section name to ensure this is clear.

[niveristand-diff-bot]

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

Embedded Data Logger Engine.lvlib--Get Data Log File Settings.vi.png


Embedded Data Logger Shared.lvlib--Constants.vi.png


Embedded Data Logger System Explorer.lvlib--Add Log File RTM.vi.png


Embedded Data Logger System Explorer.lvlib--Apply Properties from Configuration File.vi.png


Embedded Data Logger System Explorer.lvlib--File Page.vi.png


Embedded Data Logger System Explorer.lvlib--Get Configuration File Path.vi.png


Embedded Data Logger System Explorer.lvlib--Main Page.vi.png


Embedded Data Logger System Explorer.lvlib--New Log File Configuration.vi.png


[rtzoeller]

I'd prefer if we didn't discard errors from the INI file entirely, as this could potentially mask unexpected errors. Something like this should be sufficient:

Otherwise the changes look good.

[niveristand-diff-bot]

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

Embedded Data Logger Engine.lvlib--Get Data Log File Settings.vi.png


Embedded Data Logger Shared.lvlib--Constants.vi.png


Embedded Data Logger System Explorer.lvlib--Add Log File RTM.vi.png


Embedded Data Logger System Explorer.lvlib--Apply Properties from Configuration File.vi.png


Embedded Data Logger System Explorer.lvlib--File Page.vi.png


Embedded Data Logger System Explorer.lvlib--Get Configuration File Path.vi.png


Embedded Data Logger System Explorer.lvlib--Main Page.vi.png


Embedded Data Logger System Explorer.lvlib--New Log File Configuration.vi.png


[oscarfonloz]

I'd prefer if we didn't discard errors from the INI file entirely, as this could potentially mask unexpected errors. Something like this should be sufficient:

Fixed :)