realtimeradio / souk-firmware

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

PFB or PSB filter response not quite right #36

Closed sr-cdf closed 7 months ago

sr-cdf commented 7 months ago

Hi @jack-h ,

I'm working on v6.3 on the KRM board, and looking at the frequency response of a tone as I sweep the mixer frequency, and cross over to the next channel.

I'm using r.set_multi_tone() and modified versions of souk_poll_acc.py and souk_recv_acc.py in a loop to step and integrate each point in the sweep.

I expect a relatively flat response as we saw with the 4x2 in version 5, but instead I see a large rolloff come in when when i'm only about a quarter way across the filterbank channel. Its as if the channel response function is being squeezed in by a factor of 2 or 4.

The result is identical with internal loopback disabled and enabled.

Spectrum analyser measurements show the the same change in magnitude as a function of frequency, which suggests to me the issue is with the synthesis bank (or the way i'm using it), and that the rx filterbank is working fine.

Here is the response, magnitude_dB is in the top right:

(oh and ignore the fact that the neighboring channels have slightly higher magnitude than the central channel. I'm pretty sure this is just amplitude imbalance in the power combiner that combines the DAC0 and DAC1 (even/odd) physical outputs.)

image

jack-h commented 7 months ago

I've also generalized to PFB sweep to be able to use the different on-board generators. Using the CORDIC generator I get:

python ospfb_sweep.py 10.10.10.106 ../config/souk-single-pipeline-krm.yaml -o cordic Screenshot from 2024-03-18 14-09-24

Using the PSB generator I get:

python ospfb_sweep.py 10.10.10.106 ../config/souk-single-pipeline-krm.yaml -o psb Screenshot from 2024-03-18 14-10-55 Screenshot from 2024-03-18 14-14-17

So something is clearly up with the PSB out of band rejection. I'm a bit confused because the Y-axis scale is dBs, so I don't see how the ~0.12dB separation between bins (which have been through both the TX and RX PSB/PFBs) is consistent with the much larger rolloff in the plot at the start of this issue.

Thus far I can't see anything wrong in simulation or by staring at the code. I'm compiling in some new debug features to see if I can learn anything helpful.

sr-cdf commented 7 months ago

Have you tried it with internal loopback disabled?

sr-cdf commented 7 months ago

I just pulled the latest v6 and the results with the psb are not quite the same as yours...

python ospfb_sweep.py krm4 ../config/souk-single-pipeline-krm.yaml -o cordic ospfb_sweep_cordic

python ospfb_sweep.py krm4 ../config/souk-single-pipeline-krm.yaml -o psb ospfb_sweep_psb ospfb_sweep_psb_zoom

jack-h commented 7 months ago

Thanks for the sanity check. Can you confirm the git hash of your repo, and that you have installed the latest souk control code? I think with that config file you should have got firmware souk_single_pipeline_krm_2024-02-15_1525.fpg -- is that right?

jack-h commented 7 months ago

Have you tried it with internal loopback disabled?

not yet.... (though I have been looking at a spectrum analyzer and seeing things which clearly aren't right)

sr-cdf commented 7 months ago

Can you confirm the git hash of your repo, and that you have installed the latest souk control code?

My bad, I didnt install it after pulling it. I see the same as you.

jack-h commented 7 months ago

Can you confirm the git hash of your repo, and that you have installed the latest souk control code?

My bad, I didnt install it after pulling it. I see the same as you.

Thanks. If you happen to know which version your previous results are from maybe I can learn something from that, but good to know we at least see the same thing with the same code.

I'm looking at the CORDICs which generate the input tones to the PSB and seeing phase ramps which don't look right (as in, they aren't ramps) so I think that's where the problem is. However, I can't yet come up with a simulation which breaks things. Hopefully will get somewhere today.

sr-cdf commented 7 months ago

But with loopback disabled:

python ospfb_sweep.py krm4 ../config/souk-single-pipeline-krm.yaml -o cordic

ospfb_sweep_cordic_loopback_disabled ospfb_sweep_cordic_loopback_disabled_zoom

python ospfb_sweep.py krm4 ../config/souk-single-pipeline-krm.yaml -o psb

ospfb_sweep_psb_loopback_disabled ospfb_sweep_psb_loopback_disabled_zoom

sr-cdf commented 7 months ago

These lastest plots:

$ git rev-parse HEAD
b3f8908362fccddf08b8fc091cac3f071912a5d0

The original post:

$ git rev-parse HEAD
b3f8908362fccddf08b8fc091cac3f071912a5d0
jack-h commented 7 months ago

Weird, thanks. I guess I'll first try and make the loopback make sense -- there are a few other things which could be at play with the non loopback system.

jack-h commented 7 months ago

Finally getting somewhere. Some info you don't need but I'll share anyway: the CORDIC generator turns a phase into a sine/cosine and is configured to accept phase in "scaled radians" defined per the manual:

image

Now, the input must have 3 integer bits (presumably for consistency with the interface which can run between -pi and +pi) but it turns out the CORDIC will not operate correctly if the input is outside the range -1 to +1. Previous iterations of the PSB CORDICs fluked satisfying this condition. Firmware v6 (which changes the way the CORDICs are configured to satisfy the "truly arbitrary" output tone ordering) fell into this trap. With an appropriate simulation I can see the issue and see the fix working. In a couple of hours I'll have a firmware build and hopefully things will start working again.

I think that at small offsets from a bin center, the CORDIC is either working (in the +/-1 range) or not working (outside this range) but doesn't often transition between the two. As a result, my hypothesis is you get the correct tone, plus probably some spurs and/or noise. For ~0.25 bins offset, the CORDIC constantly transitions between states, so fails to properly generate the correct tone, even in short pulses. Fun.

jack-h commented 7 months ago

These lastest plots:

$ git rev-parse HEAD
b3f8908362fccddf08b8fc091cac3f071912a5d0

The original post:

$ git rev-parse HEAD
b3f8908362fccddf08b8fc091cac3f071912a5d0

Are these meant to be the same or is it a copy-paste error?

sr-cdf commented 7 months ago

Sounds very promising!

Copy paste error, sorry! The original post was with

$ git rev-parse HEAD
109a515184c8824b6a7b2edbd944b53eb1cf3011
jack-h commented 7 months ago

OK v6.3.0.1, just committed. python ospfb_sweep.py 10.10.10.106 ../config/souk-single-pipeline-krm_test.yaml -o psb

Screenshot from 2024-03-19 13-49-59

Screenshot from 2024-03-19 13-52-27

I'm still not quite happy with the details of the response, particularly the small reduction of power between the offset and non-offset bins, but I think the major bug is squashed.

sr-cdf commented 7 months ago

Woop woop!

Response looks good using the accumulator too:

image

Again, I think my power combiner has poor amplitude and phase balance.

However, the odd bins appear to be a bit more noisy than the even bins.

I'll try with internal loopback now...

sr-cdf commented 7 months ago

Yes, defintiely something not right with the odd bins:

Loopback enabled:

image

jack-h commented 7 months ago

Ok, I'll look into that next....

On Tue, 19 Mar 2024, 14:52 Sam Rowe, @.***> wrote:

Yes, defintiely something not right with the odd bins:

Loopback enabled:

image.png (view on web) https://github.com/realtimeradio/souk-firmware/assets/17500915/2546adca-db41-42c5-9fae-064a267221c9

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

sr-cdf commented 7 months ago

Let me know if I can do anything to help

jack-h commented 7 months ago

There is definitely something up with the odd bins:

In [342]: r.set_tone(0, 300000*3.2 + r.adc_clk_hz/2.)
2024-03-19 15:34:53,194 - souk_mkid_readout.blocks.block:10.10.10.146:p0_synth_input_reorder - INFO - Setting output 0 to channel -1
2024-03-19 15:34:53,196 - souk_mkid_readout.blocks.block:10.10.10.146:p0_chan_select - INFO - Setting output 0 to channel 3
2024-03-19 15:34:53,204 - souk_mkid_readout.blocks.block:10.10.10.146:p0_synth_input_reorder - INFO - Setting output 3 to channel 0

In [343]: r.adc_snapshot.plot_adc_snapshot()

image

I see glitches on all the odd bins I've tried, but none of the even ones. Digging in now.

jack-h commented 7 months ago

Got it. Rebuild in progress....

jack-h commented 7 months ago

python ./ospfb_sweep.py 10.10.10.146 ../config/souk-single-pipeline-krm.yaml -o psb

Firmware v6.3.0.2 (souk_single_pipeline_krm_2024-03-19_2005.fpg)

image

image

Fixed!

sr-cdf commented 7 months ago

Beautiful!

Dare I ask what the problem was?

I'll check the noise in the accumulator a bit later this morning.

On Tue, 19 Mar 2024, 21:32 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.

python ./ospfb_sweep.py 10.10.10.146 ../config/souk-single-pipeline-krm.yaml -o psb

Firmware v6.3.0.2 (souk_single_pipeline_krm_2024-03-19_2005.fpg)

image.png (view on web) https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frealtimeradio%2Fsouk-firmware%2Fassets%2F1402492%2F74c93313-fb0b-4de0-a2b2-4b49a013aaf3&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=HCWEXo7ZxeFq9jpe%2BWGnzpZ%2Fg%2FygsA%2BKHml862TZYhA%3D&reserved=0

image.png (view on web) https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frealtimeradio%2Fsouk-firmware%2Fassets%2F1402492%2F665a223d-31fc-4600-85f1-4fbaf11e6464&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=%2FOwUXDnFMHhGRFSFSCHP%2F88usvvDgcmixn6QqCfYWzU%3D&reserved=0

Fixed!

— 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%2F36%23issuecomment-2008171624&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=YbAHwk4pKMDhJ2%2F606FRAGS3O6yNxJvyKtorXJe6%2BtA%3D&reserved=0, or unsubscribe https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEFQV46OWAV7A75CHE44GQTYZCVHJAVCNFSM6AAAAABEYDGIGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBYGE3TCNRSGQ&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=IANXRWqCcdNGRQ0mhNUW3tLFtAhMQsp%2Fj4XuT1kcYdc%3D&reserved=0 . You are receiving this because you authored the thread.Message ID: @.***>

jack-h commented 7 months ago

Nothing exciting - an off-by-1 error where the Synth bank FIR coefficients of the second stream weren't quite being applied to the right samples. Not sure how long this bug has been present - at least since the number of channels went from 4096 to 8192, I think.

On Wed, 20 Mar 2024, 07:29 Sam Rowe, @.***> wrote:

Beautiful!

Dare I ask what the problem was?

I'll check the noise in the accumulator a bit later this morning.

On Tue, 19 Mar 2024, 21:32 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.

python ./ospfb_sweep.py 10.10.10.146 ../config/souk-single-pipeline-krm.yaml -o psb

Firmware v6.3.0.2 (souk_single_pipeline_krm_2024-03-19_2005.fpg)

image.png (view on web) < https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frealtimeradio%2Fsouk-firmware%2Fassets%2F1402492%2F74c93313-fb0b-4de0-a2b2-4b49a013aaf3&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=HCWEXo7ZxeFq9jpe%2BWGnzpZ%2Fg%2FygsA%2BKHml862TZYhA%3D&reserved=0>

image.png (view on web) < https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frealtimeradio%2Fsouk-firmware%2Fassets%2F1402492%2F665a223d-31fc-4600-85f1-4fbaf11e6464&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=%2FOwUXDnFMHhGRFSFSCHP%2F88usvvDgcmixn6QqCfYWzU%3D&reserved=0>

Fixed!

— 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%2F36%23issuecomment-2008171624&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=YbAHwk4pKMDhJ2%2F606FRAGS3O6yNxJvyKtorXJe6%2BtA%3D&reserved=0>,

or unsubscribe < https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEFQV46OWAV7A75CHE44GQTYZCVHJAVCNFSM6AAAAABEYDGIGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBYGE3TCNRSGQ&data=05%7C02%7Csam.rowe%40astro.cf.ac.uk%7C38afb695984b430efd3c08dc485c195c%7Cbdb74b3095684856bdbf06759778fcbc%7C1%7C0%7C638464807591118537%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=IANXRWqCcdNGRQ0mhNUW3tLFtAhMQsp%2Fj4XuT1kcYdc%3D&reserved=0>

. You are receiving this because you authored the thread.Message ID: @.***>

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

sr-cdf commented 7 months ago

@jack-h The latest bit file isn't arriving when pulling the latest commit. The symlinks point to non-existant files.

sr-cdf commented 7 months ago

Nothing exciting - an off-by-1 error where the Synth bank FIR coefficients of the second stream weren't quite being applied to the right samples.

I'm excited that its fixed!

jack-h commented 7 months ago

Standby [fires up VPN on train]

On Wed, 20 Mar 2024, 09:46 Sam Rowe, @.***> wrote:

The latest bit file isn't arriving when pulling the latest commit. The symlinks point to non-existant files.

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

jack-h commented 7 months ago

try now

sr-cdf commented 7 months ago

Absolutely fantastic, identical gain/phase flatness and noise across both even and odd channels.

image

Thanks Jack!!

PS: I can probably calibrate out the power combiner imbalance quite straightforwardly.

PPS: Next job (not urgent) is to address the phase delay (the plots in this issue have been corrected with 50.5us group delay)

jack-h commented 7 months ago

Fab. I'm gonna close this issue and open another for the phase slope stuff, since I have some questions

sr-cdf commented 7 months ago

Cool, just one last plot of a sweep with loopback enabled showing the 0.015 dB flatness...

image