nifgen repeated capability documentation and test updates #1681

ni-jfitzger · 2022-01-10T21:00:47Z

This contribution adheres to CONTRIBUTING.md.
I've updated CHANGELOG.md if applicable.
I've added tests applicable for this pull request

What does this Pull Request accomplish?

Updates nifgen metadata to properly document and support accessing of attributes with the following repeated capabilities:
- channels
- script_triggers
- markers
- data_markers
Side effect: removed mention of CreateWaveformFromFileHWS from documentation. Support for this entrypoint is removed in NI-FGEN 21.3.
Regenerates nifgen docs to display the changes mentioned above
Adds nifgen system tests for the repeated capabilities mentioned above

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

What testing has been done?

Ran tox locally.
Ran the NI-FGEN systems tests on a local system.

* Single-channel session caused test_channels_rep_cap failure. * After fixing attribute name, Driver claimed requested terminal name was invalid in test_data_markers_rep_cap test.

nimi-bot · 2022-01-10T21:01:05Z

Can one of the admins verify this patch?

Documented nifgen repeated capability changes in changelog.

ni-jfitzger · 2022-01-10T21:43:33Z

src/nifgen/system_tests/test_system_nifgen.py

+
+    requested_terminal_name = '/Dev1/PXI_Trig0'
+    session.markers['Marker0'].marker_event_output_terminal = requested_terminal_name
+    assert requested_terminal_name == session.markers[0].marker_event_output_terminal


Should I just use verbose strings everywhere (as an example for how good code should be written) in lieu of coverage for more inputs to the repeated capability?
If so, I'll apply the answer to all of the new tests.

Do you mean ["Marker0"] instead of [0].
No, as a matter of fact, the more pythonic thing is to pass the number or an iterable, as opposed to a fully spelled out string.

I am of the opinion that the spelled-out string shouldn't even be documented behavior.

Hmm ... we're definitely spelling out our rep_caps in the nidigital rep_caps tests, which is why I asked.
https://github.com/ni/nimi-python/blob/master/src/nidigital/system_tests/test_system_nidigital.py
Maybe it's different though, because those aren't numbers.

Yeah, if we don't document the spelled-out string, then it should be fine to only test passing the number.
I'll apply this answer to all of the tests.

Updated all of the new tests to use integers and iterables, instead of strings. I even got rid of the '0-1' channel string.

Well, I think we need to be a bit careful here.
Our documentation and examples should teach customers to use the API in a "pythonic" way.
But if using strings work, we should have test coverage to ensure we don't accidentally break it. It should be tested in addition to the recommended usage.

All this said, we have unit tests that test the conversion for repeated capability into strings that the driver accepts, so I don't think we need to test every permutation in system tests. This is testing what we care about which is that the correct prefix is used.

ni-jfitzger · 2022-01-10T21:49:37Z

src/nifgen/system_tests/test_system_nifgen.py

+
+    requested_polarity = nifgen.DataMarkerEventLevelPolarity.LOW
+    session.data_markers['DataMarker0'].data_marker_event_level_polarity = requested_polarity
+    assert requested_polarity == session.data_markers[0].data_marker_event_level_polarity


I originally wrote this test to use data_marker_event_output_terminal but reading back the value after setting it to '/Dev1/PXI_Trig0' returned

> raise DriverError(code, description) E nifgen.errors.DriverError: -1074135024: IVI: (Hex 0xBFFA0010) Invalid value for parameter or property. E E Property: Data Marker Event Output Terminal E Invalid Value: /Dev1/PXI_Trig0 E Valid Values: C:\Program Files\Python36\lib\site-packages\nifgen\errors.py:102: DriverError

It's possible that Munich doesn't support data markers. Can you do a quick experiment with a 5421 on your end?

Keep the system test as-is.

I think we should avoid using codenames in public forums.
I'll check the 5421 behavior and get back to you.

I didn't have access to a 5421 but I tried to exporting the data marker event with a simulated 5421 and a real PXI-5441. Neither one errored.

I also looked through the NI-FGEN documentation and found this page which seems to confirm that the PXIe-5433 (2CH) that we simulate with by default, does not support Data Marker Events. Marker Events are listed, but Data Marker Events are not.

I'll leave the test code basically as is, since you've already reviewed and okayed it, but I'm going to change this test to simulate with the 5421, instead. I don't want us to rely on the behavior for a property related to an unsupported event remaining unchanged.

I think we should avoid using codenames in public forums.

😳

I'll leave the test code basically as is, since you've already reviewed and okayed it, but I'm going to change this test to simulate with the 5421, instead.

Please don't do this, as simulating DAQmx based instruments has its own set of problems* and we're aggressively moving away from it in order to simplify things and speed up testing even if it means a bit worse test coverage.

I'd rather stick to 54x3 instruments. We could look at the error. Unfortunately it does not appear to tell you which Data Marker Event is being configured.

Okay, I reverted the 5421 change.
Don't we know which event is being configured from our code?
As I mentioned, the documentation appears to confirm that the 5433 does not support Data Marker Events.

We know which event we are trying to configure.

We can't tell that the appropriate event was configured on the driver runtime because the error doesn't include that info in the dynamic elaboration.

ni-jfitzger · 2022-01-10T21:58:53Z

Key items to review:

new tests
changelog

Metadata changes came from an internal export of nifgen_nimi_python that contained relevant metadata changes implemented and reviewed internally.

Other changes are the result of running tox -e codegen.

sbethur · 2022-01-11T16:51:36Z

/AzurePipelines run

azure-pipelines · 2022-01-11T16:51:44Z

Azure Pipelines successfully started running 1 pipeline(s).

marcoskirsch · 2022-01-14T22:49:58Z

src/nifgen/system_tests/test_system_nifgen.py

+
+    requested_terminal_name = '/Dev1/PXI_Trig0'
+    session.markers['Marker0'].marker_event_output_terminal = requested_terminal_name
+    assert requested_terminal_name == session.markers[0].marker_event_output_terminal


Do you mean ["Marker0"] instead of [0].
No, as a matter of fact, the more pythonic thing is to pass the number or an iterable, as opposed to a fully spelled out string.

I am of the opinion that the spelled-out string shouldn't even be documented behavior.

marcoskirsch · 2022-01-14T22:50:52Z

src/nifgen/system_tests/test_system_nifgen.py

+
+    requested_polarity = nifgen.DataMarkerEventLevelPolarity.LOW
+    session.data_markers['DataMarker0'].data_marker_event_level_polarity = requested_polarity
+    assert requested_polarity == session.data_markers[0].data_marker_event_level_polarity


It's possible that Munich doesn't support data markers. Can you do a quick experiment with a 5421 on your end?

Keep the system test as-is.

marcoskirsch · 2022-01-14T22:56:00Z

docs/nifgen/rep_caps.rst

+
+            session.data_markers['0-2'].channel_enabled = True
+
+        passes a string of :python:`'DataMarker0, DataMarker1, DataMarker2'` to the set attribute function.


This is very bad documentation, because it documents what the implementation does under the hood which is irrelevant to the Python programmer.

We should (separate PR naturally) rewrite this to be relevant to the user. @ni-jfitzger want to tackle it? I can help wordsmith.

Yeah, I'm not thrilled with this documentation either, but for a different reason. I don't like the use of channel_enabled in the example code. I understand that it just an example attribute which isn't necessarily supported. It might be confusing to users, though.

I don't mind tackling the improvement of this documentation, but I'll have to do some digging to figure out how, since it's generated.

It should just be a matter of updating rep_caps.rest.mako

Related issues, for which there appears to be a fair bit of duplication:
#1388
#1338
#1334
#1126

#1351 would be a nice one to fix too.

Replaced verbose strings in rep_cap tests with integers to be more pythonic.

Replacing another verbose string that I missed in the last commit.

Replace '0-1' channels string with an iterable to be more pythonic.

ni-jfitzger · 2022-01-15T23:25:42Z

/AzurePipelines run

azure-pipelines · 2022-01-15T23:25:46Z

Commenter does not have sufficient privileges for PR 1681 in repo ni/nimi-python

Change test_data_markers_rep_cap to use a simulated 5421 (supports data marker events) instead of a 5433 (does not support data marker events).

Reviewer's request to avoid usage of DAQmx device simulation.

ni-jfitzger and others added 7 commits December 8, 2021 08:03

Update attributes.py

01d52ff

Update config.py

f84c29c

Update enums.py

9ab5cc0

Update functions.py

5072663

Add rep_cap tests to test_system_nifgen.py.

ba2ff01

Add codegen changes from nifgen metadata changes.

8980937

Test Fixes

7727328

* Single-channel session caused test_channels_rep_cap failure. * After fixing attribute name, Driver claimed requested terminal name was invalid in test_data_markers_rep_cap test.

Update CHANGELOG.md

2212a53

Documented nifgen repeated capability changes in changelog.

ni-jfitzger marked this pull request as ready for review January 10, 2022 21:36

ni-jfitzger requested review from marcoskirsch and sbethur as code owners January 10, 2022 21:36

ni-jfitzger commented Jan 10, 2022

View reviewed changes

ni-jfitzger closed this Jan 10, 2022

ni-jfitzger reopened this Jan 10, 2022

marcoskirsch requested changes Jan 14, 2022

View reviewed changes

ni-jfitzger added 3 commits January 15, 2022 16:35

Update test_system_nifgen.py

040722a

Replaced verbose strings in rep_cap tests with integers to be more pythonic.

Update test_system_nifgen.py

f184b7c

Replacing another verbose string that I missed in the last commit.

Update test_system_nifgen.py

aa0683e

Replace '0-1' channels string with an iterable to be more pythonic.

ni-jfitzger added 2 commits January 16, 2022 16:17

Update test_system_nifgen.py

ed2b41d

Change test_data_markers_rep_cap to use a simulated 5421 (supports data marker events) instead of a 5433 (does not support data marker events).

Revert previous change

c396561

Reviewer's request to avoid usage of DAQmx device simulation.

marcoskirsch approved these changes Jan 20, 2022

View reviewed changes

marcoskirsch merged commit f81923a into ni:master Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nifgen repeated capability documentation and test updates #1681

nifgen repeated capability documentation and test updates #1681

ni-jfitzger commented Jan 10, 2022 •

edited

Loading

nimi-bot commented Jan 10, 2022

ni-jfitzger Jan 10, 2022

marcoskirsch Jan 14, 2022

ni-jfitzger Jan 15, 2022

ni-jfitzger Jan 15, 2022

marcoskirsch Jan 20, 2022

marcoskirsch Jan 20, 2022

ni-jfitzger Jan 10, 2022

marcoskirsch Jan 14, 2022

ni-jfitzger Jan 15, 2022

ni-jfitzger Jan 16, 2022

marcoskirsch Jan 20, 2022

marcoskirsch Jan 20, 2022

ni-jfitzger Jan 20, 2022 •

edited

Loading

marcoskirsch Jan 20, 2022

ni-jfitzger commented Jan 10, 2022

sbethur commented Jan 11, 2022

azure-pipelines bot commented Jan 11, 2022

marcoskirsch Jan 14, 2022

marcoskirsch Jan 14, 2022

marcoskirsch Jan 14, 2022

ni-jfitzger Jan 15, 2022 •

edited

Loading

ni-jfitzger Jan 15, 2022

marcoskirsch Jan 20, 2022

ni-jfitzger commented Jan 15, 2022

azure-pipelines bot commented Jan 15, 2022


		session.data_markers['0-2'].channel_enabled = True

		passes a string of :python:`'DataMarker0, DataMarker1, DataMarker2'` to the set attribute function.

nifgen repeated capability documentation and test updates #1681

nifgen repeated capability documentation and test updates #1681

Conversation

ni-jfitzger commented Jan 10, 2022 • edited Loading

What does this Pull Request accomplish?

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

What testing has been done?

nimi-bot commented Jan 10, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni-jfitzger Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni-jfitzger commented Jan 10, 2022

sbethur commented Jan 11, 2022

azure-pipelines bot commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni-jfitzger Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni-jfitzger commented Jan 15, 2022

azure-pipelines bot commented Jan 15, 2022

ni-jfitzger commented Jan 10, 2022 •

edited

Loading

ni-jfitzger Jan 20, 2022 •

edited

Loading

ni-jfitzger Jan 15, 2022 •

edited

Loading