Unconditionally generate output text when a session packet is received
which carries analog or logic sample data. Even if the data gets queued
and is not shown immediately, in that case the output text remains empty
but needs to be present. Otherwise applications may assume that the CSV
output module had not handled the data at all, which would result in
unexpected "screen output" with fallback data being interleaved with the
CSV output.
This resolves bug #1026 in its strictest sense (the unexpected presence
of fallback data). But leaves all other issues mentioned in comment 1.
The current implementation of the CSV output module makes assumptions
which don't hold. Which results in incorrect or incomplete output for
some combinations of logic and analog signals.
Check for some of the known problematic conditions, and warn the user
about potentially unexpected results. This is a workaround until the
issues properly get addressed in the implementation.
This is motivated by but does not resolve bug #1026.
Unbreak the timestamp calculation when session data is received in
multiple packets. Avoid a division by zero when the samplerate is not
known yet the time column is requested. Only calculate timestamps when
the time column is requested. Use floating point during the scaling,
only convert to integer immediately before printing. Change line breaks
to not split a complex sub-expression across several lines.
The conversion of sample rates to sample periods and the repeated
addition of truncated values (integer variables) resulted in the
accumulation of errors in the timestamp column for odd samplerate
values. How to reproduce:
$ sigrok-cli -d demo:analog_channels=0 \
-c samplerate=6000000 --samples 1200001 \
-O csv:time=true | tail
Accept the additional cost to reduce the error. Always get the timestamp
in a new calculation based on the sample number and the sample rate.
This addresses bug #1027.
Signed-off-by: Earle F. Philhower, III <earlephilhower@yahoo.com>
[ gsi: rephrased commit message, how to reprodue ]
Use size types for counters, unsigned for bit manipulation. Trigger
position needs to remain a signed int (must be possible to go negative
for "not here", and strictly remains within the output text line length,
so should be good).
Rephrase the nested loop during bit extraction from logic packets, and
how a channel's value at a given sample number gets accessed. Eliminate
redundancy in that spot, to improve readability and simplify maintenance.
Unobfuscate the implementation of the recent channel name alignment and
trigger position flush, address other style nits of earlier versions:
Don't need a GString for runtime constant channel names (which also
suffered from a mismatch of declaration and allocation). Don't need to
"construct space" when printf(3) can align the value. Pre-allocate text
buffers with more appropriate length when known in advance. Drop another
unused variable. Eliminate data type redundancy in malloc(3) calls. Make
sure to get zeroed memory, disabled channels can result in assignment
gaps. Use consistent brace style and separate variable declaration from
use (no RAII here).
Excess text line length remains, there has been a lot of it in the
previous implementation. It is left for another commit.
Tested with:
$ sigrok-cli -d demo:analog_channels=0:logic_channels=4 --samples 40 -O ascii -t D3=r -w
The trigger position would be missing in the output text when the number of
available samples is less than the configured text line length. Do flush the
trigger marker for the last chunk of accumulated samples, too.
How to reproduce:
$ sigrok-cli -d ... --samples 32 -O ascii:width=128
[ gsi: rephrased commit message ]
This results in vertical alignment of sample data and trigger positions.
The implementation assumes that the channel names' byte count corresponds
to the space which they occupy on screen. Channel names with umlauts still
may suffer from misalignment.
[ gsi: rephrased commit message ]
For pure analog acquisition without logic data the ZIP creation code
path resulted in a division by zero. Skip the bytes to samples math in
that case. How to reproduce:
$ sigrok-cli -d demo:logic_channels=0:analog_channels=1 --samples 20 -o file.sr
Avoid a dependency on malloc(0) behaviour while we are here. Add a
warning on data feed submitter implementation issues, to not silently
drop the data, which could surprise users. This ShouldNotHappen(TM) for
correct implementations where channel counts and unit size agree, but
was observed with incomplete out-of-tree implementations. Eliminate
a data type redundancy in another malloc() call.
Accumulate samples from multiple session feed packets before sending
them off to ZIP archive operations. This improves throughput for those
setups where acquisition devices or input modules provide only few
samples per session feed send call.
This version also splits large packets from applications into smaller
ZIP members (if the application's packet size is larger than the output
module's local buffer size). If that is not desired, the implementation
needs adjustment to immediately pass larger blocks to ZIP operations
(after potentially flushing previously queued data) instead of looping.
This fixes bug #974.
Extend and rephrase the VCD output module, to support mixed signal data,
support higher channel counts, and address other minor issues.
Increase the number of VCD identifiers which can get generated. Bump the
limit from 94 to 18346 channels. Prefer single letter names for backwards
compatibility for the first channels. Use two or three letter identifiers
as needed for higher channel counts.
Add support for analog channels, and carefully organize a queue such
that timestamps and their data only get written after input data for
_all_ channels was received from the session feed. Provide IEEE754
double precision values for maximum compatibility with other VCD aware
software, although sigrok internally passes analog data with single
precision. This makes potential later adjustment transparent to external
software.
Factor out and rephrase code while we are here. This implementation
avoids glib calls where they'd hurt performance. A local pool reduces
malloc() pressure to increase throughput. String manipulation is tuned
for simplicity and reduced cost. Special code paths were added to tune
the use cases where mixed signals are not involved (immediate write to
the output text, bypassing the output module's local queue).
An srzip input implementation detail still makes the VCD output consume
lots of memory during merge sort of channels' data. See bug #1566.
Other nits got addressed in bypassing: Adjust data types. Separate the
gathering of detail information and the construction of the VCD header
text to simplify review and future maintenance. Skip VCD identifiers for
disabled channels. Emit a final timestamp to flush the last sample, and
communicate the total capture length.
Update comments. Update the copyright for recent non-trivial changes.
The previous implementation inspected the input stream's samplerate, and
simply used the next 1kHz/1MHz/1GHz timescale for VCD export. Re-import
of the exported file might suffer from rather high an overhead, where
users might have to downsample the input stream. Also exported data
might use an "odd" timescale which doesn't represent the input stream's
timing well.
Rephrase the samplerate to VCD timescale conversion such that the lowest
frequency is used which satisfies the file format's constraints as well
as provides high enough a resolution to communicate the input stream's
timing with minimal loss. Do limit this scaling support to at most three
orders above the input samplerate, to most appropriately cope with odd
rates.
As a byproduct the rephrased implementation transparently supports rates
above 1GHz. Input streams with no samplerate now result in 1s timescale
instead of the 1ms timescale of the previous implementation.
The 'period' member of the VCD output module's context is supposed to
hold frequencies that correspond to the timescale used during export.
An 'int' (in combination with VCD's 1/10/100 constraint) thus would
result in a 1GHz limit, use uint64_t instead to support higher rates.
Iterate over the received sample set first, before iterating over the
respective sample's number of channels. This avoids redundant extraction
of sampled bits (which saves only little), but also increases locality
of processed data (though string accumulation still may be expensive).
It also adds the future option of RLE compression during accumulation of
output data, which perfectly matches the WaveDrom syntax for repeated
bit patterns.
Rearrange the order of routines in the wavedrom output module. Keep the
flow of .receive() -> .process_logic() -> .wavedrom_render() in one common
group of routines, which is not disrupted by the .init() and .cleanup()
routines which are kind of boilerplate in the source file. This increases
readability and maintainability.
Adjust brace style, use C language comments, drop camel case. Use size_t
for indices and offsets. Unobfuscate the open/close logic of rendered
output. Allocate zero-filled memory, reduce sizeof() redundancy. Don't
SHOUT in the module's .name property.
[ Changes indentation, see 'git diff -w -b' for review. ]
Adjust the calculation of the '^' marker's position in T: lines of the
-O ascii/bits/hex output modules such that it matches the sample data
lines' layout. Add comments which discuss the motivation of the marker
position's calculation, which differs among each of those modules.
Strictly speaking -O bits was already correct. But I chose to adjust and
comment the logic such that multiple output modules follow a common
pattern. If performance is an issue, the bits.c change might be worth
reverting.
This commit fixes bug #1238.
Make sure to not exceed the ctx->analog_samples[] array bounds. Don't
use the (huge) channel's index in the device's(!) channel list, instead
use the zero-based and dense index into the array of analog samples in
the accumulation buffer, before writing to the external file.
This fixes the segfault reported in bug #1124.
The process_analog() logic is rather complex, dealing with the total
list of channels in the device (which can be of different types), and a
number of submitted samples for a specified list of channels. Replace
the rather short variable names for i, j, c (and num_channels) with
something longer that hopefully increases readability of the complex
loop bodies.
Note that this change merely renames identifiers, and does not change
behaviour.
Instead of nesting indentation levels upon equality of a value, skip
iterations upon inequality. This reduces indentation, and might improve
readability.
[ Indentation changes, see 'diff -w -b' for the essence. ]
The config.h file must always be included as first file.
src/output/csv.c: In function 'gen_header':
src/output/csv.c:64:20: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'uint64_t {aka long long unsigned int}' [-Wformat=]
#define LOG_PREFIX "output/csv"
^
./src/libsigrok-internal.h:753:42: note: in expansion of macro 'LOG_PREFIX'
#define sr_info(...) sr_log(SR_LOG_INFO, LOG_PREFIX ": " __VA_ARGS__)
^
src/output/csv.c:244:3: note: in expansion of macro 'sr_info'
sr_info("Set sample period to %" PRIu64 " %s",
^
src/output/csv.c: In function 'dump_saved_values':
src/output/csv.c:462:34: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'uint64_t {aka long long unsigned int}' [-Wformat=]
g_string_append_printf(*out, "%" PRIu64 "%s",
^
In file included from src/hardware/ftdi-la/protocol.c:21:0:
src/hardware/ftdi-la/protocol.c: In function 'send_samples':
src/hardware/ftdi-la/protocol.h:28:20: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'uint64_t {aka long long unsigned int}' [-Wformat=]
#define LOG_PREFIX "ftdi-la"
^
./src/libsigrok-internal.h:751:42: note: in expansion of macro 'LOG_PREFIX'
#define sr_spew(...) sr_log(SR_LOG_SPEW, LOG_PREFIX ": " __VA_ARGS__)
^
src/hardware/ftdi-la/protocol.c:29:2: note: in expansion of macro 'sr_spew'
sr_spew("Sending %" PRIu64 " samples.", samples_to_send);
^
The compiler marks a potential use after free, which the current
implementation won't trigger. The error only occurs when a sigrok
channel is neither logic nor analog.
Address the issue nevertheless, to silence the compiler warning, and to
protect against possible programming errors when a future implementation
should support more channel types.
This was reported by clang's scan-build.
Fixup unbalanced braces for more complex if statements, to better
reflect the project's official coding style.
Adjust data types in the float_to_le() routine. A float value gets
copied to a buffer of bytes (uint8_t). Don't use 'char' on the other
side of assignments, to not assume a specific width for char, and to
avoid potential signedness issues. Copy from bytes to bytes instead.
The WAV output module supports an optional 'scale' factor, in its
absence the samples will pass unmodified. The builtin help text is
unexpected, and reads:
$ sigrok-cli -O wav --show
...
Options:
scale: Scale values by factor (default 0.0)
Setup a default scale factor of 1.0, which results in identical
behaviour and better reflects what is happening.
The previous implementation only emitted data for the first enabled
channels, and "saw no changes" after emission of the initial value for
channels on positions that followed a disabled channel.
Assume that the received data from the session bus communicates the bits
of enabled channels in a packed representation. Skip the mapping of
output bit indices to sigrok channel numbers.
This fixes the remaining part of bug #519.
Tested by inspecting in gtkwave the result of command:
$ sigrok-cli -d demo -C D1,D3,D6 -c samplerate=2M --samples 2500 -O vcd -o trace.vcd
When we find that all input sources (device drivers, and input modules)
provide a dense bit field, all of the mapping logic can get removed
here. This commit just quickly disables the logic.
Identifiers for channels in the VCD header section could be "sparse"
when sigrok channels were disabled. Make sure to not assign names to
disabled channels. This will e.g. assign !, ", and # to channels D1, D3,
and D6, when D0, D2, D4-D5, and D7 are disabled.
This addresses part of bug #519.
The srzip output module dropped support for the "filename" option in
commit 37875f7506 on 2015-07-30, but still used to assign to slot
options[0] which clobbers the array's sentinel. Remove those accesses
to the non-existing option.
../src/output/csv.c: In function ‘receive’:
../src/output/csv.c:580:8: warning: this statement may fall through [-Wimplicit-fallthrough=]
*out = g_string_new(ctx->frame);
~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
../src/output/csv.c:582:2: note: here
case SR_DF_END:
^~~~
Gerhard Sittig
Earle F. Philhower, III
v1ne
milan hauth
Uwe Hermann
Marc Jacobi
Martin Ling
Frank Stettner