Feature/dem 564/driver for zi hdawg8

[qutech-sd]

Changes proposed in this pull request:

• New Instrument driver for ZI HDAWG8
• Unittests for testable methods

@WilliamHPNielsen

[codecov]

Codecov Report

Merging #1331 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1331   +/-   ##
=======================================
  Coverage   74.13%   74.13%           
=======================================
  Files          85       85           
  Lines        9822     9822           
=======================================
  Hits         7282     7282           
  Misses       2540     2540

[WilliamHPNielsen]

@qSaevar, this generally looks good. Since I don't have this unit myself, I can only give pretty limited feedback and mainly on style.

Before we merge this PR, the code needs to be in compliance with our "house style guide". Therefore, I'd like to ask you to

• Adhere to PEP8, in particular regarding line lengths (comments are no exception)
• Use type annotations throughout the driver.
• Remove the type annotations from the (otherwise splendid) docstrings. There is no need to write the type twice. This also includes the return value.

And then I have a question/concern. Although the create_parameters_from_node_tree function is kind of nifty, I would be hesitant to use it on the ZI Instrument that I do have, namely the UHF-LI. The reason being that many parameters are interlinked (e.g. sweep frequencies center/span and min/max) and therefore must be updated together lest the snapshot of the instrument become inconsistent. Is this a problem for the hdawg as well?

[qSaevar]

I have addressed the code style comment and adjusted the code to the “house style guide”.

Regarding the question/concern regarding creating parameters from the device tree: We do not share this concern as every parameter that can be set via this method can also be set in the LabOne web GUI. One has to have a basic understanding of the instrument before setting some variables, but at the end of the day generating parameters from the device node tree should not set the snapshot of the instrument in an inconsistent state.

[astafan8]

Looks good (see only one comment of mine). +1 for the tests!

[astafan8]

would be nice if there was a dict that maps human-readable names to these flags, and let this method accept a list of those human-readable names. this will make the experimental code much more readable.

[qSaevar]

Jebb, I removed it from the docstring to fit it to pep8 line length, but this is how the docstring was:

             flags: ziPython.ziListEnum.settingsonly -> 0x08: Returns only nodes which are marked as setting
                    ziPython.ziListEnum.streamingonly -> 0x10: Returns only streaming nodes
                    ziPython.ziListEnum.subscribedonly -> 0x20: Returns only subscribed nodes
                    ziPython.ziListEnum.basechannel -> 0x40: Return only one instance of a node in case of multiple
                    channels
                    Or any combination of flags can be used.

But I can add it again

[astafan8]

ah, are you saying that zhinst (or similar) has these flags defined? in case that module is necessary for working with the instrument, then it'd be really helpful to have these ziPython.ziListEnum.* constants mentioned in the docstring. and in order to make it pep8 compatible, just reformat it a bit so that lines are shorted than 80 characters :)

[qSaevar]

Yes, they are predefined constants provided by ZI. I reintroduced them to the docstring.

[WilliamHPNielsen]

@qSaevar, thank you, the code is nice and tidy now. I am happy to merge now, but I'd like to understand the parameter business.

I simply cannot see how this:

every parameter that can be set via this method can also be set in the LabOne web GUI.

is relevant. Perhaps you can elaborate?

I will also try to reformulate. An example (thought) situation: The instrument has parameters channel_1_amplitude, channel_1_offset, channel_1_high, and channel_1_low. Let's say that the amplitude is 1 V and the offset 0 V. Now the user does channel_1_high(0.1). What will channel_1_amplitude.get_latest() (as called by the snapshot) return? I fear that it will return 1 V and not 0.2 V, because the set method of channel_1_high does not know about its "interlinked" parameters.

[qSaevar]

@WilliamHPNielsen, if you mean by channel_1_amplitude awgs_n_outputs_1_amplitude and by channel_1_high channel_1_range then I can clarify.
If I'm playing a sine wave that is 1Vpp, i.e. [0.5, -0.5]V and set awgs_n_outputs_1_amplitude(1.0) (This has no unit) the wave stays the same. Then I set channel_1_range(0.1) V, the output will still be a sine wave but 100mVpp. Now I call awgs_n_outputs_1_amplitude.get_latest() and it will return 1.0, same as calling get, as is expected as the two values are not interlinked. On the other hand if I now set awgs_n_outputs_1_amplitude(0.5) the wave that is being played is only 50mVpp. And calling channel_1_range.get_latest() will return a 0.1V or the same as just calling get to get the latest value from the actual device.
Or in other words, the parameters are very related but do not affect each others values.

[WilliamHPNielsen]

@qSaevar we seem to misunderstand each other. My concern is not with those particular parameters (but thanks for the explanation), but is a more general one.

I will try to explain:
sometimes instruments have settings that are interlinked. Famous examples are frequency (center, span) and (start, stop) or (amplitude, offset) and (voltage_high, voltage_low). But it could also be something like: setting an input impedance "corrects" the output amplitude of a signal. Now, to be clear, I do not know which parameters the AWG has, but it is very common for instruments in general to have settings that depend on each other. Does this make sense? Changing A affects not only A but also B.

The question is simply: are you sure that none of the parameters added via create_parameters_from_node_tree are interlinked? If you say "yes", then I will be able to sleep safely at night.

Some background: For the UHF-LI a lot of parameters are interlinked. Changing one via the LabOne Interface immediately updates all its interlinked parameters in the LabOne interface, but extra care has to be taken to also update them in QCoDeS (basically, the set call of one parameter must also call get or save_val for other parameters). I am worried that create_parameters_from_node_tree does not take this extra care.

[qSaevar]

@WilliamHPNielsen I see what you are saying.

Short answer, no I'm not sure that no parameters are interlinked, it could very well be.

The method create_parameters_from_node_tree does not know about or takes care of such interlinking. It has just one job and that is to create a QCoDeS parameter for every read- or writable parameter on the device. So if you want to know the state of the instrument I highly recommend using the parameters get methods in order to get a fresh actual value from the device. And if creating a snapshot, then set the update flag to True so that you do get actual values and not outdated values due to possible interlinking of parameters.

[WilliamHPNielsen]

@qSaevar

Short answer, no I'm not sure that no parameters are interlinked, it could very well be.
The method create_parameters_from_node_tree does not know about or takes care of such interlinking

Okay, that's what I suspected. But you do not seem shocked, so I think we can take an approach of "it's not a problem until it becomes a problem". But maybe the snapshot of this instrument should always update?

[qSaevar]

@WilliamHPNielsen Should I then overwrite snapshot_base to force the instrument to always update?

[qSaevar]

@WilliamHPNielsen @astafan8 This has been tested on hardware. Is there anything that needs to be done so the driver can me merged?

[WilliamHPNielsen]

@qSaevar I think overwriting the snapshot_base is the sensible approach. Do that, and we'll merge.

[qSaevar]

@WilliamHPNielsen
I did override the snapshot_base method, and this has now been tested on hardware again