labscript-suite / labscript-devices

A modular and extensible plugin architecture to control experiment hardware using the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦.
http://labscriptsuite.org
Other
5 stars 58 forks source link

Allow arbitrary terminal connections for NI DAQmx devices #103

Closed carterturn closed 1 year ago

carterturn commented 1 year ago

This pull request adds the ability to connect any (hardware supported) pair of terminals in NI DAQmx devices, generalizing the mirror clock terminal feature.

This is useful for sharing clocks between devices that lack the ability to get all of their clocks from the same trigger line, which I observed in a setup that mixed PXI-6733 and PXIe-6535 NI cards. Although not particularly useful for setups configured with a clockline per NI card, this may be helpful for transitioning setups designed around a single clockline.

I have verified that this works as expected to connect one PXI trigger to another. In particular, our setup (for now) has a trigger mirrored from the input of the PXI-6733 connected to another trigger, and uses that as the clock for the PXIe-6535.

philipstarkey commented 1 year ago

This looks cool. Thanks for the contribution! Do you think you would be able to add something to the documentation in this repository about the feature and how (and why) to use it? There should already be some NI card documentation that isn't just the docstrings that you can add it to.

philipstarkey commented 1 year ago

@dihm @chrisjbillington Any objection to merging this?

dihm commented 1 year ago

I think this looks very good! Thanks @carterturn for putting it together.

I have one question for you to answer before merging: How does this change handle loading a connection table that was compiled without the connected_terminals flag? Basically, most people are going to get this change and start up BLACS without recompiling the connection table. Does it cleanly handle that? The other typical edge case is reloading an old shot that won't have this flag either, though I think your code handles that situation OK.

Looking at the code, I think the only suspicious point is the call to connected_terminals = properties['connected_terminals'] in the BLACS tab. I forget the behavior of the device properies object. If it defaults to None if the key isn't present, then I think it all works. If it throws a nasty error, we should probably intercept and set to None or inform the user to recompile.

carterturn commented 1 year ago

Excellent catch @dihm. It does request a recompile, but also throws errors for each device (with a key error in the properties object). Using get with a default avoids that (and also suppresses the recompile request). I think that since this feature has no effect without changes to the connection table, this is fine.

I also want to check that my definition of "terminal" (based on reading NI documentation) works in practice on the hardware (specifically, I want to clarify if terminal connecting is only for PFIO ports or if it also buffered digital outs), so I would like to run a couple of tests in lab before finalizing the documentation.

philipstarkey commented 1 year ago

@carterturn let us know when you think it's ready for rereview πŸ™‚

carterturn commented 1 year ago

I think it is ready now.

dihm commented 1 year ago

@carterturn I'll let this sit for another day or two just to make sure no one else has anything to add, then go ahead and merge.