Fix #23: Enforce minimum file span size and time

ni / niveristand-embedded-data-logger-custom-device

VeriStand embedded data logger custom device

MIT License

3 stars 10 forks source link

Fix #23: Enforce minimum file span size and time #25

Closed Karl-G1 closed 4 years ago

Karl-G1 commented 4 years ago

[x] This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

This change enforces a minimum file span size of 1MB and time of 30 seconds by coercing the new value set in the control. This was implemented in the block diagram code instead of the control's properties to allow a different span value depending on span mode, as well as to be more readable.

Why should this Pull Request be merged?

Address Issue #23 where the user was allowed to enter a file span size of 0MB and time of 0 seconds. This behavior causes the logging engine to break.

What testing has been done?

Built the custom device and tested in VeriStand 2018.

Note

If users set a value lower than the minimum in the system definition file or in a build before this change goes in, the value will be coerced silently when the Log File page is opened next.

niveristand-diff-bot commented 4 years ago

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--File Page.vi.png

![capture](https://raw.githubusercontent.com/niveristand-diff-bot/diff-images/master/VeriStand/ni/niveristand-embedded-data-logger-custom-device/PR-25/2020-04-24/11%3A29%3A21/Embedded%20Data%20Logger%20System%20Explorer.lvlib--File%20Page.vi.png)

rtzoeller commented 4 years ago

Random musings more than actual feedback:

We should fix the spelling of "Segement"
I really wish there was a better way to indicate "max value of a U32", but I can't think of one in LabVIEW. E.g. in C++ numeric_limits<uint32_t>::max().
That case structure switching on numeric values is gross, but you didn't add the control so I'm fine leaving it as is.

Karl-G1 commented 4 years ago

I corrected the control's spelling. If you have a better recommendation for selecting the Segment File By value, I'm fine with implementing it, but I'm also fine with leaving it be :)

rtzoeller commented 4 years ago

I believe it is currently a ring, which is why we don't get type safety when hooking it up to a case structure. Ideally it would have been an enum.

I don't consider the onus on you to fix this; it should have been caught during the original submission. Then we would not have to rely on "not-equal-to-zero" checks elsewhere in the code.

niveristand-diff-bot commented 4 years ago

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--File Page.vi.png

![capture](https://raw.githubusercontent.com/niveristand-diff-bot/diff-images/master/VeriStand/ni/niveristand-embedded-data-logger-custom-device/PR-25/2020-04-24/13%3A09%3A18/Embedded%20Data%20Logger%20System%20Explorer.lvlib--File%20Page.vi.png)

Karl-G1 commented 4 years ago

To @rtzoeller 's comment, this is the equivalent of C++ numeric_limits<uint32_t>::max() and I would recommend using that.

Committed the replacement with some mild snark.

niveristand-diff-bot commented 4 years ago

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--File Page.vi.png

![capture](https://raw.githubusercontent.com/niveristand-diff-bot/diff-images/master/VeriStand/ni/niveristand-embedded-data-logger-custom-device/PR-25/2020-04-28/11%3A39%3A08/Embedded%20Data%20Logger%20System%20Explorer.lvlib--File%20Page.vi.png)