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

Dev/performance #17

Merged
merged 7 commits into from
Nov 8, 2018
Merged

Dev/performance #17

merged 7 commits into from
Nov 8, 2018

Conversation

csjall
Copy link
Collaborator

@csjall csjall commented Nov 8, 2018

[x] This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

Add system definition benchmark test. Test will fail if target loop rates are not achieved. Refactored some of the system definition file manipulation to enable modifying the target loop rate.

Why should this Pull Request be merged?

This is a general purpose benchmark test utility that can be used by multiple custom devices.

What testing has been done?

Added a test for new utility method. All tests are green.

…tion to call common private methods. Added ConfigureSystemDefinitionFile to utilities.
…y a list of rates to test. The method will fail if PCL is late or actual loop rate is outside tolerance.
@csjall csjall requested review from rtzoeller and buckd November 8, 2018 17:33
@niveristand-diff-bot
Copy link
Collaborator

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.

VeriStandTestCase.lvclass--4dELq9_SetMultipleChannelValues.vi.png: capture

VeriStandTestCase.lvclass--sIMIBa_OpenVeriStandConnection.vi.png: capture

VeriStandTestCase.lvclass--uHZ8lb_OpenSystemDefinitionFile.vi.png: capture

VeriStandTestUtilities.lvlib--m9Wqwb_QuerySystemDefinitionFile.vi.png: capture

@rtzoeller
Copy link
Contributor

rtzoeller commented Nov 8, 2018

  • @csjall you added a test that makes sure benchmarking passes for a trivial system definition with a low target rate and a high tolerance. Can we also add one that makes sure benchmarking fails when we request an impossibly-high target rate?

@rtzoeller
Copy link
Contributor

rtzoeller commented Nov 8, 2018

  • Two of the VIs have the same icon (GetSystemChannelValues.vi and GetSystemChannelValuesForDuration.vi). Can we differentiate them somehow?

@rtzoeller
Copy link
Contributor

rtzoeller commented Nov 8, 2018

  • In GetSystemChannelValuesForDuration.vi you are searching a 1D array to check for duplicates. Given that the system time is monotonically increasing, it is sufficient to compare with only the last element of the array.

@rtzoeller
Copy link
Contributor

rtzoeller commented Nov 8, 2018

  • Should GetSystemChannelNames.vi be called GetSystemChannelFullPaths.vi instead? The use case appears to be "given a known set of names, get the associated full paths".

@csjall
Copy link
Collaborator Author

csjall commented Nov 8, 2018

The benchmark tests are super flaky on Windows host. Most of the times it passes, but sometimes it fails with HP count != 0. We should run on RT target or get rid of these tests altogether.

image

@rtzoeller
Copy link
Contributor

I am fine with requiring some sort of RT host to run these tests, assuming we don't require anything else beyond VeriStand. We could allow the user to specify the target OS in an overrides file, which would mitigate pain associated with requiring an RT target for the testing tools.

@csjall csjall merged commit ed9e620 into master Nov 8, 2018
@csjall csjall deleted the dev/performance branch November 8, 2018 21:51
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