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

Add enable_match_fail_combination method to nidigital API #2076

Merged

Conversation

ni-jfitzger
Copy link
Collaborator

@ni-jfitzger ni-jfitzger commented Feb 2, 2025

  • 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?

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."

I have chosen not to add an nisync dependency to the nidigital API, based on the following reasoning:

  • We don't need to import nisync because we're passing in the session object and using it to call a class method.
  • 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.
  • Most users will not be using this entrypoint, and we also don't want to force an unnecessary dependency on them.

Example Code

Anyone wondering how to use it can tread the system test as an example. It's based on the LabVIEW Example.
The pattern doesn't do the best job of demonstrating the feature, but the code, itself, demonstrates proper usage.

Usage Steps:

  1. Load and apply specs and patterns, etc.
  2. Open an nisync session and pass it to enable_match_fail_combination
  3. Burst the pattern

Reading the sequencer flag is not a necessary step to use the feature. In the example, it's intended to demonstrate that feature is working (though that requires real hardware and, like I said, this pattern does a poor job of demonstrating whether the feature actually is working).

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

Evaluates comparisons (H, L, M, or V) on the vector without affecting the fail count and pass or fail results.

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):

failed - any comparison failed in the current pattern burst on any pin from any site. This condition does not include the vectors that executed within the last 80 cycles or vectors that contain the match opcode. You can use the repeat opcode to implement an 80 cycle delay.


On the use of EnableMatchFailedCombinations:

When you configure a system to combine matched and failed results, if a failed condition occurs on any of the sites enabled for bursting on any of the multiple synchronized digital pattern instruments, the combined system result for the failed condition is TRUE and influences the flow of pattern execution through opcodes that use the failed condition as a parameter. Calling the niDigital Get Site Pass Fail API in the test program on all digital pattern instruments returns failed results only for the site(s) on the instrument(s) that caused the failure.

When you configure a system to combine matched and failed results, the combined system result for the matched condition is TRUE only when the matched condition is satisfied by all sites enabled for bursting across multiple synchronized digital pattern instruments that include pins in the specified match condition.

@ni-jfitzger
Copy link
Collaborator Author

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.

@ni-jfitzger
Copy link
Collaborator Author

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.

@ni-jfitzger ni-jfitzger added this to the nimi-python 1.4.9 milestone Feb 2, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.35%. Comparing base (dd57eae) to head (6219cb8).
Report is 3 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
codegenunittests 84.95% <ø> (ø)
nidcpowersystemtests 94.59% <ø> (+0.04%) ⬆️
nidcpowerunittests 89.53% <ø> (ø)
nidigitalsystemtests 92.26% <100.00%> (+0.05%) ⬆️
nidigitalunittests 68.44% <66.66%> (-0.01%) ⬇️
nidmmsystemtests 92.72% <ø> (ø)
nifakeunittests 87.24% <ø> (ø)
nifgensystemtests 94.86% <ø> (ø)
nimodinstsystemtests 73.85% <ø> (ø)
nimodinstunittests 94.20% <ø> (ø)
niscopesystemtests 92.94% <ø> (ø)
niscopeunittests 43.20% <ø> (ø)
nisesystemtests 91.50% <ø> (ø)
niswitchsystemtests 82.03% <ø> (ø)
nitclksystemtests 94.87% <ø> (ø)
nitclkunittests 98.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
generated/nidigital/nidigital/_library.py 95.04% <100.00%> (+0.05%) ⬆️
...erated/nidigital/nidigital/_library_interpreter.py 90.37% <100.00%> (+0.07%) ⬆️
generated/nidigital/nidigital/session.py 95.92% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd57eae...6219cb8. Read the comment docs.

@ni-jfitzger
Copy link
Collaborator Author

Should we add a required or optional dependency on nisync to the nidigital setup.py or leave it as is?

@ni-jfitzger
Copy link
Collaborator Author

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.

@ni-jfitzger
Copy link
Collaborator Author

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. :(

@ni-jfitzger
Copy link
Collaborator Author

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. :(

Nope, I was just passing the wrong handle.

@ni-jfitzger ni-jfitzger marked this pull request as ready for review February 3, 2025 17:31
Copy link
Member

@marcoskirsch marcoskirsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get someone more familiar with this functionality to review the API.
We should consider an example. Maybe? Maybe not.

@marcoskirsch
Copy link
Member

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.

We handle it for then in nitclk.

@marcoskirsch
Copy link
Member

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.

What happens if they try it then?
Does NI-Digital expose this function in gRPC Device (it should not!)

@marcoskirsch
Copy link
Member

Should we add a required or optional dependency on nisync to the nidigital setup.py or leave it as is?

What do we do for nitclk?
For other things that are not required like numpy we don't install it for you.

@ni-jfitzger
Copy link
Collaborator Author

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.

What happens if they try it then? Does NI-Digital expose this function in gRPC Device (it should not!)

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.

@ni-jfitzger
Copy link
Collaborator Author

ni-jfitzger commented Feb 7, 2025

Should we add a required or optional dependency on nisync to the nidigital setup.py or leave it as is?

What do we do for nitclk? For other things that are not required like numpy we don't install it for you.

For nidigital, we list nitclk as a dependency, but we have to because we import nitclk in session.py.
We don't need to import nisync because we're passing in the session object and using it to call a class method.

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.

@ni-jfitzger ni-jfitzger merged commit 93b5c85 into ni:master Feb 19, 2025
35 checks passed
@ni-jfitzger ni-jfitzger deleted the nidigital_enable_match_fail_combination branch February 19, 2025 23:19
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.

niDigital_EnableMatchFailCombination is missing from nidigital API
3 participants