labscript-suite-temp / labscript_devices

Module containing labscript suite hardware compatibility, separate from the main programs. Device compatibility is implemented with a plugin architecture, for modularity and extensibility. Each file in this module contains a labscript device class, a BLACS tab class, a BLACS worker class and a runviewer parser class for a particular device. These implement functionality for the device which the programs in question call on when they encounter each device in user labscript code, hdf5 files, or connection tables.
0 stars 0 forks source link

Setting pulseblaster pulse_width has incorrect results; spurious error. #33

Closed philipstarkey closed 5 years ago

philipstarkey commented 5 years ago

Original report (archived issue) by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


If the PulseBlaster's pulse_width keyword argument is used, the shot ends up being approximately twice as long as it should be:

from labscript import start, stop, ClockLine, labscript_init
from labscript_devices.PulseBlaster import PulseBlaster

labscript_init('test.h5', new=True, overwrite=True)
PulseBlaster('pulseblaster', pulse_width=1e-6)
ClockLine('clockline', pseudoclock=pulseblaster.pseudoclock, connection='flag 0')

start()
stop(3)

import h5py
with h5py.File('test.h5') as f:
    duration = f['devices/pulseblaster/PULSE_PROGRAM']['length'].sum() / 1e9
    print(duration)

This prints 6.000053678714858 as the total duration of PulseBlaster instructions, although the requested stop time was 3 seconds.

This is a regression introduced by 8de5e8fb815b859102163c2cb724ac9d927dea2c. If I backout that changeset, the above prints the more expected value 3.0000429429718882.

Backing out the changeset had a minor merge conflict. It was trivial, but for what it's worth here is a commit that backs it out, if you want to see the diff without all the noise of what happened in between then and now.

There is also a spurious error raised if there is a single segment of the experiment in which no non-external clocks tick. The error is clearly intended to prevent you using pulse_width when there are no external clocks in use, but it is being tested on a per-instruction basis. It almost looks like the error checking can be removed with no consequence.

from labscript import start, stop, ClockLine, labscript_init, DigitalOut, DDS
from labscript_devices.PulseBlaster import PulseBlaster
from labscript_devices.NovaTechDDS9M import NovaTechDDS9M

labscript_init('test.h5', new=True, overwrite=True)
PulseBlaster('pulseblaster', pulse_width=1e-6)
ClockLine('clockline', pseudoclock=pulseblaster.pseudoclock, connection='flag 0')
NovaTechDDS9M('novatech', parent_device=clockline)
DDS('NTDDS', parent_device=novatech, connection='channel 1')
DigitalOut('DO', parent_device=pulseblaster.direct_outputs, connection='flag 1')

start()

DO.go_high(0.1)
NTDDS.setamp(0.2, 1.0)

stop(3)
Traceback (most recent call last):
  File "261.py", line 17, in <module>
    stop(3)
  File "/home/bilbo/labscript_suite/labscript/labscript.py", line 2337, in stop
    generate_code()
  File "/home/bilbo/labscript_suite/labscript/labscript.py", line 2213, in generate_code
    device.generate_code(hdf5_file)
  File "/home/bilbo/labscript_suite/labscript_devices/PulseBlaster.py", line 579, in generate_code
    pb_inst = self.convert_to_pb_inst(dig_outputs, dds_outputs, freqs, amps, phases)
  File "/home/bilbo/labscript_suite/labscript_devices/PulseBlaster.py", line 405, in convert_to_pb_inst
    raise LabscriptError('You cannot set a pulse_width for %s (%s) if it is not used as a pseudoclock for another device'%(self.name, self.description))
labscript.labscript.LabscriptError: You cannot set a pulse_width for pulseblaster (PB-DDSII-300) if it is not used as a pseudoclock for another device
philipstarkey commented 5 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


philipstarkey commented 5 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


It's possible that the lab using this feature doesn't even need it anymore. It was always a hack to use the short pulseblaster pulses to trigger the novatech off falling edges that were shortly after rising edges. I think they may have moved on to less hacky triggering of novatechs. In that case we can just remove the pulse width setting. I'll find out.

Clearly no other lab is using this feature, since this bug has existed for many years in mainline labscript.

philipstarkey commented 5 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


It turns out that both labs here do need this, so no removing it for now.

I think I can fix the issue though. If you're going to do any work on it @philipstarkey let me know, otherwise I'll have a go at it.

philipstarkey commented 5 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


Addressed by PR #67

philipstarkey commented 5 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


philipstarkey commented 5 years ago

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).


Resolve issue #33.

Pulseblasters now have a long delay time computed from their core clock freq, which has been added as an instance attribute to every subclass. (this is messy, as it is duplicated in the BLACS worker classes too. It should be passed as a connection table property ideally, but this will have to wait for a more general 'pulseblaster unification' change I think, along the lines of the NI DAQ unification).

For each clock tick, the high time is half the clock period if pulse_width='symmetric' (the default), the minimum possible if pulse_width='minimum', or a fixed value if pulse_width is given as a number.

The high time is then clipped to self.long_delay if it is larger, in which case the pulse_width argument is not honoured, but 57 second high times ought to be visible on scopes and not too fast for any devices, so this is preferable to the alternative of adding LONG_DELAY instructions to both the high and low times of the clock ticks.

The low time is then computed as whatever is left, and split into a LONG_DELAY instruction plus the remainder as an END_LOOP instruction, as before.

If the PulseBlaster has no external clocks for this clock tick, it uses a LONG_DELAY plus the remainder as a CONTINUE instruction, as before.

→ \<\<cset 0178a80aded7fa2c547f3ced1d55509808ec25d8>>