Bug 1027 - Time stamps wrong by 1 part in 64 in CSV file output
Summary: Time stamps wrong by 1 part in 64 in CSV file output
Status: RESOLVED FIXED
Alias: None
Product: libsigrok
Classification: Unclassified
Component: Output: csv (show other bugs)
Version: unreleased development snapshot
Hardware: x86 All
: Normal normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-08 16:35 CEST by Thomas
Modified: 2020-08-05 21:20 CEST (History)
4 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas 2017-09-08 16:35:51 CEST
Capturing a GPS 1PPS signal, using a fx2lafw device with
--config samplerate=24MHz --output-format csv --output-file test

I find the times reported are wrong by almost exactly 1 part in 64.

A clipping from the CSV file showing the first two PPS rising edges:

 ; CSV generated by libsigrok 0.6.0-git-9014d45
 ; from fx2lafw (generic driver for FX2 based LAs) on Fri Sep 08 08:24:07 2017
 ; Channels (1/8): GPS
 ; Samplerate: 24 MHz
 nanoseconds,logic
     snip
 414454855,0
 421740637,1
     snip
 1398774655,0
 1406060929,1
     etc

Calculating (with commas for clarity):
1,406,060,929 - 421,740,637 = 984,320,292 ns  (not 1 000,000,000 ns as expected)
the error of 1.5% is not seen when using PulseView and the timing plugin, so I assume it is in the handling of the timestamps of the CSV file.
Comment 1 Earle F. Philhower, III 2020-05-02 19:05:52 CEST
I got the exact same error %age while using sigrok-cli to debug a software PWM module on the ESP8266.

A uint64_t period is calculated here (with the rounding error in going from a non-integer # of ns per cycle to int):

https://sigrok.org/gitweb/?p=libsigrok.git;a=blob;f=src/output/csv.c;h=ea328c4b092a8eee5c498c71ea946ce8e4dd8594;hb=a6b07d7e28fe445afccf36922ef7d20e63e54fe6#l240 

And then the output time in the CSV just adds this (slightly off) period over and over to dump the time here:
https://sigrok.org/gitweb/?p=libsigrok.git;a=blob;f=src/output/csv.c;h=ea328c4b092a8eee5c498c71ea946ce8e4dd8594;hb=a6b07d7e28fe445afccf36922ef7d20e63e54fe6#l445

A more appropriate setup would be to store the period as a double, and then multiply that by the sample number in the CSV output:
-                         ctx->sample_time += ctx->period;
+                         ctx->sample_time = (uint64_t)(ctx->period * i);

You could also store the samplerate and scalefactor sr and do the calculation something like:
> ctx->sample_time = (sr * i) / samplerate
to ensure that any error is not increasing while avoiding any floating point math.
Comment 2 Earle F. Philhower, III 2020-05-02 19:50:11 CEST
Fixed with the patch available here:
https://github.com/earlephilhower/libsigrok/commit/5f314d2f708ae0721e743814f5236df46dee538a
Comment 3 Gerhard Sittig 2020-08-05 21:20:28 CEST
Fixed in 4feb6ec9a2c8. Thank you for reporting and for the approach how to 
fix it. Have rephrased your commit message to match the project's style. And 
have amended the fix to work for larger sample counts and to avoid a division 
by zero when the time column was requested yet the samplerate is unknown. The 
use of floats for a longer time during calculation reduces precision but avoids 
integer overruns when samplerates in the GHz range and large sample counts 
are seen in combination. It's assumed that 53bits cover the most use cases, 
and "not so odd" samplerates don't suffer from the rounding issue.