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.

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

NanoVNA fixes #968

Merged
merged 6 commits into from
Dec 2, 2023
Merged

NanoVNA fixes #968

merged 6 commits into from
Dec 2, 2023

Conversation

miek
Copy link
Contributor

@miek miek commented Oct 29, 2023

I've been trying to use the NanoVNA support included in the recent VNA overhaul, but I ran into lots of basic runtime errors. Looking back over #744, I'm not sure if anyone with a NanoVNA was able to test it before merging, so that probably explains it.

I've gone through and fixed everything I could find to get it functioning, doing my best to keep to the API that was there. I've been testing with a LiteVNA (which implements the NanoVNAv2 protocol) using the following script:

import matplotlib.pyplot as plt
import skrf
from skrf.vi.vna.nanovna import NanoVNA

nv = NanoVNA('ASRL/dev/ttyACM0::INSTR')
print(nv.device_info)

nv.freq_start = 1e6
nv.freq_stop = 6e9
nv.npoints = 201

s11, s21 = nv.get_s11_s21()
n = skrf.network.four_oneports_2_twoport(s11, s21, s21, s11)
n.plot_it_all(n=0)
plt.show()

and it seems to be working well with this set of changes:
skrf_litevna
(uncorrected output of course)

Previously this would error out because attributes of a Frequency object
can't be changed.
Previously, some default stimulus settings were applied locally but not
written to the device, so they could get out of sync.

The protocol does not allow for observing the user-entered sweep
parameters, so the alternative option of reading the device's current
settings at startup is not possible.
Previously this would error out as it was passing `numpy.float64` objects
to `range`.
@cafeclimber
Copy link
Contributor

Thanks for this! Someone had submitted a PR to my fork but I didn't get the chance to merge it before the PR into upstream. I'll take a look at this in a bit!

cafeclimber#1

@jhillairet
Copy link
Member

@cafeclimber how about this PR?

@jhillairet jhillairet merged commit 3f2e40b into scikit-rf:master Dec 2, 2023
12 checks passed
@jhillairet
Copy link
Member

Thank you @miek

@jhillairet jhillairet mentioned this pull request Dec 2, 2023
@cafeclimber
Copy link
Contributor

Hey sorry about not having the chance to review this PR. I know it's merged now so I'll try my best to take a look this week and submit PRs if I find anything :) thanks for the contribution, @miek

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.

None yet

3 participants