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

hackrf_sweep: normalized timestamp #1350

Merged

Conversation

matrix
Copy link
Contributor

@matrix matrix commented Jul 14, 2023

Hi,

I think I found a problem on hackrf_sweep, regarding the handling of the timestamp of the detections.
Using the maximum frequency range, 0:7250, you can see that within the same sweep the timestamp of the detections changes.
This change causes the unexpected spectrogram plotted by plotsweep.

how to reproduce the issue:

bash-3.2$ ./hackrf-tools/src/hackrf_sweep -f 0:7250 -w 1000000 -N 50 -r test1.csv
call hackrf_sample_rate_set(20.000 MHz)
call hackrf_baseband_filter_bandwidth_set(15.000 MHz)
Sweeping from 0 MHz to 7260 MHz
Stop with Ctrl-C
1 total sweeps completed, 0.98 sweeps/second
2 total sweeps completed, 0.98 sweeps/second
3 total sweeps completed, 0.98 sweeps/second
4 total sweeps completed, 0.98 sweeps/second
5 total sweeps completed, 0.98 sweeps/second
6 total sweeps completed, 0.98 sweeps/second
7 total sweeps completed, 0.98 sweeps/second
9 total sweeps completed, 1.10 sweeps/second
10 total sweeps completed, 1.09 sweeps/second
11 total sweeps completed, 1.08 sweeps/second
12 total sweeps completed, 1.07 sweeps/second
13 total sweeps completed, 1.06 sweeps/second
14 total sweeps completed, 1.05 sweeps/second
16 total sweeps completed, 1.12 sweeps/second
17 total sweeps completed, 1.11 sweeps/second
18 total sweeps completed, 1.10 sweeps/second
19 total sweeps completed, 1.09 sweeps/second
20 total sweeps completed, 1.09 sweeps/second
21 total sweeps completed, 1.08 sweeps/second
22 total sweeps completed, 1.07 sweeps/second
24 total sweeps completed, 1.12 sweeps/second
25 total sweeps completed, 1.11 sweeps/second
26 total sweeps completed, 1.10 sweeps/second
27 total sweeps completed, 1.10 sweeps/second
28 total sweeps completed, 1.09 sweeps/second
29 total sweeps completed, 1.09 sweeps/second
30 total sweeps completed, 1.09 sweeps/second
32 total sweeps completed, 1.12 sweeps/second
33 total sweeps completed, 1.11 sweeps/second
34 total sweeps completed, 1.11 sweeps/second
35 total sweeps completed, 1.10 sweeps/second
36 total sweeps completed, 1.10 sweeps/second
37 total sweeps completed, 1.10 sweeps/second
38 total sweeps completed, 1.09 sweeps/second
40 total sweeps completed, 1.12 sweeps/second
41 total sweeps completed, 1.11 sweeps/second
42 total sweeps completed, 1.11 sweeps/second
43 total sweeps completed, 1.11 sweeps/second
44 total sweeps completed, 1.10 sweeps/second
45 total sweeps completed, 1.10 sweeps/second
47 total sweeps completed, 1.12 sweeps/second
48 total sweeps completed, 1.12 sweeps/second
49 total sweeps completed, 1.11 sweeps/second

Exiting...
Total sweeps: 50 in 44.64602 seconds (1.11 sweeps/second)
hackrf_close() done
hackrf_exit() done
fclose() done
exit
bash-3.2$ tail -n 30 test1.csv
2023-07-14, 23:37:35.603240, 7105000000, 7110000000, 1000000.00, 20, -64.75, -62.24, -59.87, -66.31, -61.23
2023-07-14, 23:37:35.603240, 7115000000, 7120000000, 1000000.00, 20, -66.84, -70.08, -64.72, -60.97, -92.97
2023-07-14, 23:37:35.603240, 7120000000, 7125000000, 1000000.00, 20, -73.76, -67.24, -75.12, -63.85, -59.50
2023-07-14, 23:37:35.603240, 7130000000, 7135000000, 1000000.00, 20, -63.03, -60.98, -82.09, -65.11, -70.58
2023-07-14, 23:37:35.603240, 7125000000, 7130000000, 1000000.00, 20, -69.14, -61.82, -64.45, -64.65, -68.08
2023-07-14, 23:37:35.603240, 7135000000, 7140000000, 1000000.00, 20, -63.82, -66.82, -63.33, -62.57, -68.06
2023-07-14, 23:37:35.622856, 7140000000, 7145000000, 1000000.00, 20, -58.71, -59.03, -70.01, -77.75, -67.84
2023-07-14, 23:37:35.622856, 7150000000, 7155000000, 1000000.00, 20, -66.86, -60.03, -69.82, -65.09, -63.82
2023-07-14, 23:37:35.622856, 7145000000, 7150000000, 1000000.00, 20, -67.17, -64.37, -67.70, -69.21, -58.40
2023-07-14, 23:37:35.622856, 7155000000, 7160000000, 1000000.00, 20, -60.41, -62.23, -73.57, -61.64, -64.94
2023-07-14, 23:37:35.622856, 7160000000, 7165000000, 1000000.00, 20, -69.56, -66.56, -68.15, -60.04, -58.39
2023-07-14, 23:37:35.622856, 7170000000, 7175000000, 1000000.00, 20, -60.00, -68.82, -66.85, -60.11, -63.37
2023-07-14, 23:37:35.622856, 7165000000, 7170000000, 1000000.00, 20, -58.86, -60.56, -68.23, -69.95, -65.80
2023-07-14, 23:37:35.622856, 7175000000, 7180000000, 1000000.00, 20, -77.33, -66.93, -70.73, -63.25, -60.38
2023-07-14, 23:37:35.622856, 7180000000, 7185000000, 1000000.00, 20, -66.35, -66.91, -65.38, -69.26, -65.83
2023-07-14, 23:37:35.622856, 7190000000, 7195000000, 1000000.00, 20, -59.22, -59.06, -66.52, -73.21, -68.71
2023-07-14, 23:37:35.622856, 7185000000, 7190000000, 1000000.00, 20, -67.06, -67.41, -66.64, -63.41, -65.11
2023-07-14, 23:37:35.622856, 7195000000, 7200000000, 1000000.00, 20, -62.03, -58.35, -59.08, -60.27, -65.60
2023-07-14, 23:37:35.622856, 7200000000, 7205000000, 1000000.00, 20, -79.70, -60.88, -65.98, -66.00, -62.15
2023-07-14, 23:37:35.622856, 7210000000, 7215000000, 1000000.00, 20, -62.92, -63.64, -65.69, -61.27, -59.68
2023-07-14, 23:37:35.622856, 7205000000, 7210000000, 1000000.00, 20, -57.82, -59.66, -64.70, -64.05, -66.34
2023-07-14, 23:37:35.622856, 7215000000, 7220000000, 1000000.00, 20, -64.16, -65.64, -81.90, -64.54, -62.27
2023-07-14, 23:37:35.622856, 7220000000, 7225000000, 1000000.00, 20, -65.78, -64.52, -62.66, -67.82, -66.49
2023-07-14, 23:37:35.622856, 7230000000, 7235000000, 1000000.00, 20, -60.58, -58.98, -62.84, -71.23, -63.07
2023-07-14, 23:37:35.622856, 7225000000, 7230000000, 1000000.00, 20, -61.86, -62.60, -71.10, -63.03, -64.56
2023-07-14, 23:37:35.622856, 7235000000, 7240000000, 1000000.00, 20, -63.01, -62.36, -64.75, -77.32, -66.70
2023-07-14, 23:37:35.622856, 7240000000, 7245000000, 1000000.00, 20, -76.28, -81.99, -71.86, -66.14, -65.01
2023-07-14, 23:37:35.622856, 7250000000, 7255000000, 1000000.00, 20, -63.17, -61.33, -60.64, -61.16, -63.35
2023-07-14, 23:37:35.622856, 7245000000, 7250000000, 1000000.00, 20, -66.41, -62.43, -60.27, -69.06, -67.41
2023-07-14, 23:37:35.622856, 7255000000, 7260000000, 1000000.00, 20, -60.45, -61.96, -60.39, -66.32, -62.57
bash-3.2$ ../../../plotsweep/target/debug/plotsweep test1.csv test1.png
bash-3.2$ open test1.png 

test1

To solve this issue, I managed the timestamp so that it is updated only at the end of each sweep.
Below is the new graph produced, reusing the same commands as before.

test2

Thanks :)

@dmaltsiniotis
Copy link
Contributor

dmaltsiniotis commented Jul 14, 2023

It seems like with this change the timestamp column will be doing double-duty: It will act as a timestamp indicator of the rx_callback, but also as an unofficial sweep_count index. As such, we lose a little bit of accuracy by not having the "exact" timestamp for the row during large sweeps (and I don't know if this would break any existing application that might rely on that behavior) but we gain a column (timestamp) that we can also use for indexing individual sweeps for plotting purposes, is that correct?

Is the goal to be able to consume multiple sweeps in a file and plot them all, or plot a single sweep (which maybe can be accomplished with one shot flag and then you don't have to worry about timestamps?)

@matrix
Copy link
Contributor Author

matrix commented Jul 14, 2023

I understand that it could be a problem if someone makes use of this data and takes into account this deviation, even if it is minimal, within the same sweep.

An extra option could be introduced that handles this change in timestamp handling.

I tried rx_tools, the timestamp is set equal within each sweep.

@dmaltsiniotis about your questions: the first yes, for the second the goal is plot a graph that is readable

@martinling
Copy link
Member

Hi @matrix,

We were a bit wary about changing the hackrf_sweep output because of how that might affect other tools using it, but I see you've now updated this to make it a command line option. We'd be happy to have this as an option.

There have been some other changes to hackrf_sweep in the meantime. Are you able to resolve the conflicts and then I'll give this a final review?

@matrix
Copy link
Contributor Author

matrix commented Dec 7, 2023

Hi @matrix,

We were a bit wary about changing the hackrf_sweep output because of how that might affect other tools using it, but I see you've now updated this to make it a command line option. We'd be happy to have this as an option.

There have been some other changes to hackrf_sweep in the meantime. Are you able to resolve the conflicts and then I'll give this a final review?

Hi @martinling

certainly, I am able to resolve conflicts.
I will send an update as soon as possible.

Thanks :)

@matrix matrix force-pushed the hackrf_sweep-normalizedTimestamp branch from 90788da to ac30119 Compare December 7, 2023 18:41
@matrix
Copy link
Contributor Author

matrix commented Dec 7, 2023

done but there're some errors with workflows
let me know if I can eventually do anything myself to solve it

Thanks

@martinling
Copy link
Member

Thanks for the update.

The workflow error isn't your fault, that's just broken due to something else that we're fixing.

I'll add this to my list to review but I'm about to be away for a bit so it may not be right away.

@martinling martinling self-assigned this Dec 7, 2023
@martinling martinling self-requested a review December 7, 2023 20:43
Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

I've tested this out and all looks good, just a couple of things to fix I think:

  • There's one memset call that I think is superfluous.
  • Your commit fixing the clang-format errors doesn't seem to have appeased the CI check; I have a commit 91ab2e5 that may work instead.

if (timestamp_normalized == true)
{
// set the timestamp of the next sweep
memset(&usb_transfer_time, 0, sizeof (usb_transfer_time));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any need for this memset call - the structure is immediately overwritten by the following gettimeofday call.

@matrix
Copy link
Contributor Author

matrix commented Jan 5, 2024

I've tested this out and all looks good, just a couple of things to fix I think:

  • There's one memset call that I think is superfluous.
  • Your commit fixing the clang-format errors doesn't seem to have appeased the CI check; I have a commit 91ab2e5 that may work instead.

if you can make these last changes yourself we will speed up. Let me know if you want to proceed differently

@martinling martinling self-requested a review February 1, 2024 17:20
Copy link
Member

@martinling martinling left a comment

Choose a reason for hiding this comment

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

I've made those changes and will merge this now. Thanks!

@martinling martinling merged commit f6598e7 into greatscottgadgets:master Feb 1, 2024
14 of 15 checks passed
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.

3 participants