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

Fix frequency_string_short and its usage #20

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Fix frequency_string_short and its usage #20

merged 1 commit into from
Sep 3, 2020

Conversation

wutje
Copy link
Contributor

@wutje wutje commented Jul 11, 2020 •
edited
Loading

When showing multiple markers there where two issues:

  • The frequency shown for 1-9Mhz did not have the MHz string appended. (>=10Mhz did)
  • Above ~2.1 Ghz it showed negative values due to frequency being stored in int32_t

@douardda
Copy link
Contributor

why changing the frequency_string_short() signature to make it return an int? Is this returned values used / planned to be used somewhere ?

@wutje
Copy link
Contributor Author

wutje commented Jul 17, 2020

Yes, I am working on code that uses this same function to print the current frequency on recall buttons. That is how I found this bug.
Printing frequency with suffix can be hard and I rather reuse code then invent it again. But for my code to work I need the result of the snprintf.

@DiSlord
Copy link
Contributor

DiSlord commented Jul 20, 2020

See NanoVNA chprintf.c file for print frequence need only use "%q" and also support any formatting
For print value vs suffix like 0.0001 use "%F" and get 100u
Possible format "%+5.2F" for example and other printf formats

@douardda
Copy link
Contributor

Yes, I am working on code that uses this same function to print the current frequency on recall buttons. That is how I found this bug.
Printing frequency with suffix can be hard and I rather reuse code then invent it again. But for my code to work I need the result of the snprintf.

Ok then this should be explained in the commit message IMHO (or be part of a dedicated commit).

@wutje
Fix two bugs regarding the shown marker frequency:
8116623 
The function frequency_string_short did not append 'Mhz' properly for 1-9Mhz
cell_draw_marker_info stored frequency in a int32_t instead of freqHz_t,
which caused sign conversion after 2.1Ghz, making marker reading beyond
that point bogus. Note that this is only visible with multiple markers.
@wutje wutje force-pushed the marker_plot_fix branch from d5ebaab to 8116623 Compare July 23, 2020 20:18
@wutje
Copy link
Contributor Author

wutje commented Jul 23, 2020

@douardda I changed the code. It no longer returns the amount of characters printed. It is better take small steps. My other code is obsolete now due to @DiSlord great work on the GUI.
@DiSlord https://github.com/gabriel-tenma-white/mculib/blob/master/printf.cpp does not have %q?

@wutje
Copy link
Contributor Author

wutje commented Sep 2, 2020

Can this please be merged? It came up again as #40 .
Measuring a 2400 (Wifi, Bluetooth etc) filter is nearly impossible.
Is there anything I can or need to do so this can be merged?

@gabriel-tenma-white gabriel-tenma-white merged commit e017aab into nanovna-v2:master Sep 3, 2020
@gabriel-tenma-white
Copy link
Collaborator

Sorry, thought this was already included in another PR. It's merged now.

@wutje
Copy link
Contributor Author

wutje commented Sep 3, 2020

Great! Thank you so much for the quick response.

@wutje wutje deleted the marker_plot_fix branch September 3, 2020 07:55
@DG9BFC
Copy link

DG9BFC commented Sep 4, 2020

when can we expect a new release ?? i am no programer (just a user) so i have no idea how to compile a fw ... (i can upload a new one ... thats easy) ...
i own TWO saa2 ... one with small and one with big display (4 inch and n sockets) so best would be compile both versions and release them (like we have seen some months ago)
THANK YOU dg9bfc sigi

@douardda
Copy link
Contributor

douardda commented Sep 4, 2020

when can we expect a new release ?? i am no programer (just a user) so i have no idea how to compile a fw ... (i can upload a new one ... thats easy) ...
i own TWO saa2 ... one with small and one with big display (4 inch and n sockets) so best would be compile both versions and release them (like we have seen some months ago)
THANK YOU dg9bfc sigi

indeed, I've opened an issue for this #41

@douardda
Copy link
Contributor

douardda commented Sep 5, 2020

@DG9BFC I've compiled the current master (rev e017aab) and put them here https://github.com/douardda/NanoVNA-V2-firmware/releases/tag/20200905

I've only tested the 2.8" (ili9341) version since I do not have a 4" display yet (should arrive in a few days hopefully).

Hope this helps.

@DG9BFC
Copy link

DG9BFC commented Sep 6, 2020

tested on small and big screen ... seems to work fine (both fw versions tested!! ... small and big screen!!)
so i think you can close issue 41 :-)
thank you

@lzqgdt
Copy link

lzqgdt commented Sep 10, 2020

tested on big screen ,no problem .

nanovna pushed a commit that referenced this pull request Dec 6, 2021
@gabriel-tenma-white
Merge pull request #20 from wutje/marker_plot_fix
48bfa7e 
Fix frequency_string_short and its usage
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wutje @douardda @DiSlord @gabriel-tenma-white @DG9BFC @lzqgdt