Add enable_match_fail_combination method to nidigital API #2076
Conversation
|
I feel like there are a couple of ways to approach the session method: We could either pass in the nisync Python Session handle and retrieve the internal handle for the user or have them pass in the internal handle (telling them how to get it in the documentation). I think the kinder approach is to handle it for them, so that's what I went with. |
|
I believe we should be able to support calling this with gRPC device, though it may be awkward to test, due to the nisync session (which probably won't have gRPC Device Support). Because nisync has no gRPC support, I'm not bothering adding gRPC support for this method. |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2076 +/- ##
==========================================
+ Coverage 91.33% 91.35% +0.01%
==========================================
Files 66 66
Lines 16274 16292 +18
==========================================
+ Hits 14864 14883 +19
+ Misses 1410 1409 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Should we add a required or optional dependency on nisync to the nidigital setup.py or leave it as is? |
|
There's either something wrong with my implementation of the API or my test. I've run the LabVIEW example with simulated hardware and get no error. |
nisync.Session.session_handle is returning a ctype. :( |
…timing before calling method
Nope, I was just passing the wrong handle. |
There was a problem hiding this comment.
Is this empty because this is not supported in gRPC Device?
What happens if invoked?
Consider leaving a mako comment in here.
There was a problem hiding this comment.
Is this empty because this is not supported in gRPC Device?
It's empty to disable calling _grpc_stub_interpreter.py for the method. When specifying an interpreter template we have to have one for each interpreter.
It technically is supported in gRPC Device
You can see it in the .proto file
What happens if invoked?
If I populated this file, it would generate the method in _grpc_stub_interpreter.py but that method still wouldn't be called when using gRPC (because I excluded the method for _grpc_strub_interpreterer.py). It would error because it would try to call a method that doesn't exist in the interpreter. If we were to enable this for gRPC and call it on a local machine, it would maybe? work, but only because we had access to the local nisync session (nisync Python API does not have gRPC support). Honestly, I'm not 100% sure it would work because the nisync session might live in a different process than the nidigital session.
| with nisync.Session('6674T') as sync_session: | ||
| multi_instrument_session.enable_match_fail_combination(sync_session) | ||
| multi_instrument_session.burst_pattern(start_label='new_pattern') | ||
| multi_instrument_session.read_sequencer_flag(nidigital.SequencerFlag.FLAG0) |
There was a problem hiding this comment.
I'm surprised this feature works with multi-instrument sessions. Is the system test complete enough to exercise things?
There was a problem hiding this comment.
I based it on the LabVIEW Example, but I intend to hand it off to Greg to test once we're in agreement on the API.
There was a problem hiding this comment.
The system test should be complete enough to exercise the API, but it can't validate functionality.
The LabVIEW Example that it's based on is supposed to return a True, out of the box, but does not when run with simulated hardware.
There was a problem hiding this comment.
Greg was able to confirm that the feature works.
| } | ||
| ], | ||
| 'documentation': { | ||
| 'description': 'Configures digital pattern instruments and the PXIe-6674T timing and synchronization instrument to combine pattern comparison results and control subsequent pattern execution across digital pattern instruments based on those results. You must initialize the PXIe-6674T using NI-Sync and call this method from a multi-instrument session.\n' |
There was a problem hiding this comment.
the original function doc says to use NI-TClk
this says to use multi-instrument sessions
one is wrong
pretty sure it's this one
There was a problem hiding this comment.
There are 2 versions of this entry point in the LabVIEW API:
The name of C function that we're calling matches the one for TClk, but the other entrypoint for LabVIEW essentially passes the multi-instrument session as an array to the same function (an LV wrapper function, but still). This should work, but we'll have Greg validate.
The way I've written the LibraryInterepreter method, this definitely wouldn't work with NI-TClk, because I'm not allowing the user to pass in a list of NI-Digital sessions and I'm turning the session used to make the call into a list, instead.
There was a problem hiding this comment.
I believe we had agreed to only add support for multi-instrument sessions.
Would you prefer a slightly different method name to disambiguate from the C Function / LabVIEW VI with this name?
We handle it for then in |
What happens if they try it then? |
What do we do for |
If they try to call it with a gRPC session, as the code is written now, they will get an error when the session method tries to call a non-existent method in _grpc_stub_interpreter.py. NI-Digital does expose it in gRPC Device. |
For nidigital, we list nitclk as a dependency, but we have to because we import nitclk in session.py. Additionally, though NI-Digital's LabVIEW API ships with an example for this entrypoint, which uses NI-Sync, the NI-Digital feed does not depend on NI-Sync, under any circumstances. Based on this, I think we should leave out the dependency. |
What does this Pull Request accomplish?
Add support for
enable_match_fail_combination, now that we have an nisync Python API with an official way to access the session reference.Official description for what it does: "Configures digital pattern instruments in a multi-instrument session as well as the PXIe-6674T timing and synchronization instrument to combine pattern comparison results and control subsequent pattern execution across all digital pattern instruments in the multi-instrument session, based on those results."
List issues fixed by this Pull Request below, if any.
What testing has been done?
System Test Added based on the shipping LabVIEW example for Multi-Instrument Enable Match Fail Combination. Copied the config files right out of the example.
What exactly does this entry point actually do?
This explanation comes from the following pages:
The "match" opcode is used to
Then, exactly 80 cycles later you can use jump_if([!]matched, label) or exit_loop_if([!]matched, label).
The "failed" condition, used in jump_if([!]failed, label) or exit_loop_if([!]failed, label):
On the use of EnableMatchFailedCombinations: