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 NI-TClk Support #293

Closed
Fladolcetta opened this issue Sep 11, 2017 · 18 comments · Fixed by #1054
Closed

Add NI-TClk Support #293

Fladolcetta opened this issue Sep 11, 2017 · 18 comments · Fixed by #1054

Comments

@Fladolcetta
Copy link
Contributor

Fladolcetta commented Sep 11, 2017

Add NI-TClk support.

How do we want to do this? NI-TClk is different from the other drivers supported by nimi-python in a fairly major way.

The C API for nitclk is not session based like other drivers. Instead, the functions are stateless and you pass in an array of sessions you have gotten from drivers. The only functions that take a single session are the get/set functions. Some typical function prototypes:

ViStatus _VI_FUNC niTClk_ConfigureForHomogeneousTriggers(ViUInt32 sessionCount,
                                                         const ViSession sessions[]);
ViStatus _VI_FUNC niTClk_SetAttributeViReal64(ViSession session,
                                              ViConstString channelName,
                                              ViAttr attributeId,
                                              ViReal64 value);

From the examples I have seen we always pass in the same array of sessions to all functions, never one set to one function and a different set to a different function. This is not always the case.

Given this, I propose that:

  • If the driver supports NI-TClk and nitclk is installed, add the attributes to the driver session
    • This will be via a nitclk.properties() object that the driver's _SessionBase() will contain.
      class _SessionBase(object):  # the driver session class
          @property
          def tclk(self):
              import nitclk
              return nitclk.properties(self.get_session_reference(), self._repeated_capability)
    • This will allow us to use the already existing repeated capabilities mechanism
  • Create a nitclk.Session() object that will be instantiated with the list of sessions and will own all non-attribute function calls.

An example of how this might look:

import niscope
import nifgen
import nitclk

scope1 = niscope.Session('PXI1Slot2')
scope2 = niscope.Session('PXI1Slot3')
fgen = nifgen.Session('PXI1Slot42')
tclk_all = nitclk.Session([scope1, scope2, fgen])
tclk_scope = nitclk.Session([scope1, scope2])
scope1.tclk.sync_pulse_source = 'PXI_Trig0'
fgen.script_triggers[0].tclk.script_trigger_master_session = scope1.get_session_reference()
tclk_all.configure_for_homogeneous_triggers()
tclk_all.synchronize()
tclk_all.initiate()

while not cows_are_home:
    tclk_scope.initiate()

scope1.close()
scope2.close()
fgen.close()

Even though the nitclk.Session() doesn't need to call a close or do any other cleanup, it would still be a context manager. No reason to do this.

Some alternatives would be:

  • Make everything a separate, stateless function, similar to C and LabVIEW
  • Have an attribute "session" but leave the other function as individual stateless functions
  • Something else?
@Fladolcetta Fladolcetta added this to the nimi-python 0.6 release (NI-TClk) milestone Sep 11, 2017
@marcoskirsch
Copy link
Member

marcoskirsch commented Oct 23, 2017

We've decided not to do this for the 0.9 December 2017 release.
Once/when/if there's customer demand for an nitclk module at a later date, we'll have to work on it.

@marcoskirsch marcoskirsch reopened this Oct 23, 2017
@marcoskirsch marcoskirsch modified the milestones: nimi-python 0.6 release (NI-TClk), Post 0.9 Release Oct 23, 2017
@Fladolcetta Fladolcetta modified the milestones: Post 0.6 Release, nimi-python 0.7 release (NI-TClk) Dec 20, 2017
@marcoskirsch
Copy link
Member

If the driver supports NI-TClk and nitclk is installed, add the attributes to the driver session

This is a very interesting idea. There is a bit of "surprise" / "side effect" / "clever" factor to this that makes me feel a bit uncomfortable. If we do go ahead with it, we should consider grouping the NI-TClk attributes in a container: scope.nitclk.sync_pulse_source = 'PXI_Trig0'

This will allow us to use the already existing repeated capabilities mechanism

Do you mean something like this? niscope.channels[0].nitclks.sync_pulse_source = 'PXI_Trig0

@auchter
Copy link

auchter commented Nov 1, 2018

Even though the nitclk.Session() doesn't need to call a close or do any other cleanup, it would still be a context manager.

Why make it a context manager then?

The example provided makes it look like there's something special about the TClk context. I'm concerned that this is going to mislead users into thinking that the context implies being synchronized, and that leaving the context will cause the devices to be "unsynchronized" and revert back to their pre-synchronization state. But that's not the case: the devices will still be synchronized upon leaving that context, the trigger resynchronizers will still be configured for a TClk'd acquisition, etc.

@epage
Copy link

epage commented Nov 1, 2018

If the driver supports NI-TClk and nitclk is installed, add the attributes to the driver session

My default is to be against dynamic functionality but this might be a case where it makes sense. I do agree with @marcoskirsch about having it in a dedicated container.

@auchter
Copy link

auchter commented Nov 1, 2018

If the driver supports NI-TClk and nitclk is installed, add the attributes to the driver session

I agree with both @marcoskirsch and @epage.

Please consider how Python APIs for drivers that aren't in this repository will implement this. I wouldn't be in favor of this approach if it's implemented as part of the codegeneration of each driver API. However, if all of the attribute implementation lives in an nitclk module, such that driver APIs can implement this along the lines of:

class Session(object):  # the driver session class
    @property
    def tclk(self):
        import nitclk
        return nitclk.properties(self.get_session_reference())

then I think this is a reasonable approach.

@texasaggie97-zz
Copy link
Contributor

texasaggie97-zz commented Nov 1, 2018

Putting the nitclk attributes into a separate container negates one of the benefits of adding them to the driver. In particular, the 'ScriptTriggerX' repeated capability is used. If we make it a separate container, then fgen.script_triggers[0].script_trigger_master_session = scope.get_session_reference() won't work unless we want to do something like fgen.nitclk.script_triggers[0].script_trigger_master_session = scope.get_session_reference(), and I am not sure how that would look implementation wise.

As for making nitclk.Session() a context manager, my only reasoning was for consistency with other drivers. I would have no problem not making it one.

This is getting into the implementation details, but I was not planning on adding the attributes at codegen time.

  1. This would complicate the code generator to pull in the attributes from another driver
  2. Would require rebuilding all drivers that support nitclk when there is a new attribute (not the end of the world, but still an issue)

Instead, my tentative idea was to create an nitclk attribute only session internally, and then when an attribute didn't exist on the driver session, look there to see if it was a nitclk attribute. This is similar to what @auchter was talking about.

EDIT: Just to be clear, the "nitclk attribute only session" would be part of the nitclk module, not the driver module.

EDIT2: I thought about this some more. I was originally thinking that the nitclk object would be contained by Session(). If instead it is contained by _SessionBase(), then this could work. @auchter's example might look like:

# Changes from original
# - Session -> _SessionBase
# - properties -> _properties - we would never expect a customer to use this directly
# - Add repeated capabilities when constructing the nitclk properties object
class _SessionBase(object):  # the driver session class
    @property
    def tclk(self):
        import nitclk
        return nitclk._properties(self.get_session_reference(), self._repeated_capability)

The usage from above would now look like:

scope.tclk.sync_pulse_source = 'PXI_Trig0'
fgen.script_triggers[0].tclk.script_trigger_master_session = scope.get_session_reference()

If this is what everyone meant, then I can update the description above.

@marcoskirsch
Copy link
Member

Please consider how Python APIs for drivers that aren't in this repository will implement this.

@auchter I assume you are referring to nifpga-python. Is there anything else at the moment?

We'd want to make sure that the nitclk module sourced here will work with that API.

We also should make sure that sourcing nitclk with the rest of nimi-python makes sense.

@texasaggie97-zz
Copy link
Contributor

I updated the issue text:

  1. Properties get added to tclk container rather than directly to Session
  2. Add tclk property snippet - properties not private though since there may be other drivers that need to access it
  3. No context manager - strikethrough
  4. Recieved information offline about one of the assumptions - strikethrough
  5. Updated example

@texasaggie97-zz
Copy link
Contributor

One more area for discussion: Should we just add a dependency on nitclk from the other drivers and assume it is installed?

As far as I can tell, all other languages/ADEs handle NI-TClk this way. If the driver support is installed for the given language/ADE, then NI-TClk support is also installed.

Now, just because we "have always done it this way" doesn't automatically mean we need to here, but it is a precedent.

@epage
Copy link

epage commented Nov 5, 2018

As far as I can tell, all other languages/ADEs handle NI-TClk this way. If the driver support is installed for the given language/ADE, then NI-TClk support is also installed.

In my mind, this is an unfortunate thing and not an ideal. We should be moving towards software integration pieces being add ons rather than required.

@marcoskirsch
Copy link
Member

marcoskirsch commented Nov 5, 2018

As far as I can tell, all other languages/ADEs handle NI-TClk this way. If the driver support is installed for the given language/ADE, then NI-TClk support is also installed.

This is true for NI-ModInst as well, except in the Python case. I like how we did it in the Python case.

@bjmuld
Copy link

bjmuld commented Nov 28, 2018

I see this issue was active fairly recently... can you guys comment on the status at present? What timescale are you thinking for a usable package?

@marcoskirsch
Copy link
Member

marcoskirsch commented Nov 28, 2018

We are debating priorities internally.

Some favor work on internal improvements to the code to make future work easier.
Some believe #940 should come first.
Some believe #293 (this) should be our next large deliverable.

User feedback can influence things. Can you provide information

  • What you're trying to do and why? Maybe old-school synchronization using triggers is good enough?
  • How much pain you're willing to endure if we put out something to get you unstuck but far from final – which would require retrofits as we refine things?
  • What is your project timeframe?
  • Would you be willing to help out in development of the nitclk module?

Thanks!

@bjmuld
Copy link

bjmuld commented Nov 28, 2018

What you're trying to do and why? Maybe old-school synchronization using triggers is good enough?

I'm researcher who has an automated experimentation platform for circuits implemented in python. Live mostly in python. I do (try to do) synchronous generation and capture across a couple PXI modules).

How much pain you're willing to endure if we put out something to get you unstuck but far from final – which would require retrofits as we refine things?

TBH, I probably wouldn't notice the pain (as far as NI-Tclk is concerned) provided my setups still run. Meaning, a sub-optimal synchronization would be tolerated.

What is your project timeframe?

I'm going to defend my diss. in Jan, God-willing, so will be running all experiments btw now and then.

Would you be willing to help out in development of the nitclk module?

Perhaps. But probably not in any significant way. Austin sounds nice, tho :)

@marcoskirsch
Copy link
Member

marcoskirsch commented Nov 28, 2018

I'm researcher who has an automated experimentation platform for circuits implemented in python. Live mostly in python. I do (try to do) synchronous generation and capture across a couple PXI modules).

Are these digitizers? Waveform generators? A mix?

Meaning, a sub-optimal synchronization would be tolerated.

This is what I was trying to get at with "Maybe old-school synchronization using triggers is good enough". With NI-TClk you get picosecond-level synchronization. But you can still use triggers to synchronize your devices without using NI-TClk. Your devices will be synchronized within a sample clock period and you may have some jitter; but if this is acceptable for your application then you don't need NI-TClk - rather you'd be settting up some trigger routes using the device's API. If that's the case and you need help with it, then it becomes a regular NI support question which is a good thing as you would not be tied to nimi-python development schedule.

@bjmuld
Copy link

bjmuld commented Nov 28, 2018

Are these digitizers? Waveform generators? A mix?

A mix.

If that's the case ... you would not be tied to nimi-python development schedule.

Doesn't sound like nimi-python/tclk is in the cards for that reason.

@marcoskirsch
Copy link
Member

Doesn't sound like nimi-python/tclk is in the cards for that reason.

I wouldn't bet on nitclk being ready by January, and if you don't need NI-TClk-level synchronization for your application, I would suggest you don't add this risk to your work and simply set up trigger synchronization yourself.

@bjmuld
Copy link

bjmuld commented Nov 29, 2018

Roger that. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants