open-sdr / openwifi-hw

open-source IEEE 802.11 WiFi baseband FPGA (chip) design: FPGA, hardware
GNU Affero General Public License v3.0
701 stars 237 forks source link

invalid(may) LTS correaltion coefficients in sync_long.v #8

Closed littlelid closed 4 years ago

littlelid commented 4 years ago

In file sync_long.v line 418-421, the original code is stage_X0 <= cross_corr_buf[1]; stage_X1 <= cross_corr_buf[2]; stage_X2 <= cross_corr_buf[3]; stage_X3 <= cross_corr_buf[4];

I think the code should be fixed with: stage_X0 <= cross_corr_buf[0]; stage_X1 <= cross_corr_buf[1]; stage_X2 <= cross_corr_buf[2]; stage_X3 <= cross_corr_buf[3];

mmehari commented 4 years ago

I remember looking at this when I was studying the rx core. I also have tried the same modification you did and I haven't seen that much of a difference. For the sake of leaving the originality of the code, I left it as it is but you are free to change it and please let us know if there is a difference in rx performance.

littlelid commented 4 years ago

There might be one explanation. The best practice to detect LTS is to use all 64 coefficients. To save computing resources, the rx core only uses 16 coefficients, and also works well. If these four buffs have mistakes, the actual LTS detection can be modeled as using 12 coefficients to cross-correlate with base-band samples. At high SNR, the detection still works. But in low SNR, I believe, the performance might decrease.

mmehari commented 4 years ago

What you said is true as the author reduced the size of the cross-validation to 16 IQ samples (i.e. https://openofdm.readthedocs.io/en/latest/sync_long.html#fig-match-size).

And it might be the case that I tested the changes (don't remember exactly) at high SNR which hasn't made any significant change.

JiaoXianjun commented 4 years ago

Thanks for reporting. Did you ever try low SNR such as by longer distance?

Anyway we will fix and test in low SNR.

JiaoXianjun commented 4 years ago

I have updated all stuffs accordingly. Issue is closed.

JiaoXianjun commented 4 years ago

I have done a test between Thinkpad T420 (Intel wifi card: Intel Corporation Centrino Advanced-N 6205) and zcu102+fmcs2. openwifi as AP, T420 as client.

The results shows that original sync_long.v index has better performance clearly. We need to figure out further. But for now, let's use the original index.

Original index: stage_X0 <= cross_corr_buf[1]; stage_X1 <= cross_corr_buf[2]; stage_X2 <= cross_corr_buf[3]; stage_X3 <= cross_corr_buf[4];

New index: stage_X0 <= cross_corr_buf[0]; stage_X1 <= cross_corr_buf[1]; stage_X2 <= cross_corr_buf[2]; stage_X3 <= cross_corr_buf[3]; Iperf TCP performance of original index: DL: 27M; UL: 22M

Iperf TCP performance of new index: DL: 18M; UL: 16M

JiaoXianjun commented 4 years ago

Hello,

How are you doing with openwifi now? Do you still have any issue? The openwifi has been improved a lot during the last .5 year. It supports more boards (high end to low end) and becomes more stable.

Would you please tell us your email (if you could also introduce yourself bit, that would be perfect). We might send out some questions to listen for user feed feedback.

Thanks.