Updating enums_addon and system tests for nifgen #586

bhaswath · 2017-11-13T12:35:55Z

[X ] This contribution adheres to CONTRIBUTING.md.

~~[] I've updated CHANGELOG.md if applicable.~~

[X] I've added tests applicable for this pull request

What does this Pull Request accomplish?

Adds enums for NI-Fgen
- ByteOrder
- HardwareState
- RelativeTo
- Signal
- Trigger
- TriggerWhen
Adds system tests for NI-Fgen

List issues fixed by this Pull Request below, if any.

What testing has been done?

System tests pass

Adding RelativeTo enum to enums_ addon

Adding system tests

texasaggie97-zz · 2017-11-13T17:22:04Z

src/nifgen/metadata/enums_addon.py

+    'RelativeTo': {
+        'values': [
+            {
+                'name': 'WAVEFORM_POSITION_START',


I have a change I am working on that will remove common prefixes and suffix like WAVEFORM_POSITION_ from enum names, if they all have it.

Since we are explicitly adding this enum, I think we should not have that common prefix. I.e. make these START and CURRENT.

Updating with changes requested

injaleea · 2017-11-14T09:55:06Z

src/nifgen/metadata/enums_addon.py

+        'values': [
+            {
+                'name': 'START',
+                'value': 0,


Do we need prefix key as 'WAVEFORM_POSITION' ?

@marcoskirsch and I discussed this. Instead of removing the common prefix, we think you should make the name match the C API completely. Then the codegen step can operate like normal and it will put the prefix in the processed metadata for you. So this 'name' should be changed to NIFGEN_VAL_WAVEFORM_POSITION_START and the other should be changed to NIFGEN_VAL_WAVEFORM_POSITION_CURRENT.

Sorry about changing this again.

fixing flake8 errors

Fixing flake8 errors

injaleea · 2017-11-16T13:54:04Z

Why this branch is not showing conflicts ? There should be 142 systemtests in master now. This branch shows 79!!

texasaggie97-zz · 2017-11-16T22:08:26Z

src/nifgen/metadata/enums_addon.py

+    'TriggerWhen': {
+        'values': [
+            {
+                'name': 'HighLevel',


Use name 'NIFGEN_VAL_ACTIVE_HIGH'

In other words, all names should be identical to the names used by the C API.

texasaggie97-zz · 2017-11-16T22:08:55Z

src/nifgen/metadata/enums_addon.py

+                'value': 101,
+            },
+            {
+                'name': 'LowLevel',


Use name 'NIFGEN_VAL_ACTIVE_LOW'

marcoskirsch · 2017-11-13T19:08:54Z

src/nifgen/system_tests/test_system_nifgen.py

-    delay = 1e-09
-    session.adjust_sample_clock_relative_delay(delay)
+def test_adjust_sample_clock_relative_delay():
+    with nifgen.Session('', False, 'Simulate=1, DriverSetup=Model:5421;BoardType:PXI') as session:  # 5433 is not supporting adjust_sample_clock_relative_delay right now


The comment makes it impossible for us to follow up in the future. Anything that says "right now" can be fixed at some point. So the comment should indicate so either by pointing to an issue or by being a TODO.

In this specific case, however, support has been added to NI-FGEN in 17.8 which will be released soon. I'm going to upgrade nimi-bot to pre-release NI-FGEN 17.8d25. This means you will be able to use 5433 in this test and it should work and hopefully the prerelease software doesn't break anything.

This will leave a window of time between now and NI-FGEN 17.8 being publicly released in which non-NI contributors will have a problem running this system test,

marcoskirsch · 2017-11-14T14:47:53Z

src/nifgen/metadata/enums_addon.py

+    'trigger_when': {
+        'values': [
+            {
+                'name': 'HighLevel',


@texasaggie97 shouldn't these names match the names of the corresponding C API constants?

marcoskirsch · 2017-11-16T22:10:51Z

src/nifgen/metadata/enums_addon.py

+    'TriggerWhen': {
+        'values': [
+            {
+                'name': 'HighLevel',


In other words, all names should be identical to the names used by the C API.

marcoskirsch · 2017-11-16T22:12:10Z

src/nifgen/metadata/functions_addon.py

@@ -71,6 +71,7 @@
    'SetWaveformNextWritePosition':             { 'parameters': { 3: { 'enum': 'RelativeTo',                }, }, },  # TODO: issue #538
    'GetHardwareState':                         { 'parameters': { 1: { 'enum': 'HardwareState',             }, }, },  # TODO: issue #538
    'SendSoftwareEdgeTrigger':                  { 'parameters': { 1: { 'enum': 'Trigger',                   }, }, },  # TODO: issue #538
+    'ConfigureDigitalLevelScriptTrigger':       { 'parameters': { 3: { 'enum': 'TriggerWhen',               }, }, },  # TODO: issue #538


Why are these TODOs still here? Isn't this precisely what this PR is taking care of?

marcoskirsch · 2017-11-16T22:13:05Z

src/nifgen/system_tests/test_system_nifgen.py

+    session.clear_user_standard_waveform()
+
+
+# TODO(bhaswath): Doesn't work, issue #596


Did you try my suggestion?
#596 (comment)

Yes, I tried and it did not throw a driver error.
Have updated in the issue

@texasaggie97 got to the bottom of it, we'll have to tackle it separately.

Fladolcetta · 2017-11-17T17:43:25Z

Does this fix #538

marcoskirsch · 2017-11-17T23:49:09Z

Does this fix #538

It appears to be a partial fix. One enum "Trigger" used by SendSoftwareEdgeTrigger is still missing.

@bhaswath Maybe you can add that in this PR so we can close that too?

marcoskirsch · 2017-11-17T23:51:30Z

src/nifgen/system_tests/test_system_nifgen.py

+    session.configure_digital_level_script_trigger('ScriptTrigger0', 'PXI_Trig0', nifgen.TriggerWhen.HIGH)
+
+
+# TODO(bhaswath): Enable after Issue 597 is fixed


You added all except one enum in this PR, maybe you can finish it and close #597 too?

nimi-bot · 2017-11-21T09:30:28Z

system tests succeeded

bhaswath · 2017-11-22T07:19:27Z

Putting 'Triggers' enum in a different dictionary than other enums, because 'Trigger' matches multiple entries like 'TriggerMode' and 'TriggerSource', so they all end up with the values from 'Trigger' if it is in the enums_additional_enums dictionary.

It can be moved to be with the others once issue #624 is fixed.

nimi-bot · 2017-11-22T07:23:24Z

system tests succeeded

nimi-bot · 2017-11-22T12:18:37Z

system tests succeeded

Talked with Bhaswath and these changes have been implemented

marcoskirsch · 2017-11-22T15:54:11Z

Diff looks fine, but the PR description appears to be very outdated. PR does more, and fixes more issues. Please make sure it's accurate.

nimi-bot · 2017-11-22T15:58:12Z

system tests succeeded

bhaswath and others added 3 commits November 13, 2017 18:02

Update enums_addon.py

955749d

Adding RelativeTo enum to enums_ addon

Update test_system_nifgen.py

971ec27

Adding system tests

fixed failing system test for nifgen and flake-8 failure

9a39ad4

texasaggie97-zz suggested changes Nov 13, 2017

View reviewed changes

bhaswath added 2 commits November 14, 2017 12:46

Update enums_addon.py

5462d10

Updating with changes requested

Update test_system_nifgen.py

6c428a2

Updating with changes requested

injaleea reviewed Nov 14, 2017

View reviewed changes

bhaswath added 13 commits November 14, 2017 18:42

Update test_system_nifgen.py

b3f3e5d

Update enums_addon.py

4590732

Update functions_addon.py

bdf5ad9

Update functions_addon.py

e613514

fixing flake8 errors

Update enums_addon.py

676c708

Fixing flake8 errors

Update test_system_nifgen.py

0c0b218

Fixing flake8 errors

Merge branch 'master' into bhaswath

c3e0683

Update enums_addon.py

f49bd8c

Update test_system_nifgen.py

29bc428

Update functions_addon.py

f6d06ff

Update enums_addon.py

41566f5

Update test_system_nifgen.py

cad0577

Update test_system_nifgen.py

a76ca54

Update enums_addon.py

8175cc6

texasaggie97-zz previously requested changes Nov 16, 2017

View reviewed changes

marcoskirsch previously requested changes Nov 16, 2017

View reviewed changes

bhaswath added 6 commits November 17, 2017 01:10

Merge branch 'master' into bhaswath

fdbb288

Update enums_addon.py

174b1dc

Update functions_addon.py

1ffab89

Update enums_addon.py

8338685

Update test_system_nifgen.py

0feb453

Update test_system_nifgen.py

20da0ad

bhaswath added 3 commits November 17, 2017 17:43

Update test_system_nifgen.py

7d7db9a

Update test_system_nifgen.py

7cd8a42

Update test_system_nifgen.py

58a3cbc

marcoskirsch reviewed Nov 17, 2017

View reviewed changes

Update test_system_nifgen.py

2eeaf31

texasaggie97-zz mentioned this pull request Nov 21, 2017

System test error on adding Triggers enum to enums_addon.py (Not for merging) #622

Closed

bhaswath added 2 commits November 22, 2017 12:46

Update enums_addon.py

5faf801

Update test_system_nifgen.py

0db1949

Update test_system_nifgen.py

0a3b472

texasaggie97-zz added 4 commits November 22, 2017 09:43

Merge branch 'master' into bhaswath

9d4a7fa

Don't use session from fixture since we are creating our own

5666e73

Clean up formatting

95aced3

Update generated files

d1ff118

texasaggie97-zz approved these changes Nov 22, 2017

View reviewed changes

texasaggie97-zz merged commit eaf2c75 into master Nov 22, 2017

marcoskirsch deleted the bhaswath branch December 13, 2017 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating enums_addon and system tests for nifgen #586

Updating enums_addon and system tests for nifgen #586

bhaswath commented Nov 13, 2017 •

edited by texasaggie97-zz

Loading

texasaggie97-zz Nov 13, 2017

injaleea Nov 14, 2017

texasaggie97-zz Nov 14, 2017

injaleea commented Nov 16, 2017

texasaggie97-zz Nov 16, 2017

marcoskirsch Nov 16, 2017

texasaggie97-zz Nov 16, 2017

marcoskirsch Nov 13, 2017

marcoskirsch Nov 14, 2017

marcoskirsch Nov 16, 2017

marcoskirsch Nov 16, 2017

marcoskirsch Nov 16, 2017

bhaswath Nov 17, 2017

marcoskirsch Nov 17, 2017

Fladolcetta commented Nov 17, 2017

marcoskirsch commented Nov 17, 2017

marcoskirsch Nov 17, 2017

nimi-bot commented Nov 21, 2017

bhaswath commented Nov 22, 2017

nimi-bot commented Nov 22, 2017

nimi-bot commented Nov 22, 2017

marcoskirsch commented Nov 22, 2017

nimi-bot commented Nov 22, 2017

		session.clear_user_standard_waveform()


		# TODO(bhaswath): Doesn't work, issue #596

		session.configure_digital_level_script_trigger('ScriptTrigger0', 'PXI_Trig0', nifgen.TriggerWhen.HIGH)


		# TODO(bhaswath): Enable after Issue 597 is fixed

Updating enums_addon and system tests for nifgen #586

Updating enums_addon and system tests for nifgen #586

Conversation

bhaswath commented Nov 13, 2017 • edited by texasaggie97-zz Loading

What does this Pull Request accomplish?

List issues fixed by this Pull Request below, if any.

What testing has been done?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

injaleea commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fladolcetta commented Nov 17, 2017

marcoskirsch commented Nov 17, 2017

Choose a reason for hiding this comment

nimi-bot commented Nov 21, 2017

bhaswath commented Nov 22, 2017

nimi-bot commented Nov 22, 2017

nimi-bot commented Nov 22, 2017

marcoskirsch commented Nov 22, 2017

nimi-bot commented Nov 22, 2017

bhaswath commented Nov 13, 2017 •

edited by texasaggie97-zz

Loading