search

labscript-suite / labscript-devices

A modular and extensible plugin architecture to control experiment hardware using the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦.
http://labscriptsuite.org
Other
5 stars 60 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 4093ed4213969fb3b196699ed4c74ba3cb452660. 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 9e334987a0458e61085b5ca57af3435d4a3248ac>>