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

LiteVNA support #754

Merged
merged 21 commits into from
Jan 21, 2025
Merged

LiteVNA support #754

merged 21 commits into from
Jan 21, 2025

Conversation

redrathnure
Copy link
Contributor

@redrathnure redrathnure commented Jan 14, 2025 •
edited
Loading

Initial version for LiteVNA support. Based on #534 discussion.
Followimg features were tested:

  • more then 101 measurement points
  • callibration
  • measurement
  • screenshots (from device)
  • battery monitoring

Pull Request type

Please check the type of change your PR introduces:

  • [] Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • [] Refactoring (no functional changes, no API changes)
  • [] Build-related changes
  • [] Documentation content changes
  • [] Other (please describe):

What is the current behavior?

LiteVNA64 is detected as SAA2 device. This works somehow however it isn't possible to unitize all hardware functionality (e.g. enable more than 1023 points)

Issue Number: 534

What is the new behavior?

  • LiteVNA device is detected as LIteVNA64 device
  • It's possible to specify up to 65K points

Does this introduce a breaking change?

  • [] Yes
  • No

Other information

na

@redrathnure redrathnure requested a review from zarath as a code owner January 14, 2025 06:46
@redrathnure redrathnure force-pushed the feat/litevna-support-screenshots branch 2 times, most recently from 714fb90 to e99b710 Compare January 18, 2025 14:40
zarath
zarath reviewed Jan 19, 2025
View reviewed changes
.vscode/launch.json Outdated Show resolved Hide resolved
zarath
zarath reviewed Jan 19, 2025
View reviewed changes
.vscode/launch.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@redrathnure redrathnure requested a review from zarath January 20, 2025 21:58
@redrathnure redrathnure force-pushed the feat/litevna-support-screenshots branch from e99b710 to 7954ae2 Compare January 20, 2025 21:58
@redrathnure
chore: refactor version class
c5744b2
@redrathnure
chore: refactor version class tests
b5bc02c
@redrathnure
chore: improve version class tests
dc1fd3d
@redrathnure
chore: refactor version reading
79fb023 
readVersion() -> read_fw_version()
@redrathnure
feat: drop dummy 2 seconds delay for LiteVNA
611b75a
@redrathnure
chore: use reentrant lock
026b1f9
@redrathnure
chore: refactor features reading
299ca99 
read_features() -> init_features()
@redrathnure
fix: features reading for LiteVNA
1e88841
@redrathnure
fix: add more datapoints for LiteVNA
4096787
@redrathnure
chore: typing for valid_datapoints
4b1e87d
@redrathnure
feat: handle LiteVNA disconnection
d0269b2
@redrathnure
chore: formatting
c1ccb6a
@redrathnure
feat: disconnect on exit
83d50ce
@redrathnure
feat: add Vbat feature reading for LiteVNA
fa78161
@redrathnure
chore: rename get_features
7eeaf54 
getFeatures() to get_features()
@redrathnure
chore: optimize disconnect method
d210485
@redrathnure
feat: go to idle mode after data reading
7d5a1c7
@redrathnure
feat: add screenshots capture
119bbc4
@redrathnure redrathnure force-pushed the feat/litevna-support-screenshots branch from 7954ae2 to 119bbc4 Compare January 20, 2025 22:16
@redrathnure redrathnure mentioned this pull request Jan 20, 2025
Closed
2 tasks
@redrathnure redrathnure changed the title litevVNA support LiteVNA support Jan 20, 2025
@PA3CCE
Copy link

PA3CCE commented Jan 21, 2025 •
edited
Loading

Is it possible now to set for "Enhanced Response" in Calibration for correct S21 calibration in NanoVNA-Saver ?

@redrathnure
Copy link
Contributor Author

@PA3CCE could you please give more details about this feature? How it supported by device and do we have something similar in other client apps e.g. NanoVNA App?

zarath
zarath approved these changes Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@zarath zarath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank's for you additions...
I'll merge, even if I dislike the Tuple to List conversion of numbers of datapoints

src/NanoVNASaver/Hardware/VNA.py Show resolved Hide resolved
src/NanoVNASaver/Hardware/litevna_64.py Outdated Show resolved Hide resolved
@zarath zarath merged commit 30a3dc8 into NanoVNA-Saver:main Jan 21, 2025
2 checks passed
@zarath
Copy link
Collaborator

zarath commented Jan 21, 2025

Is it possible now to set for "Enhanced Ressponse" in Calibration for correct S21 calibration in NanoVNA-Saver ?

Can you make a feature request and tell about the problem there. I don't think it is something what was touched in this PR

@redrathnure
Copy link
Contributor Author

@zarath

I'll merge, even if I dislike the Tuple to List conversion of numbers of datapoints

well, I may rollback these changes if it's a problem (may be as separate PR, because the current one was just merged).

However there are few remarks from my:

  • list/set are easier to handle for type check tools. Tuples with variable size is hard to check.
  • handling even middle size codebase without a strict typing and tools like Ruff and MyPy is a pain. It is just non realistic. Typing should be introduced into whole codebase which would boost development.
  • ... but now, looks like Ruff/Black/MyPy/Isort tools cannot be "just enabled". Because codebase is too inconsistent and amount of findings would be too high.
  • In theory having more automatized way to validate code (pre-commit + Ruff/Black/MyPy/Isort) would help to e.g. review PRs and detect (not always) obvious mistakes, however .. (see previous point). IMO this is kind of weird situation when these tools may help developers, but it's not possible to use them because already existed "legacy" stuff, which lead to more mistakes. Perhaps more active actions should be performed to go out from this loop.

And about Qt related findings:

  • there are lot of places which looks not very good, starting from non annotated slotsand ending with executing heavy operation (including blocking IO) inside UI thread. However touching these points without helper tools looks risky. Especially with current test coverage.
  • using native approach to manage image resources and UI forms may help to solve a few issues, but
    a) current code has M and V component in one place (business logic sits next to UI code)
    and b) PyQt6 tools has some issues. E.g. does not support all Python types and I failed to start tools like Qt Designer. Switching to PySide6 would help, however doing these without proper tooling support looks bit challenging.

At the end it's just set of tasks/steps which may be done. So if you have a plans about some of these points or need a help, let's discuss it.

@redrathnure redrathnure mentioned this pull request Jan 21, 2025
Open
@zarath
Copy link
Collaborator

zarath commented Jan 21, 2025

I agree that this project is much older then the listed tools...
Tuples are ok in linting, since python3.9. - and the Charts code is a big mess.. mainly created by copying and pasting existing charts..
Prime tests were mostly in the mathematical parts, to ensure reliable results - I'll even be happy to get rid of this string based data transmission between the hardware classes and sweep_worker.. (Not yet changed, as the worker is also a big mess)
Long term goal is also switching to skrf for most of data...

@redrathnure
Copy link
Contributor Author

Long term goal is also switching to skrf for most of data...

skrf ?

@zarath
Copy link
Collaborator

zarath commented Jan 21, 2025

https://scikit-rf.readthedocs.io/en/latest/

redrathnure added a commit to redrathnure/nanovna-saver that referenced this pull request Jan 21, 2025
@redrathnure
fix: leftowers from refactoring
79aaf2e 
A new class was missed in NanoVNA-Saver#754
@redrathnure redrathnure mentioned this pull request Jan 21, 2025
Merged
2 tasks
zarath pushed a commit that referenced this pull request Jan 21, 2025
@redrathnure
Fix issues from LiteVNA related PR (#765)
ea1d20e 
* fix: leftowers from refactoring

A new class was missed in #754

* fix(LiteVNA): change default datapoints to 201

Just to speedup "connection" process
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zarath zarath zarath approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@redrathnure @PA3CCE @zarath