Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FLL bandedge: fix math & thread safety bugs #7127

Merged
merged 3 commits into from
Jun 9, 2024
Merged

FLL bandedge: fix math & thread safety bugs #7127

merged 3 commits into from
Jun 9, 2024

Conversation

marcusmueller
Copy link
Member

  • digital: FLL band edge QA with noisy signal
  • digital: FLL band edge: fix power calculation and thread safety

Description

Related Issue

Closes #6466

Found the normalization bug and the numerical accuracy TODO while doing that

Which blocks/areas does this affect?

FLL bandedge

Testing Done

Added a test to demonstrate it now actually works oin noisy weather

Checklist

@marcusmueller

This comment was marked as outdated.

Sign in to view
@marcusmueller marcusmueller force-pushed the fll_bandedge_make_right branch 2 times, most recently from 1e6bd01 to bdc2e3f Compare February 12, 2024 19:45
@willcode willcode added the Backport-3.10 Should be backported to 3.10 label Feb 12, 2024
@willcode
Copy link
Member

fll_band_edge_cc fails on Debian 11

@willcode
Copy link
Member

@marcusmueller any clue on the failure? It's a comparison that's not even close ... not a tolerance thing.

@marcusmueller
Copy link
Member Author

huh, no, no idea. Need to reproduce locally.

@willcode
Copy link
Member

Any progress? This looks like some sort of precision difference on 32-bit.

@marcusmueller marcusmueller force-pushed the fll_bandedge_make_right branch 2 times, most recently from 4a42df2 to 303e18f Compare June 3, 2024 11:36
@marcusmueller
Copy link
Member Author

locally reproduced the 32 bit test failure. having a hard time figuring out the reason. The tolerance seems very generous already.

@marcusmueller
Copy link
Member Author

Tap generation broken on 32 bit, it seems:

In [11]: numpy.array(lower64) - numpy.array(lower32)
Out[11]:
array([ 0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j, -1.00000e-08+0.00000e+00j,
        0.00000e+00-1.00000e-08j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  0.00000e+00+0.00000e+00j,
        0.00000e+00+0.00000e+00j,  1.53370e-01+1.34883e-01j,
        4.02050e-02+1.21602e-01j, -6.09500e-03+2.84810e-02j,
        3.23280e-02-3.53350e-02j,  7.40480e-02-2.28800e-02j,
        6.21260e-02+1.45766e-02j,  2.28080e-02+2.17049e-02j,
        1.71180e-03+5.95190e-03j,  1.90400e-04-7.45290e-04j,
       -3.81820e-03+3.85770e-03j, -1.21268e-02+3.23130e-03j,
       -1.22004e-02-3.37310e-03j, -4.67950e-03-4.81710e-03j,
       -2.10870e-04-8.58820e-04j, -8.51000e-05+2.85870e-04j,
        1.83753e-03-1.71620e-03j,  5.18820e-03-1.16640e-03j,
        4.87660e-03+1.55677e-03j,  1.61557e-03+1.79928e-03j,
        4.88600e-10+2.39179e-09j,  3.80000e-05-1.11690e-04j,
       -1.18273e-03+1.02076e-03j])

@marcusmueller
Copy link
Member Author

baseband taps seem to be different, but not much (10⁻⁷, still surprisingly much for these relatively well-conditioned numbers)

64 bit:

-0.019462407, -0.0125468485, 2.5296996e-08, 0.0153915845, 0.029321931, 0.036896005, 0.033928595, 0.018343471, -0.008656152, -0.04196944, -0.072771795, -0.08956845, -0.08002813, -0.033312637, 0.05752234, 0.193539, 0.3687871, 0.57033694, 0.7795744, 0.97460705, 1.1334091, 1.2371683, 1.2732395, 1.2371683, 1.1334091, 0.97460705, 0.7795744, 0.57033694, 0.3687871, 0.193539, 0.05752234, -0.033312637, -0.08002813, -0.08956845, -0.072771795, -0.04196944, -0.008656152, 0.018343471, 0.033928595, 0.036896005, 0.029321931, 0.0153915845, 2.5296996e-08, -0.0125468485, -0.019462407

32 bit:

-0.019462408, -0.012546843, 2.5296996e-08, 0.015391588, 0.029321939, 0.036896, 0.03392859, 0.018343467, -0.008656151, -0.041969433, -0.072771855, -0.08956845, -0.08002814, -0.033312567, 0.05752234, 0.19353898, 0.3687871, 0.57033694, 0.77957445, 0.97460705, 1.1334091, 1.2371682, 1.2732395, 1.2371682, 1.1334091, 0.97460705, 0.77957445, 0.57033694, 0.36878705, 0.19353908, 0.057522338, -0.033312637, -0.0800281, -0.08956844, -0.072771765, -0.041969433, -0.008656152, 0.01834341, 0.033928588, 0.036896005, 0.029321868, 0.015391684, 2.5296996e-08, -0.012546842, -0.019462448

But here's the real part of the (frequency shifted baseband==) lower edge filter:

real parts

😮

@marcusmueller
Copy link
Member Author

Yep, k calculation totally borked on 32 bit:

 index: 44, k=536870900, tap=-0.0017034719, lower=0.0015804379+0.0006356355j, upper=0.0015804379+-0.0006356355j
 index: 43, k=536870900, tap=-0.0010981782, lower=0.0010188618+0.00040977553j, upper=0.0010188618+-0.00040977553j
 index: 42, k=536870900, tap=2.2141515e-09, lower=-2.0542335e-09+-8.2619117e-10j, upper=-2.0542335e-09+8.2619117e-10j
 index: 41, k=536870900, tap=0.0013471682, lower=-0.0012498684+-0.000502684j, upper=-0.0012498684+0.000502684j
 index: 40, k=536870900, tap=0.0025664398, lower=-0.0023810775+-0.0009576444j, upper=-0.0023810775+0.0009576444j
 index: 39, k=536870900, tap=0.0032293692, lower=-0.0029961264+-0.0012050107j, upper=-0.0029961264+0.0012050107j
 index: 38, k=536870900, tap=0.0029696429, lower=-0.002755159+-0.0011080961j, upper=-0.002755159+0.0011080961j
 index: 37, k=536870900, tap=0.0016055352, lower=-0.0014895747+-0.0005990913j, upper=-0.0014895747+0.0005990913j
 index: 36, k=536870900, tap=-0.0007576405, lower=0.0007029196+0.0002827069j, upper=0.0007029196+-0.0002827069j
 index: 35, k=536870900, tap=-0.0036734275, lower=0.0034081126+0.0013707072j, upper=0.0034081126+-0.0013707072j
 index: 34, k=536870900, tap=-0.0063694483, lower=0.005909412+0.0023767035j, upper=0.005909412+-0.0023767035j
 index: 33, k=536870900, tap=-0.007839591, lower=0.007273373+0.0029252744j, upper=0.007273373+-0.0029252744j
 index: 32, k=536870900, tap=-0.0070045637, lower=0.006498656+0.0026136914j, upper=0.006498656+-0.0026136914j
 index: 31, k=536870900, tap=-0.0029157244, lower=0.002705135+0.0010879769j, upper=0.002705135+-0.0010879769j
 index: 30, k=536870900, tap=0.0050347154, lower=-0.004671081+-0.0018786597j, upper=-0.004671081+0.0018786597j
 index: 29, k=536870900, tap=0.016939742, lower=-0.015716262+-0.0063209157j, upper=-0.015716262+0.0063209157j
 index: 28, k=536870900, tap=0.032278556, lower=-0.029947223+-0.01204446j, upper=-0.029947223+0.01204446j
 index: 27, k=536870900, tap=0.04991946, lower=-0.046314005+-0.018627007j, upper=-0.046314005+0.018627007j
 index: 26, k=536870900, tap=0.06823324, lower=-0.06330506+-0.025460633j, upper=-0.06330506+0.025460633j
 index: 25, k=536870900, tap=0.08530371, lower=-0.07914261+-0.03183033j, upper=-0.07914261+0.03183033j
 index: 24, k=536870900, tap=0.099203065, lower=-0.09203808+-0.03701675j, upper=-0.09203808+0.03701675j
 index: 23, k=536870900, tap=0.108284704, lower=-0.10046379+-0.040405486j, upper=-0.10046379+0.040405486j
 index: 22, k=0, tap=0.111441895, lower=0.111441895+-0j, upper=0.111441895+0j
 index: 21, k=0.125, tap=0.108284704, lower=0.052910212+-0.094477974j, upper=0.052910212+0.094477974j
 index: 20, k=0.25, tap=0.099203065, lower=-0.05183345+-0.08458452j, upper=-0.05183345+0.08458452j
 index: 19, k=0.375, tap=0.08530371, lower=-0.08523794+0.0033490167j, upper=-0.08523794+-0.0033490167j
 index: 18, k=0.5, tap=0.06823324, lower=-0.030977253+0.060796257j, upper=-0.030977253+-0.060796257j
 index: 17, k=0.625, tap=0.04991946, lower=0.027733758+0.041506518j, upper=0.027733758+-0.041506518j
 index: 16, k=0.75, tap=0.032278553, lower=0.032179046+-0.0025325527j, upper=0.032179046+0.0025325527j
 index: 15, k=0.875, tap=0.016939752, lower=0.0070919897+-0.01538372j, upper=0.0070919897+0.01538372j
 index: 14, k=1, tap=0.0050347154, lower=-0.0029593299+-0.0040731714j, upper=-0.0029593299+0.0040731714j
 index: 13, k=1.125, tap=-0.0029157307, lower=0.0028955203+-0.00034270622j, upper=0.0028955203+0.00034270622j
 index: 12, k=1.25, tap=-0.0070045604, lower=0.0026805322+-0.006471369j, upper=0.0026805322+0.006471369j
 index: 11, k=1.375, tap=-0.007839591, lower=-0.0048534404+-0.006156566j, upper=-0.0048534404+0.006156566j
 index: 10, k=1.5, tap=-0.0063694404, lower=-0.0062910216+0.0009964026j, upper=-0.0062910216+-0.0009964026j
 index: 9, k=1.625, tap=-0.0036734275, lower=-0.0012714347+0.0034463783j, upper=-0.0012714347+-0.0034463783j
 index: 8, k=1.75, tap=-0.0007576406, lower=0.0004920484+0.0005761143j, upper=0.0004920484+-0.0005761143j
 index: 7, k=1.875, tap=0.00160553, lower=-0.0015746802+0.0003132238j, upper=-0.0015746802+-0.0003132238j
 index: 6, k=2, tap=0.0029696424, lower=-0.000917672+0.0028242972j, upper=-0.000917672+-0.0028242972j
 index: 5, k=2.125, tap=0.0032293694, lower=0.0021920966+0.0023714004j, upper=0.0021920966+-0.0023714004j
 index: 4, k=2.25, tap=0.0025664335, lower=0.002495523+-0.00059912j, upper=0.002495523+0.00059912j
 index: 3, k=2.375, tap=0.0013471766, lower=0.00036567936+-0.0012965968j, upper=0.00036567936+0.0012965968j
 index: 2, k=2.5, tap=2.2141515e-09, lower=-1.5656402e-09+-1.565643e-09j, upper=-1.5656402e-09+1.565643e-09j
 index: 1, k=2.625, tap=-0.0010981782, lower=0.0010569476+-0.000298089j, upper=0.0010569476+0.000298089j
 index: 0, k=2.75, tap=-0.0017034753, lower=0.00039767005+-0.0016564077j, upper=0.00039767005+0.0016564077j

@willcode
Copy link
Member

willcode commented Jun 3, 2024

Is this because signed_type is different?

@marcusmueller
Copy link
Member Author

I think there was something fishy going on with integer promotion rules. Having made N of signed type, this now works, even if (signed int a) - (unsigned int) shouldn't be a problem for integers like 44 (this is nowhere close to 2³²).

Note that the 32 bit build uses -O3, the 64 bit build -O2.

@marcusmueller
Copy link
Member Author

marcusmueller commented Jun 3, 2024 •
edited

@willcode see 1ed75cd ; that was indeed all that was necessary. I'm slightly confused why this should make a difference, anywhere.

(as soon as CI passes, I'll squash and force-push)

@marcusmueller
runtime: block: make d_setlock mutex mutable to be useful in const me…
5914e2e 
…thods

Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
@marcusmueller
digital: FLL band edge QA with noisy signal
f7a41dc 
Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
@marcusmueller
digital: FLL band edge: fix power calculation and thread safety
5f0a484 
While fixing a bug in thread safety, an error in how the filter is normalized was noticed and fixed

Also, figuring out what the critical sections are becomes easier when methods actually use the members they were supposed to use, instead of always being called with them as arguments.

Signed-off-by: Marcus Müller <mmueller@gnuradio.org>
@willcode willcode merged commit 5c1edfe into gnuradio:main Jun 9, 2024
15 checks passed
@marcusmueller marcusmueller deleted the fll_bandedge_make_right branch June 9, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Backport-3.10 Should be backported to 3.10 Bug Fix digital QA threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

digital::fll_band_edge_cc::set_samples_per_symbol() is not thread safe
2 participants
@marcusmueller @willcode