labscript-suite / labscript

The ๐—น๐—ฎ๐—ฏ๐˜€๐—ฐ๐—ฟ๐—ถ๐—ฝ๐˜ library provides a translation from expressive Python code to low-level hardware instructions.
http://labscriptsuite.org
BSD 2-Clause "Simplified" License
9 stars 48 forks source link

Incorrect comparison of time intervals between clocklines for error checking #85

Closed ispielma closed 2 years ago

ispielma commented 2 years ago

There are several issue reports (#53 and #51) that seem to relate to the same thing, which result from from a commit that I will touch on below. I will describe our issue here before describing the fix.

Emine and I were debugging an issue in the lab where we were pretriggering a novatech to get its update to occur at a desired time. This fails when any device on the same pulseblaster is clocked before the required 110 us delay between novatech updates, even if the are on different pseudoclock lines.

Here is the relevant code:

            # Check that no two instructions are too close together:

            for i, t in enumerate(change_time_list[:-1]):
                dt = change_time_list[i+1] - t
                if dt < 1.0/clock_line.clock_limit:
                    raise LabscriptError('Commands have been issued to devices attached to clockline %s at t= %s s and %s s. '%(clock_line.name, str(t),str(change_time_list[i+1])) +
                                         'One or more connected devices on ClockLine %s cannot support update delays shorter than %s sec.'%(clock_line.name, str(1.0/clock_line.clock_limit)))

                all_change_times_len = len(all_change_times)
                # increment j until we reach the current time
                while all_change_times[j] < t and j < all_change_times_len-1:
                    j += 1
                # j should now index all_change_times at "t"
                # Check that the next all change_time is not too close (and thus would force this clock tick to be faster than the clock_limit)
                dt = all_change_times[j+1] - t
                if dt < 1.0/clock_line.clock_limit:
                    raise LabscriptError('Commands have been issued to devices attached to %s at t= %s s and %s s. '%(self.name, str(t),str(all_change_times[j+1])) +
                                         'One or more connected devices on ClockLine %s cannot support update delays shorter than %s sec.'%(clock_line.name, str(1.0/clock_line.clock_limit)))

from line 826 of labscript.py. The problem is with the second error check (line 840) that compares the update time of all channels with the clock_line.clock_limit of the current channel. The resolution that worked for us is to replace this with the maximum clock_limit of all the clock_lines. It may have been better to replace with self.clock_limit to get the actual limit of the pseudoclock.

Looking through this history, this commit is the one that causes this bug https://github.com/labscript-suite/labscript/commit/56184458fbfdf04e5f3da6c9e19267c4e1d796b4 . From what I can see I think the issue is that for sequences using the 50/50 duty cycle pulses the change I propose above would change the time of the falling edge of the clock line.

One compromise would be keep the current check for the 50/50 duty cycle pulses and accept my change for the minimum duration pulses, but it seems to me that the better solution is to treat "returning edges" on the same footing as the change times and do an error check for these in this same code block.

The issue specifically is present on line 480 of the PB source code where we see

                if self.pulse_width == 'symmetric':
                    high_time = instruction['step']/2
                else:
                    high_time = self.pulse_width

so the PB code is deciding where the returning edges are located just before programming, whereas the more logical time to make this decision would have been during the high-level compilation stage not the low-level stage.