Skip to content

EOO Tx magnitude fix#9

Closed
drowe67 wants to merge 2 commits intopeterbmarks:mainfrom
drowe67:main
Closed

EOO Tx magnitude fix#9
drowe67 wants to merge 2 commits intopeterbmarks:mainfrom
drowe67:main

Conversation

@drowe67
Copy link
Copy Markdown
Collaborator

@drowe67 drowe67 commented Mar 17, 2026

@peterbmarks @tmiw as per drowe67/freedv-gui#1251 (comment) the C EOO Tx magnitude was off. Following @tmiw hints I found the offending functions and set Claude to fix it. Ctests all pass.

The amplitude looks good now, you can see the well behaved EOO Tx data at the end of this plot:

image

@tmiw - suggest you try it out/review before we merge

drowe67 and others added 2 commits March 17, 2026 17:28
pilot_gain and bottleneck-3 tanh were being applied to the
frequency-domain symbol before the IDFT, but the Python set_eoo_bits
applies them to the time-domain signal after IDFT and CP insertion.
Move both operations to after rade_ofdm_insert_cp to match Python.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@drowe67
Copy link
Copy Markdown
Collaborator Author

drowe67 commented Mar 17, 2026

@tmiw - snap. We should probably decide who is going to work on bugs in future? 🙂

@tmiw
Copy link
Copy Markdown
Collaborator

tmiw commented Mar 17, 2026

@tmiw - snap. We should probably decide who is going to work on bugs in future? 🙂

Heh, just saw this. I'm happy to close the other PR if this works better. I'll try both and see. (freedv-gui just finished recompiling, currently running ctest.)

@drowe67
Copy link
Copy Markdown
Collaborator Author

drowe67 commented Mar 17, 2026

@tmiw - perhaps try running ctest -V -R radae_nopy_eoo_data for both, and compare rx.32 files (or send them to me to compare). That way we can use our accidental double up to x-check the fix. If they are much different I'd be concerned.

@drowe67
Copy link
Copy Markdown
Collaborator Author

drowe67 commented Mar 17, 2026

@tmiw - actually if you like just send me the rx.f32 from your PR #8 and I can compare with the 'rx.f32 from this PR.

@tmiw
Copy link
Copy Markdown
Collaborator

tmiw commented Mar 17, 2026

@tmiw - actually if you like just send me the rx.f32 from your PR #8 and I can compare with the 'rx.f32 from this PR.

See rx.zip. There are two files in there, one for radae_nopy_eoo_data_c and the other for radae_nopy_eoo_data_mpp.

@tmiw
Copy link
Copy Markdown
Collaborator

tmiw commented Mar 17, 2026

But FWIW both PRs seem to work for me with the extended reporting tests in freedv-gui.

@drowe67
Copy link
Copy Markdown
Collaborator Author

drowe67 commented Mar 17, 2026

I'm getting some differences but in a section of the file away from the EOO symbols at the end:

image

I used Claude to check both implementations and he says they have identical outputs 🤷

So the difference could be a numerical thing from different machines.

So given your freedv-gui level tests check out I figure we're done 👍 I'm OK with using either PR.

Good X-check if we both arrived at a similar solution I guess!

@tmiw
Copy link
Copy Markdown
Collaborator

tmiw commented Mar 17, 2026

I looked at both PRs again and there might be a theoretical performance benefit for mine, but TBH it likely doesn't matter much which one goes in. Your call :)

@drowe67 drowe67 closed this Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants