Closed debryant closed 2 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.
I've not tested the new deployed functionality yet, but the overall approach is about what I was expecting! I do have a few questions and some feedback I'd like to see addressed.
Asynchronous Logging Loop.vi
: Why the difference in behavior between "Open File" and "Open File then Log Data"? In what cases are the difference code-paths used? I'm not yet convinced that this will do the right thing in all cases.
Initialize Logging Data.vi
: The change to how Max Group Size is computed is a leaky abstraction, and the output terminal name is no longer accurate.
I'd prefer if this VI returned the actual Max Group Size and the maximum number of channel groups in a file as separate values (to make the abstraction not leaky), and have the calling VI take the maximum of the two (probably with a comment as to why, not what).
Read Decimation Channel.vi
: Rather than return an error, I'm tempted to just coerce the value to be as least one internally. Causing logging to stop due to an invalid decimation value is IMO worse behavior than falling back to capturing every value. @buckd likely has thoughts on this as well.
ActionVIOnLoad.vi
: The latest version in the array constant appears to be incorrect. It corresponds to 2020.0.2.1
, but you modified the custom device XML to be 2021.0.1.0
.
Mutate Channel Group Commands.vi
: There should be exactly one item matching "Channel Groups GUID" per file. Use a singular instance of the poly-VI and ditch the middle for loop.
Mutate Channel Group Commands.vi
: We should remove the Decimation property from the channel group after creating the new channel with that as the default value, so we don't end up with conflicting values in the XML.
Initialize Logging Data.vi
: Keep data flowing left to right.
Mutate Channel Group Commands.vi
: The call to NI VeriStand - Get Item Property.vi
doesn't have its error terminals wired (but looks like it does).
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.
Asynchronous Logging Loop.vi: Why the difference in behavior between "Open File" and "Open File then Log Data"? In what cases are the difference code-paths used? I'm not yet convinced that this will do the right thing in all cases.
“Open File then Log Data” is used when in the middle of a logging session and we are spanning files. Basically we have been given more data to log but it is time to close the current file and open the next in the set of spanned data files. In this case, we don't want to change the decimation value. “Open File” is used when the user has explicitly commanded to open a file. We are not in the midst of a data logging session. A new decimation value should be used/applied.
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.
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.
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.
Can you replace the use of the Time Delay
express VI in Perform Decimation Test.vi
with DelayDataFlow.vim
from the testing tools repo? That malleable skips the delay if an error has occurred, which will help this test fail faster.
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.
Apologies for the scattered review feedback, but I just noticed that Channel Groups Page.vi
provides a way to set the decimation which needs to be updated (along with Refresh Groups Table.vi
). This VI wasn't modified in the PR, which is why I missed it.
I don't have any feedback other than what @rtzoeller has already provided. Ryan has the gating review.
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.
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.
@debryant I am not seeing the decimation channels behave as expected in all cases.
Here is a reproducing project.
Steps:
C:\log-archive
) in DIAdem. The first channel group appears to correctly have a decimation of 10, and the second has a decimation of 1.C:\log-archive
) in DIAdem. The first channel group appears to incorrectly have a decimation of 10. I would have expected it to be 1.Am I misunderstanding when the decimation should be applied?
The UI changes, including setting the values, look correct.
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.
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.
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.
What does this Pull Request accomplish?
Move the Decimation property from the Channel Group page to be a channel so that it can be programmatically changed from a test.
Why should this Pull Request be merged?
This is a feature request from IES who has the following use cases:
Use case: Some tests are 20 minutes some tests are 3 days some tests are 3 months. Want to adjust the decimation rate when starting a test. However the customer does not want to interface with the system definition file. They want a restricted access model where only dev engineers can make changes and technicians have different access levels and users can just run. So a configuration file loaded onto the system ingested by TestStand can configure the system correctly.
Use case: We want to have a set of standard log configurations (groups of channels to record) that can be configured per test without having to deploy. So, not programmatic change, VeriStand channel that can be set live. NOTE: Change would only apply before a log was started.
What testing has been done?
Unit tests have been run, and a new unit test created for this feature.