realtimeradio / souk-firmware

Simons Observatory UK RFSoC Firmware
GNU General Public License v3.0
3 stars 1 forks source link

Group Delay compensation #37

Closed jack-h closed 5 months ago

jack-h commented 8 months ago

The current (v6.3) system uses the same LO to generate tones and mix the received tones. No compensation is made for the travel time out the TX system, through the MKID, and through the RX system.

Compensating this delay as a true-time delay on the LO would be very expensive in RAM. Compensating this delay as a phase rotation on the LO would be less expensive. The group delay first needs obtaining through some calibration sweep, and then an LO-freq-dependent, but constant in time, rotation can be applied to the mixer in either the TX or RX paths. This rotation would need updating whenever the tone frequencies are tweaked, so would marginally slow the update rate.

However, applying this rotation is the same as rotating the data out of the RX accumulators, which can be performed at (at least) the same cadence as tone updates. Therefore (@sr-cdf) what is the tangible benefit of correcting in the firmware (other than it making the plots look prettier, though maybe this is reason enough)?

sr-cdf commented 7 months ago

TL;DR Software rotation is probs fine, but can we have independent RX and TX LOs anyway?

Updating the tone frequency to follow the moving resonator involves reading the RX accumulator, calculating the phase shift since the last reading, looking up how much frequency shift this corresponds to and applying the new frequency to the LO, with an update of the channel selector if necessary.

It is certainly important to correct the phase delay imparted by the system, two reasons come to mind:

  1. If the phase wraps between samples it will be difficult to estimate the frequency shift.

  2. If the system phase is changing with frequency faster than the resonator phase changes with frequency then it will be harder to track the resonator phase.

As you say, if there is a firmware complex multiplier to apply a rotation on the RX or TX LO (or the accumulator input) to correct for the system phase delay, the value of the rotation is frequency dependent and will need updating updating whenever we set a new frequency. A one time calibration sweep would suffice to inform us of the frequency dependent phase shift values.

Alternatively, we software complex multiply every sample out of the accumulator by the required system phase shift value in the main loop. The value could be pre-computed and stored an array somewhere in memory.

So, which takes up fewer clock cycles? Looking up 1000 formatted phase shift values and writing them to 1000 registers, or looking up 1000 phase shift integer values, and complex multiplying 1000 iq pairs in the software loop?

If software multiply if faster than firmware register reload, then I would agree there is no need for a firmware correction.

Alternatively, (and you may have hinted at this), we could have separate independent LOs for RX and TX, each with its own programmable offset. We can perform software rotation in the main loop, or apply independent offsets to each LO if it turns out to be useful.

The dual LO approach would also allow us sample a wider band around the resonators, by generating a 1GHz tone, receiving at 1GHz and accumulating down to 1Khz, then receiving at 1G+1k, 1G+2k 1G+3k... until we've measured the tone out to across the full resonator bandwidth.

jack-h commented 7 months ago

TL;DR -- Sure, I'll put in the rotation blocks as we've previously discussed.

Alternatively, we software complex multiply every sample out of the accumulator by the required system phase shift value in the main loop. The value could be pre-computed and stored an array somewhere in memory.

So, which takes up fewer clock cycles? Looking up 1000 formatted phase shift values and writing them to 1000 registers, or looking up 1000 phase shift integer values, and complex multiplying 1000 iq pairs in the software loop?

I'm not sure, but my guess is that writing to the firmware will be slower. I think my more general point was to make sure I'm not missing something in thinking that -- performance aside -- there is no difference between compensating for this effect before the accumulator (in firmware) or after the accumulator (in software). I think we're on the same page.

The dual LO approach would also allow us sample a wider band around the resonators, by generating a 1GHz tone, receiving at 1GHz and accumulating down to 1Khz, then receiving at 1G+1k, 1G+2k 1G+3k... until we've measured the tone out to across the full resonator bandwidth.

I don't understand this, but maybe it's easier to explain in real-time rather than here.

sr-cdf commented 7 months ago

Cool.

-- there is no difference between compensating for this effect before the accumulator (in firmware) or after the accumulator (in software). I think we're on the same page.

Right, I think that's right.

I don't understand this, but maybe it's easier to explain in real-time rather than here.

Maybe it is. There are a couple of other cases that come to mind that would benefit from independent RX and TX LOs. Lets arrange a call some time this week. Tomorrow?

jack-h commented 7 months ago

On Mon, 8 Apr 2024, 17:28 Sam Rowe, @.***> wrote:

Cool.

-- there is no difference between compensating for this effect before the accumulator (in firmware) or after the accumulator (in software). I think we're on the same page.

Right, I think that's right.

I don't understand this, but maybe it's easier to explain in real-time rather than here.

Maybe it is. There are a couple of other cases that come to mind that would benefit from independent RX and TX LOs. Lets arrange a call some time this week. Tomorrow?

I'm free all day, just drop me an email

Reply to this email directly, view it on GitHub https://github.com/realtimeradio/souk-firmware/issues/37#issuecomment-2043182613, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWM7G4SW76C7QXVW7CKIDY4LARZAVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBTGE4DENRRGM . You are receiving this because you authored the thread.Message ID: @.***>

jack-h commented 7 months ago

Sort of added this feature in v6.4, but, planning to abandon this in favour of v7.0 which makes the RX and TX LOs independent (they can be different frequencies). In this model it would also be possible to add a starting time delay that cancels out the TX->RX propagation time, at least in the DSP pipeline (if not in the ADC interface and external cabling) which forms the bulk of the delay.

jack-h commented 7 months ago

Give tag v7.1 a go and see how it looks. I've tried to propagate a sync pulse through the entire pipeline so that it should automatically compensate for delay in loopback mode. It won't compensate for delays outside the DSP firmware, but this could be accounted for in software or by adding a further programmable delay element to the sync pulses.

sr-cdf commented 6 months ago

@jack-h this does not appear to have eliminated the group delay.

Sweeping frequencies in loopback (with a sync.arm_sync(wait=False) and sync.sw_sync(wait=False) between steps) still shows large group delay, on the order of 11.6635 microseconds.

sr-cdf commented 6 months ago

Setting different values for sync_delay in mixer.set_delay() does not affect the group delay.

Should it?

jack-h commented 6 months ago

I think it should affect the delay only after a new arm/sync

On Thu, 16 May 2024, 15:27 Sam Rowe, @.***> wrote:

Setting different values for sync_delay in mixer.set_delay() does not affect the group delay.

Should it?

— Reply to this email directly, view it on GitHub https://github.com/realtimeradio/souk-firmware/issues/37#issuecomment-2115408042, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWM7ELI5UDWTPCOL5BKXDZCS663AVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJVGQYDQMBUGI . You are receiving this because you were mentioned.Message ID: @.***>

sr-cdf commented 6 months ago

Even with an arm/sync I see no difference.

On Thu, 16 May 2024 at 15:29, Jack Hickish @.***> wrote:

External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni.

I think it should affect the delay only after a new arm/sync

On Thu, 16 May 2024, 15:27 Sam Rowe, @.***> wrote:

Setting different values for sync_delay in mixer.set_delay() does not affect the group delay.

Should it?

— Reply to this email directly, view it on GitHub < https://github.com/realtimeradio/souk-firmware/issues/37#issuecomment-2115408042>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAKWM7ELI5UDWTPCOL5BKXDZCS663AVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJVGQYDQMBUGI>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frealtimeradio%2Fsouk-firmware%2Fissues%2F37%23issuecomment-2115410925&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C8833af078c5a4dde638d08dc75b48e18%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638514665545720601%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Jl64Z927aYkZZicJRf%2BLkoHkq7bpGfIycJ6wztPjmNY%3D&reserved=0, or unsubscribe https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEFQV42MURN5EYIORLUUYEDZCS7DNAVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJVGQYTAOJSGU&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C8833af078c5a4dde638d08dc75b48e18%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638514665545876840%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=ZtGSvcp%2Fpa6NSvb6do2jxBYmO0bMO5RHmO0afgKC9v4%3D&reserved=0 . You are receiving this because you were mentioned.Message ID: @.***>

sr-cdf commented 6 months ago

Try this:

import sys,os,time
import numpy as np

import souk_mkid_readout
r = souk_mkid_readout.SoukMkidReadout('krm4',configfile='/home/sam/souk/souk-firmware/software/control_sw/config/souk-single-pipeline-krm-2G.yaml')

r.program()
r.initialize()
r.adc_clk_hz =  r.config['adc_clk_hz']

import logging
r.accumulators[0].logger.setLevel(logging.ERROR)

r.fpga.print_status()
print(r.config)

r.output.use_psb()
r.psb.set_fftshift(0b1)
r.pfb.set_fftshift(0b11111111)
r.accumulators[0].set_acc_len(1000)

chans  = [0,1]
freqs  = np.array([1e9,1.5555555555e9])
amps   = np.array([1.0,1.0])
phases = np.array([0.0,np.pi])

delays = np.arange(19090-10,19090+11.,1.)
# delays = np.arange(1,20000,1000)

r.mixer.warning = r.mixer.logger.warning

print()
print('Set independent tx:')
r.mixer.set_independent_tx()
# print('Set independent tx (not offset or step):')
# r.mixer.set_independent_tx(sync=True, offset=False, scale=True, step=False)
print('r.mixer.get_tx_independence():',r.mixer.get_tx_independence())
print()
print('Enable loopback')
r.input.enable_loopback()

group_delays = np.zeros((len(delays),len(chans)))
pipeline_latencies = np.zeros((len(delays)))

for j in range(len(delays)):
    print()
    print('set tone 0')
    #set tones
    r.set_multi_tone(np.atleast_1d(freqs),amplitudes=np.atleast_1d(amps),phase_offsets_rads=np.atleast_1d(phases))
    r.sync.arm_sync(wait=False)
    r.sync.sw_sync(wait=False)
    time.sleep(0.1)

    print('Set sync delay',int(delays[j]))
    r.sync.set_delay(int(delays[j]))
    r.sync.arm_sync(wait=False)
    r.sync.sw_sync(wait=False)
    time.sleep(0.1)
    print('sync_delay:',r.sync.get_delay(),'pipeline_latency:',r.sync.get_pipeline_latency())

    z0 = r.accumulators[0].get_new_spectra()[:len(chans)]

    #set tones + 100 Hz
    print('set offset tone ')
    offset_hz = 100

    r.set_multi_tone(np.atleast_1d(freqs+offset_hz),amplitudes=np.atleast_1d(amps),phase_offsets_rads=np.atleast_1d(phases))
    r.sync.arm_sync(wait=False)
    r.sync.sw_sync(wait=False)
    time.sleep(0.1)

    print('Set sync delay',int(delays[j]))
    r.sync.set_delay(int(delays[j]))
    r.sync.arm_sync(wait=False)
    r.sync.sw_sync(wait=False)
    time.sleep(0.1)
    print('sync_delay:',r.sync.get_delay(),'pipeline_latency:',r.sync.get_pipeline_latency())
    pipeline_latencies[j] = r.sync.get_pipeline_latency()

    z1 = r.accumulators[0].get_new_spectra()[:len(chans)]

    gd = -(np.angle(z1)-np.angle(z0))/(2*np.pi*offset_hz)
    group_delays[j] = gd
    print('z0,z1,group_delay',z0,z1,gd)

print(delays)
print(group_delays)
jack-h commented 6 months ago

Perfect. Will have a look

On Thu, 16 May 2024, 16:05 Sam Rowe, @.***> wrote:

Try this:

import sys,os,time import numpy as np

import souk_mkid_readout r = souk_mkid_readout.SoukMkidReadout('krm4',configfile='/home/sam/souk/souk-firmware/software/control_sw/config/souk-single-pipeline-krm-2G.yaml')

r.program() r.initialize() r.adc_clk_hz = r.config['adc_clk_hz']

import logging r.accumulators[0].logger.setLevel(logging.ERROR)

r.fpga.print_status() print(r.config)

r.output.use_psb() r.psb.set_fftshift(0b1) r.pfb.set_fftshift(0b11111111) r.accumulators[0].set_acc_len(1000)

chans = [0,1] freqs = np.array([1e9,1.5555555555e9]) amps = np.array([1.0,1.0]) phases = np.array([0.0,np.pi])

delays = np.arange(19090-10,19090+11.,1.)

delays = np.arange(1,20000,1000)

r.mixer.warning = r.mixer.logger.warning

print() print('Set independent tx:') r.mixer.set_independent_tx()

print('Set independent tx (not offset or step):')

r.mixer.set_independent_tx(sync=True, offset=False, scale=True, step=False)

print('r.mixer.get_tx_independence():',r.mixer.get_tx_independence()) print() print('Enable loopback') r.input.enable_loopback()

group_delays = np.zeros((len(delays),len(chans))) pipeline_latencies = np.zeros((len(delays)))

for j in range(len(delays)): print() print('set tone 0')

set tones

r.set_multi_tone(np.atleast_1d(freqs),amplitudes=np.atleast_1d(amps),phase_offsets_rads=np.atleast_1d(phases))
r.sync.arm_sync(wait=False)
r.sync.sw_sync(wait=False)
time.sleep(0.1)

print('Set sync delay',int(delays[j]))
r.sync.set_delay(int(delays[j]))
r.sync.arm_sync(wait=False)
r.sync.sw_sync(wait=False)
time.sleep(0.1)
print('sync_delay:',r.sync.get_delay(),'pipeline_latency:',r.sync.get_pipeline_latency())

z0 = r.accumulators[0].get_new_spectra()[:len(chans)]

#set tones + 100 Hz
print('set offset tone ')
offset_hz = 100

r.set_multi_tone(np.atleast_1d(freqs+offset_hz),amplitudes=np.atleast_1d(amps),phase_offsets_rads=np.atleast_1d(phases))
r.sync.arm_sync(wait=False)
r.sync.sw_sync(wait=False)
time.sleep(0.1)

print('Set sync delay',int(delays[j]))
r.sync.set_delay(int(delays[j]))
r.sync.arm_sync(wait=False)
r.sync.sw_sync(wait=False)
time.sleep(0.1)
print('sync_delay:',r.sync.get_delay(),'pipeline_latency:',r.sync.get_pipeline_latency())
pipeline_latencies[j] = r.sync.get_pipeline_latency()

z1 = r.accumulators[0].get_new_spectra()[:len(chans)]

gd = -(np.angle(z1)-np.angle(z0))/(2*np.pi*offset_hz)
group_delays[j] = gd
print('z0,z1,group_delay',z0,z1,gd)

print(delays) print(group_delays)

— Reply to this email directly, view it on GitHub https://github.com/realtimeradio/souk-firmware/issues/37#issuecomment-2115500694, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWM7DTZYLLMD3C6C2TGLTZCTDKJAVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJVGUYDANRZGQ . You are receiving this because you were mentioned.Message ID: @.***>

jack-h commented 6 months ago

Oh, sorry, I've misled you. In this version of the firmware the set_delay method doesn't actually do anything (it is something of a previous experiment) though it will change the read back get_pipeline_latency. I.e., when the delay matches the latency (I read this as 19091 on my board) then pipeline_latency will read back as 0. But, the actual latency compensation is in the chaining of sync pulses, so whatever you do you should see group delay at least close to compensated. Which is to say, I think it's fine that the delay doesn't change, but it's not fine that it remains ~14 usec. Poking around now to see if I can see anything amiss

jack-h commented 6 months ago

And, naturally, the poking around leads me to issue #49

jack-h commented 6 months ago

Can you give branch v7.2 a go -- https://github.com/realtimeradio/souk-firmware/tree/v7.2

I've removed all the dependent/independent control, since I think it makes things a bit more complicated than I anticipated. I also reinstated the sync_delay logic, since the alternative also is more complicated than I thought.

When I run your script (I updated the fpg file in the yaml config, so you should be good to go), removed the now obsolete dependence setting, and change the delay sweep to cover 6224 plus/minus a bit, I see the group delay cross zero. What I'm confused about is that there is clearly some frequency-dependent phase in addition to the group delay, but maybe this all makes sense given the DSP

sr-cdf commented 6 months ago

Looks good to me.

Confirmed that ~6224 gives minimal group delay.

Not sure what you mean by seeing frequency dependent phases in addition to the group delay. Do you mean a variation in the zero-crossing point that changes with the tone frequency?

I do see an unusual variation in the group delay depending on the tone offset from the bin centers, and the phase offsets...

With 10 tones at exact bin centers and all phase offsets zero, there is a much larger scatter in the group delay compared with tones at increasing offset from bin centers and zero phase offsets. Not sure if I should be concerned about this. gd_tones_at_bincenters gd_tones_offset_from_bincenters

Also, the zero crossing seems to depend slightly on the phase offset value. Which maybe it should? But its a small effect, and it doesnt bother me enough right now to do the calculations. gd_tones_at_bincenters_increasing_phase gd_tones_offset_from_bincenters_increasing_phase

Averaging a bunch of accumulations seems to reduce the variation... gd_tones_offset_from_bincenters_increasing_phase_averaging

sr-cdf commented 6 months ago

Oh, and whats the deal with the pipeline latency value?

With sync_delay = 0, I get pipeline latency = 9283

And with sync_delay = 6224, I get pipeline latency = 3059, which do add up to 9283.

Where does this remaining pipeline latency come from?

jack-h commented 6 months ago

On Fri, 17 May 2024, 11:17 Sam Rowe, @.***> wrote:

Oh, and whats the deal with the pipeline latency value?

With sync_delay = 0, I get pipeline latency = 9283

And with sync_delay = 6224, I get pipeline latency = 3059, which do add up to 9283.

Where does this remaining pipeline latency come from?

The pipeline latency reports how long a sync pulse takes to propagate through the system, and takes into account what the sync_delay is set to. So, it shouldn't change when you change sync_delay.

But, I'm very surprised that the sync_delay required to make the group delay disappear is not 9283. That's why the previous version of the firmware, which used the sync propagation delay to offset the tx and Rx LOs didn't work.

I'm also very surprised that the zero-offset-from-bin-center frequencies don't have perfectly straight delay-vs-sync_delay lines, with frequency-independent zero crossing. In fact, the behaviour of the offset-from-bin-center plots make more sense to me, with straight lines but some phase variation across a PFB bin.

— Reply to this email directly, view it on GitHub https://github.com/realtimeradio/souk-firmware/issues/37#issuecomment-2117231077, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWM7BKM5GLE3APBWKWEJLZCXKJ5AVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJXGIZTCMBXG4 . You are receiving this because you were mentioned.Message ID: @.***>

sr-cdf commented 6 months ago

So, it shouldn't change when you change sync_delay.

Shouldn't? Or should?

But, I'm very surprised that the sync_delay required to make the group delay disappear is not 9283. That's why the previous version of the firmware, which used the sync propagation delay to offset the tx and Rx LOs didn't work.

I see. That is surprising.

I'm also very surprised that the zero-offset-from-bin-center frequencies don't have perfectly straight delay-vs-sync_delay lines, with frequency-independent zero crossing. In fact, the behaviour of the offset-from-bin-center plots make more sense to me, with straight lines but some phase variation across a PFB bin.

That's what I'd expect too.

I'm glad the group delay is now down at the 10s of nanoseconds, rather than microseconds, that should make life a lot easier.

I'll let you know if come across any issues with it varying like it does.

jack-h commented 6 months ago

On Fri, 17 May 2024, 11:41 Sam Rowe, @.***> wrote:

So, it shouldn't change when you change sync_delay.

Shouldn't? Or should?

Shouldn't - the software measures the observed delay, subtracts the loaded sync_delay, and returns the result.

But, I'm very surprised that the sync_delay required to make the group

delay disappear is not 9283. That's why the previous version of the firmware, which used the sync propagation delay to offset the tx and Rx LOs didn't work.

I see. That is surprising.

In the spirit of fearing the unknown, I'd blame the synthesis bank, but I don't have any particular theories. Hopefully sitting down with the math would give some answers.

I'm also very surprised that the zero-offset-from-bin-center frequencies don't have perfectly straight delay-vs-sync_delay lines, with frequency-independent zero crossing. In fact, the behaviour of the offset-from-bin-center plots make more sense to me, with straight lines but some phase variation across a PFB bin.

That's what I'd expect too.

I'm glad the group delay is now down at the 10s of nanoseconds, rather than microseconds, that should make life a lot easier.

I'll let you know if come across any issues with it varying like it does.

:+1:

Reply to this email directly, view it on GitHub https://github.com/realtimeradio/souk-firmware/issues/37#issuecomment-2117281880, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWM7HPZAFJOT5IG52F3EDZCXNG3AVCNFSM6AAAAABE7HBJCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJXGI4DCOBYGA . You are receiving this because you were mentioned.Message ID: @.***>