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

Response calibration #349

Closed
PA0JOZ opened this issue Nov 16, 2020 · 34 comments
Closed

Response calibration #349

PA0JOZ opened this issue Nov 16, 2020 · 34 comments
Labels
enhancement New feature or request stale Long time no reaction, will be closed soon

Comments

@PA0JOZ
Copy link

PA0JOZ commented Nov 16, 2020

Hello Zarath,

I am using nanoVNASaver v3.8 in combination with a nanoVNA V2 and a nanoVNA V2Plus4.
I have noticed that if I measure the same THRU as used for the s21 calibration the result shows considerable ripple, while the result should be a (almost) flat line.
Although I am not a Python programmer at all I looked through your code for the calibration in calibration.py and found that the formula for e10e32 is not correct. The e11**2 should have been e11*e22. But since e22 is not calculated it would be better to set it zero so e10e32 = measured s21 of the THRU - isolation. This reduces the ripple a little bit depending on the actual hardware of the nanoVNA.

It would be even (much) better to implement the so-called enhanced response calculation. The formulas can be found in the book by Dunsmore or in the blog by K6JCA. The blog by K6JCA is most clear and adheres the most with the notation in calibration.py. See: http://k6jca.blogspot.com/search/label/VNA%3A%20Thru%20De-embedding. See the formulas for e22 and e10e32 in the figure "12-Term Error Model, Modified Error Terms if calibrating with a Defined THRU Standard".
I have double-checked the formulas and they are correct and correspond with the Dunsmore book, although with different nomenclature.
To be able to implement the enhanced response calibration it is necessary to measure not only the forward response but also the reflection while performing the calibration step with the through connector. The reflection is required to be able to estimate e22. As far as I can see form the saved calibration file now only the forward response is measured during calibration. And I say "estimate" because e22 can be calculated under the assumption that the through only has delay and no reflection, which for the frequencies involved is a reasonable assumption.
Of course I would be glad to test if you can fix this issue. In fact I already had figured out the required modifications for calibration.py and sweepworker.py, but I got stuck because the s11 of the through calibration step was not available.
If required you can reach me at pa0joz"-at-"veron.nl.
Best Regards, 73, Jos, PA0JOZ
@zarath zarath added the enhancement New feature or request label Nov 18, 2020
@PA0JOZ
Copy link
Author

PA0JOZ commented Nov 25, 2020

Hello Zarath,

I have made an effort to implement the so-called Enhanced Response Correction as suggested in issue #349.
I also corrected the small max_gain bug as mentioned in #issue 352.
Enclosed are the modified files Calibration.py, Sweepworker.py, NanoVNASaver.py and CalibrationSettings.py.
The modifications are (in short and with approximate line numbers):
CalibrationSettings.py: added the reflection measurement to the calibration step of the through (line 791)
Sweepworker.py: modified the port2 calibration including the s11 of the through calibration step (line 216)
Calibration.py: added "thrurefl" to RXP_CAL_LINE (line 37), added "thrurefl", "e10e01" and "e22" to CalData (line 59), added "thrurefl" to complete2port in CalDataSet (line 125), added "thrurefl" to Calibration (line 132), modified calculation of error coefficients for the second port (line 233), added "e22" and e10e01" to the gen_interpolation(line 310), modified correct21 to take the through reflection into account (line 345), added a temporary dump-routine to print values to the console in order to verify them externally (line 351), added "thrurefl" to the cal save routine (line 379) and added "thrurefl" to the cal load routine (line 405).
NanoVNASaver.py: corrected the max_gain bug (line 664)

If you want to include or use my modifications, please check them carefully as I am not a python programmer (not at all, in fact). I hope this helps.
73, Jos, PA0JOZ
NanoVNASaver_modifications_PA0JOZ.zip

@galileo-pkm
Copy link
Contributor

I took a look at your changes and applied them atop of master
branch. It seems that you have included in your zip file
SweepWorker.py.org file instead of your modified version.
Without it the code breaks at the apply calibration stage.
Can you provide your modifications for that file?

@PA0JOZ
Copy link
Author

PA0JOZ commented Feb 18, 2021 via email

@galileo-pkm
Copy link
Contributor

galileo-pkm commented Feb 18, 2021 •
edited
Loading

It seems that you forgot to attach the zip file :)

@PA0JOZ
Copy link
Author

PA0JOZ commented Feb 19, 2021 via email

@galileo-pkm
Copy link
Contributor

Unfortunately no but I think I know what is going on.
You are responding to github notification email and
attaching the file there. You will have to respond to
the issue on github directly and attach the file there.

@PA0JOZ
Copy link
Author

PA0JOZ commented Feb 19, 2021

Sorry, I didn't realize. Here it is once more.
Regards, Jos
SweepWorker.zip

@galileo-pkm
Copy link
Contributor

Great.
Thanks

@galileo-pkm
Copy link
Contributor

I have applied your changes to the master branch and published them at: https://github.com/galileo-pkm/nanovna-saver/tree/PA0JOZ if anyone else wants to test them.
The test was done with a recent version of NanoVNA H4, dislord firmware 1.45.
I used 2 male to male RG405 cables and the supplied standards, treated as ideal since these are the new improved version and the parameters for them are not yet published.
Frequency span 50KHz - 1500MHz, BW 333Hz, 2010 points, averaged sweep (5/2) for calibration and the measurements.

As can be seen from the screenshot (through measurement) the modification does bring a significant improvement.
image

I have attached the zip file with the measurements, made with the nano in uncalibrated state, calibrated with standard algorithm, and with this new method. Cal files are also included.

Your changes, in the branch that I published, are as you made them but I removed your comments, as those are
considered a bad form and there are no need for them, as your authorship will be preserved if you publish a branch
and submit a pull request to the maintainer. It's quite easy to do and if your pull request is accepted, you will be properly
credited in the git history. Also it would make any further changes easier to merge.

You did a great job, I will integrate it with the rest of the changes that I use and update this issue if I find something of significance.
Thanks.
erc.zip

@PA0JOZ
Copy link
Author

PA0JOZ commented Feb 20, 2021

Good morning,
Thanks for testing and taking care of the github matters!
Best Regards, 73, Jos, PA0JOZ

@galileo-pkm
Copy link
Contributor

First bug:
Load calibration from file seems to ignore the "thrurefl" points.
image

@galileo-pkm
Copy link
Contributor

It seems to a cosmetic issue only, I have updated the
branch at https://github.com/galileo-pkm/nanovna-saver/tree/PA0JOZ
with a fix.

@PA0JOZ
Copy link
Author

PA0JOZ commented Feb 28, 2021 via email

@MCE66-GitHub
Copy link

Hello,
is there any reason why this Enhanced calibration is not included in the master release of NanoVNA-Saver? Is there any drawback in using theseenhanced formulas instead of less accurate formulas?
Marco.

@galileo-pkm
Copy link
Contributor

Hello,
is there any reason why this Enhanced calibration is not included in the master release of NanoVNA-Saver? Is there any drawback in using theseenhanced formulas instead of less accurate formulas?
Marco.

There hasn't been much of a feedback on the feature and maintainer time is finite ...

@MCE66-GitHub
Copy link

This feature is very important since it reduces inaccuracy in S21 measurements of about 0.5 dB (it depends on VNA Port 2 return loss). This feature is missing in NanoVNA-QT, NanoVNA-APP and NanoVNA-saver while has been integrated in the NanoVNA V2 firmware! It is very strange that a so simple modification of the calibration formulas, that results in a so big improvement of accuracy, is not on the top of the improvements list of NanoVNA-Saver (and other VNA SW applications). Anyway I understand that maybe many people are more interesed in "Features" than Accuracy. Thanks for your work on this very nice SW. Best Regards, Marco.

@galileo-pkm
Copy link
Contributor

Agree on the features vs accuracy POW, I guess that is what most users push for.
Maybe post your evaluation of this feature and why it is important to you, as a nudge
for maintainers to merge it? If more people asked for it ...

@MCE66-GitHub
Copy link

As far as I understand there is no binary build that includes this enhancment. If at least an exe build would exist to test it, many people could try use it and appreciate the S21 accuracy improvements. I hope that at least a devel binary build can be made available.

@galileo-pkm
Copy link
Contributor

For people using Linux/Unix like OS-es it is easy, they can just download my repository.
For Windows it should work the same but I guess most people are not familiar with running
python code from source on Windows. I have no idea if there is a way to build a Windows binary
from Linux.

@silbe
Copy link
Contributor

silbe commented Jan 15, 2022

I'm surprised this pretty obvious bug is labelled as "enhancement" and so few people have noticed it. By definition measuring the thru standard after calibration should result in a straight line, with just random noise as deviations from 0dB. In my case (span of ~3GHz, NanoVNA v2) I'm seeing more than ±1dB of variation with the shape staying the same on every measurement, i.e. a large systematic error. Simply performing a sweep directly after 2-port calibration without touching anything will show this problem. When using the NanoVNA v2 directly there's a straight line with just < 0.1dB of random noise, just as expected. Only nanovna-saver has this bug.

I've tried out @PA0JOZ changes from @galileo-pkm's branch and the results look a lot better at first glance (thanks!). However I'm not deep enough into calibration methods to judge whether those changes are correct. I also don't have a second VNA to verify the results against.

@PA0JOZ @galileo-pkm Your branch is out of date; there are conflicts when trying to cherry-pick them on top of current master. In addition there are some debug prints and whitespace issues in your code. The maintainer may not want to merge the code in that state. If you need help with rebasing and clean-up please let me know.

@silbe silbe mentioned this issue Jan 15, 2022
Closed
@galileo-pkm
Copy link
Contributor

The reason why you are not seeing those ripples, when using the V2 standalone is that the V2 implements
this algorithm during the calibration.

When I merged @PA0JOZ changes I did not want to submit a pull request because it would appear
that I was the author. I guess that is not important any more, I will submit a pull request latter ...

You can check out new branch that was rebased (and squash merged) on top of develop:

https://github.com/galileo-pkm/nanovna-saver/tree/PA0JOZ_devel

@galileo-pkm galileo-pkm mentioned this issue Jan 16, 2022
Merged
@PA0JOZ
Copy link
Author

PA0JOZ commented Jan 16, 2022 via email

@galileo-pkm
Copy link
Contributor

For a non programmer you did a great job.
Many experienced programmers would not dare to tackle an unfamiliar project on
a fairly complex issue.

@PA0JOZ
Copy link
Author

PA0JOZ commented Jan 16, 2022 via email

@MartinThorn74
Copy link

Just joined Github - at first just to post this reply. I agree with a poster (MCE66) above - performance should rate above adding features. Without this feature, S21 thru response is significantly inaccurate, so it really should have been included in the main build months ago - I downloaded it back then (and installed all the python stuff needed to do that - complete newbie on that front!) and checked it vs. a few circuits - much improved response.

@galileo-pkm
Copy link
Contributor

Pull request has been submitted, we wait for the approval ...

@MartinThorn74
Copy link

MartinThorn74 commented Feb 13, 2022 via email

@MartinThorn74
Copy link

For what it's worth, here's measurements of a 6dB attenuator and a high pass filter before & after changing to the enhanced calibration update - noticeable improvement in the results for both:

6dB attenuator old
high pass filter old
6dB attenuator new
high pass filter new
.

@galileo-pkm
Copy link
Contributor

This has been merged into develop branch.

@zarath
Copy link
Collaborator

zarath commented Feb 18, 2022

I've refactored and linted some part of the changes, so that i need reviews / tests with current testing branch

@galileo-pkm
Copy link
Contributor

Did a quick test using the same setup as before (firmware was different thou). Calibration and s2p files are attached.
Green trace is from the previous test.

image
erc_testing_branch.zip

@silbe
Copy link
Contributor

silbe commented Mar 8, 2022

Current testing branch looks much better, thanks! I had some unrelated trouble with infinite recursion during version comparison. Will send an MR for that.

@blinken
Copy link
Contributor

blinken commented Mar 14, 2022

I've been beating my head against a wall for the last few weeks trying to figure out this discrepancy, until I realised my V2Plus4 device was showing different results to nanovna-saver. I assumed it was USB interference or something esoteric until I stumbled across this issue just now.

I can confirm the testing branch fixes it for me. Thank you!

@github-actions
Copy link

There hasn't been any activity on this issue recently, and in order to prioritize active issues, it will be marked as stale.
Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a 👍
Because this issue is marked as stale, it will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions bot added the stale Long time no reaction, will be closed soon label Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request stale Long time no reaction, will be closed soon
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@blinken @zarath @silbe @galileo-pkm @PA0JOZ @MCE66-GitHub @MartinThorn74