openquantumhardware / qick

QICK: Quantum Instrumentation Control Kit
MIT License
199 stars 84 forks source link

Resetting ADC phase #120

Closed sebastianhorvath closed 1 year ago

sebastianhorvath commented 1 year ago

Hi all, I ran into an issue trying to use the QickProgram reset_phase method for readout channels. Everything worked as expected for DAC channels, but when I included a readout channel I got an exception suggesting that ch_mgr is None:

  2093                 # keeps a record of the last set registers and the default registers
-> 2094                 last_set_regs_ = ch_mgr.last_set_regs
   2095                 defaults_regs_ = ch_mgr.defaults
   2096                 # temporarily ignore the default registers

AttributeError: 'NoneType' object has no attribute 'last_set_regs'

This was using version 7fc89853 of Qick. I'm not totally sure if this is an issue with reset_phase or something in my code. Here's a minimal example that had this issue:

class LoopbackProgram(AveragerProgram):
    def __init__(self,soccfg,cfg):
        super().__init__(soccfg,cfg)

    def initialize(self):
        cfg=self.cfg            
        self.declare_gen(ch=cfg['aom_ch'], nqz=1, mixer_freq=cfg['mixer_freq'])
        self.declare_readout(ch=cfg['ro_ch'], freq=cfg['demod_freq'], length=cfg['readout_length'], gen_ch=cfg['aom_ch'])

        freq=self.freq2reg(cfg['pulse_freq'], gen_ch=cfg['aom_ch'], ro_ch=cfg['ro_ch'])  
        self.set_pulse_registers(ch=cfg['aom_ch'], freq=freq, style='const', phase=0, gain=self.cfg['init_gain'], length=cfg['length'])
        self.synci(200) 

    def body(self):
        if self.cfg['phase_rst']:
            self.reset_phase(gen_ch = [self.cfg['aom_ch']], ro_ch = [self.cfg['ro_ch']])
        self.trigger(adcs=self.ro_chs,pins=[0], adc_trig_offset=self.cfg['adc_trig_offset']) 
        self.pulse(ch=self.cfg['aom_ch']) 
        self.wait_all(50)

config={'aom_ch':4, 
        'mixer_freq':0.0, # MHz
        'reps':1,
        'length' : 50,
        'ro_ch':2, #

        'init_gain':30000, # [DAC units]

        'pulse_freq': 100, # [MHz]
        'demod_freq': 100,
        'readout_length':100,
        'read_delay':50,

        'adc_trig_offset': 100+20,  #[Clock ticks]
        'soft_avgs':10,

        'phase_rst': True,
       }

prog =LoopbackProgram(soccfg, config)
soc.reset_gens() 
iq_list = prog.acquire_decimated(soc, load_pulses=True, progress=False, debug=False)

I also had a slightly more general question in this context: looking over the code it seems like the ADC DDS phase is reset whenever the DAC DDS phase is reset for the channel on the same page. Is this correct, or is there some other means for resetting the ADC phase?

Thanks a lot for your help!

meeg commented 1 year ago

Unless you are using tproc-controlled readouts (currently only available in a special firmware that isn't in wide distribution), you won't be able to use reset_phase to reset readout phases - in fact there is no way to reset readout phases of PYNQ-controlled readouts.

The output of print(soccfg) will tell you whether your readouts are tproc-controlled or PYNQ-controlled:

    4 readout channels:
    0:  axis_pfb_readout_v2 - controlled by PYNQ
        ADC tile 0, ch 0, 35-bit DDS, fabric=512.000 MHz, fs=4096.000 MHz
        maxlen 16384 (avg) 1024 (decimated), trigger bit 12, tProc input 0
    1:  axis_pfb_readout_v2 - controlled by PYNQ
        ADC tile 0, ch 0, 35-bit DDS, fabric=512.000 MHz, fs=4096.000 MHz
        maxlen 16384 (avg) 1024 (decimated), trigger bit 13, tProc input 1
    2:  axis_pfb_readout_v2 - controlled by PYNQ
        ADC tile 0, ch 0, 35-bit DDS, fabric=512.000 MHz, fs=4096.000 MHz
        maxlen 16384 (avg) 1024 (decimated), trigger bit 14, tProc input 2
    3:  axis_pfb_readout_v2 - controlled by PYNQ
        ADC tile 0, ch 0, 35-bit DDS, fabric=512.000 MHz, fs=4096.000 MHz
        maxlen 16384 (avg) 1024 (decimated), trigger bit 15, tProc input 3

So which kind of readout are you using? From your program I guess that it is PYNQ-controlled, and so we probably need a more descriptive error message. If it's tproc-controlled I need to dig a little.


The intended effect of reset_phase is to reset all the specified channels simultaneously. I don't think pages should matter, and you should be able to only reset ADCs or only reset DACs? Can you walk me through your thinking?

Practically speaking, why would you want to reset ADC phases on their own? You might want to reset multiple DACs simultaneously to sync their phases, and you might want to reset DACs and their coupled ADCs simultaneously to sync up the DACs and maintain the readout phase synchronization, but what's the use case where you'd want to reset one or multiple ADCs?

sebastianhorvath commented 1 year ago

Thanks that's very helpful. I didn't realize there were two distinct readout controls -- I now see that my readouts are all PYNQ controlled.

I think the fact that ADC phase reset only works with a tproc controlled readouts also clears up my confusion about whether the register page matters. I noticed that both the ADC and DAC phase is reset via a set instruction which with my firmware would always affect the DAC phase (and I incorrectly assumed that the ADC phase reset may also be triggered by this).

To explain why I want reset the ADC phases I'll have to briefly outline what we're physically trying to set up, which is unfortunately a bit convoluted. We are using two DAC tones (108 and 112 MHz at the moment) to modulate two laser beams using acousto-optic modulators. The frequency shifted lasers are then mixed with an unshifted laser yielding two beat notes with frequencies matching the DAC frequencies.

Electrically, all that really matters is that we obtain two tones at 108 MHz and 112 MHz, but the phase of these tones can drift due to path length changes of the laser. We're then using the RF SoC to perform sample and hold PID to stabilize the relative phase of these two tones (and therefore the phase of the two lasers). This all seems to work, and dumping some feedback parameters to memory seems to confirm that the control loop converges. However, this setup is pulsed (typically ~100 us long), and we sample and adjust the phase many times during a pulse. The trouble is that we'd like to reset the ADC phase at the start of the pulse to synchronize it with a generated trigger (since we need to time other operations phase coherently with respect to the beat note).

I hope that explanation makes some sense. We are also resetting the DAC phases of the relevant channels at the start of the pulse, but since we're trying to synchronize the phase with respect to a trigger, doing a soft reset of all ADC and DAC phases at the start of the sequence isn't sufficient. How involved is it to try out the ADC reset firmware?

meeg commented 1 year ago

OK, I think that makes sense. If I understand correctly, then, this is one of those cases where you want to reset both a pair of DACs and their coupled ADCs, all at the same time.

The ADC reset firmware for the 216 is at https://www.slac.stanford.edu/~meeg/qick/fw/2022-10-03_216_test-phase/ with a basic demo notebook. This is a proof of concept but maybe it will work for you.

I don't understand what you mean by a soft phase reset. How were you doing that?

sebastianhorvath commented 1 year ago

Thanks Sho, I've done some initial testing (just trying out your notebook so far) and I think this should work!

Unfortunately we would need an additional readout channel to try this out with our hardware, since we're stabilizing the phase of two separate tones and need to sync the phases of both ADCs.

In total, we would need firmware with three DACs and three ADCs, although only two ADCs need phase reset capability (the third ADC is for amplitude stabilization).

And sorry, soft reset was poorly chosen terminology on my part -- I meant using force_init_clks=True when loading the bitstream, which I assume also syncs the phase of all DACs and ADCs.

meeg commented 1 year ago

I think we can make you that firmware. Is it OK if it takes a couple of weeks?

Loading the bitstream will reset everything in the FPGA logic, so the DAC and ADC phases will be re-initialized to what are effectively random values. If that works for you, great! But it might not be what you thought you were doing.

force_init_clks=True forces reconfig/reset of the reference oscillator (on-board, off-FPGA) which drives the DAC and ADC clocks. I don't think you actually want to do this - it's harmless but there is no benefit in your case, and it takes a few seconds.

sebastianhorvath commented 1 year ago

Great, thank you. Given the overhead of putting together a new firmware revision on your part, let me discuss this with a couple of people just to be sure we have all of our requirements covered. It would be unfortunate if I missed some requirement in the new firmware that we've been taking for granted in the old revision (one thing that comes to mind is that we're using several PMOD input triggers).

I'll figure out exactly what we'd need for the complete system, and then get back to you. In terms of timing, while this does put that particular project on hold until we have the new firmware we do understand you guys have are supporting a lot of groups, and we really appreciate all the help!

Yep, I agree that resetting all the phases to random values unfortunately won't suffice. I mentioned this as it was something we initially tried and then realized it was not sufficient. Also, that's good to know about force_init_clocks, I'll disable that and save the extra seconds.

sebastianhorvath commented 1 year ago

I've gone through all our requirements with the people involved in this experiment and think aside from having two readouts controlled via the tproc our the current revision of the qick_amo firmware covers everything!

This translates to the following key items: 3 x full sig gen 2 x tproc controlled ADCs 1 x pynq controlled ADC 2 x PMOD trigger output 2 x PMOD trigger input

The trigger input capability was an add on to the original qick_amo firmware: https://github.com/sarafs1926/QICK_ThompsonLab/commit/8b8e4f65741f5a89d440e8fa5f81408bdc107252

There's two remaining tproc outputs, and if it's not too much extra effort retaining the 4x interpolated sig gens like we have in the original amo firmware would be helpful (and the extra pynq controlled I/Q outputs in the original firmware would also be helpful, but not essential).

For quick reference, here's the current config we're using:

QICK configuration:

    Board: ZCU216

    Global clocks (MHz): tProcessor 349.997, RF reference 245.760

    7 signal generator channels:
    0:  axis_sg_int4_v1 - tProc output 0, envelope memory 4096 samples
        DAC tile 2, ch 0, 16-bit DDS, fabric=430.080 MHz, fs=1720.320 MHz
    1:  axis_sg_int4_v1 - tProc output 1, envelope memory 4096 samples
        DAC tile 2, ch 1, 16-bit DDS, fabric=430.080 MHz, fs=1720.320 MHz
    2:  axis_sg_int4_v1 - tProc output 2, envelope memory 4096 samples
        DAC tile 2, ch 2, 16-bit DDS, fabric=430.080 MHz, fs=1720.320 MHz
    3:  axis_sg_int4_v1 - tProc output 3, envelope memory 4096 samples
        DAC tile 2, ch 3, 16-bit DDS, fabric=430.080 MHz, fs=1720.320 MHz
    4:  axis_signal_gen_v6 - tProc output 4, envelope memory 65536 samples
        DAC tile 3, ch 0, 32-bit DDS, fabric=599.040 MHz, fs=9584.640 MHz
    5:  axis_signal_gen_v6 - tProc output 5, envelope memory 65536 samples
        DAC tile 3, ch 1, 32-bit DDS, fabric=599.040 MHz, fs=9584.640 MHz
    6:  axis_signal_gen_v6 - tProc output 6, envelope memory 65536 samples
        DAC tile 3, ch 2, 32-bit DDS, fabric=599.040 MHz, fs=9584.640 MHz

    4 constant-IQ outputs:
    0:  DAC tile 1, ch 0, fs=6881.280 MHz
    1:  DAC tile 1, ch 1, fs=6881.280 MHz
    2:  DAC tile 1, ch 2, fs=6881.280 MHz
    3:  DAC tile 1, ch 3, fs=6881.280 MHz

    3 readout channels:
    0:  axis_readout_v2 - controlled by PYNQ
        ADC tile 2, ch 0, 32-bit DDS, fabric=307.200 MHz, fs=2457.600 MHz
        maxlen 1024 (avg) 1024 (decimated), trigger bit 4, tProc input 0
    1:  axis_readout_v2 - controlled by PYNQ
        ADC tile 2, ch 1, 32-bit DDS, fabric=307.200 MHz, fs=2457.600 MHz
        maxlen 1024 (avg) 1024 (decimated), trigger bit 5, tProc input 1
    2:  axis_readout_v2 - controlled by PYNQ
        ADC tile 2, ch 2, 32-bit DDS, fabric=307.200 MHz, fs=2457.600 MHz
        maxlen 1024 (avg) 1024 (decimated), trigger bit 6, tProc input 2

    11 DACs:
        DAC tile 1, ch 0 is 0_229, on JHC1
        DAC tile 1, ch 1 is 1_229, on JHC2
        DAC tile 1, ch 2 is 2_229, on JHC1
        DAC tile 1, ch 3 is 3_229, on JHC2
        DAC tile 2, ch 0 is 0_230, on JHC3
        DAC tile 2, ch 1 is 1_230, on JHC4
        DAC tile 2, ch 2 is 2_230, on JHC3
        DAC tile 2, ch 3 is 3_230, on JHC4
        DAC tile 3, ch 0 is 0_231, on JHC3
        DAC tile 3, ch 1 is 1_231, on JHC4
        DAC tile 3, ch 2 is 2_231, on JHC3

    3 ADCs:
        ADC tile 2, ch 0 is 0_226, on JHC7
        ADC tile 2, ch 1 is 1_226, on JHC8
        ADC tile 2, ch 2 is 2_226, on JHC7

    4 digital output pins (tProc output 7):
    0:  PMOD0_0_LS
    1:  PMOD0_1_LS
    2:  PMOD0_2_LS
    3:  PMOD0_3_LS

    tProc: program memory 1024 words, data memory 1024 words
        external start pin: PMOD1_0_LS

Let me know if that seems reasonable, or if some changes would make life easier for you. Thank you!

meeg commented 1 year ago

Hi, sorry about the silence but I've been busy. But this should be everything you asked for: https://www.slac.stanford.edu/~meeg/qick/fw/2023-03-23_216_testphase_q3diamond/

I've barely tested this, but if you see anything weird please let me know.

sebastianhorvath commented 1 year ago

Awesome, thank you! I'll probably not get a chance to work try this out until mid next week, but will let you know if there's any issues.

Will close this for now and open a new issue if anything does come up.

sebastianhorvath commented 1 year ago

Hi Sho, I got around to implementing the new ADC reset functionality into our larger codebase and unfortunately did run into an issue.

In particular, when down converting at lowish frequencies the demodulation has some additional frequency component. With the old firmware we noticed that for a demod frequency of <80 MHz we got some 2*IF components leaking through the FIR filter after the ADC mixer. The optical design we ultimately came up with yielded a beatnote of 110 MHz, so this turned out to be a problem we could avoid. However, is it possible the FIR design for this new firmware has a higher cutoff?

The short example below shows the issue when setting both the pulse and demodulation frequency to ~110 MHz, but works fine above 150 MHz.

class LoopbackProgram(AveragerProgram):
    def __init__(self,soccfg,cfg):
        super().__init__(soccfg,cfg)

    def initialize(self):
        cfg=self.cfg   

        freq=self.freq2reg(cfg['pulse_freq'], gen_ch=cfg['aom_ch'])          
        self.declare_gen(ch=cfg['aom_ch'], nqz=1, mixer_freq=cfg['mixer_freq'], ro_ch=cfg['ro_ch'])
        self.set_pulse_registers(ch=cfg['aom_ch'], freq=freq, style='const', phase=cfg['phase'], gain=cfg['init_gain'], length=self.us2cycles(cfg['pulse_length'], gen_ch=cfg['aom_ch']))

        ro_ch = cfg['ro_ch']
        length_ro = soccfg.us2cycles(cfg['readout_length'], ro_ch=ro_ch)
        self.declare_readout(ch=ro_ch, length=length_ro)
        freq_ro = self.freq2reg_adc(cfg['demod_freq'], ro_ch=ro_ch, gen_ch=cfg['aom_ch'])
        self.set_readout_registers(ch=ro_ch, freq=freq_ro, length=5)
        self.synci(200) 

    def body(self):
        self.readout(ch=self.cfg['ro_ch'], t=0)

        self.trigger(adcs=[self.cfg['ro_ch']], pins=[0], adc_trig_offset=self.us2cycles(self.cfg['adc_trig_offset']))  
        self.pulse(ch=self.cfg['aom_ch']) 

        self.waiti(0, self.us2cycles(self.cfg['adc_trig_offset'])) 
        self.sync_all(self.us2cycles(self.cfg['readout_length']) + self.us2cycles(self.cfg['pulse_length']))

config={'aom_ch':0, 
        'mixer_freq':0.0, # MHz
        'ro_ch':0,

        'readout_length': 1, # [us]
        'init_gain': 25000, # [DAC units]

        'pulse_freq': 100, # [MHz]
        'pulse_length': 0.5, #[us]
        'phase':0,
        'demod_freq': 100, # [MHz]

        'adc_trig_offset': 0.2, #[us]
        'soft_avgs':1,
        'reps':1, 
       }

prog =LoopbackProgram(soccfg, config)
iq_list = prog.acquire_decimated(soc, load_pulses=True, progress=True, debug=False)

plt.figure(1)
for ii, iq in enumerate(iq_list):
    plt.plot(iq[0], label="I value, ADC")
    plt.plot(iq[1], label="Q value, ADC")
    plt.plot(np.abs(iq[0]+1j*iq[1]), label="mag, ADC")
plt.ylabel("a.u.")
plt.xlabel("Clock ticks")
plt.title("Averages = " + str(config["soft_avgs"]))
plt.legend()
meeg commented 1 year ago

Yes, you are correct - the cutoff comes from the decimation filter, and the phase-resettable readout (v3) has a 1/4 decimation factor instead of 1/8. Roughly speaking, your max IF is (sampling freq)/(2, for Nyquist)/(decimation factor)/(2, for the 2*IF spur). So before, the cutoff on IF was roughly 2457.6 MHz/2/8/2 = 76.8, and now it's 1966.08 MHz/2/4/2 = 122.88. The different decimation factor is needed in order for the readout and generators to have the same fabric clock.

Is this something you can live with by changing frequencies in the rest of your setup? Assuming not:

sebastianhorvath commented 1 year ago

Ah that makes sense. Unfortunately changing the frequency in the rest of the setup isn't very feasible, since the acousto-optic modulators have a narrow bandwidth. However, lowering the sample rate seems like a good solution.

Of the scenarios I can foresee us implementing, I don't believe we'd need to go below 50 MHz. If you'd be able to re-compile the firmware with that as a target cut-off that would be great!

In the long run a bespoke filter for low frequency applications might be good, but since we're still just working the parameter space as we're implementing various components I think it makes sense to go for the lowest effort workaround.

Thanks a lot for the help!

meeg commented 1 year ago

OK, try this: https://www.slac.stanford.edu/~meeg/qick/fw/2023-04-03_216_testphase_q3diamond/

I dropped the sampling freqs by a factor of 3/8, for what should be a min IF of around 46.08 MHz.

sebastianhorvath commented 1 year ago

Cool, thank you! This now works for IF = 110 MHz.

sebastianhorvath commented 1 year ago

Hi Sho, I have a another follow up about the new firmware. I can mention that all ADC phase reset features from you firmware are working and we actually got our laser phase locking scheme implemented and working!

We're now in the process of integrating this into our main AMO experiment, and one feature we need for this is the ability to flag different experimental conditions to the tproc using the PMOD input. Sara implemented something like that for us here: https://github.com/sarafs1926/QICK_ThompsonLab/commit/8b8e4f65741f5a89d440e8fa5f81408bdc107252

As far as I can tell the new firmware doesn't include this input, unless it doesn't show up for print(soc)? If it's not in there at the moment, how hard would it be to include this change into this firmware revision?

Thank you!

meeg commented 1 year ago

Pretty good!

The inputs should be there - QickConfig just doesn't know how to print digital inputs (that was the case for your previous firmware also, no?). I think what I listed in https://www.slac.stanford.edu/~meeg/qick/fw/2023-04-03_216_testphase_q3diamond/README.txt is correct - PMOD1_4 through _7 go to tProc input 3.

sebastianhorvath commented 1 year ago

Ah thank you, I should have looked at your readme more carefully!

We admittedly never actually ended up needing that feature until now, so I hadn't tested it in the past and therefore never noticed it was unlisted.

Unfortunately this means I've also only now realized that there may be an issue with the current implementation. Reading the lower 32 bits, I'm just getting that all four pins are high irrespective of the input. Given this is now quite off topic from the original issue, I'll perhaps close this out and open a new issue.